Revert "Adjust SetupUser to Fchown all open file descriptors so they'…

[bitglue]

…re appropriately usable by the forked process"

This reverts commit 75150acc2b2baf01a8ce5b106f226464f3c737e0.

There's no guarantee that gosu will be able to change the owner of open files. Consequently, in some environments, this causes gosu to fail with the error:

error: failed switching to "$user": fchown fds permission denied

For some examples of failures people have encountered, see https://github.com/docker-library/mongo/issues/56

Any open file descriptors may not even be files, or they may not be on a filesystem that supports changing owners, or even has any concept of owners (FAT16, for example).

File descriptors may not even be files. They may be sockets, for example.

This behavior is also quite surprising. I would not expect a utility that behaves something like sudo to modify my filesystem. Many times, I may explicitly not want a file to be owned by the target user because I want the process I'm launching to be restricted to read-only access. Consequently, this behavior could introduce security issues for some use cases.

[tianon]

I'd be open to having this ignore all errors, but I'm not sure I agree that file descriptors left open is a security issue from the perspective of gosu.

For a little bit of context, the kinds of things this feature makes possible that weren't before are cases where the process we launch closes stdin/stdout, but we want one of its children to be able to write to stdout easily.

In this case, it was mongod in --fork mode, which requires either --logpath or --syslog, but it closes stdout before passing along to either of those, so this fchowning of file descriptors is what makes something like the following possible: (which would error out previously since the 42 file descriptor wasn't usable by the mongodb user)

$ gosu mongodb mongod --fork --logpath /proc/self/fd/42 42>&1
about to fork child process, waiting until server is ready for connections.
forked process: 19
2015-10-06T13:57:26.856+0000 I JOURNAL  [initandlisten] journal dir=/data/db/journal
2015-10-06T13:57:26.890+0000 I JOURNAL  [initandlisten] recover : no journal files present, no recovery needed
2015-10-06T13:57:29.417+0000 I JOURNAL  [initandlisten] preallocateIsFaster=true 21.8
2015-10-06T13:57:31.764+0000 I JOURNAL  [initandlisten] preallocateIsFaster=true 25.74
2015-10-06T13:57:35.039+0000 I JOURNAL  [initandlisten] preallocateIsFaster=true 28.38
...

Additionally, the inspiration for fixing the problem in this way came from the inspiration for gosu itself, namely the code in libcontainer that's responsible for switching users and execing the actual process: https://github.com/opencontainers/runc/pull/280/files (although in their case they only need to run it on stdin, stderr, and stdout because they close all other open file descriptors, where we explicitly leave them open).

One of my main problems with gosu not doing this is that there isn't an easy way for a script to do it otherwise -- there's no fchown command we can use to even attempt it. :confused:

[pwaller]

What happens when you do gosu mongodb echo > /dev/null? Does this change ownership of /dev/null to mongodb? It seems to here.

[tianon]

Crap, ok, that's definitely an issue. :cry:

[bitglue]

I think the reason https://github.com/opencontainers/runc/pull/280/files is acceptable is in that case, docker owns the file being fchowned. So docker can know it's not a problem.

But what if I do this?

#!/bin/bash
exec 3< /etc/shadow
gosu mongodb echo 'hi mom'

I guess that's cool and all, but now /etc/shadow is owned by mongodb. Whoops!

[tianon]

Maybe it's time to finally break down and add some flags parsing so this behavior can be optional. :cry:

[bitglue]

I think better would be to just implement gofchown or whatever as a separate tool. There are a few edge cases where you both need to change the ownership of a file that doesn't have a name (or that yu don't know) and you need to switch users, but generally I don't see a reason for implicit file-ownership-changing behavior in a tool for switching users. To the contrary, generally when I'm switching users I'm trying to shed privileges, not gain them.

[tianon]

Yeah, I suppose that's a fair argument. I really thought the kernel tracked file descriptor permissions separate from the underlying file permissions. :cry:

[tianon]

Thanks for bearing with me and explaining the issue. :+1:

LGTM

[bitglue]

It just occurred to me that /proc/self/fd is full of symlinks, so you might just be able skip implementing gofchown and instead do:

chown --dereference mongodb /proc/self/fd/42

[tianon]

root@19fc818b6d59:/# ls -lAFh /proc/1/fd/1
lrwx------ 1 root root 64 Oct  6 16:10 /proc/1/fd/1 -> /30
root@19fc818b6d59:/# chown -h mongodb:mongodb /proc/1/fd/1
root@19fc818b6d59:/# ls -lAFh /proc/1/fd/1
lrwx------ 1 root root 64 Oct  6 16:10 /proc/1/fd/1 -> /30
root@19fc818b6d59:/# chown mongodb:mongodb /proc/1/fd/1
root@19fc818b6d59:/# ls -lAFh /proc/1/fd/1
lrwx------ 1 root root 64 Oct  6 16:10 /proc/1/fd/1 -> /30
root@19fc818b6d59:/# chown --dereference mongodb:mongodb /proc/1/fd/1
root@19fc818b6d59:/# ls -lAFh /proc/1/fd/1
lrwx------ 1 root root 64 Oct  6 16:10 /proc/1/fd/1 -> /30

:cry:

[bitglue]

Are you sure it's not working? Rarely are the permissions of a symlink relevant, because when you try to open it, it's the permissions of the referenced file that are used.

$ docker run -ti debian bash
root@44c85532e278:/# ls -l /proc/1/fd
total 0
lrwx------ 1 root root 64 Oct  6 16:21 0 -> /2
lrwx------ 1 root root 64 Oct  6 16:21 1 -> /2
lrwx------ 1 root root 64 Oct  6 16:21 2 -> /2
lrwx------ 1 root root 64 Oct  6 16:21 255 -> /2
root@44c85532e278:/# ls -l --dereference /proc/1/fd
total 0
crw------- 1 root root 136, 2 Oct  6  2015 0
crw------- 1 root root 136, 2 Oct  6  2015 1
crw------- 1 root root 136, 2 Oct  6  2015 2
crw------- 1 root root 136, 2 Oct  6  2015 255
root@44c85532e278:/# chown --dereference nobody /proc/1/fd/1
root@44c85532e278:/# ls -l --dereference /proc/1/fd/
total 0
crw------- 1 nobody root 136, 2 Oct  6 16:22 0
crw------- 1 nobody root 136, 2 Oct  6 16:22 1
crw------- 1 nobody root 136, 2 Oct  6 16:22 2
crw------- 1 nobody root 136, 2 Oct  6 16:22 255

[bitglue]

There is for sure some weirdness here, because ls is telling me that /proc/1/fd references /2, and /2 isn't even a file. If you look on not a container, usually it will reference /dev/pts/0 or something like that.

But, chown --dereference seems to work anyway. I guess there's something funny happening with /proc in docker. Maybe because the things being referenced exist outside the container?

[tianon]

Yeah, I think that's exactly what's happening there and our poor procfs is horribly confused. :smile:

Well, if that works, I'm happy enough. :smile:

[pwaller]

@tianon You just hit https://github.com/docker/docker/issues/11462, the bane of my existence. :wink:

[tianon]

Heh, sounds about right :sweat_smile:

[tianon]

https://github.com/tianon/gosu/releases/tag/1.6 -- sorry for the trouble. :disappointed: