Message-ID: <CALCETrUDMucb_NDgehstZ08GZEsYCpV4ShUZfh6S25Hd2z+crQ@mail.gmail.com>
Date:	Fri, 15 Apr 2016 09:49:21 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	"H. Peter Anvin" <hpa@...or.com>, security@...ian.org,
	"security@...nel.org" <security@...nel.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	"security@...ntu.com >> security" <security@...ntu.com>,
	Peter Hurley <peter@...leysoftware.com>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	Willy Tarreau <w@....eu>,
	Aurelien Jarno <aurelien@...el32.net>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Jann Horn <jann@...jh.net>, Greg KH <greg@...ah.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jiri Slaby <jslaby@...e.com>, Florian Weimer <fw@...eb.enyo.de>
Subject: Re: [PATCH 01/16] devpts: Attempting to get it right

On Fri, Apr 15, 2016 at 8:34 AM, Eric W. Biederman
<ebiederm@...ssion.com> 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?  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.)

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.)

--Andy

Powered by blists - more mailing lists