Closed Bug 1438025 (CVE-2018-5173) Opened 2 years ago Closed 2 years ago

browser.downloads.download should remove \u202E (RLO) chars

Categories

(WebExtensions :: Untriaged, defect, P2)

60 Branch
defect

Tracking

(firefox-esr52- wontfix, firefox59 wontfix, firefox60+ verified, firefox61+ verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 - wontfix
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: qab, Assigned: Tomislav)

Details

(Keywords: csectype-spoof, sec-moderate, Whiteboard: [adv-main60+][post-critsmash-triage])

Attachments

(4 files, 3 obsolete files)

Attached file PoC
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36

Steps to reproduce:

Added "\u202E" to a filename passed to downloads.download() results in a filename spoof.


Actual results:

Downloading with filename="test\u202Egpj.bat" results in a filename appearing to be "testtab.jpg" in the downloads panel.

When attempting to open this file, the prompt that appears reminding user that this is an executable is all reversed text and illegible. 


Expected results:

Strip "\u202E"
Is this really a "web extension" issue? Can you do the same thing using the HTML download= attribute or Content-Disposition: attachment? It's the same download panel AFAIK.

In the end wouldn't we warn that it's an executable type still? (yes, you've filed other bugs wrt that. I meant usually)
Flags: needinfo?(qab)
Attached image pef.png
(In reply to Daniel Veditz [:dveditz] from comment #1)
> Is this really a "web extension" issue? Can you do the same thing using the
> HTML download= attribute or Content-Disposition: attachment? It's the same
> download panel AFAIK.

Unless I am missing something but I could not reproduce using an anchor tag download or 'Content-Disposition' header. It looks like in those cases the RLO character is stripped. 


> In the end wouldn't we warn that it's an executable type still? (yes, you've
> filed other bugs wrt that. I meant usually)

Yes, the usual 'Open executable file?' prompt appears but like I mentioned its text is reversed since its trying to display the filename that contains the RLO char. (Check attached screenshot)

Also, mark of the web does apply here too.
Flags: needinfo?(qab)
Assignee: nobody → tomica
Priority: -- → P2
Zombie, I think this bug might interest you. There are a few other places where we need to encode special characters like this already:

https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/browser/base/content/browser.js#4415
https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/browser/base/content/browser.js#2799-2810

It would be nice to have a centralized helper to handle this, and then apply it in the above two places as well as the download manager UI (and probably a bunch of other places, too...)
Assignee: tomica → nobody
Flags: needinfo?(tomica)
> it in the above two places as well as the download manager UI
You don't think we should nerf the actual filename, only when we display it in the UI?  That seems prone to error.

> (and probably a bunch of other places, too...)
uh, that's a little open-ended.
Assignee: nobody → tomica
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(tomica)
(In reply to Tomislav Jovanovic :zombie from comment #4)
> > it in the above two places as well as the download manager UI
> You don't think we should nerf the actual filename, only when we display it
> in the UI?  That seems prone to error.

Don't particularly care about the filename. That's really up to the OS to deal with. But I don't really object to it.

> > (and probably a bunch of other places, too...)
> uh, that's a little open-ended.

It's a lot open ended, really. I'm not sure how far we should go with this, but it's potentially a problem anywhere we display strings from untrusted sources. Extension install dialogs, for instance.

I just mention it for completeness. At this point, I'm mainly just looking to fix this instance and have a starting point for fixing the rest.
I have a patch to escape BIDI characters in the Downloads Panel and the "Open Executable File" dialog, but after further investigation, I'm not sure that's the right place to handle this.

Quoting from section 4.1 of https://www.ietf.org/rfc/rfc3987.txt

>   [...]  IRIs MUST NOT contain bidirectional formatting characters
>   (LRM, RLM, LRE, RLE, LRO, RLO, and PDF).  They affect the visual
>   rendering of the IRI but do not appear themselves.  It would
>   therefore not be possible to input an IRI with such characters
>   correctly.

We seem to already %encode BIDI characters in URIs, and that probably explains why other approaches from comment 1 don't work, so the only remaining way they could appear in the Downloads UI is from extensions.

And since the whole design of the downloads API is to support functionality already possible using the browser, I don't think we should even try to encode/filter BIDI characters, but instead simply throw on them, just like we do for other special characters:

https://searchfox.org/mozilla-central/rev/b29daa/toolkit/components/extensions/ext-downloads.js#433-435
Attached patch fix-1438025.patch (obsolete) — Splinter Review
This patch strips all BIDI characters from the filename in the DownloadPaths.santize() method.

ext-downloads code will note that sanitized filename is different from the original, and throw:
https://searchfox.org/mozilla-central/rev/b29daa/toolkit/components/extensions/ext-downloads.js#433

I believe other non-extension uses of Downloads already strip or escape BIDI characters before getting to this stage, but in case some of them slip, this will serve as belt+suspenders.

No comments or tests in this patch, and the commit message is vague while technically correct.  I'm adding Paolo as a reviewer, since the patch is in toolkit/downloads code.
Attachment #8961201 - Flags: review?(kmaglione+bmo)
Attachment #8961201 - Flags: review?(paolo.mozmail)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #3)
> It would be nice to have a centralized helper to handle this, and then apply
> it in the above two places as well as the download manager UI

Forgot to note, I would rather refactor unrelated code in a followup bug (6 weeks after this), where I'll be able to add both tests and comments, and safely use Try.
Comment on attachment 8961201 [details] [diff] [review]
fix-1438025.patch

Looks good to me. I guess we may find out that we regress file names with mixed scripts, but it's probably a necessary compromise at the moment.

I don't see a reason not to land a test at the same time, since it's obvious which characters we're stripping anyways. In particular it can be useful to test that things are done in the right order, for example "\u200e \u202b\u202c\u202d\u202etest\x7f\u200f" should become "test" with no spaces around it.

We can still consider leaving detailed comments to a later patch, but we should definitely add them eventually, in order to explain why this change can't just be reverted to make mixed scripts work. I recommend preparing a separate patch with the comments.
Attachment #8961201 - Flags: review?(paolo.mozmail) → review+
Attached patch fix-1438025-v2.patch (obsolete) — Splinter Review
(In reply to :Paolo Amadini from comment #9)
> Looks good to me. I guess we may find out that we regress file names with
> mixed scripts, but it's probably a necessary compromise at the moment.

This won't regress most of filenames with mixed scripts.  I tested the "common" way they can appear in document titles, and words using "strong bidi characters" still preserve the correct ordering, for example:

    Your search "הצהרות" didn't match anything.

which is a Hebrew word in the middle of an English title, and works as expected.  What doesn't work is using explicit BIDI formatting characters, which is so rarely done, that my file manager (Total Commander) simply refuses to execute such files with the message:

    WARNING: The filename contains characters typical for a virus/worm.  Function aborted.

I believe we are in good company here.


> I don't see a reason not to land a test at the same time, since it's obvious
> which characters we're stripping anyways. 

Oh, my original instinct was to test this in web extensions test, which would clearly point to the current known way of exploiting this.  But I think landing a unit test for toolkit/downloads is much safer, so I included it in the updated patch.


> In particular it can be useful to test that things are done in the right order,
> for example "\u200e\u202b\u202c\u202d\u202etest\x7f\u200f" should become "test"
> with no spaces around it.

Good thinking.  This example did work as expected, but multiple illegal chars that get converted to space, separated by a bidi character didn't, so I reordered the operations in the updated patch.


> We can still consider leaving detailed comments to a later patch, but we
> should definitely add them eventually, in order to explain why this change
> can't just be reverted to make mixed scripts work. I recommend preparing a
> separate patch with the comments.

Added comments to the other patch that also includes the web extensions test (to land in 6 weeks).
Attachment #8961201 - Attachment is obsolete: true
Attachment #8961201 - Flags: review?(kmaglione+bmo)
Attachment #8962075 - Flags: review?(kmaglione+bmo)
Attachment #8962075 - Flags: review?(paolo.mozmail)
Attached patch test-1438025-v2.patch (obsolete) — Splinter Review
Comment and test, for landing in 6 weeks.
Attachment #8962076 - Flags: review?(kmaglione+bmo)
Comment on attachment 8962075 [details] [diff] [review]
fix-1438025-v2.patch

Review of attachment 8962075 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the details about mixed scripts in comment 10! I would add a short note in the other patch about these characters being rare and not necessary to render mixed scripts correctly.

::: toolkit/components/downloads/test/unit/test_DownloadPaths.js
@@ +79,5 @@
>    testSanitize(" . ", "");
> +
> +  // Stripping of BIDI formatting characters.
> +  testSanitize("\u200e\u202b\u202c\u202d\u202etest\x7f\u200f", "test");
> +  testSanitize("AB\x7f\u202e\x7f\u202e\x7fCD", "AB CD");

And thanks for the second test case and associated fix too!

Let's put \u202a somewhere for completeness, I forgot it in the example.
Attachment #8962075 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8962075 [details] [diff] [review]
fix-1438025-v2.patch

Review of attachment 8962075 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks
Attachment #8962075 - Flags: review?(kmaglione+bmo) → review+
Attachment #8962076 - Flags: review?(kmaglione+bmo) → review+
Hey Daniel, this bug is (likely) present in previous ESR 52, but the fix would need to be in a different part of the code, which more clearly hints at what the exploit is.

Since this is sec-moderate, Kris suggested we should only uplift to current beta, which will be the next ESR 60 in a ~month.  Are you ok with that?  And if so, does this bug need sec-approval to uplift to beta?
Flags: needinfo?(dveditz)
Attachment #8962075 - Attachment is obsolete: true
Attachment #8962076 - Attachment is obsolete: true
Attachment #8962099 - Flags: review+
Comment on attachment 8962100 [details] [diff] [review]
test-1438025-v3.patch

carried over
Attachment #8962100 - Flags: review+
(In reply to Tomislav Jovanovic :zombie from comment #14)
> Since this is sec-moderate, Kris suggested we should only uplift to current
> beta, which will be the next ESR 60 in a ~month.  Are you ok with that?

I can live with that (though note that ESR-52 will be supported for another 3 months after 60 is released)

> if so, does this bug need sec-approval to uplift to beta?

It doesn't need sec-approval to land (on nightly) but you do need to request normal approval to land on beta.
Thanks Daniel.
Keywords: leave-open
Comment on attachment 8962099 [details] [diff] [review]
fix-1438025-v3.patch

Sheriffs: please land only this patch, and leave the bug open.
Attachment #8962099 - Flags: checkin?
Comment on attachment 8962099 [details] [diff] [review]
fix-1438025-v3.patch

> Approval Request Comment
> [Feature/Bug causing the regression]:
N/A

> [User impact if declined]:
Filename extension spoofing by extensions.

> [Is this code covered by automated tests?]:
Yes

> [Has the fix been verified in Nightly?]:
Yes

> [Needs manual test from QE? If yes, steps to reproduce]: 
Not sure if QA could try downloading files from some popular RTL download websites (Arabic, Hebrew...), to check my assumptions from comment 10.

> [List of other uplifts needed for the feature/fix]:
None

> [Is the change risky?]:
Not exactly.

> [Why is the change risky/not risky?]:
We know it _will_ break some use-cases (see comment 10), but we believe they are rare, and have evaluated that is an acceptable trade-off in the name of security. 

> [String changes made/needed]:
None
Attachment #8962099 - Flags: approval-mozilla-beta?
Comment on attachment 8962099 [details] [diff] [review]
fix-1438025-v3.patch

Approved for 60.0b8.
Attachment #8962099 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/4702ec51711e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite?
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: sec-bounty?
Flags: in-testsuite?
adding flag for myself..
Flags: in-testsuite?
Flags: sec-bounty? → sec-bounty+
Group: toolkit-core-security → core-security-release
Whiteboard: [adv-main60+]
Alias: CVE-2018-5173
Flags: qe-verify+
Whiteboard: [adv-main60+] → [adv-main60+][post-critsmash-triage]
I have managed to reproduce this issue using Firefox 60.0a1 (BuildId:20180213220104).

This issue is verified fixed using Firefox 61.0a1 (BuildId:20180503220110) and Firefox 60.0 (BuildId:20180430165945) on Windows 10 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Toolkit → WebExtensions
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.