nixos: setuid-wrapper: simplify readlink logic #35893

shlevy · 2018-02-27T18:44:50Z

No description provided.

shlevy · 2018-03-07T10:19:12Z

Would love some additional eyes on this... @ixmatus can you take a look?

ixmatus · 2018-03-07T15:22:46Z

nixos/modules/security/wrappers/wrapper.c

-    // not positive it's safe...
-    char selfPath[PATH_MAX];
-    int selfPathSize = readlink("/proc/self/exe", selfPath, sizeof(selfPath));
+    int selfExe = open("/proc/self/exe", O_PATH | O_CLOEXEC | O_NOFOLLOW);


I understand why you're opening with O_PATH | O_CLOEXEC but I'm slightly confused by O_NOFOLLOW which prevents opening a symbolic link, which /proc/self/exe is, no?

@ixmatus O_NOFOLLOW prevents following the symlink. So when we later fstat and readlinkat we address the symlink itself, not the target.

From open(2), the section on O_PATH

If pathname is a symbolic link and the O_NOFOLLOW flag is also specified, then the call returns a file descriptor referring to the symbolic link.

@shlevy ah! That was the piece I was missing. Thank you for explaining.

ixmatus · 2018-03-07T15:27:23Z

nixos/modules/security/wrappers/wrapper.c

-    char selfPath[PATH_MAX];
-    int selfPathSize = readlink("/proc/self/exe", selfPath, sizeof(selfPath));
+    int selfExe = open("/proc/self/exe", O_PATH | O_CLOEXEC | O_NOFOLLOW);
+    assert(selfExe != -1);


One of my todos after adding setcap wrapper support that got lost in the cracks was to improve error messaging in the wrapper program. What do you think about checking the return and printing the error code to the user? Or perhaps we can do that if the debug env var is set? I think it would be useful if we did that.

Don't consider this a blocking comment, if you agree, we can address it in a refactoring-pass patch.

I do agree, yes. No need for a debug env var I think, the error path is allowed to be slow :)

@shlevy we already support a WRAPPER_DEBUG env var in this program code, so we could use that if you felt that was a slightly nicer way to surface these wrapper-specific errors to the user.

For example, you can turn it on like this:

$ WRAPPER_DEBUG=1 ping google.com cap_setpcap in set, skipping it raised cap_net_raw+p into the Ambient capability set PING google.com(iad23s42-in-x200e.1e100.net (2607:f8b0:4004:80c::200e)) 56 data bytes 64 bytes from iad23s42-in-x200e.1e100.net (2607:f8b0:4004:80c::200e): icmp_seq=1 ttl=51 time=40.1 ms 64 bytes from iad23s42-in-x200e.1e100.net (2607:f8b0:4004:80c::200e): icmp_seq=2 ttl=51 time=39.9 ms

I think WRAPPER_DEBUG makes sense for debug info, but on the error path I don't think it's necessary. Especially since it's often too late to turn on WRAPPER_DEBUG when you hit an issue 😄

Yeah, that is a good point.

ixmatus · 2018-03-07T15:30:08Z

nixos/modules/security/wrappers/wrapper.c

+    struct stat st;
+    assert(fstat(selfExe, &st) != -1);
+    size_t selfPathCap = st.st_size + 1;
+    // Purposefully leak this, it's not going to be around for long...


By "it's not going to be around for long..." you mean that the operating system will clean this up once the program exits? What is the purpose of purposefully leaking (sorry if I'm being thick)?

@ixmatus Our last use of selfPath is just before execve. If that succeeds, the memory will be freed. If it fails, it will be freed when we exit a few lines later. The only reason to explicitly call free would be to satisfy humans checking for good coding practices, and the explicit comment is easier to find than scanning for the matching free IMO. But happy to throw in a free if that would be better.

The "purposefully" bit is just to say "I know this is leaked, it's not an error" for reviewers :)

@shlevy oh I understand. I think being explicit with a free would be clearer than being implicit and explaining that with a comment.

ixmatus · 2018-03-07T15:36:46Z

nixos/modules/security/wrappers/wrapper.c

+    assert(selfExe != -1);
+    struct stat st;
+    assert(fstat(selfExe, &st) != -1);
+    size_t selfPathCap = st.st_size + 1;


I like this, I think it's better than the PATH_MAX that I was pretty unhappy about to begin with.

ixmatus · 2018-03-07T15:45:39Z

nixos/modules/security/wrappers/wrapper.c

-    // contents are being truncated.
-    assert(selfPathSize < sizeof(selfPath));
+    // This probably isn't possible with readlinkat following fstat, but not hard to check...
+    assert(selfPathSize < selfPathCap);


I agree, given that we are sizing selfPathCap correctly this code path is unlikely to be hit since we do not need to be concerned about truncation. I also think it is fine to leave the check in here.

However, would it be too much trouble to explain this a bit more in the comment?

@ixmatus Sure, will do.

ixmatus · 2018-03-07T15:49:52Z

Looking good, thanks for doing this. I also recommend @7c6f434c provide a review too, he had good feedback on my C code changes for the setcapwrapper work in PR #16654.

shlevy · 2018-03-07T15:53:12Z

@ixmatus Updated based on feedback.

ixmatus

@shlevy has addressed my feedback and some confusion with explanations in the PR comments and additional comments in the code that I also think will help future authors understand this code.

LGTM though I recommend waiting for one more qualified reviewer before merging.

ixmatus · 2018-03-07T15:57:07Z

nixos/modules/security/wrappers/wrapper.c

-    // not positive it's safe...
-    char selfPath[PATH_MAX];
-    int selfPathSize = readlink("/proc/self/exe", selfPath, sizeof(selfPath));
+    // O_PATH | O_NOFOLLOW gives us a fd pointint to the symlink


Nit: s/pointint/pointing

😄 Caught this simultaneously

shlevy · 2018-03-07T16:46:18Z

Thanks for taking such a detailed look!

7c6f434c · 2018-03-07T16:50:34Z

nixos/modules/security/wrappers/wrapper.c

+    // between opening a symlink fd and readlinkat on it, so this is
+    // probably not necessary. Doubly so since this is /proc/self/exe,
+    // not a normal symlink. But it's trivial to check.
+    assert(selfPathSize < selfPathCap);


Shouldn't this always fail? I mean, selfPathSize is the number of bytes put into the buffer, terminating-zero included, and selfPathCap is the size without terminating zero, plus 1.

No, selfPathSize is the result of readlinkat which also lacks the 0.

I'll update the comments with this though.

Just in case there were not enough ways to start a heap walk… Missed that, yes, my bad.

shlevy · 2018-03-07T16:58:24Z

@GrahamcOfBorg eval

coreyoconnor · 2018-03-07T21:03:51Z

Receiving a failure in

nix-build --no-out-link nixos/release.nix -A tests.jenkins.x86_64-linux --show-trace

as of this patch. Error message is:

master: running command: sudo -u jenkins groups
master# sudo: /nix/store/542sjh7nc2vz3cz28ii864lif3k4h98z-wrapper.c:190: main: Assertion `selfPathSize < selfPathCap' failed.
master: exit status 134
master: must succeed: sudo -u jenkins groups | grep jenkins | grep users
master# sudo: /nix/store/542sjh7nc2vz3cz28ii864lif3k4h98z-wrapper.c:190: main: Assertion `selfPathSize < selfPathCap' failed.
master: exit status 1
master: output:
error: command `sudo -u jenkins groups | grep jenkins | grep users' did not succeed (exit code 1)
command `sudo -u jenkins groups | grep jenkins | grep users' did not succeed (exit code 1)

No further investigation has been done yet.

Revert "Merge branch 'setuid-wrapper-readlink'" This reverts commit 6dab907, reversing changes made to eab479a.

7c6f434c · 2018-03-07T21:51:39Z

Ah right, that's a kernel virtual symlink, and strace says st_size is 0.

7c6f434c · 2018-03-07T21:54:03Z

I guess starting with PATH_MAX*100 and multiplying by 10 is the way to go…

shlevy · 2018-03-07T22:02:42Z

Ack. I'll just revert :(

shlevy · 2018-03-07T22:09:45Z

a183563

7c6f434c · 2018-03-07T22:41:28Z

Given that the path is «something short» + «make it more than 255 bytes and people with ext4 will complain a lot», and PATH_MAX is actually 4096, II guess PATH_MAX is safe-ish.

shlevy · 2018-03-07T22:43:43Z

Yeah, just wanted to be cleaner ☹️

7c6f434c · 2018-03-07T22:48:02Z

For every dirty hack you want to remove, there is a dirty hack in the kernel that makes your dirty hack neccessary…

7c6f434c · 2018-03-07T22:50:12Z

Basically, realpath (3) with NULL target sounds like a way to offload the pain onto glibc…

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Feb 27, 2018

shlevy force-pushed the setuid-wrapper-readlink branch from f84eba5 to a7c8ce7 Compare February 27, 2018 22:52

ixmatus reviewed Mar 7, 2018

View reviewed changes

shlevy force-pushed the setuid-wrapper-readlink branch from a7c8ce7 to c32a14b Compare March 7, 2018 15:53

shlevy requested a review from 7c6f434c March 7, 2018 15:53

shlevy force-pushed the setuid-wrapper-readlink branch from c32a14b to 38e85b0 Compare March 7, 2018 15:58

ixmatus approved these changes Mar 7, 2018

View reviewed changes

7c6f434c reviewed Mar 7, 2018

View reviewed changes

7c6f434c approved these changes Mar 7, 2018

View reviewed changes

nixos: setuid-wrapper: simplify readlink logic

Verified

This commit was signed with the committer’s verified signature.

shlevy Shea Levy

GPG key ID: 5C0BD6957D86FE27

Verified
Learn about vigilant mode

dffa3d2

shlevy force-pushed the setuid-wrapper-readlink branch from 38e85b0 to dffa3d2 Compare March 7, 2018 16:57

shlevy merged commit dffa3d2 into NixOS:master Mar 7, 2018

shlevy deleted the setuid-wrapper-readlink branch March 7, 2018 17:49

coreyoconnor added a commit to coreyoconnor/nixpkgs that referenced this pull request Mar 7, 2018

Work around error referenced in NixOS#35893

287b250

Revert "Merge branch 'setuid-wrapper-readlink'" This reverts commit 6dab907, reversing changes made to eab479a.

nixos: setuid-wrapper: simplify readlink logic #35893

nixos: setuid-wrapper: simplify readlink logic #35893

Conversation

shlevy commented Feb 27, 2018

shlevy commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixmatus commented Mar 7, 2018

shlevy commented Mar 7, 2018

ixmatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlevy commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlevy commented Mar 7, 2018

coreyoconnor commented Mar 7, 2018

7c6f434c commented Mar 7, 2018

7c6f434c commented Mar 7, 2018

shlevy commented Mar 7, 2018

shlevy commented Mar 7, 2018

7c6f434c commented Mar 7, 2018

shlevy commented Mar 7, 2018

7c6f434c commented Mar 7, 2018

7c6f434c commented Mar 7, 2018