Closed Bug 417780 Opened 12 years ago Closed 12 years ago

content-disposition with filename containing "./" attempts to create temporary file called "/tmp"

Categories

(Firefox :: File Handling, defect, P2)

x86
Linux
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: taviso, Assigned: dveditz)

References

()

Details

(Keywords: testcase, verified1.8.1.13)

Attachments

(3 files)

Attached image screenshot
testcase provided at URL.

Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre
Keywords: testcase
This is certainly unexpected behavior, and would have slightly worse effect if running as root.  By itself, it's not clear to me that this should block Firefox3, but there certainly be affiliated exploits that should...
Flags: blocking-firefox3?
s/certainly be/certainly could be/
This will not block the final release of Firefox 3.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Or maybe it will.
Flags: blocking-firefox3- → blocking-firefox3+
Priority: -- → P2
The problem is here
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp&rev=1.290.4.18&mark=1617-1618#1608
(using the 1.8 branch version because it's clearer).

The extension, taken from the content-disposition header, is "./"

nsLocalFileWin::Append() fails due to the '/' character (there's checks for that and a couple other things). We don't check for errors and don't notice the file remains "c:\tmp". We then try to create a unique file based on "c:\tmp" -- but that's already a directory so we fail to create it (it gets a different status from NS_ERROR_FILE_ALREADY_EXISTS so it gives up). We don't check the error here either so we try to open an output stream to c:\tmp, and finally notice the failure. Then we try to rmdir c:\tmp, but that fails because the directory isn't empty.

The problem can be safely band-aided at this location. It would probably be better to make sure mMimeInfo didn't have bogus illegal file characters in it in the first place but that touches more code and might have more unexpected effects.
Assignee: nobody → dveditz
Flags: blocking-firefox3+
Priority: P2 → --
Attached patch 1.8 branch patchSplinter Review
Attachment #304929 - Flags: superreview?(benjamin)
Attachment #304929 - Flags: review?(benjamin)
Attached patch same for trunkSplinter Review
Attachment #304931 - Flags: superreview?(benjamin)
Attachment #304931 - Flags: review?(benjamin)
Comment on attachment 304931 [details] [diff] [review]
same for trunk

I'd like to get a second sr for this.
Attachment #304931 - Flags: superreview?(neil)
Attachment #304931 - Flags: superreview?(benjamin)
Attachment #304931 - Flags: review?(benjamin)
Attachment #304931 - Flags: review+
Attachment #304929 - Flags: superreview?(neil)
Attachment #304929 - Flags: superreview?(benjamin)
Attachment #304929 - Flags: review?(benjamin)
Attachment #304929 - Flags: review+
Comment on attachment 304931 [details] [diff] [review]
same for trunk

>+  rv = mTempFile->Append(NS_ConvertUTF8toUTF16(tempLeafName));
>   // make this file unique!!!
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = mTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
>+  NS_ENSURE_SUCCESS(rv, rv);
Strange comment placement ;-)
Attachment #304931 - Flags: superreview?(neil) → superreview+
Attachment #304929 - Flags: superreview?(neil) → superreview+
(In reply to comment #5)
>It would probably be better to make sure mMimeInfo didn't
>have bogus illegal file characters in it in the first place
Bug filed, I hope ;-)
Actually the bogus extension is courtesy of GetFilenameAndExtensionFromChannel - it simply chops off at the last . without extracting the basename first.
So, as well as validating the return from GetPrimary Extension, we could:
a) Improve GetFilenameFromDisposition so that it doesn't return a path
b) Improve GetFilenameAndExtensionFromChannel
either by cutting or replacing slashes with underscores.
Whiteboard: [has patch][has reviews]
Priority: -- → P2
Attachment #304929 - Flags: approval1.8.1.13?
Fix checked into trunk
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Comment on attachment 304929 [details] [diff] [review]
1.8 branch patch

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #304929 - Flags: approval1.8.1.13? → approval1.8.1.13+
Checked into branch
Keywords: fixed1.8.1.13
Verified for 1.8 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/2008031114 Firefox/2.0.0.13.
Verified on Trunk also: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008041604 Minefield/3.0pre

This seems like something we would not want to regress, I'll flag the in-testsuite ? as well, though I think it might be a rather difficult mochitest to write.  It might be easier to add it to Litmus.

--> Verified
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.