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.

Sign up for GitHub

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

Jump to bottom

setuid-wrapper activation: Approximate atomicity #18126

Closed
wants to merge 1 commit into from
Closed

setuid-wrapper activation: Approximate atomicity #18126

wants to merge 1 commit into from
+58 −7

Conversation

shlevy
Copy link
Member

@shlevy shlevy commented Aug 30, 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 #18124

@mention-bot
Copy link

@shlevy, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @MarcWeber and @peti to be potential reviewers

@shlevy
Copy link
Member Author

shlevy commented Aug 30, 2016

@domenkozar please test, I don't have a working 16.09 system yet.

domenkozar
domenkozar reviewed Aug 30, 2016
View reviewed changes
nixos/modules/security/setuid-wrappers.nix
@@ -17,6 +17,12 @@ let
'';
};

setuid-swap - pkgs.runCommand "setuid-swap" {} ''
Copy link
Member

Choose a reason for hiding this comment

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

should be s/-/=/

Copy link
Member Author

Choose a reason for hiding this comment

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

Darn, too slow!

@shlevy shlevy force-pushed the setuid-wrapper-almost-atomic branch from 1ef9f95 to be6f5d0 Compare August 30, 2016 14:17
domenkozar
domenkozar reviewed Aug 30, 2016
View reviewed changes
nixos/modules/security/setuid-swap.c
@@ -0,0 +1,36 @@
#define _GNU_SOURCE
Copy link
Member

Choose a reason for hiding this comment

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

(nitpick) Missing header comment explaining what this source does

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, normally I'd agree, but IMO this is widespread enough not to need it. From https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fGNU_005fSOURCE:

If you define this macro, everything is included: ISO C89, ISO C99, POSIX.1, POSIX.2, BSD, SVID, X/Open, LFS, and GNU extensions. In the cases where POSIX.1 conflicts with BSD, the POSIX definitions take precedence.

In other words, "this is non-portable, give me all the extensions"

Copy link
Member

Choose a reason for hiding this comment

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

No, I meant for the whole file, would be nice to have a description what these 36 lines do :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, OK, incoming

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed

@domenkozar domenkozar added this to the 16.09 milestone Aug 30, 2016
@domenkozar domenkozar self-assigned this Aug 30, 2016
@shlevy shlevy force-pushed the setuid-wrapper-almost-atomic branch from be6f5d0 to 612b1a6 Compare August 30, 2016 14:29
@domenkozar
Copy link
Member

domenkozar commented Aug 30, 2016 •
edited
Loading

I fear this will need a migration path also:

...
dummy> chmod: cannot access '/var/setuid-wrappers-new/polkit-agent-helper-1': No such file or directory
dummy> chown: cannot access '/var/setuid-wrappers-new/polkit-agent-helper-1': No such file or directory
dummy> chmod: cannot access '/var/setuid-wrappers-new/polkit-agent-helper-1': No such file or directory
dummy> cp: cannot create regular file '/var/setuid-wrappers-new/unix_chkpwd': No such file or directory
dummy> /nix/store/7jcj13r1a9r14436z6lmimxr0lhqi3lg-nixos-system-dummy-16.09.git.612b1a6/activate: line 269: /var/setuid-wrappers-new/unix_chkpwd.real: No such file or directory
dummy> chmod: cannot access '/var/setuid-wrappers-new/unix_chkpwd': No such file or directory
dummy> chown: cannot access '/var/setuid-wrappers-new/unix_chkpwd': No such file or directory
dummy> chmod: cannot access '/var/setuid-wrappers-new/unix_chkpwd': No such file or directory
dummy> swapping /var/setuid-wrappers-new and /var/setuid-wrappers: No such file or directory

@domenkozar
Copy link
Member

Oh ${wrapperDir}-new needs to mkdir-ed first probably

@shlevy shlevy force-pushed the setuid-wrapper-almost-atomic branch from 612b1a6 to d8f14b8 Compare August 30, 2016 16:13
@shlevy
Copy link
Member Author

shlevy commented Aug 30, 2016

Fixed

@domenkozar
Copy link
Member

dummy> swapping /var/setuid-wrappers-new and /var/setuid-wrappers: Device or resource busy

@shlevy shlevy force-pushed the setuid-wrapper-almost-atomic branch from d8f14b8 to 21e5d51 Compare August 31, 2016 09:04
@shlevy
Copy link
Member Author

shlevy commented Aug 31, 2016

Hmm I wonder if that's because you're running under sudo and so one of the files is open? Anyway, pushed a fix.

@shlevy
setuid-wrapper activation: Approximate atomicity
14e5cde 
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 shlevy force-pushed the setuid-wrapper-almost-atomic branch from 21e5d51 to 14e5cde Compare August 31, 2016 09:15
@domenkozar
Copy link
Member

domenkozar commented Aug 31, 2016 •
edited
Loading

@shlevy I'm using what ever nixops is using.

Next error: dummy> moving /var/setuid-wrappers-new to /var/setuid-wrappers-tmp: Directory not empty

[root@dummy:~]# ls /var/setuid-wrappers-tmp
chfn                            newgrp          ping6.real                  su
chfn.real                       newgrp.real     ping.real                   sudo
dbus-daemon-launch-helper       newuidmap       pkexec                      sudoedit
dbus-daemon-launch-helper.real  newuidmap.real  pkexec.real                 sudoedit.real
fusermount                      passwd          polkit-agent-helper-1       sudo.real
fusermount.real                 passwd.real     polkit-agent-helper-1.real  su.real
newgidmap                       ping            sg                          unix_chkpwd
newgidmap.real                  ping6           sg.real                     unix_chkpwd.real

@shlevy
Copy link
Member Author

shlevy commented Aug 31, 2016

Hmm, what behavior do we want here, when there's an old tmp hanging around? Just clear it unconditionally? Keep one backup if it exists?

@domenkozar
Copy link
Member

@shlevy so that would happen only in case of failure during the activationScript?

I'd just error out and instruct to delete it manually since it's hard to know what happened.

@edolstra
Copy link
Member

Why not make /var/setuid-wrappers a symlink? Then it can be replaced atomically without requiring bleeding edge kernel features. Also, I've wanted to put /var/setuid-wrappers on a tmpfs for a long time anyway (so all other filesystems can be mounted nosuid), so we could just make it a symlink to somewhere on /run or some other tmpfs.

@shlevy
Copy link
Member Author

shlevy commented Aug 31, 2016

Originally I had the impression that setuid scripts living in a symlink dir was dangerous, but I misunderstood that issue. So will close this, go @edolstra's route

@shlevy shlevy closed this Aug 31, 2016
@domenkozar
Copy link
Member

domenkozar commented Sep 1, 2016 •
edited
Loading

Moved the discussion to #18124

@domenkozar domenkozar reopened this Sep 1, 2016
@domenkozar domenkozar closed this Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@domenkozar domenkozar

Labels
None yet
Projects
None yet
Milestone
16.09
Development

Successfully merging this pull request may close these issues.
4 participants
@shlevy @mention-bot @domenkozar @edolstra