lkml.org 
[lkml]   [2012]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] fs: add link restrictions
From
On Sat, Jun 30, 2012 at 2:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Jun 25, 2012 at 02:05:26PM -0700, Kees Cook wrote:
>
>> +config PROTECTED_LINKS
>> + bool "Evaluate vulnerable link conditions"
>> + default y
>
> Remember Linus' rants about 'default y' in general?

I added these configs due to other people's requests. I am happy to
remove them all and have the sysctls start their life == 1. It would
eliminate all the #ifdef logic too.

>> + /* Check parent directory mode and owner. */
>
> I suspect that we ought to simply pass that parent directory as argument - caller
> *does* have a reference to it, so we don't need to mess with ->d_lock, etc.

I don't see where the parent is held in either path_openat nor
path_lookupat. What should I be passing into may_follow_link() for the
parent?

>> + err = may_follow_link(&link);
>> + if (unlikely(err))
>> + break;
>
> No. This is definitely wrong - you are leaking dentries and vfsmount here.

What should I do to avoid the leak? I thought it was avoiding the need
to call put_link because it aborts before calling follow_link.

>> + error = may_follow_link(&link);
>> + if (unlikely(error))
>> + break;
>
> Ditto.

Same thing here -- it aborts before the follow_link. I must be
misunderstanding something. What am I missing?

Thanks for the feedback! I'll clean up the other things you mentioned as well.

-Kees

--
Kees Cook
Chrome OS Security


\
 
 \ /
  Last update: 2012-07-02 03:21    [from the cache]
©2003-2011 Jasper Spaans