Closed Bug 417780 Opened 12 years ago Closed 12 years ago
content-disposition with filename containing "./" attempts to create temporary file called "/tmp"
15.11 KB, image/png
1.49 KB, patch
|Details | Diff | Splinter Review|
2.50 KB, patch
|Details | Diff | Splinter Review|
testcase provided at URL. Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre
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...
s/certainly be/certainly could be/
This will not block the final release of Firefox 3.
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
Priority: P2 → --
Comment on attachment 304931 [details] [diff] [review] same for trunk I'd like to get a second sr for this.
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.
Attachment #304929 - Flags: approval126.96.36.199?
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 188.8.131.52, a=dveditz for release-drivers
Attachment #304929 - Flags: approval184.108.40.206? → approval220.127.116.11+
Checked into branch
Verified for 1.8 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168) Gecko/2008031114 Firefox/22.214.171.124.
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
You need to log in before you can comment on or make changes to this bug.