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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux