Closed Bug 43314 Opened 24 years ago Closed 24 years ago

nsIFile unique file creation is racy and insecure

Categories

(Core :: XPCOM, defect, P3)

Product:
Core
Component:
XPCOM
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: roc, Assigned: dougt)

Details

(Whiteboard: [nsbeta2-])

Attachments

(6 files)

Patch to get nsLocalFile::Create to return correct error codes on Windows
1.87 KB, patch
Details | Diff | Splinter Review
Patch to nsIFile/nsLocalFile to implement createUnique
3.94 KB, patch
Details | Diff | Splinter Review
Patch to get nsIFileTest working properly on Windows, and testing createUnique
3.36 KB, patch
Details | Diff | Splinter Review
nsIFile/nsLocalFileCommon createUnique patch: v2
4.26 KB, patch
Details | Diff | Splinter Review
nsLocalFile::Create patch for Windows: v2
1.71 KB, patch
Details | Diff | Splinter Review
Suggested (untested) changes for Unix and OS/2
1.59 KB, patch
Details | Diff | Splinter Review 
See bug #33768. The very idea of makeUnique is a disaster. It should be 
createUnique --- selecting the unique file name and creating the file in one 
atomic action.

This is easy to fix. I will attach a patch.
Assigning to dougt
Assignee: rayw → dougt
OK, so it wasn't as easy as I thought, due to breakage in various places that I 
had to fix. But things seem to be working now.

By the way, the current MakeUnique leaks the leafName and suffix strings (fixed 
in my patch).
nice.  r=dougt. mscott, can you update your stuff to use createUnique?
Keywords: nsbeta2
- (nit) else after return is a non-sequitur.
- (related nit) return errors early if possible, keep NS_OK path least-indented.

- Typo ("This" s.b. "The", I think) in @param permissions
+     *       This unix style octal permissions.  This may
+     *       be ignored on systems that do not need to do
+     *       permissions.

- Too much copying in
+        strncpy(suffix, lastDot, kMaxFilenameLength); // include '.'
+        suffix[kMaxFilenameLength] = 0; // make sure it's null terminated
  Why not
         strncpy(suffix, lastDot, PR_MIN(strlen(lastDot), kMaxFilenameLength);
  Or even
         strcpy(suffix, lastDot);
  if you've already computed lastDot from a string that cannot be longer than
  kMaxFilenameLength?

- It seems nsLocalFile::Create does not call open(..., O_CREAT|O_EXCL|...),
  and it does not keep the exclusively-created file's descriptor open.  This
  means nsIFile's createUnique method is still racy in the way that mktemp was,
  before mkstemp came along to save us from evil tmpfile attackers who replace
  what you thought was your exclusive tmpfile with their own content, linked
  under the same unique-ified name.

  What can be done here?  Would it be kosher to expose exclusive create as an
  XP method (createExclusively?) and use it here?  Could we keep the fd open
  for later writing, without wasting fd table space needlessly?  I think we
  should look into this, as we have come this far.  Roc, you up for it?

/be
> - (nit) else after return is a non-sequitur.
> - (related nit) return errors early if possible, keep NS_OK path
> least-indented.

I will fix.

> - Too much copying in
> +        strncpy(suffix, lastDot, kMaxFilenameLength); // include '.'
>  Why not
>        strncpy(suffix, lastDot, PR_MIN(strlen(lastDot), kMaxFilenameLength);

I will fix.

>  Or even
>         strcpy(suffix, lastDot);
>  if you've already computed lastDot from a string that cannot be longer than
>  kMaxFilenameLength?

I don't know that anyone promises GetLeafName() will always return <= 31 
characters. Even if it does, I prefer to minimize reliance on subtle invariants 
(unless performance-critical, of course).

> - Typo ("This" s.b. "The", I think) in @param permissions

Oops, copied that from Create above. Will fix.

> - It seems nsLocalFile::Create does not call open(..., O_CREAT|O_EXCL|...),
>   and it does not keep the exclusively-created file's descriptor open.

That's OK. If you're doing this in a world-writeable directory, then the 
directory must be marked sticky. If not, then on Unix even if you're holding an 
open file descriptor for the file, someone else can remove your file from the 
directory and put their own file in with the same name (leaving you with an fd 
open on an anonymous file).

mktemp() is the analogue of makeUnique, because it does not create the file. 
createUnique is not exactly like mkstemp(), but it's just as secure.

Stand by for another patch...
> - Too much copying in
> +        strncpy(suffix, lastDot, kMaxFilenameLength); // include '.'
>  Why not
>        strncpy(suffix, lastDot, PR_MIN(strlen(lastDot), kMaxFilenameLength);

Actually, my code is correct, and has the same effect as this.

Is libc strncpy the right thing to use? I can never figure out when I'm supposed 
to be using functions from nsCRT, NSPR, libc, or somewhere else...

BTW, all this code is quite bad i18n-wise. But my patches don't make things any 
worse in that regard.
>> - Too much copying in
>> +        strncpy(suffix, lastDot, kMaxFilenameLength); // include '.'
>>  Why not
>>        strncpy(suffix, lastDot, PR_MIN(strlen(lastDot), kMaxFilenameLength);
>
>Actually, my code is correct, and has the same effect as this.

Not quite -- strncpy NUL-pads to the passed-in count.  That's what I was whining 
about, not "too much copying" -- sorry for the confusion.  But for small counts, 
who cares?

>That's OK. If you're doing this in a world-writeable directory, then the 
>directory must be marked sticky.

Ah, right -- sticky directories save us here.  But is there still no benefit to 
a mkstemp analogue?  Perhaps only efficiency.

How about the lack of mutual exclusion?  creat(2) is equivalent to open with 
O_CREAT|O_WRONLY|O_TRUNC.

/be
Good point about the strncpy. I sometimes forget just how hideous the C string 
API is.

> But is there still no benefit to a mkstemp analogue?  Perhaps only efficiency.

We could return an output stream from Create and CreateUnique, I suppose. But 
there isn't anything stream-related in nsIFile right now.

> How about the lack of mutual exclusion?  creat(2) is equivalent to open with
> O_CREAT|O_WRONLY|O_TRUNC.

I'm not sure what you're asking for. There is nothing preventing someone from 
opening a file just after you creat(2) it.
I'm not changing the strncpy, it's just too ugly, and performance-wise, it's a 
wash (we'd have to do an extra strlen()).

Later, this code's string handling should be cleaned up to be i18n safe and to 
use the standard String APIs, anyway.

We should just get this in.
Roc, thanks for your patience -- I agree we should get this in.  I'm just 
about done, and should have already said "a=brendan@mozilla.org".

>> How about the lack of mutual exclusion?  creat(2) is equivalent to open with
>> O_CREAT|O_WRONLY|O_TRUNC.
>
>I'm not sure what you're asking for. There is nothing preventing someone from 
>opening a file just after you creat(2) it.

Yes, but two processes could race through createUnique and both believe they 
exclusively created the file.  Your Windows patch adds PR_EXCL to the PR_Open 
flags used for NORMAL_FILE_TYPE, but nsLocalFileUnix.cpp does not use O_EXCL 
with open for NORMAL_FILE_TYPE -- it uses creat, and creat is not exclusive.

It's also worth considering whether making nsILocalFile::Create be exclusive is 
the right thing, or whether we want to expose create and createExclusively, as I 
suggested earlier.  I didn't want a semantic change (adding PR_EXCL to PR_Open's 
flags for NORMAL_FILE_TYPE in nsLocalFileWin.cpp) to be both implicit but not 
obviously implied by the "create" method name, and Windows-only!

/be

/be
Excellent point.

nsIFile::Create says
> If the file or directory already exists create() will return
> NS_ERROR_FILE_ALREADY_EXISTS.

This implies that Create is intended to be "exclusive". (That's why I assumed 
it was.) Therefore nsLocalFileUnix is broken. Examining the code, 
nsLocalFileUnix appears to be exclusive for directories and non-exclusive for 
files.

I'll attach a suggested patch, but I have no Unix build environment for Mozilla 
so I can't compile or test it.

The Mac code appears to be exclusive already. The OS/2 code is not; I'll attach 
a suggested patch for that too. (The OS/2 code doesn't report errors related to 
directories, but I don't know enough about OS/2 to fix that.)

Note that if anyone wants non-exclusive create, they can implement it in terms 
of exclusive create (but not vice versa):

Repeat
  Try to open the file
  If not found, try to create the file (exclusively)
Until opened

This is non-racy because if someone creates or deletes your file during the 
process, you get the same results as if you'd just started the process a little 
bit later.
Go ahead and checkin.
Putting on [nsbeta2-] radar. Not critical to beta2.
Whiteboard: [nsbeta2-]
brendan, this is a needed fix.  can we get this submitted into mozilla.org?
Whiteboard: [nsbeta2-]
Dougt, you have the patches, right?  Either you or roc+moz@cs.cmu.edu may check 
them in at any time, I gave approval already.  I don't know whether roc has what 
he considers to be secure CVS access yet (he wants to use ssh; dmose may know). 
But you can check in for him, with my approval -- you don't need the pdt to yea 
or nay patch checkin for a non-netscape.com contributor -- that's part of being 
a module owner, it can't be overridden by one company or another.

/be
brendan, sorry for not jumping on this sooner.  

all checked in except the os patch.  I can't test or build this.  (although, it 
looks harmless).  Maybe Henry Sobotka <sobotka@axess.com>, could look at this?


Thanks for checking in.

I've been thinking things over. On Unix, we should tighten up 
nsLocalFile::Create() with fdstat/lstat checks to make sure the file we think we 
created is really a file belonging to us, and not a link ... otherwise we fall 
to the same problem that spawned the need for CreateUnique. See, e.g.
http://www.securityportal.com/list-archive/bugtraq/1998/Nov/0227.html
and related discussions. That guy's analysis is slightly wrong, by the way; 
there is an attack on sticky dirs that defeats lstat() alone, although it's 
rather involved.

Maybe we should take a close look at everyone who's using 
makeUnique/createUnique and see if we can avoid putting the data in the file 
system at all...

Of course it was still good to get this patch in. It makes things better and 
gets the interface right, even if the implementation isn't perfect yet.
It's okay here.

Henry
Whiteboard: [nsbeta2-]
Did someone check in the OS/2 patch?

Roc, what attacks on sticky directories are there, how do they work?

/be
I checked in the os2 patch.
As is customary, Alice is our protagonist and Mallory is the villain.

Mallory: Guesses Alice's temporary name "foo"
         ln -s /tmp/foo ~/foo

Alice:   open("/tmp/foo", O_CREAT | O_EXCL | O_TRUNC | O_WRONLY, 077)
            // creates and opens ~mallory/foo
         write();
         close();

Mallory: rm ~/foo
         cat > ~/foo

Alice:   open("/tmp/foo", O_RDONLY)
         read();
         BOOM

This is basically the same as the one I described before; the problem is that, 
according to POSIX semantics, even exclusive create doesn't guarantee that the 
file will be in the directory you asked for. If there's already a symbolic link 
there that references a nonexistent file, it creates the file pointed to by the 
link. (BTW, this stupid behavior has been changed in the very latest version of 
the Linux kernel, but that's no help to us of course.)

So, to check that she created a file in the directory she asked for, Alice can 
lstat() the file after creating it. If it's there and owned by her, then no-one 
else can change that directory entry (thanks to the sticky bit) so the file path 
is trustworthy. Or is it? ...
Suppose we happen to have two Alices running at the same time. Mallory might be 
able to encourage this.

Mallory: Guesses Alice's temporary name "foo"
         ln -s /tmp/foo ~/foo

Alice1:  open("/tmp/foo", O_CREAT | O_EXCL | O_TRUNC | O_WRONLY, 077)
            // creates and opens ~mallory/foo

Mallory: rm /tmp/foo

Alice2:  open("/tmp/foo", O_CREAT | O_EXCL | O_TRUNC | O_WRONLY, 077)
            // chooses same temporary name because /tmp/foo does not exist

Alice1:  lstat("/tmp/foo");    // looks OK

Alice2:  lstat("/tmp/foo");    // looks OK
         write();
         close();
         open("/tmp/foo", O_RDONLY)
         read();
         close();
         unlink("/tmp/foo");

Mallory: cat > /tmp/foo

Alice1:  open("/tmp/foo", O_RDONLY)
         read();
         BOOM

OK, so it'd be hard to pull off, but I wouldn't bet against it. The solution is 
to do an fstat() after the lstat() and check that the device and inode numbers 
match.

The OTHER solution is to avoid world-writable directories like the plague. 
Problem is, in many environments you really want to have temporary files in 
/tmp. So if you really need *named* temporary files then you have to be able to 
cope with this.

For some clients, it probably suffices to just have an anonymous file attached 
to an input-output stream (or stream pair). That can be implemented much more 
conveniently and securely (no need to reopen the file). If we want such a thing,  
it should be provided outside nsIFile.
Roc, thanks again: some nits and comments:

>Mallory: Guesses Alice's temporary name "foo"
>         ln -s /tmp/foo ~/foo

You've transposed ln's arguments here -- easy to do.  The target(s) come first, 
the link name (or directory in which to link target basenames) last.

>OK, so it'd be hard to pull off, but I wouldn't bet against it. The solution
>is to do an fstat() after the lstat() and check that the device and inode
>numbers match.

This is because opened, unlinked files hold onto their zero-link-count inode 
until last close.  But it means that Alice2 is using an fd to an anonymous file, 
so no one should ever uses its name again to rendezvous through the filesystem. 
In which case, why didn't Alice2 unlink it just after opening it, herself?  Hrm.

>For some clients, it probably suffices to just have an anonymous file attached
>to an input-output stream (or stream pair). That can be implemented much more
>conveniently and securely (no need to reopen the file). If we want such a
>thing, it should be provided outside nsIFile.

My point exactly (which I forgot, mumbling instead about "efficiency", a valid 
point in addition to convenience and security) in favor of mkstemp(3)!

I believe we should do the anonymous file thang.  Where?

/be
>>Mallory: Guesses Alice's temporary name "foo"
>>         ln -s /tmp/foo ~/foo
>
> You've transposed ln's arguments here -- easy to do.  The target(s) come
> first, the link name (or directory in which to link target basenames) last.

Oops, yeah.

>>OK, so it'd be hard to pull off, but I wouldn't bet against it. The solution
>>is to do an fstat() after the lstat() and check that the device and inode
>>numbers match.
>
> This is because opened, unlinked files hold onto their zero-link-count inode 
> until last close.  But it means that Alice2 is using an fd to an anonymous
> file, so no one should ever uses its name again to rendezvous through the
> filesystem. In which case, why didn't Alice2 unlink it just after opening it,
> herself?  Hrm.

Er, no. Alice2 is not unlinking her file until after she's finished using it. 
Imagine that Alice2 is really two processes: Mozilla (which writes the file) and 
a helper app (which reads the file).

The fstat()/lstat() thing simply ensures that the file you have an fd for is 
truly in the directory you asked for.

>>For some clients, it probably suffices to just have an anonymous file attached
>>to an input-output stream (or stream pair). That can be implemented much more
>>conveniently and securely (no need to reopen the file). If we want such a
>>thing, it should be provided outside nsIFile.
>
> My point exactly (which I forgot, mumbling instead about "efficiency", a valid
> point in addition to convenience and security) in favor of mkstemp(3)!

mkstemp(3) creates a named file which can be reopened. Therefore a truly secure 
mkstemp implementation has to jump through the fstat/lstat hoops. I think 
tmpfile(3) is closer to the API we're talking about here. It can be safely 
implemented as open()/unlink(). (Mallory may be able to use links to capture 
a directory entry for the file, but he can't read or write the file, although he 
can stat() it. If stat()ing is bad, then we could put in an fstat() after the 
unlink() to ensure there are no outstanding links.)

Besides, if you want an anonymous file, you shouldn't have to specify a name for 
it :-).

> I believe we should do the anonymous file thang.  Where?

Couldn't we just extend nsIStorageStream to Do The Right Thing if the maxSize is 
large?
>> This is because opened, unlinked files hold onto their zero-link-count inode 
>> until last close.  But it means that Alice2 is using an fd to an anonymous
>> file, so no one should ever uses its name again to rendezvous through the
>> filesystem. In which case, why didn't Alice2 unlink it just after opening it,
>> herself?  Hrm.
>
>Er, no. Alice2 is not unlinking her file until after she's finished using it. 
>Imagine that Alice2 is really two processes: Mozilla (which writes the file)
>and a helper app (which reads the file).

Understood, but I was concerned that rendezvous through a /tmp file is 
inherently dangerous.  It's not once you do the lstat/fstat check -- then you 
can hand out the filename for others to use safely (because stickyness keeps 
rogue unlinkers at bay).  Or can you?

I guess I'm still worried about sticky attacks.  The fact that namei or lookuppn 
or whatever it's called these days follows terminal links for exclusive create 
(except for recent Linux kernels) makes for a big hole: now my tmp file may be 
in fact hard-linked elsewhere than the sticky /tmp directory, even on another 
filesystem.  Other users still can't read or write it, given 600 permissions, 
but a miscreant could unlink it, and make a new file in its place.  Could this 
not happen after the lstat and before the fstat?

Then the miscreant's file is used by the rendezvousing process, who is fed 
misleading data.  Sounds like a helper app attack.  Roc, help me out here -- I'm 
not up-to-date on Unix attacks, but I am an old Unix hacker who memorized man 2 
and man 3.

/be
> It's not once you do the lstat/fstat check -- then you can hand out the
> filename for others to use safely (because stickyness keeps rogue unlinkers at
> bay).

I think that is correct.

> Could this not happen after the lstat and before the fstat?

No. If our file is created in /tmp, then Mallory can't unlink it. If it's 
created somewhere else and Mallory moves it into /tmp, then again he can't 
unlink it so we can safely use it. If it's created somewhere else and Mallory 
doesn't move it into /tmp, then lstat() will reveal that it's not there (or 
that something else is there). Even if it's created somewhere else and Mallory 
somehow puts another empty file belonging to us in /tmp, then lstat() and 
fstat() together will detect that it's the wrong file.

I think this is solid, although it would be a nice project to formally verify 
it.

There is an interesting point about the anonymous file implementation, though. 
What if Mallory puts in a symbolic link pointing inside an NFS mount (or 
similar)? Then simply creating (and even fstat()ing) the file would leave it 
vulnerable to an attack against NFS itself (or more generally, the weakest 
mounted writable network filesystem), which is generally quite easy. So even an 
anonymous file scheme should use fstat()/lstat() to guarantee that the file has 
been created on the right device. Ugh.

I wonder if libc mkstemp/tmpfile and friends actually do this. If not, there 
must be a whole barrel of vulnerabilites out there.
>>Could this not happen after the lstat and before the fstat?
>
>No. If our file is created in /tmp, then Mallory can't unlink it.

I was stipulating (sorry if unclear) your attack scenario: Mallory has already
created a symlink from /tmp/victim to ~/victim.

>If it's created somewhere else and Mallory doesn't move it into /tmp, then
>lstat() will reveal that it's not there (or that something else is there).

Right, lstat won't follow the link.  Duh.

>There is an interesting point about the anonymous file implementation, though.

Yes, NFS is a great source of attacks.  Guess who did the first Irix NFS port,
way back in 1986?

Let me know if you stir up any fun among Unix security worriers about the lack
of fstat/lstat checks.

Should we have a new bug on nsIStorageStream needing to use a secure, anonymous
temporary file?

/be
Sigh. I've just wasted your time in most of this discussion.

Turns out, creating with O_EXCL will not resolve symbolic links; it will just 
fail if there's a link in the way. Therefore the code we have now should be safe 
and sound.

Somehow neither I nor any of the other people I talked to about this remembered 
this little fact.

Sorry!
Oh good!  Because (I was going to brag) when I hacked on Irix's namei (later,
pn_lookup), it did not follow terminal symlinks in the O_EXCL.  But now that you
mention it, all the Unix kernel namei-like code I've seen worked that way.

So is there any real, proven attack on sticky directories known to security
gurus?

/be
I still read about a lot of /tmp race problems, but I guess those are just bugs 
in the user code. And we won't have any of those, will we :-).

The confusing thing is that O_CREAT *without* O_EXCL usually does follow the 
link and create the targeted file if it doesn't already exist. And the O_EXCL 
behavior isn't documented in any of the DUX or Linux man pages I looked at. 
Blah.

About nsIStorageStream: hardly anyone's using it right now. We should encourage 
people to use it when they can; if they run into problems with all their data 
sitting in VM, then we can fix it. Anonymous files and swap space aren't all 
that different in theory...
Thanks for checking in!
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
- Per last comments, age of bug, and no reopen - Marking Verified/Fixed.  Please 
reopen if still a problem.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: