1438025 - (CVE-2018-5173) browser.downloads.download should remove \u202E (RLO) chars

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Abdulrahman Alqabandi]

Attachment: 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"

Comment 1

[Daniel Veditz [:dveditz]]

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)

Comment 2

[Abdulrahman Alqabandi]

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

Comment 3

[Kris Maglione [:kmag]]

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

Comment 4

[Tomislav Jovanovic :zombie]

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

Comment 5

[Kris Maglione [:kmag]]

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

Comment 6

[Tomislav Jovanovic :zombie]

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

Comment 7

[Tomislav Jovanovic :zombie]

Attachment: fix-1438025.patch

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.

Comment 8

[Tomislav Jovanovic :zombie]

(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 9

[:Paolo Amadini]

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.

Comment 10

[Tomislav Jovanovic :zombie]

Attachment: fix-1438025-v2.patch

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

Comment 11

[Tomislav Jovanovic :zombie]

Attachment: test-1438025-v2.patch

Comment and test, for landing in 6 weeks.

Comment 12

[:Paolo Amadini]

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.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8962075 [details] [diff] [review]
fix-1438025-v2.patch

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

Thanks

Comment 14

[Tomislav Jovanovic :zombie]

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?

Comment 15

[Tomislav Jovanovic :zombie]

Attachment: fix-1438025-v3.patch

Comment 16

[Tomislav Jovanovic :zombie]

Attachment: test-1438025-v3.patch

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8962100 [details] [diff] [review]
test-1438025-v3.patch

carried over

Comment 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[Tomislav Jovanovic :zombie]

Thanks Daniel.

Comment 20

[Tomislav Jovanovic :zombie]

Comment on attachment 8962099 [details] [diff] [review]
fix-1438025-v3.patch

Sheriffs: please land only this patch, and leave the bug open.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962099 [details] [diff] [review]
fix-1438025-v3.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/9728993469256a713ec5e8b953f2ea01089d250c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/972899346925

Comment 23

[Tomislav Jovanovic :zombie]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962099 [details] [diff] [review]
fix-1438025-v3.patch

Approved for 60.0b8.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4702ec51711e

Comment 26

[Tomislav Jovanovic :zombie]

adding flag for myself..

Comment 27

[Emil Ghitta, QA [:emilghitta]]

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.