Closed
Bug 43314
Opened 24 years ago
Closed 24 years ago
nsIFile unique file creation is racy and insecure
Categories
(Core :: XPCOM, defect, P3)
Tracking
()
VERIFIED
FIXED
People
(Reporter: roc, Assigned: dougt)
Details
(Whiteboard: [nsbeta2-])
Attachments
(6 files)
1.87 KB,
patch
|
Details | Diff | Splinter Review | |
3.94 KB,
patch
|
Details | Diff | Splinter Review | |
3.36 KB,
patch
|
Details | Diff | Splinter Review | |
4.26 KB,
patch
|
Details | Diff | Splinter Review | |
1.71 KB,
patch
|
Details | Diff | Splinter Review | |
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.
Reporter | ||
Comment 2•24 years ago
|
||
Reporter | ||
Comment 3•24 years ago
|
||
Reporter | ||
Comment 4•24 years ago
|
||
Reporter | ||
Comment 5•24 years ago
|
||
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).
Assignee | ||
Comment 6•24 years ago
|
||
nice. r=dougt. mscott, can you update your stuff to use createUnique?
Keywords: nsbeta2
Comment 7•24 years ago
|
||
- (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
Reporter | ||
Comment 8•24 years ago
|
||
> - (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...
Reporter | ||
Comment 9•24 years ago
|
||
> - 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.
Reporter | ||
Comment 10•24 years ago
|
||
Reporter | ||
Comment 11•24 years ago
|
||
Comment 12•24 years ago
|
||
>> - 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
Reporter | ||
Comment 13•24 years ago
|
||
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.
Reporter | ||
Comment 14•24 years ago
|
||
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.
Comment 15•24 years ago
|
||
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
Reporter | ||
Comment 16•24 years ago
|
||
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.
Reporter | ||
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
a=brendan@mozilla.org. /be
Comment 19•24 years ago
|
||
Go ahead and checkin.
Comment 20•24 years ago
|
||
Putting on [nsbeta2-] radar. Not critical to beta2.
Whiteboard: [nsbeta2-]
Assignee | ||
Comment 21•24 years ago
|
||
brendan, this is a needed fix. can we get this submitted into mozilla.org?
Whiteboard: [nsbeta2-]
Comment 22•24 years ago
|
||
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
Assignee | ||
Comment 23•24 years ago
|
||
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?
Reporter | ||
Comment 24•24 years ago
|
||
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.
Comment 25•24 years ago
|
||
It's okay here. Henry
Updated•24 years ago
|
Whiteboard: [nsbeta2-]
Comment 26•24 years ago
|
||
Did someone check in the OS/2 patch? Roc, what attacks on sticky directories are there, how do they work? /be
Assignee | ||
Comment 27•24 years ago
|
||
I checked in the os2 patch.
Reporter | ||
Comment 28•24 years ago
|
||
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? ...
Reporter | ||
Comment 29•24 years ago
|
||
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.
Comment 30•24 years ago
|
||
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
Reporter | ||
Comment 31•24 years ago
|
||
>>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?
Comment 32•24 years ago
|
||
>> 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
Reporter | ||
Comment 33•24 years ago
|
||
> 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.
Comment 34•24 years ago
|
||
>>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
Reporter | ||
Comment 35•24 years ago
|
||
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!
Comment 36•24 years ago
|
||
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
Reporter | ||
Comment 37•24 years ago
|
||
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...
Reporter | ||
Comment 38•24 years ago
|
||
Thanks for checking in!
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 39•24 years ago
|
||
- 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.
Description
•