Stephen Gallagher wrote:
On 04/05/2010 08:13 AM, Dmitri Pal wrote:
> Stephen Gallagher wrote:
>> On 04/02/2010 06:00 AM, Jakub Hrozek wrote:
>>> On 04/01/2010 10:20 PM, Stephen Gallagher wrote:
>>>> There is a small window between running lstat() on a filename and
>>>> opening it where it's possible for the file to have been modified.
>>>> We were protecting against this by saving the stat data from the
>>>> original file and verifying that it was the same file (by device
>>>> and inode) when we opened it again, but this is an imperfect
>>>> solution, as it is still possible for an attacker to modify the
>>>> permissions during this window.
>>>> It is much better to simply open the file and test on the active
>>>> file descriptor.
>>>> Resolves
https://fedorahosted.org/sssd/ticket/425 incidentally, as
>>>> without the initial lstat, we are implicitly accepting symlinks
>>>> and only verifying the target file.
>>
>>> Well, this changes the semantics of the calls a little as it is not
>>> possible to check for symlinks anymore..granted we don't need it
in the
>>> code right now
>> Realistically, it's going to be exceptionally rare that we would
>> actually care whether something was a symlink. If we desperately
need to
>> know this, check_file() can be called individually.
>>
>> The problem with symlinks is that there's no way to test for them that
>> is not a race-condition. When you call stat() or fstat(), it's
>> impossible to determine whether the resulting file being stat()-ed
>> exists at the original specified location or was resolved through
>> symlinks. So the only way to determine if a file is a symlink is by
>> polling it with lstat() before or after doing the stat()/fstat(). In
>> either case, it's a race-condition, because someone with an inotify()
>> watch on the file could change it between one stat and the next.
>>
>> So it's always a security risk to care about whether a file is a
>> symlink. It's always better to work only with the resulting file.
>>
> I might be missing something but I do not get it.
> If you open the file first and then do fstat() on the file descriptor
> you do not care if it is a symlink or not.
> You care that the file you opened has the right mode and ownership and
> this is what you are checking.
> And nobody can do anything using inotify() to the file you already
opened.
> I do not see a race condition here and I do not see a security issue
here.
> Am I missing something?
No, you've got it exactly correct. I'm saying that if you do fstat(),
it's perfectly safe. lstat() is an easy way to guarantee a
race-condition. And lstat() is the only way to be certain that you're
using a symlink. So in short, it's unsafe to care whether a symlink is
in use.
In the older code, we were first checking whether a file was a symlink,
and then opening it if it was a real file. This is an unsafe premise, as
there's no way to guarantee that the file is STILL not a symlink when
the open() call happens. And it's not really sensible to care if a
symlink is in use anyway.
Ah Ok. Yes. What I did not get is why we ever cared about symlinks vs.
files but I guess it does not matter since we are on the same page.
So then why do we need check_file function that has a race condition?
Why it is not just open file using passed in path and call check_fd on
the file descriptor?
But I guess you already have another function to do exactly that.
So why not just remove check_file function then?
>>> As per the code only couple of nits: the DEBUG() call on line 163
>>> should say "check_fd failed"
>> Thanks, fixed.
>>
>>
>>> Why does the check_fd call accept an external stat buf? It is not
>>> defined in any header file, perhaps it should be made static..did you
>>> envision exporting it in the future similar to check_file()?
>> I didn't just envision it, I planned it and then forgot to include
it in
>> the header file :)
>>
>> Fixed. I also added several comments to the header file to identify the
>> recommended approach to performing file checks and warn of the
potential
>> race-conditions.
>>
>>
> -------------------------
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/