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

Jakub Hrozek jhrozek at redhat.com
Tue Apr 6 12:25:36 UTC 2010


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

On 04/05/2010 01:35 PM, 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.
> 
> 
> 
>> 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.
> 
> 

Thank you, ACK, but please remove line 150 which is currently commented
out before pushing.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAku7KEAACgkQHsardTLnvCUe0ACbBVaV7PhoNd5F036Rf8ZmKhTt
yUwAmwX2CkCWdZ9wiLsmfk1L26pjviqO
=vZsb
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list