nixos/security/wrappers: simplifications and a fix for #98863 (respin of #199599) #251770
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Wrappers generate pieces of apparmor policies for inclusion, which are used only in a single place in nixpkgs, for `ping`. They are built only if apparmor is enabled. This change causes the test to test: - that the apparmor includes can be generated, - that `ping` works with apparmor enabled (as the only policy that references these includes). Ideally there would be some other NixOS test that verifies that `ping` specifically works. Sadly, there isn't one.
…or policy fragment for each wrapper This change includes some stuff (e.g. reading of the `.real` file, execution of the wrapper's target) that belongs to the apparmor policy of the wrapper. This necessitates making them distinct for each wrapper. The main reason for this change is as a preparation for making each wrapper be a distinct binary.
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. This change removes part of the test that is obsoleted by the removal of `.real` files.
/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
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
10.rebuild-linux: 1-10
labels
|
@ofborg test sudo wrappers apparmor |
delroth
approved these changes
Aug 27, 2023
13 tasks
wegank
added
the
12.approvals: 1
|
In absence of further comments and since most discussion already happened on the original PR, I'll go and merge this tomorrow. |
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Simplification of SUID wrappers, which should make us care less about weird subtle properties of their environment and removal of assertions about those subtle properties. This (partially) fixes #98863: only for S[UG]ID binaries, so I'm also adding a regression test for that.
This is a respin of the previous PR, which got reverted due to breakage in apparmor. This PR fixes that breakage and adds a very rudimentary test for that.
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/)Reviewers from the previous PR: @rnhmjoj @Mic92 @delroth @xfix
@ju1m because he seems to have authored relevant parts of apparmor configuration