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

Don't activate Nix if the user's profile is not owned by that user #3134

Closed
wants to merge 11 commits into from

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Oct 9, 2019

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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…

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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 ]

@edolstra
Copy link
Member

edolstra commented Oct 9, 2019

I think it's better to remove world writable permission, see #509. Any approach that tries to detect tampering with permissions seems inherently racy.

@grahamc
Copy link
Member Author

grahamc commented Oct 9, 2019

cc #3129 (comment)

@zimbatm
Copy link
Member

zimbatm commented Oct 9, 2019

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.

@alyssais
Copy link
Member

alyssais commented Oct 9, 2019 via email

@flokli
Copy link
Contributor

flokli commented Oct 10, 2019

With #3136 merged in, this can be closed, right?

@edolstra edolstra closed this Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants