Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup

[Eric W. Biederman]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Mon, Apr 4, 2016 at 6:29 PM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> In practice I expect the permission checks are a non-issue as the
>> permissions on /dev/ptmx and /dev/pts/ptmx are always 0666.
>
> So I think this is still entirely wrongheaded, and thinking about the
> problem the wrong way around.

No. You are missing my concern.

My concern is that I suspect someone somewhere has created a chroot
environment.  That chroot environment has devpts mounted with
"-o newinstance" and has set the permissions of /dev/pts/ptmx such
that only users in that container can create ptys on that instance
of devpts.

Being a mischevious user outside the container I can create a new user
namespace and a new mount namespace and bind mount our new and improved
version of /dev/ptmx right next to the chroot's /dev/pts mount.

Then because the permissions on /dev/ptmx are different than on the
chroots /dev/pts/ptmx I can create ptys that I could not have before
hand.  Bypassing the existing permissions.

Given that concern under the rule we don't break userspace we have to
check the permissions of /dev/pts/ptmx when we are creating a new pty,
on a instance of devpts that was created with newinstance.

Short of saying we simply don't care about such users I don't see a way
we can allow bypassing the existing permission check.



Now I do think we can remove the permission check altogether.  At this
point POSIX does not even require the existince of any files or device
nodes, and FreeBSD proves out that users of ptys don't care by
implementing a dedicated system call to create ptys that does contain
any permission checks.  So only the small set of linux specific
chroot/container creating applications might care.  As the permissions
were not in any way the focus of this patchset I choose not to tackle
a possible user visible change like this.

>
> So get rid of all the pointless "inode_permission()" crap. We already
> checked that by virtue of us opening "/dev/ptmx". THOSE permissions
> matter, but they were already done. Now we're just saying "ok, the
> user has a right to open the ptmx node, now _which_ devpts is that
> ptmx node for?"

I wish I could in conscience do that.  But unless we decide that
permission are irrelevant we are adding a permission bypass for an
existing operation.  Typically that is called a security bug.  I am not
comfortable doing that unless we simply decide we don't care.

If we decide we don't care I will add a patch at the front of the
patchset that implements don't care before all of the rest of this.

> So also get rid of this:
>
> +       /* Advance path to the ptmx dentry */
> +       old = path.dentry;
> +       path.dentry = dget(fsi->ptmx_dentry);
> +       dput(old);
>
> entirely. It's wrong. It's entirely pointless. We don't even care
> about "what does pts/ptmx point to". We care about "which superblock
> do we get when we look up the "pts/" subdirectory in the dentry cache
> for this user (without permissions)"/

Actually it is not pointless.  There is a second issue in all of this.
Right now it is possible to confuse the pt_chown setuid root binary
about which instance of devpts it should be calling chmod on.  I am not
certain it even ensures it is calling chown on a devpts entry.  It is
just hard coded paths today.

Right now userspace does something like:

	masterfd = posix_openpt(O_RDWR);
	grantpt(masterfd);
	char *slave_name = ptsname(masterfd);
	slavefd = open(slave_name);

Furthemore invoked by grantpt execs the pt_chown binary which does:

	int pty_number;
	ioctl(masterfd, TIOCGPTN, &pty_number)
	sprintf(slave_name, "/dev/pty/%u", pty_number);
	chown(slave_name, some_uid, some_gid);

It would be very nice if we could have a way to close the races
in this mess and allow a program like pt_chown to actually only affect
the pty it cares about.  There is only one way I know to implement this
in a backwards compatible way and that is to have
readlink("/proc/self/fd/${masterfd}"), stat("/proc/self/fd/${masterfd}",
and open("/proc/self/fd/${masterfd}") talk about "/dev/pts/ptmx"
for a non-system instance of devpts

That will at least allow ptsname to find the proper instance of devpts.

I suppose it becomes hameless if grantpt stops calling a setuid root
exectuable if the connection between the master and the slave pty
gets confused and pt_chown, does not exist anymore.  But it feels
wrong to allow userspace no way to ask the question which mounted
instance of devpts does this masterfd belong to.  Especially
as readlink("/proc/self/fd/${masterfd}") is the natural way to ask that.

If anyone has a better idea on how userspace should connect the master
pty file descriptor the slave file descriptor, I would be willing to
implement that instead.

> So get rid of all the pathname games. Just save the superblock pointer
> in file->f_private or somewhere like that, and make it really clear
> that what we are doing is making "/dev/ptmx" work sanely! The user is
> not looking up "/dev/pts/ptmx". They are looking up "/dev/ptmx".
>
> See the difference?

The change that introduces devpts_add_ref devpts_del_ref takes care of
all of the needed reference counting of the super block level.
Apparently the interactions with /dev/tty require the tty's to have a
reference to the superblock.

The natural place to store the superblock in tty->driver_data.  I even
took a look at that before posting my patches.  Unfortunately there
is at least devpts_pty_kill that actually need the slave inode.  So for
the slave side there does not appear to be a location where we can just
use the superblock.  Furthermore most of the methods between the master
side and the slave side are shared so having tty->driver_data be an
inode pointer in one place and super_block pointer in another is tricky
to implement.

Given that it was all crazy weird and goofy and I was have a very hard
time tracing it, I figured the better part of valour was not attempting
to change code I was having trouble tracing.

That is the only reason why I do not refer to the super_block directly
from the tty layer.  I actually have patches that get as far as killing
pts_sb_from_inode.


Or in summary the decisions you question most I have made not for
implementation reasons but for user space api reasons.  The permission
check because I don't want to break existing userspace, and the
change of file path to allow the natural way to ask the question which
devpts does this master file descriptor belong to.

I am open to better ideas.

Eric

p.s.  In the long term I think we should just update glibc and the
handful of ther libraries that care to prefer to use /dev/pts/ptmx if it
is available over /dev/ptmx and then make /dev/ptmx and all of the hacks
for supporting it a configuration option which in a decade or so can be
turned off by default.