Re: [PATCH] ioctl_tty.2: add TIOCGPTPEER documentation

[Christian Brauner]

On Mon, Nov 20, 2017 at 11:20:13AM +0100, Michael Kerrisk (man-pages) wrote:
> On 08/16/2017 07:14 PM, Eric W. Biederman wrote:
> > Aleksa Sarai <asarai@xxxxxxx> writes:
> > 
> >>> A couple of things to note on the bigger picture.
> >>>
> >>> The glibc library on all distributions has been changed to not have a
> >>> setuid binary pt_chown, that uses ptsname.  This was the primary fix
> >>> for the security issue.
> >>>
> >>> The behavior of opening /dev/ptmx has been changed to perform a path
> >>> lookup relative to the location of /dev/ptmx of  ./pts/ptmx and open
> >>> it it is a devpts filesystem and to fail otherwise.    This further
> >>> makes it hard to confuse userspace this way as /dev/ptmx always
> >>> corresponds to /dev/pts/ptmx.  Even in chroots and in other mount
> >>> namespaces.
> >>
> >> I have a feeling that there might be a way to trick glibc if you use
> >> FUSE, but I haven't actually tried to create a PoC for it. Fair point
> >> though.
> > 
> > To trick glibc fuse would have to be mounted somewhere on /dev.
> > 
> >>> That makes TIOCGPTPEER a very nice addition, but not something people
> >>> have to scramble to use to ensure their system is secure.  As a hostile
> >>> environment now has to work very hard to confuse the existing mechanisms.
> >>
> >> There are usecases where you simply need TIOCGPTPEER, and no other
> >> userspace alternative will do, but maybe if we modified the paragraph
> >> to read (as suggested):
> >>
> >>   Security-conscious programs interacting with namespaces may
> >>   wish  to  use  this  operation rather than open(2) with the
> >>   pathname returned by ptsname(3).
> >>
> >> This would clarify that there are usecases where you need this
> >> particular feature, without saying causing people to panic over
> >> inaccurate claims of glibc being broken. Does that sound better?
> > 
> > I think your original words sounded fine.  I would even go for new
> > programs may want to use the new ioctl as it fundamentally less racy
> > and more of what is actually trying to be implemented with the userspace
> > pieces.
> > 
> > I just wanted to point out that TIOCGPTPEER while being the interface
> > that it would have been nice had we had since the beginning (and would
> > have avoided all of the problems) is actually not something we need to
> > scramble and use it is just a very nice to have.  As the immediate
> > issues have been fixed in other ways.    It was not clear to me from the
> > other discussions if you and Michael Kerrisk were aware of the
> > mitigations that had been made to address the security issue.
> 
> So, my takeaway is that we leave this text:
> 
>               Security-conscious programs interacting with namespaces may
>               wish  to  use  this  operation rather than open(2) with the
>               pathname returned by ptsname(3), and similar library  funcâ
>               tions that have insecure APIs.  (For example, confusion can
>               occur in some cases using ptsname(3) with a pathname  where
>               a  devpts  filesystem has been mounted in a different mount
>               namespace.)

This sounds fine to me. I probably should point out that I wrote a patch for
glibc to use TIOCGPTPEER based slave fd allocation when supported and only use
the insecure way as a fallback. I've pushed this to glibc master on 8 October.
That means it is still in our current glibc development cycle and will thus be
available in glibc 2.27. The relevant commit hash is
645ac9aaf89e3311949828546df6334322f48933 and the link to the diff
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=645ac9aaf89e3311949828546df6334322f48933;hp=98e0742024d4c13c08a6076b3d119c250e7d0118

At least for us at glibc this should mean that *most* insecure
path-based-operations have been eliminated since other path-based bits in these
codepaths are basically no-ops by now. That being said glibc needs to be
backward compatible and so will (see comment above) execute path-based fallback
code if the new ioctl() fails. So if you really^2 care about using TIOCGPTPEER
you should do the ioctl() dance yourself.

(Fwiw, I'm not directly involved with Musl I've written a patch for them as well
but Rich had some reservations and I haven't been able to pick that patch back
up yet.)

Christian

> 
> as is?
> 
> > The change to the behavior of /dev/ptmx may need to be documented
> > somewhere.   I am not certain if anything has been documented since
> > devpts has started allowing multiple mounts.
> 
> Eric can you say more about this. When did this change occur?
> What is the model: mount devpts once in each mount namespace?
> 
> Cheers,
> 
> Michael
> 
> 
> -- 
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
> _______________________________________________
> Containers mailing list
> Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/containers

Attachment: signature.asc
Description: PGP signature