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

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P2)

Description

[Tavis Ormandy]

Attachment: screenshot

testcase provided at URL.

Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

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...

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

s/certainly be/certainly could be/

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

This will not block the final release of Firefox 3.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Or maybe it will.

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: 1.8 branch patch

Comment 7

[Daniel Veditz [:dveditz]]

Attachment: same for trunk

Comment 8

[Benjamin Smedberg]

Comment on attachment 304931 [details] [diff] [review]
same for trunk

I'd like to get a second sr for this.

Comment 9

[[e-mail]]

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 ;-)

Comment 10

[[e-mail]]

(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 ;-)

Comment 11

[[e-mail]]

Actually the bogus extension is courtesy of GetFilenameAndExtensionFromChannel - it simply chops off at the last . without extracting the basename first.

Comment 12

[[e-mail]]

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.

Comment 13

[Daniel Veditz [:dveditz]]

Fix checked into trunk

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 304929 [details] [diff] [review]
1.8 branch patch

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 15

[Daniel Veditz [:dveditz]]

Checked into branch

Comment 16

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 17

[cmtalbert]

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