Don't activate Nix if the user's profile is not owned by that user #3134
Conversation
And emit a warning if the root channels file is not owned by root.
Co-Authored-By: Sarah Brofeldt <sbrofeldt@gmail.com>
| @@ -1,4 +1,6 @@ | |||
| if [ -n "$HOME" ] && [ -n "$USER" ]; then | |||
| # __safe: if it is safe to "activate" Nix. 0 is safe, 1 is unsafe | |||
| __safe=0 | |||
There was a problem hiding this comment.
That’s confusing — should the variable be called __unsafe if the values are that way around?
I see that this has been flipped around a few times over the course of this patchset…
There was a problem hiding this comment.
Yes, I was just about to comment about this.
The condition for "is this unsafe?" is a bit complicated. I found trying to flip the logic to be fraught, and I wasn't sure it was correct anymore.
If you can help with this, that would be great.
One note: this profile script gets executed at the start of every incantation, and must not fail.
There was a problem hiding this comment.
You could call it __safety and use strings: safe and unsafe. Thus the "is this safe?" check is the relatively easy-to-digest expression [ "$__safety" == safe ]
|
I think it's better to remove world writable permission, see #509. Any approach that tries to detect tampering with permissions seems inherently racy. |
|
One issue with moving the profiles to the homes is that if there are users with LUKS-encrypted homes, the nix-daemon won't be able to see the GC roots and will garbage-collect them every time. |
|
Any approach that tries to detect tampering with permissions seems inherently racy.
Not in this case, at least, because of the sticky bit. If you check
that the directory exists and is owned by you before doing anything with
it, the only way that can change from under you is if root changes the
permissions.
|
|
With #3136 merged in, this can be closed, right? |
re https://www.openwall.com/lists/oss-security/2019/10/09/4
My deepest apologies on dropping the ball on this changeset. I don't remember what happened, but I completely forgot to continue this patchset and get it released.
I believe the multi-user profile still needs to be updated.
I would like to work to improve our handling of security matters to ensure they don't get delayed like this again.