bubblewrap
bubblewrap copied to clipboard
containers
Use seccomp instead of setsid() to workaround CVE-2017-5226
The setsid() workaround of https://github.com/projectatomic/bubblewrap/pull/143 is problematic, because it e.g. breaks shell job control for bubblewrap instances. So, instead we use a seccomp approach based on: https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2 However, since we don't want to pull in any more dependencies into the setuid binary we pre-compile the seccomp code during the build.
If libseccomp is not available on your architecture, we still support the old fix with --disable-seccomp-tty-fix.
This fixes https://github.com/projectatomic/bubblewrap/issues/147
@smcv I tested this with your test.c from https://github.com/projectatomic/bubblewrap/issues/142 and it seems to work.
Hmm. But this still leaves bwrap users on other arches with that behavior, which we'd still have to then document how to work around in examples, etc.
Note that util-linux reverted their seccomp change; the commit message doesn't document extensively why though.
I'm not opposed to this though if you think it's worth it (which I guess you do having written the patch 😄 )
Its definately worth it, I've updated flatpak to the bubblewrap 0.1.6 in master, and it is completely useless on the command line. Whatever you do you end up with multiple processes reading from the terminal and you have to start a new terminal to find which one to kill. Its a complete show-stopper imho,
Are there any arches that don't have seccomp yet though? If so its probably just a matter of time before they get it.
Also, I don't see how you can work around any issues with this...
You can work around it by creating a new pty and having the shell use that as the controlling terminal. I don't have a handy one liner for this, but e.g. running tmux seems to work just fine.
You can work around the sh: cannot set terminal process group (-1): Inappropriate ioctl for device" error, but you can't work around the fact that if you e.g. run bwrap --bind / / sh, then ctrl-C then you get two shell reading the input from the same terminal with intermingled output. Trying to use flatpak for development and debugging just one day with this makes it obvious that it won't work at all.
For instance, you can't do flatpak run org.my.App and expect ctrl-C to kill it.
We could address the Ctrl-C UX by having a process outside of the container that acts as a lifecycle bind to init inside the container. Basically both watch a pipe, and if one exits, the other does.
That's just a single example though. The general issue is that it doesn't behave like a UNIX command.
I.E it doesn't get sigstop when it writer to the try when backgrounded, etc
What UNIX/commandline issues wouldn't be addressed by having an "outer init"? (I realized there's no need for a pipe, the "outer init" could just have the "container init" as a child process, and the "outer init" watches it via SIGCHILD, and container init should use PR_SET_PDEATHSIG)
Eh, SIGSTOP. Yeah, true. We'd have to go to proxying read/write and signals across. I guess really we'd end up with a userspace pty emulator, just without TIOCSTI.
We could have an outer init that proxies stdin/stdout/stderror and signals (SIGSTOP/SIGCONT, etc), but it would never quite get the right thing, because e.g. /dev/tty will not work properly inside it. It also seems very complex and easy to get wrong.
I think for /dev/tty we could argue that bwrap-using software like flatpak should implement a shell command which would inject code into the container to allocate a pty and run a shell with it.
The scope for "outer init" would be a lot smaller if we specified that we didn't support ttys (e.g. TIOCGWINSZ); basically if you couldn't run vi/emacs/tmux/etc inside.
Although, "outer init + setsid()" still seems viable for the basic "ctrl-c'able" case. Not sure how many people would initially notice the SIGSTOP/backgroundable bit.
OK so...why explicitly generate the rules at build time? (And do you agree it breaks cross compilation?) Is there other precedent for this? systemd for example doesn't seem to do this.
Because I don't want to load essentially a compiler into a setuid binary.
we could argue that bwrap-using software like flatpak should implement a shell command which would inject code into the container to allocate a pty and run a shell with it
Here is that code: xterm :-P
(or an xterm clone with lighter-weight dependencies)
It is a compiler of sorts, but it's operating on known, static trusted input. I'm not sure it's worth breaking cross builds for this, though I admit to not actively maintaining a cross-built system right now myself.
One other concern that pops to mind - we're still vulnerable without the ptrace-after-seccomp patches, which are in 4.8, but probably not backported to kernels like the CentOS7/RHEL one.
For flatpak we disable ptrace by default unless you run with -d or grant "developer" permissions.
This is also disabled with ptrace, but i believe that by itself should be ok, because the ptrace-after-seccomp issue is after ptrace has been enabled, no?
So...what one could argue here is we should really have a --disable-setsid runtime option. Then, this seccomp filter could live in flatpak, where it also knows that it has a filter to disable ptrace.
Conceptually, the CVE then isn't in bwrap - it's in any program which is using bwrap with a pty connected to separate security domains. There are other ways to fix the issue externally - for example, just don't provide a pty to the child process - if it's a background daemon, connect it to the e.g. systemd journal. Or, per above, install a seccomp filter in your software.
One argument that the setsid() invocation shouldn't have been added to bwrap at all is the fact that we support not passing --unshare-pid. If one does that it's obviously trivial to kill processes outside.
Another really important example is we support providing /home into the container, which may have ssh/etc key material...
So there's all of this "best practice" stuff that needs to live outside. Hence then, why don't we add a command line option --disable-setsid to allow programs to opt-in to disabling it?
That sounds good to me, although --disable-setsid is perhaps inverted, something like --new-session might be better (or you could just run setsid bwrap ...
The reason I phrased it as --disable-setsid is theoretically (very very theoretically...) there is some software out there relying on the fact we added setsid() in 0.1.6.
That said...I think I'm convincing myself we should do this:
- Revert the addition of
setsid()(or as you suggest make it an option - why not, it's convenient) - Change
demos/bubblewrap-shell.shto do it along with other hardening (unsharing all of the namespaces, explicitly not inheriting /home, perhaps too scrubbing the environment? (one often has auth tokens there)) - Change flatpak to add the seccomp filter, and point users at that as a best practice if you want to retain a pty
By 1) do you mean invert its meaning? Or rely on apps to use setsid(1)?