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.

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

[merged] Call setsid() before executing sandboxed code (CVE-2017-5226) #143

Closed
wants to merge 1 commit into from

Conversation

smcv
Copy link
Collaborator

@smcv smcv commented Jan 9, 2017

This prevents the sandboxed code from getting a controlling tty,
which in turn prevents it from accessing the TIOCSTI ioctl and hence
faking terminal input.

Fixes: #142


Note that this breaks shell job control:

user@host:~% ~/src/bubblewrap/bwrap --ro-bind / / --chdir / --dev /dev bash
bash: cannot set terminal process group (-1): Inappropriate ioctl for device
bash: no job control in this shell
user@host:~$ 

I think that's an acceptable sacrifice, but if you disagree, the other mechanism seen in setuid things seems to be non-trivial use of seccomp: util-linux/util-linux@8e49250

This prevents the sandboxed code from getting a controlling tty,
which in turn prevents it from accessing the TIOCSTI ioctl and hence
faking terminal input.

Fixes: containers#142
@rh-atomic-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@rhatdan
Copy link
Member

rhatdan commented Jan 9, 2017

bot, add author to whitelist

@rhatdan
Copy link
Member

rhatdan commented Jan 9, 2017

bot, test pull request

@cgwalters
Copy link
Collaborator

Okay, so this affects pretty much every suid binary, and also those binaries for root to run to try to drop privileges. Basically any case where you have processes in different security domains connected to the same pty and same session. Fun. Really need some sort of "suid binary checklist".

As far as affecting functionality, one can restore job control by simply allocating a new pty - the sanity check I did was to run tmux in bwrap with this patch, which worked fine. The root cause is we're inheriting FDs to the pty outside of the namespace.

We could just tweak the demo shell script to allocate a pty I'd say.

@cgwalters
Copy link
Collaborator

Hmm, so the kernel code does:

        if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
                return -EPERM;

But is it possible for us to regain a reference to the controlling tty somehow? I tried just open(/proc/self/fd/0) and that just got EACCES, but didn't debug it further.

@cgwalters
Copy link
Collaborator

I got curious as to - why does that call to setsid() exist in util-linux? Took a bit of annotate digging through renames and such, but it leads to:

util-linux/util-linux@c6a1746 ➡️
https://bugzilla.redhat.com/show_bug.cgi?id=173008

So, this type of thing has been around a long time but not widely socialized I guess.

@cgwalters
Copy link
Collaborator

I think I'd like to do the seccomp fix in addition at some point, but this looks good. Thanks!

@rh-atomic-bot r+ 3f2f885

@rh-atomic-bot
Copy link

⌛ Testing commit 3f2f885 with merge d7fc532...

@rh-atomic-bot
Copy link

☀️ Test successful - status-redhatci
Approved by: cgwalters
Pushing d7fc532 to master...

@rh-atomic-bot rh-atomic-bot changed the title Call setsid() before executing sandboxed code (CVE-2017-5226) [merged] Call setsid() before executing sandboxed code (CVE-2017-5226) Jan 9, 2017
@smcv
Copy link
Collaborator Author

smcv commented Jan 9, 2017

But is it possible for us to regain a reference to the controlling tty somehow?

tty_ioctl(4) says no, unless the shell into which the attacker is trying to feed malicious input relinquishes its controlling terminal:

       TIOCSCTTY int arg
              Make  the given terminal the controlling terminal of the calling
              process.  The calling process must be a session leader  and  not
              have  a controlling terminal already.  For this case, arg should
              be specified as zero.

              If this terminal is already the controlling terminal of  a  dif‐
              ferent  session  group,  then the ioctl fails with EPERM, unless
              the caller has the CAP_SYS_ADMIN capability and arg equals 1, in
              which case the terminal is stolen, and all processes that had it
              as controlling terminal lose it.

@smcv smcv deleted the cve-2017-5226 branch January 9, 2017 22:35
@alexlarsson
Copy link
Collaborator

This isn't a great fix because it pretty much breaks using bwrap to create an interactive shell
For instance, if you run bwrap --bind / / sh, and then press ctrl-Zthen the "outer" shell gets unfrozen, but the "inner" shell doesn't get frozen, and both try to read terminal input at the same time.

@cgwalters
Copy link
Collaborator

Let's track regressions/issues from this via new issues?

alexlarsson added a commit to alexlarsson/bubblewrap that referenced this pull request Jan 16, 2017
The setsid() workaround of
containers#143 is problematic,
because it e.g. breaks shell job control for bubblewrap instances.
So, instead we use a seccomp approach based on:
 util-linux/util-linux@8e49250
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.
alexlarsson added a commit to alexlarsson/bubblewrap that referenced this pull request Jan 16, 2017
The setsid() workaround of
containers#143 is problematic,
because it e.g. breaks shell job control for bubblewrap instances.
So, instead we use a seccomp approach based on:
 util-linux/util-linux@8e49250
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.
alexlarsson added a commit to alexlarsson/bubblewrap that referenced this pull request Jan 16, 2017
The setsid() workaround of
containers#143 is problematic,
because it e.g. breaks shell job control for bubblewrap instances.
So, instead we use a seccomp approach based on:
 util-linux/util-linux@8e49250
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.
alexlarsson added a commit to alexlarsson/bubblewrap that referenced this pull request Jan 16, 2017
The setsid() workaround of
containers#143 is problematic,
because it e.g. breaks shell job control for bubblewrap instances.
So, instead we use a seccomp approach based on:
 util-linux/util-linux@8e49250
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 containers#147
@smcv smcv mentioned this pull request Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants