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

setuid-wrapper activationScript is not atomic #18124

Closed
domenkozar opened this issue Aug 30, 2016 · 4 comments
Closed

setuid-wrapper activationScript is not atomic #18124

domenkozar opened this issue Aug 30, 2016 · 4 comments
Assignees
Milestone

Comments

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Aug 30, 2016

It's currently possible to cancel nixos-rebuild between deletion of previous setuid wrappers and population of new ones. And yes, I've been able to lock myself out of machine (almost, if I didn't have root ssh key).

Current implementation: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/security/setuid-wrappers.nix#L118-L120

Proposed implementation (see at the bottom of blog post): https://axialcorps.com/2013/07/03/atomically-replacing-files-and-directories/

@shlevy shlevy self-assigned this Aug 30, 2016
shlevy added a commit to shlevy/nixpkgs that referenced this issue Aug 31, 2016
This makes the replacement of the old wapper dir with the new one
atomic if the kernel and FS support RENAME_EXCHANGE, and falls back to
at least ensuring the old wrapper dir remains on the FS if interrupted
during the (now smaller) inconsistent window.

Fixes NixOS#18124
shlevy added a commit to shlevy/nixpkgs that referenced this issue Aug 31, 2016
@domenkozar domenkozar reopened this Sep 1, 2016
@domenkozar
Copy link
Member Author

@domenkozar domenkozar commented Sep 1, 2016

@edolstra @shlevy so this was failing because it's ready a tmpfs upon install:

nixos/modules/installer/tools/nixos-install.sh:mount -t tmpfs -o "mode=0755" none $mountPoint/var/setuid-wrappers

So, if we want /var/setuid-wrappers/ to be a tmpfs and a symlink that won't work.

My proposal would be to make /var/setuid-wrappers/ a tmpfs and move conents to /var/setuid-wrappers/bin which could then be a symlink. This would be a backwards incompatible change, but we can fix it in nixpkgs and document it.

Thoughts?

@domenkozar domenkozar added this to the 16.09 milestone Sep 1, 2016
@shlevy
Copy link
Member

@shlevy shlevy commented Sep 1, 2016

I think it would be better to just not have /var/setuid-wrappers be a tmpfs since we're symlinking to a tmpfs anyway.

@edolstra
Copy link
Member

@edolstra edolstra commented Sep 1, 2016

@domenkozar Well, we can just change nixos-install not to create that tmpfs, right? Not sure why it's doing that anyway...

@domenkozar
Copy link
Member Author

@domenkozar domenkozar commented Sep 1, 2016

I'll have a PR ready soon, fixing a bug atm.

domenkozar added a commit to domenkozar/nixpkgs that referenced this issue Sep 1, 2016
Before this commit updating /var/setuid-wrappers/ folder introduced
a small window where NixOS activation scripts could be terminated
and resulted into empty /var/setuid-wrappers/ folder.

That's very unfortunate because one might lose sudo binary.

Instead we use two atomic operations mv and ln (as described in
https://axialcorps.com/2013/07/03/atomically-replacing-files-and-directories/)
to achieve atomicity.

Since /var/setuid-wrappers is not a directory anymore, tmpfs mountpoints
were removed in installation scripts and in boot process.

Tested:

- upgrade /var/setuid-wrappers/ from folder to a symlink
- make sure /run/setuid-wrappers-dirs/ legacy symlink is really deleted
@domenkozar domenkozar closed this in a6670c1 Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants