Last Comment Bug 511521 - (CVE-2009-3376) downloading file with RTL override (RLO) presents conflicting filenames
(CVE-2009-3376)
: downloading file with RTL override (RLO) presents conflicting filenames
Status: RESOLVED FIXED
[sg:low]
: verified1.8.1.24, verified1.9.0.15, verified1.9.1
Product: Core
Classification: Components
Component: File Handling (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9.3a1
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on: 565554
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-19 14:56 PDT by Sid Stamm [:geekboy or :sstamm]
Modified: 2010-05-12 18:44 PDT (History)
13 users (show)
benjamin: blocking1.9.2+
dveditz: blocking1.9.0.15+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.4+
.4-fixed


Attachments
dialog presented when downloading S[RLO]iva.exe on Mac (57.06 KB, image/png)
2009-08-19 14:56 PDT, Sid Stamm [:geekboy or :sstamm]
no flags Details
the file S[RLO]iva.exe (unless RLO gets stripped on upload) (14 bytes, application/octet-stream)
2009-08-19 15:01 PDT, Sid Stamm [:geekboy or :sstamm]
no flags Details
dialog presented when downloading S[RLO]iva.exe on Vista (29.59 KB, image/png)
2009-08-19 15:02 PDT, Sid Stamm [:geekboy or :sstamm]
no flags Details
proposed patch (1.91 KB, patch)
2009-08-20 11:24 PDT, Sid Stamm [:geekboy or :sstamm]
no flags Details | Diff | Review
screenshot of downloaded file in download manager and in Mac Finder (132.34 KB, image/png)
2009-08-20 11:30 PDT, Sid Stamm [:geekboy or :sstamm]
no flags Details
Patch (v1) (8.55 KB, patch)
2009-08-21 06:29 PDT, :Ehsan Akhgari (busy, don't ask for review please)
bzbarsky: review+
smontagu: review+
benjamin: approval1.9.2+
dveditz: approval1.9.1.4+
dveditz: approval1.9.0.15+
dveditz: approval1.8.1.next+
Details | Diff | Review
S[RLO]iva.bin (14 bytes, application/zip)
2009-08-30 05:13 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details

Description Sid Stamm [:geekboy or :sstamm] 2009-08-19 14:56:41 PDT
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.
Comment 1 Sid Stamm [:geekboy or :sstamm] 2009-08-19 15:01:03 PDT
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.
Comment 2 Sid Stamm [:geekboy or :sstamm] 2009-08-19 15:02:14 PDT
Created attachment 395413 [details]
dialog presented when downloading S[RLO]iva.exe on Vista
Comment 3 Jesse Ruderman 2009-08-19 15:16:02 PDT
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] 2009-08-20 11:24:37 PDT
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.
Comment 5 Sid Stamm [:geekboy or :sstamm] 2009-08-20 11:30:27 PDT
Created attachment 395626 [details]
screenshot of downloaded file in download manager and in Mac Finder
Comment 6 Jesse Ruderman 2009-08-20 11:35:55 PDT
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] 2009-08-20 12:15:02 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-21 06:28:00 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-21 06:29:05 PDT
Created attachment 395825 [details] [diff] [review]
Patch (v1)
Comment 10 Chris Weber 2009-08-21 14:17:09 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-23 00:11:44 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-25 02:48:16 PDT
http://hg.mozilla.org/mozilla-central/rev/b712ab888d9b
Comment 13 Samuel Sidler (old account; do not CC) 2009-08-25 14:01:43 PDT
Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Pushing out approval requests to releases that aren't currently frozen.
Comment 14 Chris Weber 2009-08-27 12:09:44 PDT
(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] 2009-08-28 11:07:20 PDT
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 2009-08-30 00:14:30 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-30 04:58:37 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-30 05:13:12 PDT
Created attachment 397522 [details]
S[RLO]iva.bin
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-30 05:44:56 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-30 05:45:15 PDT
Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

Clearing the approval requests based on the assigned blocking flags.
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-30 05:45:59 PDT
This should block 1.9.2 as well I think.
Comment 22 Samuel Sidler (old account; do not CC) 2009-08-30 23:16:55 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2009-08-30 23:34:30 PDT
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] 2009-08-31 14:15:00 PDT
smontagu: are you satisfied with the answer in comment 17?
Comment 25 Simon Montagu :smontagu 2009-09-01 01:56:09 PDT
Comment on attachment 395825 [details] [diff] [review]
Patch (v1)

together with comment 19, yes :)
Comment 26 Daniel Veditz [:dveditz] 2009-09-02 15:23:42 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-09-03 00:35:55 PDT
Landed on 1.9.1:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac59325cf0a9
Comment 28 :Ehsan Akhgari (busy, don't ask for review please) 2009-09-03 00:46:40 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-09-15 03:45:53 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9270914d2bda
Comment 30 Al Billings [:abillings] 2009-09-16 17:30:21 PDT
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] 2009-09-21 15:30:40 PDT
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] 2009-10-16 16:19:31 PDT
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] 2009-12-21 14:45:15 PST
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 dwitte@gmail.com 2009-12-21 16:44:49 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-12-21 16:48:18 PST
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] 2010-02-25 14:05:35 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2010-02-25 14:13:34 PST
(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] 2010-02-25 14:55:35 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2010-02-25 17:58:28 PST
(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] 2010-02-25 22:28:10 PST
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 (not working on stability any more) 2010-02-26 04:24:40 PST
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] 2010-02-26 16:19:04 PST
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.

Note You need to log in before you can comment on or make changes to this bug.