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

Stephen Gallagher sgallagh at redhat.com
Mon Apr 5 11:35:57 UTC 2010


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

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.


> 
> 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.


- -- 
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/

iEYEARECAAYFAku5yxwACgkQeiVVYja6o6PxGACfegxcu3pFNmYRoumURsxU2EkZ
FBkAoKaiQEXoxK2ZdddUHhkiPiWCos1W
=dkQD
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Protect-against-check-and-open-race-conditions.patch
Type: text/x-patch
Size: 8384 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100405/8d387419/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Protect-against-check-and-open-race-conditions.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100405/8d387419/attachment.sig>


More information about the sssd-devel mailing list