Do something about `allowNewPrivileges` setting

jade commented

2024-05-05 04:17:30 +00:00

Owner

I noticed that we have a setting allow-new-privileges which seems to be using some highly questionable seccomp APIs to disable setting NO_NEW_PRIVS. However, I am extremely dubious that this is a good idea to have as a setting, and we broke it anyway recently by redundantly explicitly setting NO_NEW_PRIVS with prctl. Also:

Disabling filterSyscalls will make this setting have no effect.
The existing NO_NEW_PRIVS thing depended on libseccomp being available, which is not always the case, so it could simply not be applied.

I am inclined to just remove the filterSyscalls and allowNewPrivileges settings altogether, since they seem like liabilities and we would like to improve the clarity in what the sandbox does. We would like to know what people are actually using these for, if anything; the use cases I can imagine are where Nix is being thoroughly broken, and it is probably a bad idea to be able to set these globally at all, rather than on individual derivations. I think my most relaxed view on these is that they should be removed from the release build and require a build option to enable; but I would rather not have the complexity of a build option for functionality I'm not convinced anyone needs.

We should probably also check that we drop ambient/inherited capabilities with cap_set_mode(CAP_MODE_NOPRIV); in the builder.

cc @k900

I noticed that we have a setting `allow-new-privileges` which seems to be using some highly questionable seccomp APIs to disable setting NO_NEW_PRIVS. However, I am extremely dubious that this is a good idea to have as a setting, and we broke it anyway recently by redundantly explicitly setting NO_NEW_PRIVS with prctl. Also: * Disabling `filterSyscalls` will make this setting have no effect. * The existing NO_NEW_PRIVS thing depended on libseccomp being available, which is not always the case, so it could simply not be applied. I am inclined to just remove the `filterSyscalls` and `allowNewPrivileges` settings altogether, since they seem like liabilities and we would like to improve the clarity in what the sandbox does. We would like to know what people are actually using these for, if anything; the use cases I can imagine are where Nix is being thoroughly broken, and it is probably a bad idea to be able to set these globally at all, rather than on individual derivations. I think my most relaxed view on these is that they should be removed from the release build and require a build option to enable; but I would rather not have the complexity of a build option for functionality I'm not convinced anyone needs. We should probably also check that we drop ambient/inherited capabilities with `cap_set_mode(CAP_MODE_NOPRIV);` in the builder. cc @k900

jade added the

stability

label 2024-05-05 04:17:30 +00:00

jade commented

2024-05-05 04:23:53 +00:00

Author

Owner

#217 note that we would kind of rather not add more unprincipled derivation settings dumped into the environment namespace of a derivation, especially if they might be unnecessary.

https://git.lix.systems/lix-project/lix/issues/217 note that we would kind of rather not add more unprincipled derivation settings dumped into the environment namespace of a derivation, especially if they might be unnecessary.

k900 commented

2024-05-05 07:50:46 +00:00

Member

My take is that we should just not allow disabling the sandbox, except maybe disabling ALL OF IT for debugging sandbox-related issues.

pennae commented

2024-05-05 13:32:29 +00:00

Owner

agreed, yeet it. looks like this was added seven years ago to fix some broken tests in nixpkgs.

if anyfew want to gain privileges they should just do it via a privsep proxy daemon passed in through extra sandbox paths. real allow-new-privileges isn't just dubious but can completely break the store at the very least, which is something we absolutely have to avoid as a builtin feature <,<

agreed, yeet it. looks like this was added *seven years ago* to fix some broken tests in nixpkgs. if anyfew want to gain privileges they should just do it via a privsep proxy daemon passed in through extra sandbox paths. *real* allow-new-privileges isn't just dubious but can completely break the store at the very least, which is something we absolutely have to avoid as a builtin feature <,<

jade commented

2024-05-05 19:43:30 +00:00

Author

Owner

Resolution: delete these two settings. Force them to always on and delete the conditionals driven by them.