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

Dmitri Pal dpal at redhat.com
Mon Apr 5 12:40:10 UTC 2010


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 at lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>
>
>
_______________________________________________
sssd-devel mailing list
sssd-devel at 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/




More information about the sssd-devel mailing list