nixos/security/wrappers: fix for #98863 #201536
Conversation
Before this change it was crucial that nonprivileged users are unable to create hardlinks to SUID wrappers, lest they be able to provide a different `.real` file alongside. That was ensured by not providing a location writable to them in the /run/wrappers tmpfs, (unless disabled) by the fs.protected_hardlinks=1 sysctl, and by the explicit own-path check in the wrapper. After this change, ensuring that property is no longer important, and the check is most likely redundant. The simplification of expectations of the wrapper will make it easier to remove some of the assertions in the wrapper (which currently cause the wrapper to fail in no_new_privs environments, instead of executing the target with non-elevated privileges). Note that wrappers had to be copied (not symlinked) into /run/wrappers due to the SUID/capability bits, and they couldn't be hard/softlinks of each other due to those bits potentially differing. Thus, this change doesn't increase the amount of memory used by /run/wrappers.
/proc/self/exe is a "fake" symlink. When it's opened, it always opens the actual file that was execve()d in this process, even if the file was deleted or renamed; if the file is no longer accessible from the current chroot/mount namespace it will at the very worst fail and never open the wrong file. Thus, we can make a much simpler argument that we're reading capabilities off the correct file after this change (and that argument doesn't rely on things such as protected_hardlinks being enabled, or no users being able to write to /run/wrappers, or the verification that the path readlink returns starts with /run/wrappers/).
…oc/self/exe) Given that we are no longer inspecting the target of the /proc/self/exe symlink, stop asserting that it has any properties. Remove the plumbing for wrappersDir, which is no longer used. Asserting that the binary is located in the specific place is no longer necessary, because we don't rely on that location being writable only by privileged entities (we used to rely on that when assuming that readlink(/proc/self/exe) will continue to point at us and when assuming that the `.real` file can be trusted). Assertions about lack of write bits on the file were IMO meaningless since inception: ignoring the Linux's refusal to honor S[UG]ID bits on files-writeable-by-others, if someone could have modified the wrapper in a way that preserved the capability or S?ID bits, they could just remove this check. Assertions about effective UID were IMO just harmful: if we were executed without elevation, the caller would expect the result that would cause in a wrapperless distro: the targets gets executed without elevation. Due to lack of elevation, that cannot be used to abuse privileges that the elevation would give. This change partially fixes NixOS#98863 for S[UG]ID wrappers. The issue for capability wrappers remains.
Note that this regression test checks only s[gu]id wrappers. The issue for capability wrappers is not fixed yet.
github-actions
bot
added
6.topic: nixos
Mic92
force-pushed
the
suidwrapnoreal
branch
from
ef5e4b4
to
99cc686
Compare
ofborg
bot
added
10.rebuild-darwin: 0
Mic92
force-pushed
the
suidwrapnoreal
branch
from
99cc686
to
7774008
Compare
Mic92
force-pushed
the
suidwrapnoreal
branch
8 times, most recently
from
5be7518
to
55d9b8d
Compare
Mic92
force-pushed
the
suidwrapnoreal
branch
from
55d9b8d
to
c01df35
Compare
| @@ -41,7 +39,7 @@ let | |||
| }; | |||
| options.permissions = lib.mkOption | |||
| { type = fileModeType; | |||
| default = "u+rx,g+x,o+x"; | |||
| default = "u+x,g+x,o+x"; | |||
| example = "a+rx"; | |||
| description = lib.mdDoc '' | |||
| The permissions of the wrapper program. The format is that of a | |||
There was a problem hiding this comment.
This is no longer strictly true, right? Maybe at least change the example?
|
|
||
| // Read the capabilities set on the wrapper and raise them in to | ||
| // the ambient set so the program we're wrapping receives the | ||
| // capabilities too! | ||
| if (make_caps_ambient(self_path) != 0) { | ||
| free(self_path); | ||
| if (make_caps_ambient(exe_fd) != 0) { |
There was a problem hiding this comment.
Hm, if we allow just-execute under no-new-priv, should we just log a warning here and go on anyway?
There was a problem hiding this comment.
According to capabilities(7) no, because we might have raised some, but not all the capabilities we wanted raised.
There was a problem hiding this comment.
Well, wouldn't it be more consistent with the other case to go on and see if the capabilities we can get are enough anyway for the task the user wants, though?
There was a problem hiding this comment.
Maybe. However, the documentation of capabilitysets is very confusing and claims that it's somehow important not to run things with subsets of capabilities they were intended to run with. I would like to not change the behaviour here until we understand why capabilities(7) makes such statements.
There was a problem hiding this comment.
Hmmm. Wording looks generic in the «race-condition-like-assumption-breaking-style»…
There was a problem hiding this comment.
You whoever plays with a system to make a point to be honest (i.e. you do have root to setup a silly situation with containers and setcap and seccomp and whatever else should not be mixed, and then as a non-root user you do something you are not supposed to be able to do!)
It just looks like the same invariants are enforced on execve and on changing the capability sets, but I did not look carefully so I definitely missed something (the question is whether it makes any difference).
If we consider things like SGID + setcap and having one but not the other applied confuses the wrapped executable, we should also fail on setuid/setgid failure, no?
There was a problem hiding this comment.
If we consider things like SGID + setcap and having one but not the other applied confuses the wrapped executable, we should also fail on setuid/setgid failure, no?
No. There is no way to partially setgid (or to setuid-but-not-setgid or v.v.). In contrast that's very much not obvious with capabilities.
E:
It just looks like the same invariants are enforced on execve and on changing the capability sets, but I did not look carefully so I definitely missed something (the question is whether it makes any difference).
I assume you mean execve of a binary with filecaps. If so, then even the interface is different (according to docs): there is supposedly no effective capset on files, but only a single effective bit that applies to all caps.
There was a problem hiding this comment.
There is no way to «partially setgid», but setcap-without-setgid might still be a partial application of intended security context.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah this exclusivity is still there! Good, I guess
If so, then even the interface is different (according to docs): there is supposedly no effective capset on files, but only a single effective bit that applies to all caps.
Hm, this part is not that interesting (different interfaces ending up with same conditions are fine), but there is indeed a capability-by-capability operation involved in the wrapper, so partial change is possible, you are right.
|
If you decide it is better to not act on my questions, that's fine too — I just wanted these things not to be forgotten by accident, consciously judged irrelevant is OK |
stale
bot
added
the
2.status: stale
This builds on top of this PR: #199599
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/)nixos/doc/manual/md-to-db.shto update generated release notes