Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC 0097] Unset read permission bit on /nix/store for other users #97

Merged
merged 12 commits into from
Feb 9, 2022

Conversation

L-as
Copy link
Member

@L-as L-as commented Jul 4, 2021

This is effectively equal to chmod o-r /nix/store.

Rendered

rfcs/0097-no-read-store-dir.md Outdated Show resolved Hide resolved
rfcs/0097-no-read-store-dir.md Outdated Show resolved Hide resolved
@L-as
Copy link
Member Author

L-as commented Jul 4, 2021

Fixed the parts about usage activity and added the drawback about tools needing more permissions.

rfcs/0097-no-read-store-dir.md Outdated Show resolved Hide resolved
rfcs/0097-no-read-store-dir.md Show resolved Hide resolved
@L-as
Copy link
Member Author

L-as commented Jul 5, 2021

I rewrote a lot of the text.

@edolstra
Copy link
Member

edolstra commented Jul 6, 2021

I once tried removing read permission for the nixbld group (NixOS/nixpkgs@224d0d5) because it gives some of the advantages of build sandboxing without needing a mount namespace. Unfortunately it broke our VM tests but that should be fixable.

I'm not opposed to this proposal, but it does have a "security through obscurity" vibe. Currently, you shouldn't store secrets in the store, and because Unix software has not traditionally treated paths as secrets, it would be dangerous to rely on the secrecy of a path name for access control.

Note that currently any user that has access to the Nix daemon socket can discover all store paths by doing nix path-info --all, or discover "secret" parts of a closure through nix path-info -r (e.g. if the /run/current-system closure contains a configuration file with a password). Of course, ideally, properly sandboxed services shouldn't have access to the daemon socket. (And we should probably make the daemon only accessible by the users group by default...)

@L-as
Copy link
Member Author

L-as commented Jul 6, 2021

Thanks, the part about /run/current-system is expected though, since the expectation is that you'd be able to access every store path that is mentioned and its dependencies.

I get why you feel it has a "security through obscurity" vibe, since it does depend on hashes being secret.
The fundamental difference between a sandbox that uses something like the systemd-confinement.nix module and one that
just expects the store to be 1771, is that in the latter case, you can not trivially compute all store paths
that will be accessed, and that can be the source of unease, since it's harder to prove that some path isn't accessed
from inside the sandbox.

However, I think it's important to take into account that making this change won't worsen security, since
it doesn't force anyone to drop the use of alternative security measures. It is objectively a net improvement in
security, and essentially I think we need to consider whether it's worth the effort, which IMO is not a lot, since
it's just changing a few numbers, adding a few calls to chmod, and possibly some other small work.

@kevincox
Copy link
Contributor

kevincox commented Jul 6, 2021

I think the key concern is: because Unix software has not traditionally treated paths as secrets.

If unix had always treated paths as a capability security mechanism kernel and software developers would design new interfaces and patches quite differently. With the current state of paths generally being considered mostly harmless to share with the entire system the risk of leaks is high, if not current interfaces then ones that may appear in the future. And without a good way to detect these leaks it is a very real threat to the security of sandboxes relying on "capability-based" /nix/store security.

Again, I don't think this is a blocker but I don't think this RFC is complete without thinking through the security risks that are present in this approach compared to options such as mounting only the needed paths that this RFC is suggesting could be replaced by this.

@L-as
Copy link
Member Author

L-as commented Jul 6, 2021

I made changes to the RFC about the drawbacks of depending on this for sandboxing and fixed some other things.

@spacekookie spacekookie added the status: open for nominations Open for shepherding team nominations label Jul 8, 2021
@spacekookie
Copy link
Member

Opening this RFC for nominations. Can nominated shepherds please respond whether they accept?

@edolstra
Copy link
Member

@kevincox @7c6f434c Are you interested in shepherding this RFC?

Also nominating myself.

@kevincox
Copy link
Contributor

I can be a shepherd.

@Mic92
Copy link
Member

Mic92 commented Aug 12, 2021

We still need one more shepherd here.

@7c6f434c
Copy link
Member

7c6f434c commented Jan 6, 2022

@L-as do you want to comment on questions by @Profpatsch or maybe update some writing to improve readability?

In particular, one thing maybe indeed we need to state clearly is that any changes in the name of implementing this RFC should create no new test failures (and if some VM tests somehow end up broken, the burden of fixing things or delaying the offending part is on the implementation PR)

@edolstra what is your current opinion on the current state of the RFC?

@L-as
Copy link
Member Author

L-as commented Jan 7, 2022

I feel that your response is good enough. Not breaking existing tests is I think not worth mentioning. Any change should of course not break tests, no matter what that change is.

@7c6f434c
Copy link
Member

7c6f434c commented Jan 7, 2022 via email

@edolstra
Copy link
Member

@7c6f434c Yeah I'm 👍 on FCP/accept.

@spacekookie spacekookie added status: FCP in Final Comment Period and removed status: in discussion labels Jan 26, 2022
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/rfc-0097-fcp-unset-read-permission-bit-on-nix-store-for-other-users/17351/1

@tomberek
Copy link
Contributor

How does this interact with https://github.com/edolstra/nix/commits/acls?

@edolstra
Copy link
Member

@tomberek It shouldn't have an effect, AFAIK ACL paths will work fine in a non-world-readable store.

`/run/current-system`, and many other places which reveal your
system's closure, making this permission change an insufficient solution for
security in many cases. This, however, is also entirely optional and is not
the default in any way.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that at least some filesystems are vulnerable to timing attacks, probing paths with stat to build prefixes of subdirectory paths until the full path is recovered; using the x bit to recover r for all paths, including ones that aren't running.

Nonetheless, it's good for practical hermeticity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we hope that people either look at the list and start, like you, name extra leak vectors, or trust «insufficient for security» claim. It would be interesting to have a survey of path leak vectors, but this RFC is shorter than any useful survey like that…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Any list we try to create in this RFC is likely incomplete so it is better to just state that as an assumption and accept it for the rationale here. If someone wants to prove that the list of leaks is empty (and expected to stay that way) they can propose a follow-up RFC to rely on that fact.

Comment on lines +64 to +65
If a user on a non-NixOS platform mistakenly sets the permissions for `/nix/store` to
something undesirable, it won't be reverted by Nix automatically.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not restrict it to sensible or at least not terrible values? Seems quite feasible and the allow list could be extended if we discover a new use case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this «undesirable» includes the idea that most likely unfortunate mistakes are someon else's «just as planned» (I am not even sure 0000 is always useless; hmmm, now I am tempted to try once this is merged and trying is easy)

@lheckemann lheckemann merged commit 331380b into NixOS:master Feb 9, 2022
@asymmetric
Copy link
Contributor

Has this ever been implemented? When I look at one of the lines mentioned in the RFC, it hasn't been changed since.

# Future work
[future]: #future-work

In the future we likely want to reduce the default permissions for `/nix/store` as much as possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes I use tab completion on a store path; saves some mousing when I fail to copy a path completely, or sometimes typing three or four letters of the hash is easy enough.

@L-as
Copy link
Member Author

L-as commented Mar 18, 2023

I realised it wasn't actually that useful due to 7c6f434c's comments about being able to query all paths known from Hydra. Also, logs leak paths. Slightly better security, but only slightly.

@roberth
Copy link
Member

roberth commented Mar 18, 2023

Has this ever been implemented?

I don't think an RFC was strictly necessary for this (potential?) change. It'd be fine to introduce it as an opt-in hardening option and then perhaps create the RFC to call attention to it? Maybe I don't see the whole history though.

EDIT: simultaneous reply by L-as explains it.

@L-as
Copy link
Member Author

L-as commented Mar 18, 2023

Yeah looking back it didn't need an RFC, but if anyone implements this at least it's decided that it will be merged. It's a very low cost change.

@L-as L-as deleted the no-read-store-dir branch March 18, 2023 12:18
@infinisil infinisil added status: accepted and removed status: FCP in Final Comment Period labels Aug 23, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixcon-governance-workshop/32705/9

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/musing-on-store-permissions/33695/2

KAction pushed a commit to KAction/rfcs that referenced this pull request Apr 13, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/is-it-possible-to-prevent-configuration-nix-from-being-read-by-unprivileged-users/52175/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.