Bug 511521 (CVE-2009-3376)

downloading file with RTL override (RLO) presents conflicting filenames

RESOLVED FIXED in mozilla1.9.3a1

Status

Core Graveyard
File Handling
P1
normal
RESOLVED FIXED
8 years ago
10 months ago

People

(Reporter: geekboy, Assigned: Ehsan)

Tracking

({verified1.8.1.24, verified1.9.0.15, verified1.9.1})

Trunk
mozilla1.9.3a1
verified1.8.1.24, verified1.9.0.15, verified1.9.1
Bug Flags:
blocking1.9.2 +
blocking1.9.0.15 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

(Whiteboard: [sg:low])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 395411 [details]
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.
(Reporter)

Comment 1

8 years ago
Created attachment 395412 [details]
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.
(Reporter)

Comment 2

8 years ago
Created attachment 395413 [details]
dialog presented when downloading S[RLO]iva.exe on Vista

Comment 3

8 years ago
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").
(Reporter)

Comment 4

8 years ago
Created attachment 395622 [details] [diff] [review]
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.

Updated

8 years ago
Whiteboard: [sg:moderate]
(Reporter)

Comment 5

8 years ago
Created attachment 395626 [details]
screenshot of downloaded file in download manager and in Mac Finder

Comment 6

8 years ago
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.
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".
(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.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Created attachment 395825 [details] [diff] [review]
Patch (v1)
Attachment #395622 - Attachment is obsolete: true
Attachment #395825 - Flags: review?(bzbarsky)
Component: General → File Handling
Product: Firefox → Core
QA Contact: general → file-handling
Version: 3.5 Branch → Trunk

Comment 10

8 years ago
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
(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?
Attachment #395825 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/b712ab888d9b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
status1.9.1: --- → ?
Flags: wanted1.9.0.x?
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Flags: wanted1.9.2?
Attachment #395825 - Flags: approval1.9.2?
Attachment #395825 - Flags: approval1.9.1.3?
Attachment #395825 - Flags: approval1.9.0.14?
Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Pushing out approval requests to releases that aren't currently frozen.
Attachment #395825 - Flags: approval1.9.1.4?
Attachment #395825 - Flags: approval1.9.1.3?
Attachment #395825 - Flags: approval1.9.0.15?
Attachment #395825 - Flags: approval1.9.0.14?
blocking1.9.1: --- → ?
status1.9.1: ? → wanted
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.15?

Comment 14

8 years ago
(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.
blocking1.9.1: ? → .4+
Flags: blocking1.9.0.15? → blocking1.9.0.15+
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)
Attachment #395825 - Flags: review?(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]).
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.
Created attachment 397522 [details]
S[RLO]iva.bin
Attachment #397522 - Attachment filename: Siva.bin → Siva.bin
Attachment #397522 - Attachment mime type: application/octet-stream → video/mpeg
Attachment #397522 - Attachment mime type: video/mpeg → application/zip
Attachment #397522 - Attachment filename: Siva.bin → Siva.bin
(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 on attachment 395825 [details] [diff] [review]
Patch (v1)

Clearing the approval requests based on the assigned blocking flags.
Attachment #395825 - Flags: approval1.9.1.4?
Attachment #395825 - Flags: approval1.9.0.15?
This should block 1.9.2 as well I think.
Flags: wanted1.9.2? → blocking1.9.2?
(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 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...
Attachment #395825 - Flags: approval1.9.1.4?
Attachment #395825 - Flags: approval1.9.0.15?
smontagu: are you satisfied with the answer in comment 17?
Whiteboard: [sg:moderate] → [sg:moderate][needs r=smontagu]
Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

together with comment 19, yes :)
Attachment #395825 - Flags: review?(smontagu) → review+
Whiteboard: [sg:moderate][needs r=smontagu] → [sg:moderate]
Attachment #395825 - Flags: approval1.9.1.4?
Attachment #395825 - Flags: approval1.9.1.4+
Attachment #395825 - Flags: approval1.9.0.15?
Attachment #395825 - Flags: approval1.9.0.15+
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
Landed on 1.9.1:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac59325cf0a9
status1.9.1: wanted → .4-fixed
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
Keywords: fixed1.9.0.15
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
Attachment #395825 - Flags: approval1.9.2? → approval1.9.2+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9270914d2bda
status1.9.2: --- → beta1-fixed
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).
Keywords: fixed1.9.0.15 → verified1.9.0.15
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).
Keywords: verified1.9.1
The filename is shown with the actual .exe extension in enough places that this is more of a "low" than a moderate.
Whiteboard: [sg:moderate] → [sg:low]
Alias: CVE-2009-3376
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Attachment #395825 - Flags: approval1.8.1.next?
Flags: blocking1.8.1.next? → blocking1.8.1.next+
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.
Attachment #395825 - Flags: approval1.8.1.next? → approval1.8.1.next+

Comment 34

7 years ago
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
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.
Keywords: fixed1.8.1.24
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.
(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?
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.
(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.
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

7 years ago
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.
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.
Keywords: fixed1.8.1.24 → verified1.8.1.24
Depends on: 565554
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.