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

Dmitri Pal dpal at redhat.com
Mon Apr 5 12:51:55 UTC 2010


Stephen Gallagher wrote:
> 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 :)
>

Yes, I have seen the warning.

> 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. 
Trust no one ...  ;-) 

> I'm still planning to open an
> enhancement request against D-BUS to request the actual file descriptor.
>



_______________________________________________
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