From: ebiederm@xmission.com (Eric W. Biederman)
To: Andy Lutomirski <luto@amacapital.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>, security@debian.org,
        "security\@kernel.org" <security@kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        "security\@ubuntu.com \>\> security" <security@ubuntu.com>,
        Peter Hurley <peter@hurleysoftware.com>,
        Serge Hallyn <serge.hallyn@ubuntu.com>, Willy Tarreau <w@1wt.eu>,
        Aurelien Jarno <aurelien@aurel32.net>, Jann Horn <jann@thejh.net>,
        Greg KH <greg@kroah.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jiri Slaby <jslaby@suse.com>, Florian Weimer <fw@deneb.enyo.de>,
        "H. Peter Anvin" <hpa@zytor.com>
References: <878u0s3orx.fsf_-_@x220.int.ebiederm.org>
	<1459819769-30387-1-git-send-email-ebiederm@xmission.com>
	<CA+55aFwv7d8XfeN8KvYaVcSs7V9XoHtS-1cqtkG4gF4yJwB5nA@mail.gmail.com>
	<87twjcorwg.fsf@x220.int.ebiederm.org>
	<CA+55aFzCQ5MEs0C21SSobgpx-zGKxK67ur1Q45s+wn-nWZ56nw@mail.gmail.com>
	<CALCETrVUHn=KWyjzCa15VffZej3+ge7Js=nN_ymJpoOuWJ=FRg@mail.gmail.com>
Date: Fri, 08 Apr 2016 16:29:52 -0500
In-Reply-To: <CALCETrVUHn=KWyjzCa15VffZej3+ge7Js=nN_ymJpoOuWJ=FRg@mail.gmail.com>
	(Andy Lutomirski's message of "Fri, 8 Apr 2016 13:43:16 -0700")
Message-ID: <87d1pzn60f.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
Subject: Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1868
Lines: 46

Andy Lutomirski <luto@amacapital.net> writes:

> On Apr 8, 2016 12:05 PM, "Linus Torvalds" <torvalds@linux-foundation.org> wrote:
>>
>> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>> >
>> > 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.
>>
>> The rule is that we don't break existing installations.
>>
>> If somebody has root and installs a "ptmx" node in an existing mount
>> space next to a pts subdirectory, that's not a security issue, nor is
>> it going to break any existing installation.
>
> What Eric's saying is that you don't have to be root for this.
>
> But Eric, I think there might be a better mitigation.  For a ptmx
> chardev, just fail the open if the chardev's vfsmount or the devpts's
> vfsmount doesn't belong to the same userns as the devpts's superblock.
> After all, setting this attack up requires the caps on one of the
> vfsmounts, and if you have those caps you could attack your own devpts
> instance quite easily.  Would that work?

I don't think so.  For one it depends on getting s_user_ns which should
happen but is not there yet.  For another the way you describe
it you would break the case of

	unshare(CLONE_NEWUSER);
	unshare(CLONE_NEWNS);
        open("/dev/ptmx");

Which is actually more likely to break userspace than anything else we
have considered.  I know people actually do that.


Also using any property from a mount namespace or a vfs mount is usually
an error, as it is an inconsistent model.

Plus I don't think what you are suggesting would make anything simpler
or easier to reason about.  It only costs me about 3 lines of code to
perform the permission checks.  The complaint is that they exist at all.

Eric