[SSSD] [PATCH] Protect against check-and-open race conditions

Stephen Gallagher sgallagh at redhat.com
Mon Apr 5 12:46:36 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/05/2010 08:40 AM, Dmitri Pal wrote:
> 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?
> 

I left it there in case there is a genuine reason that we might care
about a symlink, since this is the only way to read it. I also added a
noisy warning in the header file that it's unsafe to use in most
situations :)

Also, there's one other case in the code where we're stuck with using
it. When we open a D-BUS connection, we don't have a way of getting the
file descriptor of the socket file back from the D-BUS libraries, so the
best we can do is call check_file AFTER D-BUS has opened the socket.
This is a paranoia check, since the D-BUS libraries should already be
ensuring that the communication is safe. I'm still planning to open an
enhancement request against D-BUS to request the actual file descriptor.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAku526wACgkQeiVVYja6o6MXNwCgkcbMGRKhc4ppgbN55ZImiDj5
qOcAoKLEdcnGDjJJK+Zsw9dxv46qpqP1
=TpRX
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list