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

[ebiederm@xmission.com (Eric W. Biederman)]

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