MakeUnique() is not implemented in nsIFile, and it used to exist in nsFileSpec. A blocker support for getting XPInstall Patch routine converted to use nsIFile.

This can be done as a utility fuction. Try something like (untested): nsresult MakeUnique(nsILocalFile* file, const char* suggestedName) { PRBool exists; rv = dir->Exists(&exists); if (NS_FAILED(rv)) return rv; if (!exists) return NS_ERROR_FAILURE; char* leafName rv = file->GetLeafName(&leafName); if (NS_FAILED(rv)) return rv; char* lastDot = strrchr(leafName, '.'); char* suffix = ""; if (lastDot) { suffix = nsCRT::strdup(lastDot); // include '.' *lastDot = '\0'; // strip suffix and dot. } // 27 should work on Macintosh, Unix, and Win32. const int maxRootLength = 27 - nsCRT::strlen(suffix) - 1; if ((int)nsCRT::strlen(leafName) > (int)maxRootLength) leafName[maxRootLength] = '\0'; for (short indx = 1; indx < 1000 && exists; indx++) { // start with "Picture-1.jpg" after "Picture.jpg" exists char newName[nsFileSpecHelpers::kMaxFilenameLength + 1]; sprintf(newName, "%s-%d%s", leafName, indx, suffix); file->SetLeafName(newName); rv = dir->Exists(&exists); if (NS_FAILED(rv)) return rv; } return NS_OK; } If more people use functionality like this, we can merge it into nsIFile.

1) That code as some little buglets, so don't just copy them verbatim (will duplicate Picture-1000.jpg if it's already there, and uses ``dir'' instead of parameter name ``file''). Nothing major, just a caveat. 2) I don't think this stuff should go into nsIFile, where it would need to be implemented once on each platform, plus for all of the necko stuff that uses the interface. I would prefer an nsIFileUtils service, like the one used in RDF, as a place to put these utility functions. (It could even be written in JS, if you were into that sort of thing.)

as I stated, it was not tested. I wrote it up in this tiny "Additional Comments" bugzilla field. (Why the heck isn't this field bigger!). I like the idea of having an nsIFileUtils service. I am going to reopen this bug for that. dbragg, just use the code with the fixes shaver suggested and any that you find directly in xpinstall. When we get a service I will convert you.

I really need a function like this as I work on hooking up external helper application support. I want to stream incoming files to temp files and then spawn applications off to run with this temp file. nsFileSpec has platform specific implementations of MakeUnique....can't we try to leverage that in order to get this bug fixed faster? Of course most of this is moot because i need this now and dougt's gone for a while longer =).

An API that just returns the name of a file that does not currently exist is vulnerable to race conditions. This kind of thing is a common source of security problems. Better to have an API that is guaranteed to atomically both select the new file name and create it. (Easy to do, just use "Create" instead of "Exists" and check error returns.) Then it will be harder for people to get themselves into trouble.

roc+moz is spot-on as usual. Unixes went from the racy, insecure mktemp (now deprecated; never mind the unspeakable tempnam from AT&T) to mkstemp(3), which atomically creates the temporary file. On a related note, I hear we implement file upload in a way that litters one's tmp or temp directory with files that are not cleaned up. Possibly they are not uniquely named, either? Cc'ing pollmann. /be

> On a related note, I hear we implement file upload in a way that litters one's > tmp or temp directory with files that are not cleaned up. Possibly they are > not uniquely named, either? Cc'ing pollmann. That's true, we do litter the tmp directory. This is bug 15320. As to the uniqueness, I rewrote that not too long ago to use the nsIFile stuff: From nsFormFrame::ProcessAsMultipart // Create a temporary file to write the form post data to nsSpecialSystemDirectory tempDir(nsSpecialSystemDirectory::OS_TemporaryDirectory); tempDir += "formpost"; tempDir.MakeUnique(); I'm not sure what the underlying implementation of that is, but if it is right there, it will also be correct in form post.

s/nsIFile/nsFileSpec/; # Oops. :)

that's not using nsIFile right? 'cause nsIfile doesn't have a MakeUnique method (that's the problem I'm running into). is tempDir an nsFileSpec in your example?

nsSpecialSystemDirectory derives from nsFileSpec. It is not what you are suppose to be using. mscott, I am not going to be able to get to this for a few days maybe a week. Do you think that you can wait this long, or would you rather just add makeUnique to (nsIFile/nsILocalFile) yourself?

That usage is a great example of why it should be CreateUnique instead of MakeUnique. On Unix, that code will let another user replace the POSTed data with their own data. (Between MakeUnique and the actual creation of the file, they create a symlink with the "unique" name pointing to a non-existent file in their own directory, which Mozilla then creates and writes into. Then, regardless of the permissions associated with the created file, they can delete that file and create their own file with the desired data.)

Hey dougt, I'm going to need this before Thursday in order to finish launching of external helper applications. (Either that or my code has to use nsFileSpec). I'll find someone to add this to nsIFile. Nominating for nsbeta2 (although I'm not saying dougt will be the one that has to do this before Thursday. I'll find the resources for this).

mscott, for you... PDT team, I need an approval!

Thanks for the diffs dougt. You rock. I just checked in the patch.

Shall I write the Bugtraq advisories now, or wait for Netscape FCS? Seriously, why was this allowed to be checked in? Now people will start using it, and someone will have to waste their time finding and repairing the damage. If the answer is, as I suspect, "meeting Netscape deadlines is more important than getting it right", then this is a job for Mozilla.org.

Is the current XP file form of open() capable of establishing an initial exclusive lock? Atomic would be great, sure; maybe roc+moz has source for that. If not, all the platforms have long file names; if the operator never has to type them, it doesn't matter if the temp files are fifteen chars long with 90 bits of base64 pseudo-random entropy. Is there a specific example of a bugtraq-level attack against temporary file creation race conditions? It might be a better favor to defend against the scripting of fields that aren't supposed to be writable.

We do not want to bundle a portable, cryptographically strong pseudorandom number generator with XPCOM. See above in this bug form for an outline of a typical exploit. You don't have to look hard to find them in the wild, any security archive will do. James, do you remember Bennet Yee? /tmp races were one of his favourite tricks for rooting boxes. Getting this right is not hard. It can be done XP using Create(). I am working on a patch right now.

Well, it turns out that nsLocalFileWin is broken and never returns NS_ERROR_FILE_ALREADY_EXISTS. Ha ha ha.

This exploit can be used all over nsIFile. For better or worse, we wait to create a file until the user calls create(). please file a seperate bug on this known issue.

With pleasure. Bug #43314. I don't believe that this bug is "all over nsIFile". If you just set the file to have some fixed file name and then create it or open it, there are no race conditions; you get what you asked for. The real problem is specific to makeUnique, which promises to return the name of a file which does not exist in some specific directory, but which you can never trust, and is therefore useless.

is there a testcase I can use to verify this?

On Windows, nsIFileTest.exe (generated from nsIFileTest.cpp) demonstrates working createUnique, which superceded makeUnique.

Sounds good. Marking Verified.