Last Comment Bug 514823 - (CVE-2009-3274) Incremental filename choosing scheme for duplicate filenames vulnerable (especially with the use of /tmp)
(CVE-2009-3274)
: Incremental filename choosing scheme for duplicate filenames vulnerable (espe...
Status: VERIFIED FIXED
[sg:low local] moderate-to-critical o...
: fixed1.9.0.15
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: mozilla1.9.3a1
Assigned To: Shawn Wilsher :sdwilsh
:
Mentors:
Depends on: 301328
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-05 00:22 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2009-10-30 08:01 PDT (History)
15 users (show)
mbeltzner: blocking1.9.2+
dveditz: blocking1.9.0.15+
dveditz: wanted1.9.0.x+
sdwilsh: in‑testsuite-
sdwilsh: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.4+
.4-fixed


Attachments
PoC (1.51 KB, text/plain)
2009-09-05 00:22 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details
Cancel download on createUnique failure (1.22 KB, patch)
2009-09-10 07:48 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
sdwilsh: review+
Details | Diff | Review
v1.0 (5.05 KB, patch)
2009-09-29 12:26 PDT, Shawn Wilsher :sdwilsh
edilee: review+
dveditz: approval1.9.1.4+
dveditz: approval1.9.0.15+
Details | Diff | Review
1.8.0 patch (1.18 KB, patch)
2009-10-12 07:09 PDT, Martin Stránský
no flags Details | Diff | Review

Description Reed Loden [:reed] (use needinfo?) 2009-09-05 00:22:44 PDT
Created attachment 398828 [details]
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!
Comment 1 Reed Loden [:reed] (use needinfo?) 2009-09-05 00:24:17 PDT
I think some of Jesse's proposed general download fixes in bug 369462 could help here, too.
Comment 2 Jeremy Brown 2009-09-08 18:28:25 PDT
Would it be alright if I could view bug 369462 as well? Any ideas on a fix here?
Comment 3 Jeremy Brown 2009-09-08 18:30:07 PDT
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?
Comment 4 Jesse Ruderman 2009-09-08 18:34:04 PDT
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.
Comment 5 Jeremy Brown 2009-09-08 19:33:37 PDT
"make downloads fail once there are too many sequentially named files" sounds good!
Comment 6 Brandon Sterne (:bsterne) 2009-09-09 10:14:54 PDT
Reporter publicly announced the bug:
http://securitytube.net/Zero-Day-Demos-%28Firefox-Vulnerability-Discovered%29-video.aspx
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-09 11:30:11 PDT
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...
Comment 8 Shawn Wilsher :sdwilsh 2009-09-09 11:42:32 PDT
(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.
Comment 9 Sid Stamm [:geekboy or :sstamm] 2009-09-09 14:37:23 PDT
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 10 Jeremy Brown 2009-09-09 19:26:14 PDT
@ 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?
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-09 20:36:04 PDT
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.
Comment 12 Jeremy Brown 2009-09-09 20:45:57 PDT
"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...
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-09 20:59:33 PDT
>"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....
Comment 14 Jeremy Brown 2009-09-09 21:15:24 PDT
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.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-09 21:27:56 PDT
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....
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-10 05:55:37 PDT
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.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-10 07:20:04 PDT
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.
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-10 07:33:05 PDT
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....
Comment 19 Reed Loden [:reed] (use needinfo?) 2009-09-10 07:39:50 PDT
(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...
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-10 07:47:01 PDT
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.
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-10 07:47:30 PDT
> Hmm, this reminds me of bug 478394 for some reason...

That reason being that it's the same issue?  ;)
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-10 07:48:44 PDT
Created attachment 399728 [details] [diff] [review]
Cancel download on createUnique failure

Shawn, I think the rest of this ball is in the download manager court.
Comment 23 Jesse Ruderman 2009-09-10 09:01:57 PDT
This bug reminds me of bug 459423, too :(
Comment 24 Shawn Wilsher :sdwilsh 2009-09-10 10:18:17 PDT
(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 25 Shawn Wilsher :sdwilsh 2009-09-10 10:20:03 PDT
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 26 Jeremy Brown 2009-09-10 10:43:52 PDT
@ 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?
Comment 27 Jesse Ruderman 2009-09-10 10:53:52 PDT
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"?
Comment 28 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-10 10:57:10 PDT
> 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.
Comment 29 Shawn Wilsher :sdwilsh 2009-09-10 11:00:52 PDT
(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.
Comment 30 Jeremy Brown 2009-09-11 13:35:38 PDT
So then do we have a modified patch?
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-11 19:09:40 PDT
This bug needs an owner familiar with the download manager front end (i.e. not me) for that to happen...
Comment 32 Jeremy Brown 2009-09-19 22:52:31 PDT
Any progress on a patch?
Comment 33 Reed Loden [:reed] (use needinfo?) 2009-09-22 00:36:37 PDT
Shawn / Mardak, can one of you help out on getting a dlmgr patch written for this?
Comment 34 Shawn Wilsher :sdwilsh 2009-09-22 10:44:13 PDT
(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.
Comment 35 Shawn Wilsher :sdwilsh 2009-09-24 14:44:01 PDT
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.
Comment 36 Jeremy Brown 2009-09-24 15:03:39 PDT
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.
Comment 37 Shawn Wilsher :sdwilsh 2009-09-24 15:08:35 PDT
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 :/
Comment 38 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-24 15:57:15 PDT
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.
Comment 39 Shawn Wilsher :sdwilsh 2009-09-24 16:04:51 PDT
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)?
Comment 40 Jesse Ruderman 2009-09-24 16:19:54 PDT
Is bug 301328 a security hole?
Comment 41 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-24 17:08:31 PDT
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.
Comment 42 Shawn Wilsher :sdwilsh 2009-09-28 12:14:06 PDT
(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.
Comment 43 Shawn Wilsher :sdwilsh 2009-09-28 13:57:10 PDT
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.
Comment 44 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-09-29 08:38:34 PDT
> 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?
Comment 45 Jeremy Brown 2009-09-29 09:10:35 PDT
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.
Comment 46 Shawn Wilsher :sdwilsh 2009-09-29 12:23:41 PDT
(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 47 Shawn Wilsher :sdwilsh 2009-09-29 12:24:21 PDT
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.
Comment 48 Shawn Wilsher :sdwilsh 2009-09-29 12:26:16 PDT
Created attachment 403550 [details] [diff] [review]
v1.0

Style nits from bz's patch, as well as the toolkit fix for this.
Comment 49 Jeremy Brown 2009-09-29 12:53:16 PDT
(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 50 Ed Lee :Mardak 2009-09-29 13:25:54 PDT
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?
Comment 51 Shawn Wilsher :sdwilsh 2009-09-29 13:38:47 PDT
(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.
Comment 52 Ed Lee :Mardak 2009-09-29 13:43:44 PDT
(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 ;)
Comment 53 Shawn Wilsher :sdwilsh 2009-09-29 14:23:15 PDT
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.
Comment 54 Jeremy Brown 2009-09-30 14:23:10 PDT
I can confirm the issue is fixed in the nightly build. Good job.
Comment 55 Shawn Wilsher :sdwilsh 2009-09-30 14:48:18 PDT
VERIFIED per comment 54.  I'll land this on branch tomorrow assuming we have a cooperative tree.
Comment 56 Jeremy Brown 2009-09-30 14:58:04 PDT
When will offical patches and/or updates be issued addressing this vulnerability?
Comment 57 Reed Loden [:reed] (use needinfo?) 2009-09-30 15:05:19 PDT
(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.
Comment 58 Reed Loden [:reed] (use needinfo?) 2009-09-30 15:07:28 PDT
(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 59 Shawn Wilsher :sdwilsh 2009-09-30 15:09:52 PDT
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.
Comment 60 Shawn Wilsher :sdwilsh 2009-10-01 12:40:35 PDT
On 1.9.2 for Firefox 3.6 beta 1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/55bc5854c810
Comment 61 Daniel Veditz [:dveditz] 2009-10-02 11:37:44 PDT
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
Comment 62 Shawn Wilsher :sdwilsh 2009-10-02 12:07:09 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5b2c91fc6055
Comment 63 Shawn Wilsher :sdwilsh 2009-10-02 12:35:22 PDT
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
Comment 64 Martin Stránský 2009-10-12 07:09:56 PDT
Created attachment 405850 [details] [diff] [review]
1.8.0 patch

Note You need to log in before you can comment on or make changes to this bug.