Closed Bug 514823 (CVE-2009-3274) Opened 15 years ago Closed 15 years ago

Incremental filename choosing scheme for duplicate filenames vulnerable (especially with the use of /tmp)

Categories

(Toolkit :: Downloads API, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed

People

(Reporter: reed, Assigned: sdwilsh)

References

Details

(Keywords: fixed1.9.0.15, Whiteboard: [sg:low local] moderate-to-critical on a shared unix machine)

Attachments

(3 files, 1 obsolete file)

Attached file PoC
On 09/03/09, Jeremy Brown reported a security vulnerability in the download manager / file handling code to security@mozilla.org. Mozilla Firefox 3.x Download Manager Local Exploit Jeremy Brown 07-24-2009 TESTED Ubuntu Linux 9.04 (Jaunty) + Mozilla Firefox 3.0.12 -> Fully Exploitable Windows XP Professional SP3 + Mozilla Firefox 3.5.1 -> Partially Exploitable When downloading files through Firefox and choosing the "Open with" option, Firefox will create a temporary file in the form of RANDOM.part ("RANDOM" is random alphanumeric characters and ".part" is the extension). When the download completes, Firefox saves the completed file in the "/tmp" directory as its original filename and opens it with the program's handler (for example, Ark for compressed archives, VLC for .mp3, WINE for .exe, etc). Now, what if there is already a file with an identical filename in the temporary file directory? Firefox uses the scheme of saving and opening the completed download as "/tmp/file-#.zip", where "file" is the file's name, "-#" is a dash and the next available number in order, and ".zip" is of course the file's extension. So if "/tmp/file.zip" already exists and the user tries to download a file with the same name, Firefox saves and opens the newly downloaded file as "/tmp/file-1.zip". That scheme looked suspicious to me, and raised a couple good questions. 1) What is the maximum number in the filename? 2) What happens when it reaches that maximum number? Testing has proved that 9999, for example "/tmp/file-9999.zip", is the maximum number Firefox will use to deal with identical "Open with" filenames. Instead of using "/tmp/file-10000.zip", Firefox will just use the original identical file instead of the one it was supposed to download and open. That can get dangerous when local users can write to "/tmp" just like everybody else :) To exploit this situation, we need to know the filename that will be downloaded ahead of time. Then it is just a matter of creating the excess files, placing our "replacement" file (with the identical filename) in "/tmp", and waiting for the target user to use the "Open with" option to download and open a file of our choosing. We wouldn't even nessesarily have to know "ahead of time". According to how long it would take to complete the download (remember Firefox is writing to "/tmp/RANDOM.part" until its finished downloading), we could do our business while the file is still downloading (again, as long as we know its filename). There are many scenarios where we could leverage this vulnerability... here is one example. * Administrator is downloading openssh-5.2.tar.gz * We run the exploit to replace openssh-5.2.tar.gz with a modified version * Administrator installs our OpenSSH 5.2 with our _modifications_ For a bonus, the download history will still show the name of the site that supplied the original file, even when the target user opened the attacker's file instead. There are only two real conditions that have to be met for exploitation to succeed: 1. We need the ability to locally write in "/tmp" (shell, ftp, etc with write permissions for "/tmp") 2. The target user downloads and opens the filename using "Open with" Firefox on Windows has slightly different results. I found during testing that when the download completes, the right file will be opened. But, although unreliable, we were able to get the history of the file in download manager to show the replacement file and it will be opened if the user chooses to open it from there. mozilla-1.9.1/xpcom/io/nsLocalFileCommon.cpp -> LINES [85-174]: NS_IMETHODIMP nsLocalFile::CreateUnique(PRUint32 type, PRUint32 attributes) { nsresult rv; PRBool longName; #ifdef XP_WIN nsAutoString pathName, leafName, rootName, suffix; rv = GetPath(pathName); #else nsCAutoString pathName, leafName, rootName, suffix; rv = GetNativePath(pathName); #endif if (NS_FAILED(rv)) return rv; longName = (pathName.Length() + kMaxSequenceNumberLength > kMaxFilenameLength); if (!longName) { rv = Create(type, attributes); if (rv != NS_ERROR_FILE_ALREADY_EXISTS) return rv; } #ifdef XP_WIN rv = GetLeafName(leafName); if (NS_FAILED(rv)) return rv; const PRInt32 lastDot = leafName.RFindChar(PRUnichar('.')); #else rv = GetNativeLeafName(leafName); if (NS_FAILED(rv)) return rv; const PRInt32 lastDot = leafName.RFindChar('.'); #endif if (lastDot == kNotFound) { rootName = leafName; } else { suffix = Substring(leafName, lastDot); // include '.' rootName = Substring(leafName, 0, lastDot); // strip suffix and dot } if (longName) { PRUint32 maxRootLength = (kMaxFilenameLength - (pathName.Length() - leafName.Length()) - suffix.Length() - kMaxSequenceNumberLength); #ifdef XP_WIN // ensure that we don't cut the name in mid-UTF16-character rootName.SetLength(NS_IS_LOW_SURROGATE(rootName[maxRootLength]) ? maxRootLength - 1 : maxRootLength); SetLeafName(rootName + suffix); #else if (NS_IsNativeUTF8()) // ensure that we don't cut the name in mid-UTF8-character while (UTF8traits::isInSeq(rootName[maxRootLength])) --maxRootLength; rootName.SetLength(maxRootLength); SetNativeLeafName(rootName + suffix); #endif nsresult rv = Create(type, attributes); if (rv != NS_ERROR_FILE_ALREADY_EXISTS) return rv; } for (int indx = 1; indx < 10000; indx++) { // start with "Picture-1.jpg" after "Picture.jpg" exists #ifdef XP_WIN SetLeafName(rootName + NS_ConvertASCIItoUTF16(nsPrintfCString("-%d", indx)) + suffix); #else SetNativeLeafName(rootName + nsPrintfCString("-%d", indx) + suffix); #endif rv = Create(type, attributes); if (NS_SUCCEEDED(rv) || rv != NS_ERROR_FILE_ALREADY_EXISTS) return rv; } // The disk is full, sort of return NS_ERROR_FILE_TOO_BIG; } I also test the "Save As" option and it doesn't seem to be vulnerable (it saved, for example, file(1000000).zip). Full exploit code is provided (ffdlx.c). Example exploitation: linux@ubuntu:~$ ./ffdlx 2000 zip /home/linux/Desktop/test.zip (target downloads 2000.zip from a website but opens test.zip instead) exploit complete!
I think some of Jesse's proposed general download fixes in bug 369462 could help here, too.
Whiteboard: [sg:investigate]
Whiteboard: [sg:investigate] → [sg:low local]
Whiteboard: [sg:low local] → [sg:low local] or possibly worse
Would it be alright if I could view bug 369462 as well? Any ideas on a fix here?
I always thought maybe if there was more than 9999 files already there then throw an error and don't save the download or download history, or something like that. Ideas?
There's no really happy solution in bug 369462. This bug is easier to fix: increase the sequential-numbering limit to infinite or make downloads fail once there are too many sequentially named files.
"make downloads fail once there are too many sequentially named files" sounds good!
Hmm. So testing this on Mac, here's what I see: 1) The file is NOT automatically opened in the helper application (as expected). 2) Double-clicking the file in download manager opens the wrong file (the one that was placed by the attacker). Is the behavior different on Linux or Windows? I don't see why it should be; nsExternalAppHandler::ExecuteDesiredAction bails out if the CreateUnique fails and doesn't run the helper app...
(In reply to comment #7) > 2) Double-clicking the file in download manager opens the wrong file (the one > that was placed by the attacker). This is a different bug (don't have number handy) that has been around since forever. It's not trivial to solve, but it is solvable if we need to fix it for this.
I think the downloaded file still has a "ghost pointer" in the download manager list even though the temp file didn't get copied (because 10000 files named that way were already there). CreateUnique returns NS_ERROR_FILE_TOO_BIG if a unique name can't be created... then in nsDownloadManager.cpp: 2723 // Make sure the suggested name is unique since in this case we don't 2724 // have a file name that was guaranteed to be unique by going through 2725 // the File Save dialog 2726 rv = target->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600); 2727 NS_ENSURE_SUCCESS(rv, rv); the error encounteres NS_ENSURE_SUCCESS, which causes the OpenWithApplication() method to abort, so the file isn't auto-launched (as bz said in comment #7), but it appears that the file is still listed in the download manager. Ultimately, the problem is that the download manager doesn't clean up after or properly handle this failure to copy from temp to target -- and instead just punts and points to whatever file already existed. This won't trigger an auto-run of the malicious file, but a user will probably double-click the file if it doesn't open automatically, so it's an inconsistency in what was downloaded versus what is pointed to by the list item. Perhaps we should prompt for a new file name with a standard save-as dialog when we can't automatically rename the file. "file named X already exists, please choose a new name." They could use the save-as dialog to force overwrite of the malicious copy, or they could pick a new name. Prompting will also alert the user something fishy is going on when they see the 10000 files in the directory (unless they remember downloading the file 10000 times).
@ Comment #6: I showed a demo but have released no details yet; I am waiting on a fix from you all. Have we decided on the right way to patch?
Jeremy, I'm still on the "trying to understand what the bug is" part of things. Do you see something different from what I describe in comment 7? Comment 0 makes it sound like you do... I'll try to test this on Windows and Linux tomorrow.
"2) Double-clicking the file in download manager opens the wrong file (the one that was placed by the attacker)." <-- vulnerability Firefox should not open the replacement file at any time, not even when double clicked in the download history after the original file is downloaded, which also shows information making the user think that everything went as planned. As per previous comments, if the renaming scheme is reached at max, then Firefox should either A) Prompt to rename to another file and save as that filename B) Throw an error thats there are too many temp files C) Rename to an infinite scheme (gotta be careful, not recommended) D) A combination of A, B, and/or C Those are my thoughts...
>"2) Double-clicking the file in download manager opens the wrong file (the one > that was placed by the attacker)." <-- vulnerability Yes, yes. I agree with that, in case that wasn't clear. Is there another issue, or is that the only problem? The reason I ask is because there are at least 3 different CreateUnique calls involved, in at least 2 different areas of the code. Depending on what we're trying to fix, the fix would involve different sets of them....
As far as I know, the renaming scheme for temporary files created using "Open With" is the vulnerability I have found. If there are any others that anyone else knows of, please comment here. Otherwise, a fix for this issue should be the main priority.
In case I wasn't clear, comment 13 only refers to temporary files created for use with helper applications. There are multiple different codepaths that do that, triggered by different actions. Some do correct error checking apparently (see comment 7 item 1), right? Is there one that doesn't other than the "double-click in the download manager" codepath that we need to worry about? Which is why I'd still like a straight answer to my question from comment 13....
And to be even more clear, I just want to make sure we fix this whole bug, not just part of it. Comment 0 makes it sound like there's an issue I can't reproduce so far (per comment 7), and if there is, I'd love to know how to go about reproducing.
OK. I just tested on Linux, and the behavior I see is exactly the same as in comment 7. Therefore either comment 0 is incorrect when it talks about the two conditions for exploitability (need a third one, that being to double-click in the download manager when the file doesn't actually open) or there's something I'm missing in the steps to reproduce. I'm testing with FC9 and using a PDF file.
And the download manager double-click issue doesn't rely on overflowing the createUnique counter, as comment 8 says. If I have a /tmp/foo.pdf containing PDF A and I open a foo.pdf containing PDF B in the browser it gets saved as /tmp/foo-1.pdf and opened correctly. Now if I double-click in the download manager, I get PDF A opening (so /tmp/foo.pdf, not /tmp/foo-1.pdf). I'm happy to go ahead and cancel the download on createUnique failure in ExecuteDesiredAction, just so we don't get into the social-engineering situation of the download manager showing the file and such; I'll attach a patch for this in a few mins. But it sounds like we need to solve the download manager problem no matter what, right? And again, is there another way to trigger the bug other than explicit double-click in the download manager? I haven't found one yet....
(In reply to comment #18) > And the download manager double-click issue doesn't rely on overflowing the > createUnique counter, as comment 8 says. If I have a /tmp/foo.pdf containing > PDF A and I open a foo.pdf containing PDF B in the browser it gets saved as > /tmp/foo-1.pdf and opened correctly. Now if I double-click in the download > manager, I get PDF A opening (so /tmp/foo.pdf, not /tmp/foo-1.pdf). Hmm, this reminds me of bug 478394 for some reason...
Good news and bad news. Canceling on failure to createUnique gives a totally useless "unknown error occurred" dialog and marks the download as failed in the download manager. So far so good. Double-clicking the failed entry in the download manager seems to redownload the file and save it to the original name (foo.pdf in my case). Double-clicking again opens that file. So this allows us to clobber existing files (undesirable), but does make sure that we only open the file we meant to download, sort of. I'm not sure what a better behavior would be for the download manager here.
> Hmm, this reminds me of bug 478394 for some reason... That reason being that it's the same issue? ;)
Shawn, I think the rest of this ball is in the download manager court.
This bug reminds me of bug 459423, too :(
(In reply to comment #23) > This bug reminds me of bug 459423, too :( It, and many other bugs are all a duplicate of bug 301328. It's been a very long standing issue in the download manager.
Comment on attachment 399728 [details] [diff] [review] Cancel download on createUnique failure >@@ -1868,16 +1868,23 @@ nsresult nsExternalAppHandler::ExecuteDe should update nsDownload::ExecuteDesiredAction too I think, right?
@ comment 17: Although I cannot confirm now, when I first was testing this bug I'm pretty confident that the file opened automatically, that is why I wrote the 2 conditions. But like I said, I cannot confirm that now. It does look like the user needs to double click in the download manager for this attack to complete. 1. The ability to write in the temporary file directory, "/tmp" by default on Linux (shell, ftp, etc with write permissions could be helpful for making this work remotely) 2. The target user chooses to download the file and chooses the "Open with" preference 3. The target user also has to double click the file in the download manager (in some testing the file automatically opened, but that can no longer be confirmed) Sufficient?
Could a clever attacker replace "The ability to write in the temporary file directory" with "Convince the user to set 'automatically open files of this type with X' for any mime type"?
> should update nsDownload::ExecuteDesiredAction too I think, right? I guess; I have no idea how to change that to make it look like the download failed. > Sufficient? Yes, thank you. Looks like we match on what we see, now.
(In reply to comment #28) > I guess; I have no idea how to change that to make it look like the download > failed. You will want to call nsDownload::FailDownload. That will use the generic message if you pass an empty or NULL message.
So then do we have a modified patch?
This bug needs an owner familiar with the download manager front end (i.e. not me) for that to happen...
blocking1.9.1: --- → ?
Flags: blocking1.9.2?
Flags: blocking1.9.0.15?
Assignee: nobody → sdwilsh
Flags: wanted1.9.0.x+
Whiteboard: [sg:low local] or possibly worse → [sg:low local] moderate-to-critical on a shared unix machine
blocking1.9.1: ? → needed
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15?
Any progress on a patch?
blocking1.9.1: needed → .5+
Flags: blocking1.9.0.15? → blocking1.9.0.16?
Shawn / Mardak, can one of you help out on getting a dlmgr patch written for this?
Alias: CVE-2009-3274
(In reply to comment #33) > Shawn / Mardak, can one of you help out on getting a dlmgr patch written for > this? Possibly, but I've got a number of blockers that got assigned to me while I was away last week. I think I've pretty much said what needs to be done for a patch in this bug, but someone should write a test which will probably be the most time-intensive part of this fix.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
So, what exactly do we want/need fixed in the scope of this bug? I'll work on this, but I want to make sure I fully understand what we want/need fixed, and I don't find that terribly clear in the bug yet.
Status: NEW → ASSIGNED
When a user uses the "Open With" option when download files with download manager, the file is stored in /tmp/file.zip. If the file is downloaded and saved more than one time, the scheme is file-1.zip, file-2.zip, etc. Once the scheme reaches file-10000.zip, the file is not saved and download manager shows file.zip as the last file downloaded. We do not want that to happen, because an attacker can exploit this scheme to get the target to open the attacker's file.zip, instead of the file the target downloaded.
Right - I got that much. My question is more about what/how we actually want to fix [this]. Do we just want to propagate the failure to create a unique file? Writing a test for this bug is not going to be fun :/
Shawn, what we need here is to make the download manager actually use the final filename the data ended up at. Right now it doesn't. That bites not only in cases when we fail to create the unique file; even if we succeed at it, the download manager opens the old existing file, as far as I can tell.
Right, but that's bug 301328 and as I recall (I talked to either you or biesi about it probably two years ago) it's non trivial to fix. Is there a stopgap we can do that's safe to take on all branches until the user experience gets better (like comment 22)?
Is bug 301328 a security hole?
We could delete the file from the download manager on failure to create the file, I suppose. It's suboptimal UI, but would be a stopgap. I believe bug 301328 is a security hole, yes, as much as this one is. If the attacker places its file in /tmp first and you open yours and then ever try to open it again via the download manager, you lose.
(In reply to comment #41) > I believe bug 301328 is a security hole, yes, as much as this one is. If the > attacker places its file in /tmp first and you open yours and then ever try to > open it again via the download manager, you lose. Well, there are lots of issues with the download manager UI such as if the file is replaced with a malicious one. We'll just see that the file exists, and try to open it.
So, does this make sense to have an automated test for the file creation behavior? The only way I can see doing this is keep calling CreateUnique until it fails for a file, then try to download that same file name. This seems like a potentially bad test strategy. I can pretty easily write a test to make sure that the download manager fails appropriately though.
> such as if the file is replaced with a malicious one I'd think a different user typically can't do that with a file in /tmp that you own. Is that not the case?
The user's permissions of the file would determine that. But there is no need for it to be taken into consideration for this attack; an attacker places their own files in /tmp before the target ever begins the download.
(In reply to comment #43) > I can pretty easily write a test to make sure that the download manager fails > appropriately though. Looks to me like we already have coverage in the download manager that if we get a failure state, it gets marked as such in the UI (and shows a prompt) in http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_bug_412360.xul
Comment on attachment 399728 [details] [diff] [review] Cancel download on createUnique failure r=sdwilsh on these bits, with some minor style nits which I'll attach in my patch.
Attachment #399728 - Flags: review+
Attached patch v1.0Splinter Review
Style nits from bz's patch, as well as the toolkit fix for this.
Attachment #399728 - Attachment is obsolete: true
Attachment #403550 - Flags: review?(edilee)
Flags: in-testsuite-
Flags: in-litmus?
Whiteboard: [sg:low local] moderate-to-critical on a shared unix machine → [needs review Mardak][sg:low local] moderate-to-critical on a shared unix machine
Target Milestone: --- → mozilla1.9.3a1
(In reply to comment #45) > The user's permissions of the file would determine that. But there is no need > for it to be taken into consideration for this attack; an attacker places their > own files in /tmp before the target ever begins the download. "places" should be "can place" btw
Comment on attachment 403550 [details] [diff] [review] v1.0 r=Mardak >+ nsresult rv = ExecuteDesiredAction(); >+ if (NS_FAILED(rv)) { >+ // We've failed to execute the desired action. As a result, we should >+ // fail the download so the user can try again. >+ (void)FailDownload(rv, nsnull); Just making sure, is this the right message to alert to the user? "The download cannot be saved because an unknown error occurred. Please try again." Failing to CreateUnique is probably going to fail if retried?
Attachment #403550 - Flags: review?(edilee) → review+
(In reply to comment #50) > Just making sure, is this the right message to alert to the user? We can get a different string, but I'd like to do that in a follow-up so we can have one patch that applies to all branches if that's OK with you. > Failing to CreateUnique is probably going to fail if retried? Note, we'd display this error for anything that ExecuteDesiredAction would fail on.
Whiteboard: [needs review Mardak][sg:low local] moderate-to-critical on a shared unix machine → [sg:low local] moderate-to-critical on a shared unix machine
(In reply to comment #51) > We can get a different string, but I'd like to do that in a follow-up so we can > have one patch that applies to all branches if that's OK with you. Sure. We can even make use of the nsresult aStatus given to FailDownload to figure out what message to show ;)
http://hg.mozilla.org/mozilla-central/rev/cbbc962fcc69 I'll wait to land this on branch[es] until this gets verified as fixed by the submitter.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I can confirm the issue is fixed in the nightly build. Good job.
VERIFIED per comment 54. I'll land this on branch tomorrow assuming we have a cooperative tree.
Status: RESOLVED → VERIFIED
When will offical patches and/or updates be issued addressing this vulnerability?
(In reply to comment #55) > VERIFIED per comment 54. I'll land this on branch tomorrow assuming we have a > cooperative tree. What about 1.9.1 and 1.9.0? Does the current patch apply fine on those branches? If so, please request approval.
(In reply to comment #56) > When will offical patches and/or updates be issued addressing this > vulnerability? That depends on when this lands on the 1.9.0 and 1.9.1 branches. If comment #57 can happen quickly and the patch is approved, it can probably make the upcoming Firefox 3.0.15/3.5.4 release train in late October. Otherwise, it'll be the next release after that. You can see our upcoming releases, code freeze dates, etc. at https://wiki.mozilla.org/Releases.
Comment on attachment 403550 [details] [diff] [review] v1.0 (In reply to comment #57) > What about 1.9.1 and 1.9.0? Does the current patch apply fine on those > branches? If so, please request approval. I had made those changes, but forgot to click Submit. Doh! This patch should apply to all branches.
Attachment #403550 - Flags: approval1.9.1.5?
Attachment #403550 - Flags: approval1.9.0.16?
Attachment #403550 - Flags: approval1.9.1.4?
Attachment #403550 - Flags: approval1.9.0.15?
Depends on: 301328
blocking1.9.1: .5+ → .4+
No longer depends on: 301328
Flags: blocking1.9.0.16? → blocking1.9.0.15+
Depends on: 301328
Comment on attachment 403550 [details] [diff] [review] v1.0 Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers Please land today
Attachment #403550 - Flags: approval1.9.1.5?
Attachment #403550 - Flags: approval1.9.1.4?
Attachment #403550 - Flags: approval1.9.1.4+
Attachment #403550 - Flags: approval1.9.0.16?
Attachment #403550 - Flags: approval1.9.0.15?
Attachment #403550 - Flags: approval1.9.0.15+
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; new revision: 1.187; previous revision: 1.186 Checking in uriloader/exthandler/nsExternalHelperAppService.cpp; new revision: 1.372; previous revision: 1.371
Keywords: fixed1.9.0.15
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: