Re: [PATCH v2] vfs: Tighten up linkat(..., AT_EMPTY_PATH)

[Andy Lutomirski]

On Thu, Aug 22, 2013 at 11:53 AM, Willy Tarreau <w@xxxxxx> wrote:
> On Thu, Aug 22, 2013 at 11:48:10AM -0700, Linus Torvalds wrote:
>> On Wed, Aug 21, 2013 at 12:14 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>> >
>> > So let's be careful for now: only allow linkat(..., AT_EMPTY_PATH)
>> > if the target is I_LINKABLE.
>>
>> So I really detest this just because it's such a special case. Now
>> this is only useful for that one special case, and the thing very
>> fundamentally checks that one special case in a place that is
>> impossible to check for the /proc case, so the proc case remains
>> totally separate.
>>
>> Which just bothers me.

Agreed.  It sucks.  See below (the bottom).

>>
>> I think we could easily at least allow the "file->f_creds ==
>> current->creds" case (and yes, I literally mean comparing the pointers
>> - not only is it cheaper, but it literally means "nothing odd has
>> happened in between opening and the lookup").

I thought about that, but it won't catch chroot.  (Admittedly, chroot
without changing creds is asking for trouble.)

>>
>> And I'm wondering if we shouldn't actually do that at "path_init"
>> time. Right now the code says:
>>
>>                 /* Caller must check execute permissions on the
>> starting path component */
>>                 struct fd f = fdget_raw(dfd);
>>
>> and then uses the struct file mindlessly.
>>
>> I'm wondering if we should just do some validation in that place, and say:
>>
>>  - for directories, we require exec permissions here
>>  - for everything else, we require that f->f_cred == current->cred check.

Does this work for the procfs case?  As far as I understand it (which
isn't saying much), it goes through the symlink-following path.

>>
>> I dunno. But that I_LINKABLE thing just bothers me. It screams "I'm
>> hacky" to me.
>
> I agreed, simply because the condition here is different from the one in /proc.
>
> I have read some code last evening to try to understand how /proc/pid/fd
> entries were granted access to various processes, because I would love to
> see the same condition being used in both places. Unfortunately, it's beyond
> my skills, and I stopped after my random attempts gave me some panics.

What if we added another field to struct nameidata that's indicates
what restrictions need to be enforced when following magical symlinks
and then enforcing them when nd_jump_link gets used.  (There are only
two of these, both in procfs.)

Then open could check that the original fd was opened for a superset
of the requested permissions (or that the caller has
CAP_DAC_OVERRIDE), linkat could check whatever it feels like checking,
etc.

This would allow all of these issues to be fixed for real (controlled
by sysctl, presumably).

--Andy

>
> Willy
>



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/