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

[Hartmut Goebel]

Am 01.12.2016 um 01:01 schrieb Danny Milosavljevic:
> So I'd say to compare with (uid_t)-1 would be fine
>
> if (settings.clientUid != (uid_t) -1) {
> ...
> }

Thanks for this tip, Danny. This made it work and I just posted the
updated patch.

> > And casting uid_t to unisgned int might return the same uid as "nobody".
> [...]
>
> (-1) in two-complement 16 bit integral encoding (which is technically not 
> guaranteed to be used in C) would be 65535.

Ahh, well, yes. I'm not quite good on bit operations, esp. on
representations of negative numbers. Thanks to doing this right.

-- 
Regards
Hartmut Goebel

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

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

[Danny Milosavljevic]

Hi Hartmut,

On Mon, 28 Nov 2016 22:31:44 +0100
Hartmut Goebel  wrote:
> I just need some C programming hint: uid_t is an unsigned int, so
> comparing with -1 raises a warning (which IMO is the same as en error).

The system call handler in the Linux kernel does this, among other things:

#define low2highuid(uid) ((uid) == (old_uid_t)-1 ? (uid_t)-1 : (uid_t)(uid))

So I'd say to compare with (uid_t)-1 would be fine

if (settings.clientUid != (uid_t) -1) {
...
}

... since they do something like it in Linux anyway - which is the one 
implementing the chown operation in the first place :)

> And casting uid_t to unisgned int might return the same uid as "nobody".

What do you mean? uid_t is __uid_t - which in turn is implementation-defined - 
but yes, it is unsigned. On Linux it changed size multiple time until now where 
it is 32 bit unsigned.

On GuixSD, user "nobody" has uid 65534. That's one less than the maximum value 
for 16 bit (!) uids (the maximum is hopefully invalid as uid since chown needs 
it as a flag).

(-1) in two-complement 16 bit integral encoding (which is technically not 
guaranteed to be used in C) would be 65535.

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

[Hartmut Goebel]

Am 27.11.2016 um 22:04 schrieb Ludovic Courtès:
> The problem is that 0 also means “root”, so I’d prefer a different value
> to denote that “unset” status, -1 if you want, and explicit checks (and
> a comment explaining that ‘clientUid’ and ‘clientGid’ can be either a
> valid UID/GID or -1, in which case blablabla).
>
> How does that sound?

This basically sounds reasonable.

I just need some C programming hint: uid_t is an unsigned int, so
comparing with -1 raises a warning (which IMO is the same as en error).
And casting uid_t to unisgned int might return the same uid as "nobody".
How can I solve this?

-- 
Regards
Hartmut Goebel

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

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

[Ludovic Courtès]

Hi!

Hartmut Goebel  skribis:

> Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
>> I don’t like the idea of passing those via the big ‘Settings’
>> singleton.
>
> Well, the Settings are already used to pass options like --keep-going to
> the build process. So the "Singleton" is mutable per-process anyway.
> This is why I've put these here, too.

Good point.  This and your other message have convinced me!

> Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
>> Could we instead pass them via the ‘LocalStore’ constructor, with their
>> default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?
>
> Regarding the default values: I'm using zero as "not set" marker here.
> The files will already have owner and group from ‘getuid’ and ‘getgid’,
> so there is no need to change them. Or did I miss something?

The problem is that 0 also means “root”, so I’d prefer a different value
to denote that “unset” status, -1 if you want, and explicit checks (and
a comment explaining that ‘clientUid’ and ‘clientGid’ can be either a
valid UID/GID or -1, in which case blablabla).

How does that sound?

Thanks, and sorry for the delay!

Ludo’.

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

[Hartmut Goebel]

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> Could we instead pass them via the ‘LocalStore’ constructor,

I started implementing this, but adding clientUid/Gid as a attribute
(property) of 'LocalStore' feels wrong. The LocalStore is about the
store: files being there, querying and such. Even "build" is a bit of a
misfit IMO.

Okay, probably you mean, passing to the ‘LocalStore’ constructor and
then forward them to the goals. Which means changing
`makeDerevationGoal', which is used quite some time. From there you
would need to pass it onto the 'DerivationGoal' constructor. These are
quite some changes for the rare case of building, and only if the build
failed and only if the user passed --keep-failed.

I have the impression, that 'Settings' have been introduced exactly to
save passing around such information. "keep-failed" is passed to
'deleteTmpDir()' exactly this way (via Settings). Which means if passing
clientUid/Gid via constructors,  we would have two different ways of
passing information used in the same small function - which would be
inconsistent.

Thus said, I'm in strong favour of using Settings.

-- 
Regards
Hartmut Goebel

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

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

[Hartmut Goebel]

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> Could we instead pass them via the ‘LocalStore’ constructor, with their
> default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?

Regarding the default values: I'm using zero as "not set" marker here.
The files will already have owner and group from ‘getuid’ and ‘getgid’,
so there is no need to change them. Or did I miss something?

-- 
Regards
Hartmut Goebel

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

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

[Hartmut Goebel]

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> I don’t like the idea of passing those via the big ‘Settings’
> singleton.

Well, the Settings are already used to pass options like --keep-going to
the build process. So the "Singleton" is mutable per-process anyway.
This is why I've put these here, too.

But if you prefer passing them around, I'll try to implement this.

-- 
Regards
Hartmut Goebel

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

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

[Ludovic Courtès]

Hartmut Goebel  skribis:

> Fixes .
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (pdaemonLoop] 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.

[...]

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

I think this should use ‘lchown’.

> --- 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;

I don’t like the idea of passing those via the big ‘Settings’
singleton.

Could we instead pass them via the ‘LocalStore’ constructor, with their
default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?

Thank you!

Ludo’.

[Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Fixes .

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (pdaemonLoop] 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.
---
 nix/libstore/build.cc| 24 
 nix/libstore/globals.hh  |  6 ++
 nix/nix-daemon/nix-daemon.cc | 13 +
 3 files changed, 43 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index ae78e65..b49fb95 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2609,6 +2609,23 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+checkInterrupt();
+
+printMsg(lvlVomit, format("%1%") % path);
+
+if (chown(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 != "") {
@@ -2617,6 +2634,13 @@ 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 to "root" for security reasons. So zero is used as
+// marker for unset.
+if (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 35c284f..e900a7d 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -950,6 +950,19 @@ 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 zero means: do not change, esp. do not
+   change to "root". */
+settings.clientUid = 0;
+settings.clientGid = 0;
+#endif
+
 /* Handle the connection. */
 from.fd = remote;
 to.fd = remote;
-- 
2.7.4