Patchwork [v3] daemon: Set ownership of kept build directories to the calling user.

login
register
mail settings
Submitter Hartmut Goebel
Date Dec. 5, 2016, 8:46 p.m.
Message ID <1480970772-30287-1-git-send-email-h.goebel@crazy-compilers.com>
Download mbox | patch
Permalink /patch/18208/
State New
Headers show

Comments

Hartmut Goebel - Dec. 5, 2016, 8:46 p.m.
Fixes <http://bugs.gnu.org/15890>.

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
  caller in settings.
* nix/libstore/build.cc (_chown): New function.
  (DerivationGoal::deleteTmpDir): Use it, change ownership of build
  directory if it is kept and the new owner is not root.
---
 nix/libstore/build.cc        | 23 +++++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 12 ++++++++++++
 3 files changed, 41 insertions(+)
Ludovic Courtès - Dec. 6, 2016, 3:08 p.m.
Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Fixes <http://bugs.gnu.org/15890>.
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
>   caller in settings.
> * nix/libstore/build.cc (_chown): New function.
>   (DerivationGoal::deleteTmpDir): Use it, change ownership of build
>   directory if it is kept and the new owner is not root.

[...]

> +static void _chown(const Path & path, uid_t uid, gid_t gid)
> +{
> +    checkInterrupt();
> +
> +    printMsg(lvlVomit, format("%1%") % path);

Please remove this line (we wouldn’t be able to track where the message
comes from).

> +    /* User and groud id of the client issuing the buld request.  Used to set
                                                       ^^
Typo.  Also:

  … issuing the build request, or -1 if the UID and GID are not known.

> +       the owner and group of the keept temporary directories of failed
                                      ^^
Typo.

If you have checked that it works as intended, please push with these
changes and email the commit ID to 15890-done@debbugs.gnu.org.

Thank you!

Ludo’.
Danny Milosavljevic - Dec. 6, 2016, 8:41 p.m.
Hi Hartmut,

> +#if defined(SO_PEERCRED)
...
> +#else
> +                /* Setting these to -1 means: do not change */
> +                settings.clientUid = -1;
> +                settings.clientGid = -1;
> +#endif

I think you also have to cast them there, so

               settings.clientUid = (uid_t) -1;
               settings.clientGid = (gid_t) -1;

The reason is because (-1) is a signed integer and clientUid isn't - and neither is clientGid.
Hartmut Goebel - Dec. 8, 2016, 12:12 p.m.
Am 06.12.2016 um 16:08 schrieb Ludovic Courtès:
> If you have checked that it works as intended, please push with these
> changes and email the commit ID to 15890-done@debbugs.gnu.org.

I can only verify that it worked with revision 6634180 "gnu: guile-ssh:
Update to 0.10.2.", but not with current master.

Starting with 21531ad "offload: Use Guile-SSH instead of GNU lsh" I get
errors, which I could not work around:

guix: offload: command not found
Try `guix --help' for more information.
guix build: error: build failed: unexpected EOF reading a line

So, although I'm confident the code is correct, I can not verify with
current master. Thus I'm posting a new version of the patch asking you
to test and push.
Hartmut Goebel - Dec. 8, 2016, 12:16 p.m.
Am 06.12.2016 um 21:41 schrieb Danny Milosavljevic:
> I think you also have to cast them there, so
>
>                settings.clientUid = (uid_t) -1;
>                settings.clientGid = (gid_t) -1;

Thanks, I changes this accordingly.
Ludovic Courtès - Dec. 9, 2016, 2:22 p.m.
Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 06.12.2016 um 16:08 schrieb Ludovic Courtès:
>> If you have checked that it works as intended, please push with these
>> changes and email the commit ID to 15890-done@debbugs.gnu.org.
>
> I can only verify that it worked with revision 6634180 "gnu: guile-ssh:
> Update to 0.10.2.", but not with current master.
>
> Starting with 21531ad "offload: Use Guile-SSH instead of GNU lsh" I get
> errors, which I could not work around:
>
> guix: offload: command not found
> Try `guix --help' for more information.
> guix build: error: build failed: unexpected EOF reading a line

You need to install Guile-SSH to fix this (Guile-SSH was found at
configure-time but not at run-time, leading to this error.)

Ludo’.
Hartmut Goebel - Dec. 9, 2016, 3:50 p.m.
Am 09.12.2016 um 15:22 schrieb Ludovic Courtès:
> You need to install Guile-SSH to fix this (Guile-SSH was found at
> configure-time but not at run-time, leading to this error.)

I wonder how this can happen. I'm running within "guix environment guix".

Maybe I need to run within "./pre-inst guix environment guix"?
Ludovic Courtès - Dec. 9, 2016, 8:35 p.m.
Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 09.12.2016 um 15:22 schrieb Ludovic Courtès:
>> You need to install Guile-SSH to fix this (Guile-SSH was found at
>> configure-time but not at run-time, leading to this error.)
>
> I wonder how this can happen. I'm running within "guix environment guix".

In ‘guix environment guix’, Guile-SSH is definitely going to be found;
you can confirm this by looking at ‘config.log’.

However, if you type “make install” from there, then it may be the case
that Guile-SSH is no longer visible in the installed environment.

HTH!

Ludo’.

Patch

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 889ee3d..44ff11c 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2631,6 +2631,23 @@  void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    printMsg(lvlVomit, format("%1%") % path);
+
+    if (lchown(path.c_str(), uid, gid) == -1) {
+	throw SysError(format("change owner and group of `%1%'") % path);
+    }
+    struct stat st = lstat(path);
+    if (S_ISDIR(st.st_mode)) {
+        for (auto & i : readDirectory(path))
+            _chown(path + "/" + i.name, uid, gid);
+    }
+}
+
+
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
@@ -2639,6 +2656,12 @@  void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // Change the ownership if clientUid is set. Never change the
+            // ownership or the group to "root" for security reasons.
+            if (settings.clientUid != (uid_t) -1 && settings.clientUid != 0) {
+                _chown(tmpDir, settings.clientUid,
+                       settings.clientGid != 0 ? settings.clientGid : -1);
+            }
         }
         else
             deletePath(tmpDir);
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index 8c07e36..dc6a004 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -70,6 +70,12 @@  struct Settings {
        subgoal of the same goal) fails. */
     bool keepGoing;
 
+    /* User and groud id of the client issuing the buld request.  Used to set
+       the owner and group of the keept temporary directories of failed
+       builds. */
+    uid_t clientUid;
+    gid_t clientGid;
+
     /* Whether, if we cannot realise the known closure corresponding
        to a derivation, we should try to normalise the derivation
        instead. */
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 682f9a2..6f88e5b 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -960,6 +960,18 @@  static void daemonLoop()
                     strncpy(argvSaved[1], processName.c_str(), strlen(argvSaved[1]));
                 }
 
+#if defined(SO_PEERCRED)
+                /* Store the client's user and group for this connection. This
+                   has to be done in the forked process since it is per
+                   connection. */
+                settings.clientUid = cred.uid;
+                settings.clientGid = cred.gid;
+#else
+                /* Setting these to -1 means: do not change */
+                settings.clientUid = -1;
+                settings.clientGid = -1;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;