Messages in this thread | | | From | (Eric W. Biederman) | Date | Fri, 08 Apr 2016 13:51:43 -0500 | Subject | Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup |
| |
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Mon, Apr 4, 2016 at 6:29 PM, Eric W. Biederman <ebiederm@xmission.com> 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.
| |