511521 - (CVE-2009-3376) downloading file with RTL override (RLO) presents conflicting filenames

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Sid Stamm [:geekboy or :sstamm]]

Attachment: dialog presented when downloading S[RLO]iva.exe on Mac

When downloading a file with the unicode RTL override character (RLO) in it ("S[RLO]iva.exe"), the file download dialog presents two different filenames: one in the title ("Sexe.avi") and one in the content of the dialog ("Savi.exe").  The RLO character is stripped for the Save dialog later presented. 

This is inconsistent use of the RLO unicode character U+202E.  Perhaps it should just be removed from any downloaded filename.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: the file S[RLO]iva.exe (unless RLO gets stripped on upload)

This is a text file with a filename containing RLO.  It should display as Siva.exe when RLO is stripped out or ignored, and Sexe.avi when RLO is honored.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: dialog presented when downloading S[RLO]iva.exe on Vista

Comment 3

[Jesse Ruderman]

We should strip RLO (at least) from downloaded files, since Windows Explorer and Mac Finder both make it look like you have a .avi file (if you look at the extension rather than the "type").

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

This patch strips out \u202E in the default file name in the file chooser dialog and in the display string/title string in the init dialog displayed when the download link is clicked.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: screenshot of downloaded file in download manager and in Mac Finder

Comment 6

[Jesse Ruderman]

CCing some bidi people who might have ideas about how common RLO is in real filenames, and whether there are any other characters we should strip from download filenames.

Comment 7

[Behnam Esfahbod [:zwnj]]

A generic solution would be removing all LRE, RLE, LRO, RLO, and PDF, as any of the first four characters may have bad effects (confusing looks or security issues) if the relevant PDF is missing.  These characters are disallowed in IDNA (also IDNAbis) as well.

(In reply to comment #4)
> Created an attachment (id=395622) [details]
> proposed patch

It's better to remove these from suggestedFileName, to prevent further problems with names like: "[RLO].txt".

Comment 8

[(no longer active)]

(In reply to comment #6)
> CCing some bidi people who might have ideas about how common RLO is in real
> filenames, and whether there are any other characters we should strip from
> download filenames.

RLO and the rest of these control characters are not at all common in real file names, so I second Behnam's suggestion of removing them altogether.

I have a patch which handles this in the helper app service, with a unit test.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

Comment 10

[Chris Weber]

Jumping in with Jesse's suggestion.  I wanted to add some other perspective if I could.

If you were to strip RLO from filenames, you'd clearly be risking data-loss, and may even open the software up to other attacks – e.g. attackers could now bypass other content filters, anti-virus etc. with files like: nasty[RLO].[RLO]e[RLO]x[RLO]e.  Although from your diff it looks like you're proposing to replace the RLO with the _ character.

Another idea would be to change the UI display in cases where the end user must make a security decision, such as a file download dialog box.

If you do go with the removing/replacing change, then you should also at theRLM and LRM to your list of unsafeBidiCharacters:

U+200E LEFT-TO-RIGHT MARK
U+200F RIGHT-TO-LEFT MARK

Comment 11

[(no longer active)]

(In reply to comment #10)
> Jumping in with Jesse's suggestion.  I wanted to add some other perspective if
> I could.
> 
> If you were to strip RLO from filenames, you'd clearly be risking data-loss,
> and may even open the software up to other attacks – e.g. attackers could now
> bypass other content filters, anti-virus etc. with files like:
> nasty[RLO].[RLO]e[RLO]x[RLO]e.  Although from your diff it looks like you're
> proposing to replace the RLO with the _ character.

Yes, indeed removing those characters would be a mistake - we always replace invalid characters with '_' (and so does this patch).

> Another idea would be to change the UI display in cases where the end user must
> make a security decision, such as a file download dialog box.
> 
> If you do go with the removing/replacing change, then you should also at theRLM
> and LRM to your list of unsafeBidiCharacters:
> 
> U+200E LEFT-TO-RIGHT MARK
> U+200F RIGHT-TO-LEFT MARK

Hmm, why?  RLM and LRM don't change the representation of strong LTR/RTL characters (for example, latin alphabets), do they?  Do you have any example in which these characters can be used for spoofing?

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b712ab888d9b

Comment 13

[Samuel Sidler (old account; do not CC)]

Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Pushing out approval requests to releases that aren't currently frozen.

Comment 14

[Chris Weber]

(In reply to comment #11)
I thought I had a test case for the LRM and RLM that produced files looking like "Sexe." but I can't find those cases and couldn't reproduce.  You're right these wouldn't change strong LTR/RTL characters but the case of symbols and punctionation I thought was different.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Requesting additional review from smontagu for a sanity-check on this security change. (new super-review guidelines seem to call for sr= on all security bug fixes; I can count bz as the sr=).

Do all (web-sourced) files we write to disk go through this path? (e.g. what if there's no file picker but the user auto-saves the file)

Comment 16

[Simon Montagu :smontagu]

I think it would probably be more solid to add the Bidi control characters to the macros at the end of xpcom/ds/nsCRT.h, as in the fix for bug 393488 (attachment 313060 [details] [diff] [review]).

Comment 17

[(no longer active)]

That's the approach I tried first, but FILE_ILLEGAL_CHARACTERS is a narrow character string, and some of its consumers use it on narrow strings.  I'm not sure how to add Unicode characters without breaking compatibility with existing callers.

Comment 18

[(no longer active)]

Attachment: S[RLO]iva.bin

Comment 19

[(no longer active)]

(In reply to comment #15)
> Do all (web-sourced) files we write to disk go through this path? (e.g. what if
> there's no file picker but the user auto-saves the file)

Yes, they all do.  Please see attachment 397522 [details] as an example (you need to configure your browser to always save zip files to disk without prompting.)

Comment 20

[(no longer active)]

Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Clearing the approval requests based on the assigned blocking flags.

Comment 21

[(no longer active)]

This should block 1.9.2 as well I think.

Comment 22

[Samuel Sidler (old account; do not CC)]

(In reply to comment #20)
> (From update of attachment 395825 [details] [diff] [review])
> Clearing the approval requests based on the assigned blocking flags.

Ehsan: Patches on the stable branches (1.9.1 and 1.9.0) require explicit approval before landing.

Comment 23

[(no longer active)]

Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

(In reply to comment #22)
> (In reply to comment #20)
> > (From update of attachment 395825 [details] [diff] [review] [details])
> > Clearing the approval requests based on the assigned blocking flags.
> 
> Ehsan: Patches on the stable branches (1.9.1 and 1.9.0) require explicit
> approval before landing.

Oh, sorry about that.  Restoring the flags...

Comment 24

[Daniel Veditz [:dveditz]]

smontagu: are you satisfied with the answer in comment 17?

Comment 25

[Simon Montagu :smontagu]

Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

together with comment 19, yes :)

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers

Comment 27

[(no longer active)]

Landed on 1.9.1:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac59325cf0a9

Comment 28

[(no longer active)]

Landed on 1.9:

Checking in uriloader/exthandler//nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.371; previous revision: 1.370
done
Checking in uriloader/exthandler//tests/mochitest/Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/tests/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml,v
done
Checking in uriloader/exthandler//tests/mochitest/test_unsafeBidiChars.xhtml;
/cvsroot/mozilla/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml,v  <--  test_unsafeBidiChars.xhtml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/mochitest/unsafeBidiFileName.sjs,v
done
Checking in uriloader/exthandler//tests/mochitest/unsafeBidiFileName.sjs;
/cvsroot/mozilla/uriloader/exthandler/tests/mochitest/unsafeBidiFileName.sjs,v  <--  unsafeBidiFileName.sjs
initial revision: 1.1
done

Comment 29

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9270914d2bda

Comment 30

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

Verified for 1.9.0.15 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.15pre) Gecko/2009091606 GranParadiso/3.0.15pre (.NET CLR 3.5.30729).

Comment 31

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

Verified for 1.9.1.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20090921 Shiretoko/3.5.4pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090921 Shiretoko/3.5.4pre (.NET CLR 3.5.30729).

Comment 32

[Daniel Veditz [:dveditz]]

The filename is shown with the actual .exe extension in enough places that this is more of a "low" than a moderate.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Approved for 1.8.1.24, a=dveditz for release-drivers

If the test harness on the 1.8 branch doesn't handle these tests please just check in the patch.

Comment 34

[[e-mail]]

Landed on 1.8.1:

Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.290.4.21; previous revision: 1.290.4.20
done

Comment 35

[(no longer active)]

Patch landed on 1.8.1.24 by dwitte:

<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/uriloader/exthandler&command=DIFF_FRAMESET&file=nsExternalHelperAppService.cpp&rev1=1.290.4.20&rev2=1.290.4.21&root=/cvsroot>

I did not include the tests, because the 1.8 branch does not provide the required infrastructure.

Comment 36

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

Interesting. I tried this in the Seamonkley 1.1 nightly (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.24pre) Gecko/20100225 SeaMonkey/1.1.19pre). The naming issue seems to be fixed but Seamonkey 1.1 hangs as it does the download after you tell it where to save the file. I don't know if this is an unrelated issue.

Comment 37

[(no longer active)]

(In reply to comment #36)
> Interesting. I tried this in the Seamonkley 1.1 nightly (Mozilla/5.0
> (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.24pre) Gecko/20100225
> SeaMonkey/1.1.19pre). The naming issue seems to be fixed but Seamonkey 1.1
> hangs as it does the download after you tell it where to save the file. I don't
> know if this is an unrelated issue.

Could you please attach a debugger and get a stack for the hang?

Comment 38

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

This isn't a debug build, it is the Seamonkey nightly. I have no idea as to how to get you a stack on OS X.

Comment 39

[(no longer active)]

(In reply to comment #38)
> This isn't a debug build, it is the Seamonkey nightly. I have no idea as to how
> to get you a stack on OS X.

If the build does not come with symbols, then you probably need a debug build.

Comment 40

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

I don't QA Seamonkey and I am not set up to build 1.8.1 in any case so I don't have access to a debug build of it. I only used it to test because it was a 1.8.1 based build that had an easy browser built in. 

Once we have our blocking Thunderbird issue (not this) fixed, I will check this bug in a TB build and hopefully verify it. If you wish to dig into the Seamoney side, perhaps Kairo can be of some help.

Comment 41

[Robert Kaiser]

Anyone wanting to look into that further probably needs to do his own build, we don't build debug or symbols on my private machines that do SeaMonkey 1.x builds.

Also, we will announce the 1.8.1.24-based SeaMonkey 1.1.19 as EOL release that contains known flaws but should at least be better than 1.1.18 for those who can't switch to the 2.0.x series. In that light, I'm not sure how much time one wants to spend on that.

We'll see what Al's Thunderbird testing results in.

Comment 42

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

The fix works on TB. I verified this in Thunderbird 2.0.0.24pre using Thunderbrowse on OS X: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.24pre) Gecko/2010022613 Thunderbird/2.0.0.24pre ThunderBrowse/3.2.8.1.