Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932604AbcDHTCk (ORCPT ); Fri, 8 Apr 2016 15:02:40 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:39033 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750908AbcDHTCj (ORCPT ); Fri, 8 Apr 2016 15:02:39 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Linus Torvalds Cc: "H. Peter Anvin" , Peter Hurley , Greg KH , Jiri Slaby , Aurelien Jarno , Andy Lutomirski , Florian Weimer , Al Viro , Serge Hallyn , Jann Horn , "security\@kernel.org" , "security\@ubuntu.com \>\> security" , security@debian.org, Willy Tarreau , Linux Kernel Mailing List References: <878u0s3orx.fsf_-_@x220.int.ebiederm.org> <1459819769-30387-1-git-send-email-ebiederm@xmission.com> Date: Fri, 08 Apr 2016 13:51:43 -0500 In-Reply-To: (Linus Torvalds's message of "Thu, 7 Apr 2016 09:06:44 -0700") Message-ID: <87twjcorwg.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX1/FvRfqh6nMMIq4oLiwS9KfsP6x68aTOGg= X-SA-Exim-Connect-IP: 67.3.249.252 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.7 XMSubLong Long Subject * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa06 1397; Body=1 Fuz1=1 Fuz2=1] * 0.1 XMSolicitRefs_0 Weightloss drug X-Spam-DCC: XMission; sa06 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Linus Torvalds X-Spam-Relay-Country: X-Spam-Timing: total 1237 ms - load_scoreonly_sql: 0.16 (0.0%), signal_user_changed: 4.5 (0.4%), b_tie_ro: 3.2 (0.3%), parse: 1.30 (0.1%), extract_message_metadata: 28 (2.3%), get_uri_detail_list: 7 (0.6%), tests_pri_-1000: 12 (1.0%), tests_pri_-950: 1.73 (0.1%), tests_pri_-900: 1.46 (0.1%), tests_pri_-400: 54 (4.4%), check_bayes: 52 (4.2%), b_tokenize: 22 (1.8%), b_tok_get_all: 14 (1.2%), b_comp_prob: 6 (0.5%), b_tok_touch_all: 7 (0.6%), b_finish: 0.65 (0.1%), tests_pri_0: 1124 (90.9%), check_dkim_signature: 0.59 (0.0%), check_dkim_adsp: 3.0 (0.2%), tests_pri_500: 5.0 (0.4%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6899 Lines: 158 Linus Torvalds writes: > On Mon, Apr 4, 2016 at 6:29 PM, Eric W. Biederman 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.