allow setuid and setgid wrappers to run in user namespaces #231673
Conversation
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
10.rebuild-linux: 1-10
labels
In combination with NixOS#231673 this allows hardened services to use nullmailer's sendmail.
In combination with NixOS#231673 this allows hardened services to use nullmailer's sendmail.
In combination with NixOS#231673 this allows hardened services to use nullmailer's sendmail.
In combination with NixOS#231673 this allows hardened services to use nullmailer's sendmail.
|
|
||
| // If true, then we did not benefit from setuid privilege escalation, | ||
| // where the original uid is still in ruid and different from euid == suid. | ||
| int no_suid = ruid == euid && euid == suid; |
There was a problem hiding this comment.
I do prefer () for readability
|
I like the extra testing that's done. |
symphorien
force-pushed
the
suid_wrappers_userns
branch
from
822c6dd
to
8e6c9cf
Compare
symphorien
force-pushed
the
suid_wrappers_userns
branch
from
8e6c9cf
to
543cb16
Compare
ofborg
bot
added
10.rebuild-darwin: 0
and removed
10.rebuild-darwin: 1
10.rebuild-darwin: 1-10
labels
| @@ -177,6 +178,17 @@ int main(int argc, char **argv) { | |||
| fprintf(stderr, "cannot readlink /proc/self/exe: %s", strerror(-self_path_size)); | |||
| } | |||
|
|
|||
| unsigned int ruid, euid, suid, rgid, egid, sgid; | |||
| ASSERT(!getresuid(&ruid, &euid, &suid)); | |||
There was a problem hiding this comment.
I think those asserts are really bad at communicating what happens, I'd rather have strerror properly.
|
|
||
| // If true, then we did not benefit from setuid privilege escalation, | ||
| // where the original uid is still in ruid and different from euid == suid. | ||
| int no_suid = (ruid == euid) && (euid == suid); |
There was a problem hiding this comment.
| int no_suid = (ruid == euid) && (euid == suid); | |
| int didnt_suid = (ruid == euid) && (euid == suid); |
no_suid feels like this is a no SUID binary when in practice "it is", the difference is that we didn't actively set the UID because we are already that UID.
So the variable name should reflect this, possess_already_target_uid, or whatever. This makes the code more readable to the non-expert.
| struct stat st; | ||
| ASSERT(lstat(self_path, &st) != -1); | ||
|
|
||
| ASSERT(!(st.st_mode & S_ISUID) || (st.st_uid == geteuid())); | ||
| ASSERT(!(st.st_mode & S_ISGID) || (st.st_gid == getegid())); | ||
| ASSERT(!((st.st_mode & S_ISUID) && !no_suid) || (st.st_uid == geteuid())); |
There was a problem hiding this comment.
Add a comment saying, this expresses:
| ASSERT(!((st.st_mode & S_ISUID) && !no_suid) || (st.st_uid == geteuid())); | |
| // if the binary is SUID and this is a privilege escalation, then the file's UID has to match the effective one (`egid`) | |
| ASSERT(!((st.st_mode & S_ISUID) && !didnt_suid) || (st.st_uid == euid)); |
|
@Mic92 can you take a look to this please? |
|
@symphorien commit should follow contributing guide: |
symphorien
force-pushed
the
suid_wrappers_userns
branch
from
543cb16
to
535ae4c
Compare
|
Thank you for your feedback. I pushed a version that should take it into account. |
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
and removed
10.rebuild-darwin: 0
labels
|
Since GitHub's UI leaves a lot to be desired, that last comment is in this review: |
…paces
In user namespaces where an unprivileged user is mapped as root and root
is unmapped, setuid bits have no effect. However setuid root
executables like mount are still usable *in the namespace* as the user
already has the required privileges. This commit detects the situation
where the wrapper gained no privileges that the parent process did not
already have and in this case does less sanity checking. In short there
is no need to be picky since the parent already can execute the foo.real
executable themselves.
Details:
man 7 user_namespaces:
Set-user-ID and set-group-ID programs
When a process inside a user namespace executes a set-user-ID
(set-group-ID) program, the process's effective user (group) ID
inside the namespace is changed to whatever value is mapped for
the user (group) ID of the file. However, if either the user or
the group ID of the file has no mapping inside the namespace, the
set-user-ID (set-group-ID) bit is silently ignored: the new
program is executed, but the process's effective user (group) ID
is left unchanged. (This mirrors the semantics of executing a
set-user-ID or set-group-ID program that resides on a filesystem
that was mounted with the MS_NOSUID flag, as described in
mount(2).)
The effect of the setuid bit is that the real user id is preserved and
the effective and set user ids are changed to the owner of the wrapper.
We detect that no privilege was gained by checking that euid == suid
== ruid. In this case we stop checking that euid == owner of the
wrapper file.
As a reminder here are the values of euid, ruid, suid, stat.st_uid and
stat.st_mode & S_ISUID in various cases when running a setuid 42 executable as user 1000:
Normal case:
ruid=1000 euid=42 suid=42
setuid=2048, st_uid=42
nosuid mount:
ruid=1000 euid=1000 suid=1000
setuid=2048, st_uid=42
inside unshare -rm:
ruid=0 euid=0 suid=0
setuid=2048, st_uid=65534
inside unshare -rm, on a suid mount:
ruid=0 euid=0 suid=0
setuid=2048, st_uid=65534
symphorien
force-pushed
the
suid_wrappers_userns
branch
from
535ae4c
to
0e4b8a0
Compare
|
My bad, fixed |
|
Will this be backported into stable as well? |
Once we get more experience with this running in production (or someone explicitly tries it in user ns contexts, to confirm it works "better" for their usecases and not synthetic ones), I would not be against backporting it. |
|
I personally don't think this should be backported. If you can't wait < half a year for a potentially breaking feature such as this one, you should not be using the stable channel. |
|
@Atemu That seems like an unnecessarily accusative/confrontational comment. It's not even clear to me what exactly is "potentially breaking" here - as far as I can tell, it resolves a bug. |
|
I agree with Atemu. In my opinion, this is not the kind of low-risk, definitely-non-breaking bug-fixes that I expect to be backported to stable releases. This looks more like an improvement than a bug fix to me. |
|
While I understand from where you are coming, I would argue this qualifies definitely as a bug, even though we are adding features to the C wrapper, user namespaces are a classical feature of the Linux kernel and we kind of expect being able to run stuff in them without getting this hard to debug NixOS only error. Nevertheless, given the code delta and actual "in-production" experience, I do not see this as a "high risk" backport contrary to other stuff I have seen being backported. That being said, the next stable is in November, that is, in 3 months. If the actual "in-production" experience cannot be obtained fast enough, I would deem this to not be a priority because we would find ourselves dealing with stabilizing unstable.
Let's not urge people to use unstable :P — I think it's a valid demand and it's not a breaking change because I am almost certain no one is relying on the semantics of user namespace to make all wrapped NixOS software fail in those contexts :). Either case, I do not see a big problem with such a backport, I only think it may be counterproductive given that 23.11 will land "soon" for some value of 'soon' and I would urge anyone who wants to use this in 23.05 to just apply the patch as a PR (no negotiation) or work towards getting a backport in (negotiation with stakeholders). |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
In user namespaces where an unprivileged user is mapped as root and root is unmapped, setuid bits have no effect. However setuid root executables like mount are still usable in the namespace as the user already has the required privileges. This commit detects the situation where the wrapper gained no privileges that the parent process did not already have and in this case does less sanity checking. In short there is no need to be picky since the parent already can execute the foo.real executable themselves.
Details:
man 7 user_namespaces:
Set-user-ID and set-group-ID programs
When a process inside a user namespace executes a set-user-ID
(set-group-ID) program, the process's effective user (group) ID
inside the namespace is changed to whatever value is mapped for
the user (group) ID of the file. However, if either the user or
the group ID of the file has no mapping inside the namespace, the
set-user-ID (set-group-ID) bit is silently ignored: the new
program is executed, but the process's effective user (group) ID
is left unchanged. (This mirrors the semantics of executing a
set-user-ID or set-group-ID program that resides on a filesystem
that was mounted with the MS_NOSUID flag, as described in
mount(2).)
The effect of the setuid bit is that the real user id is preserved and the effective and set user ids are changed to the owner of the wrapper. We detect that no privilege was gained by checking that euid == suid == ruid. In this case we stop checking that euid == owner of the wrapper file.
As a reminder here are the values of euid, ruid, suid, stat.st_uid and stat.st_mode & S_ISUID in various cases when running a setuid 42 executable as user 1000:
Normal case:
ruid=1000 euid=42 suid=42
setuid=2048, st_uid=42
nosuid mount:
ruid=1000 euid=1000 suid=1000
setuid=2048, st_uid=42
inside unshare -rm:
ruid=0 euid=0 suid=0
setuid=2048, st_uid=65534
inside unshare -rm, on a suid mount:
ruid=0 euid=0 suid=0
setuid=2048, st_uid=65534
Fixes #42117
Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)