Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Vincent Legoll]

Hello,

+            // 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) {

Is "0" a magical value that don't need casting or am I missing something ?

-- 
Vincent Legoll

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[John Darrington]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On Fri, Dec 23, 2016 at 12:18:11PM +0100, Vincent Legoll wrote:
     Hello,
     
     +            // 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) {
     
     Is "0" a magical value that don't need casting or am I missing something ?
     
Presumably clientUid is an unsigned integral type.  Hence -1 does need a cast, whereas
0 does not.

J'

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Danny Milosavljevic]

On Fri, 23 Dec 2016 14:33:31 +0100
John Darrington <john@darrington.wattle.id.au> wrote:

> On Fri, Dec 23, 2016 at 12:18:11PM +0100, Vincent Legoll wrote:
>      Hello,
>      
>      +            // 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) {
>      
>      Is "0" a magical value that don't need casting or am I missing something ?
>      
> Presumably clientUid is an unsigned integral type.  Hence -1 does need a cast, whereas
> 0 does not.

Technically I agree. If it was my code I'd cast it regardless.

Also, why use the magical value 0 for gid? Why not likewise (gid_t) -1 ?

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 23.12.2016 um 22:15 schrieb Danny Milosavljevic:
> Also, why use the magical value 0 for gid? Why not likewise (gid_t) -1 ?

The "magic value" for gid is -1, too, as you can see in
nix/nix-daemon/nix-daemon.cc.

gid 0 is not used as a magic value. As you can read in the comment, the
test just prohibits changing the group to "root":

    settings.clientGid != 0 ? settings.clientGid : -1

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Danny Milosavljevic]

Hi,

On Sat, 24 Dec 2016 13:34:16 +0100
Hartmut Goebel <h.goebel@crazy-compilers.com> wrote:

> The "magic value" for gid is -1, too, as you can see in
> nix/nix-daemon/nix-daemon.cc.
> 
> gid 0 is not used as a magic value. As you can read in the comment, the
> test just prohibits changing the group to "root":
> 
>     settings.clientGid != 0 ? settings.clientGid : -1

Ah, ok then.

Sorry for the noise...

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

[Hartmut Goebel]

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.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

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        | 21 +++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 12 ++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 889ee3d..e823001 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2631,6 +2631,21 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    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 +2654,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..7beb1a5 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 build request.  Used to set
+       the owner and group of the kept 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..47b67d5 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 = (uid_t) -1;
+                settings.clientGid = (gid_t) -1;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-- 
2.7.4

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Ludovic Courtès]

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.

OK, please push.

Thank you!

Ludo’.

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 09.12.2016 um 15:23 schrieb Ludovic Courtès:
> OK, please push.
Done. Thanks for testing.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |