Re: [PATCH 01/16] devpts: Attempting to get it right

[Eric W. Biederman]

Andy Lutomirski <luto@xxxxxxxxxxxxxx> writes:

> On Fri, Apr 15, 2016 at 8:34 AM, Eric W. Biederman
> <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> To recap the situation for those who have not been following closely.
>>
>> There are programs such as xen-create-image that run as root and setup
>> a chroot environment with:
>> "mknod dev/ptmx c 5 2"
>> "mkdir dev/pts"
>> "mount -t devpts none dev/pts"
>>
>> Which mostly works but stomps the mount options of the system /dev/pts.
>> In particular the options of "gid=5,mode=620" are lost resulting in a
>> situation where creating a new pty by opening /dev/ptmx results in
>> that pty having the wrong permissions.
>>
>> Some distributions have been working around this problem by continuing
>> to install a setuid root pt_chown binary that will be called by glibc
>> to fix the permissions.
>>
>> Maintaining a setuid root pt_chown binary is not too scary until
>> multiple instances of devpts are considered at which point it becomes
>> possible to trick the setuid root pt_chown binary into operating on the
>> wrong files and directories.  Leading to all of the things one might
>> fear when a setuid root binary goes wrong.
>>
>> The following patchset digs us out of that hole by carefully devpts and
>> /dev/ptmx in a way that does not introduce any userspace regressions,
>> while making each mount of devpts distinct (so pt_chown is unnecessary)
>> and arranging things so that enough information is available so
>> that a secure pt_chown binary is possible to write if that is ever
>> needed.
>>
>> The approach I have chosen to take is to first enhance the /dev/ptmx
>> device node to automount /dev/pts/ptmx on top of it.  This leads to a
>> simple high performance solution that allows applications such as
>> xen-create-image (that call "mknod ptmx c 5 2" and mount devpts)
>> to continue to run as before even when they are given a non-system
>> instance of devpts.
>>
>> Using automountic bind mounts of /dev/pts/ptmx results in no new
>> security cases to consider as this can already be done, and actually
>> results in a simplification of the analysis of the code.  As now all
>> opens of ptmx are of /dev/pts/ptmx.  /dev/ptmx is now just a magic
>> mountpoint that does the right thing.
>
> And what happens when someone tries to rm /dev/ptmx or unmount their
> pts instance or similar?

Unlink works (that was a trivial extension of the existing semantics).
It really didn't make sense that we honor mounts that can appear and
disappear on their own as much as we honor other mounts.

> What happens if /dev/ptmx is in a mount that
> is set to propagate elsewhere but /dev/pts was replaced by an
> unprivileged user?  (Can this happen?  I'm not sure.)

Well nothing that currently works will break in such a weird scenario.
So I call what happens in the crazy cases you are imagining well defined
obvious semantics (aka all you need to do is to apply the current
rules).  Plus you have to be pretty crazy to do something like that.
Further I am 90% certain that the propogation semantics on the /dev
directory are what define how mounts of /dev/pts and /dev/ptmx
propagate.

> This seems much weirder than the previous approach.  I think I'm
> starting to come over to Linus' view -- the magic lookup was fine, and
> I still can't think of a case where the permissions matter.  If we
> care, we can cause the /dev/ptmx magic lookup to fail if the devpts it
> finds was created with newinstance.  (After all, devpts instances
> created with newinstance *never* worked via /dev/ptmx magic.)

It isn't weirder.  The rules are actually just a little simpler.

That is my take away from playing with it and working with it.  The code
is actually really boring and just works in practice.

But please take my branch and play with it, and see if you can get it to
do something weird and magical.   It is observable enough I don't expect
you can.  But please.  If I am wrong I will happily scrap this.

Right now this looks from my vanrage point like really really obvious
semantics, that don't cause regressions, and that are easy to understand
and observe.

Eric