1556230 - (CVE-2019-11720) Non-BMP code points that are WhiteSpace/LineTerminator when truncated to char16_t (e.g. U+4000D -> U+000D) should not be considered WhiteSpace

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Rakesh]

Tested On :

• Browser : Firefox Quantum Version 67.0 (64-bit)
• OS : macOS Mojave Version 10.14.5 Beta

Issue:
The character "\uD8C0\uDC0D" is ignored by the JS Parser. So below JS code is executable in Firefox browser.
For Example :
񀀍alert񀀍(񀀍) //It is equivalent to alert()
document񀀍.񀀍domain // It is equivalent to document.domain

Impact: (Security Impact) :
The impact is most of WAF or XSS filters are built on the basis of defined specifications of HTML and Javascript. So it is unlikely to consider the behaviour of "\uD8C0\uDC0D" character while developing any WAF. So this leaves most of existing WAF's vulnerable to XSS even though they are perfectly secure for browsers like Chrome or Safari. I have used this unexpected behaviour of Firefox to bypass few popular WAF's.

Cloudflare Bypass : https://coinmarketcap.com/?<img%20src=x%20onerror=񀀍alert񀀍()>
Akamai Bypass : https://www.aliexpress.com/?<img%20src=x%20onerror=񀀍alert񀀍()>

The number of ways this behaviour can be exploited are endless and depends of attackers creativity.

Proof Of Concept:

1. Open console and type below Payload and see it getting executed without any error.
   Payload : 񀀍alert񀀍(񀀍document񀀍.񀀍domain񀀍) // It should alert "document.domain" property.
2. Save below HTML code and open in Firefox browser :
   HTML code : <script>񀀍alert񀀍(񀀍document񀀍.񀀍domain񀀍)</script> // It should alert "document.domain" property.

Comment 1

[Rakesh]

JFY, Firefox Quantum ESR 60.6.0esr (latest) and latest version of Firefox for iOS are not affected by this issue.

Comment 2

[Rakesh]

Update : Latest version of Firefox for Android is also affected by this issue.

Comment 3

[:Gijs (out for now; he/him)]

I see "syntax error: illegal character" when trying to load

data:text/html,<script>alert(document%F1%80%80%8D.%F1%80%80%8Ddomain)</script>

(on current 68 beta)

so I don't think this reproduces for me. Can you re-test in a clean profile and check nightly/beta ? Perhaps attach a working testcase that exploits this? Neither the cloudflare nor aliexpress links alert anything for me, either.

Comment 4

[Rakesh]

Hi Gijs,

I have just tested this issue on Firefox Beta (Version 68.0b6) with clean profile and I can confirm that this issue is working in this browser as well.

I'm not sure why "data:text/html,<script>alert(document%F1%80%80%8D.%F1%80%80%8Ddomain)</script>" doesn't work.
But for your convenience I have setup a demo page where you can reproduce this issue directly. Simply open below URL and you'll see the result :
URL : http://bounters.team/test.php?payload=<img src=x onerror=񀀍alert񀀍(񀀍document񀀍.domain񀀍)񀀍>

Regarding Cloudflare or Akamai link, they do not alert because there is no XSS. I have given those links to show that WAF allows the payload to pass through which is enough to prove that it bypasses the WAF. If you try normal payload like "https://coinmarketcap.com/?<img%20src=x%20onerror=alert()>" or "https://www.aliexpress.com/?<img%20src=x%20onerror=alert()>" the WAF will block the request and show error page. Anyways I have mentioned this WAF thing just to give you an idea of impact this issue can have on the Security of Firefox.

Let me know if you need more information. Thanks.

Regards,
Rakesh Mane

Comment 5

[Rakesh]

Update : The payload data:text/html,<script>alert(document%F1%80%80%8D.%F1%80%80%8Ddomain)</script> which you have tried was not working because browser doesn't know which charset to use for decoding "%F1%80%80%8D". You can see the error message in console itself that clearly states The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol.

Try below below payload (which has utf-8 charset value) to see the alert :

data:text/html;charset=utf-8,<script>alert(document%F1%80%80%8D.%F1%80%80%8Ddomain)</script>

Comment 6

[Jeff Walden [:Waldo]]

Forcibly specifying UTF-8 charset is definitely a proper thing that will ensure correct interpretation here. Sane authors ought do this.

But how is an un-charset'd data: URL supposed to behave? I honestly don't know any more. There's some sort of sniffing, I think that gets invoked sometimes, but I do not understand how the spec algorithms work and I'm not honestly sure I know precisely where to look to answer the question. My best guess would be that Windows-1252 should be applied -- and then you would hit the syntax errors, as expected. But really I am not the right expert to ask to give a confident answer here. I will try to poke someone who might be.

Comment 7

[Jeff Walden [:Waldo]]

hsivonen, spec-wise what encoding is supposed to be applied to data: URLs without a specified charset? My guess would be Windows-1252 (and therefore this bug is not valid), but I dunno if content that is actually UTF-8 will get sniffed into being understood as UTF-8 per latest specs.

Comment 8

[Rakesh]

I don’t understand how data URL defaulting to any (US-ASCII,UTF-8,windows-1252,etc) charset makes this bug invalid. The bug which I reported here is related to JS Parser, not the data URL handling. TBH, the behaviour of data URL is totally irrelevant for this report and I fail to understand why it’s even being discussed here. :/

Comment 9

[Jeff Walden [:Waldo]]

The JS engine only sees script source text as handed to it by the browser, depending upon the means used to execute that script -- if it's <script> then any Content-Type encoding directive or perhaps the encoding of the document, for example. If the HTML means that what the engine is passed, is actually invalid script text, then the JS engine properly ought error -- garbage in, garbage out.

Tho, as I reread here I guess data: URLs are not relevant, they were a separate attempt to provide an explanation of the original reported behavior? If the complaint really is that the JS engine is actually executing that script, that would be garbage-in, garbage-out -- if the browser interprets the HTML text as saying a script should be created/executed at the appropriate time, that is what the JS engine will do. If no script should be executed, the HTML spec should be changed/altered to say that no script is executed (assuming it does not say so now). But then this would be an HTML bug, not a JavaScript bug.

Comment 10

[Rakesh]

Thanks for the explanation. I do not have much knowledge about how browsers work internally. However I would like to point out that the reported behaviour can also be observed by directly running the payload (POC Step 1) in console of developer tools. In that case there would be no part of HTML nor document encoding since console would directly pass the payload (in UTF-8 by default) to JS Engine? So that makes it JS engine bug? But I’m not about that though.

Comment 11

[Jeff Walden [:Waldo]]

The console itself is another sort of weird beast that is deceptively simple-looking but doesn't necessarily line up with your actual expectations in weird cases.

...or, wait. I misread all this. What you're complaining about, is that a script that contains U+4000D as a code point, doesn't error but instead interprets it -- despite that U+4000D ought not be allowed here. Never mind anything else previously said, yeah, this is weird. Looking now that I actually have properly read and understood things...

Comment 12

[Rakesh]

Yes. The actual issue I reported is about "\uD8C0\uDC0D" character gets ignored by JS Parser.
TBH, it doesn’t really gets completely ignored, it just acts like an empty multiline comment (Example : /**/)

Example : //alert//(//)//
In above example you can put "\uD8C0\uDC0D" character at the place of empty multiline comments and the code would still execute.

:)

Comment 13

[Rakesh]

Ok. The markdown changed the complete meaning of the example code. So adding it here again.
Example code : /**/alert/**/(/**/)/**/

Comment 14

[Jeff Walden [:Waldo]]

Okay, this looks to be fallout from the TokenStream adaptations to allow us to tokenize both UTF-8 and UTF-16. Namely, this revision:

https://hg.mozilla.org/mozilla-central/rev/36f3b5eb7294925bd97e77a9023234210bad928c

Before that revision we would invoke unicode::IsSpaceOrBOM2 on int c = sourceUnits.getCodeUnit(); -- which is either a valid unit value or EOF, and here EOF is implicitly excluded by prior operations, so it would be a valid char16_t unit. (Or a code unit of UTF-8, but UTF-8 support was far from complete then and not shipped anywhere and not even used for any purely internal cases like jsshell/xpcshell.)

After that revision we would invoke unicode::IsSpaceOrBOM2 on int32_t codePoint; filled with a non-ASCII code point by getNonAsciiCodePoint(unit, &codePoint) -- which is a valid code point, i.e. char32_ in the range [U+0080, U+10FFFF] (plus U+000A for line normalization). But unicode::IsSpaceOrBOM2 still takes char16_t! Things worked before because a surrogate would be treated as not-space-or-bom2. But now things don't work because a full char32_ gets truncated to its lower 16 bits, and if those bits indicate/encode space-or-bom2, the code point will be treated as whitespace. Boooooooooooooooooooooooooooooooooooooooooooooooo.

(Current code looks yet further different from that revision, but I don't believe semantics change in a way that would affect this.)

So what's needed here, is a full Unicode version of is-space-or-bom2, then we use that here. (Also we probably should audit all callers of unicode::IsSpaceOrBOM2 to be sure no one else is passing a full code point and getting a truncation.) I don't know immediately if we have one lying around. A very stopgap fix would be to just say anything outside BMP is not space-or-bom2, which would at least not allow stuff like U+4000D through. I am not certain offhand whether that would be correct as far as the spec goes in all cases, tho.

Taking, will post next with some sort of patch, hopefully. Could be a stopgap while I think harder about the fullest complete fix, if it comes to it.

Comment 15

[Rakesh]

Nice.
So will you consider it a Security bug or a Functional bug? :)

Comment 16

[Jeff Walden [:Waldo]]

I just fix things (...and try not to break them in the first place, and sometimes fail at that). Could be something XSSy is a security concern (that'd be my bet, tbh), but for my purposes I devote zero thought (especially now) to deciding how it ought be categorized. :-) Other people take care of that, and I leave them to it.

In short: patience, grasshopper. :-)

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to Jeff Walden [:Waldo] from comment #7)

hsivonen, spec-wise what encoding is supposed to be applied to data: URLs without a specified charset? My guess would be Windows-1252 (and therefore this bug is not valid), but I dunno if content that is actually UTF-8 will get sniffed into being understood as UTF-8 per latest specs.

I believe we handle unlabeled HTML over data: just like we handle unlabeled HTML over http:, i.e. the other than file:, the URL scheme doesn't matter for character encoding. We don't sniff UTF-8 for non-file: URLs, because if we did, authors would rely on it and the platform would become more brittle. (I know this doesn't answer your actual question of a spec trail.)

(FWIW, I think treating an unassigned code point as whitespace is a security bug.)

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Bug 1556230 - Handle Unicode spaces in JS better. r=arai

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Bug 1556230 - Add comments and a test. r=arai

Depends on D33674

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 9069728 [details]
Bug 1556230 - Handle Unicode spaces in JS better. r=arai

Beta/Release Uplift Approval Request

• User impact if declined: We'll recognize some bogus code points as whitespace when they should be syntax errors. Any script-text-sanitizing things that assume that if such garbage code points are passed to JS, an error will happen, may end up surprised when no error in fact happens. I somewhat lack domain expertise to say just how bad this could end up, but I'm inclined to say it is probably Not Good.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: The other rev here contains a test, but the test is a little more revealing than I'd prefer to land in a way that exposes "there's a security bug here". I plan to get all the approvals/reviews in this bug, then once I have a green light I will file a public bug, post the patch there (with bug number accordingly modified), get the perfunctory stamp, then land with hopefully people not clearly the wiser that this fixes a security issue.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Security Approval Request

• How easily could an exploit be constructed based on the patch?: If an affected filtery thing is known to the attacker, XSS should be pretty trivial.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Started in 62, so just beta/release and not ESR
• If not all supported branches, which bug introduced the flaw?: 1467336 (dependency deliberately not marked so as to not publicly raise suspicion)
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be trivial, in the unlikely event this patch doesn't apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It would be ideal if the automated test were landed with this, but security bug, so... :-(

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 9069728 [details]
Bug 1556230 - Handle Unicode spaces in JS better. r=arai

this was rated sec-moderate so doesn't require sec-approval

Comment 22

[Jeff Walden [:Waldo]]

Fixed by the landing in cover bug 1557778. This still deserves backporting, tho, so approvals/&c. here are still desired.

Comment 23

[Jeff Walden [:Waldo]]

(and at some remove the tests/comments will want to be landed, but bug 1556817 is going to build on top of bug 1557778's landing first, so the test/comment changes here will get a rebase/update well before then)

Comment 24

[Rakesh]

Good work. Can anyone confirm if it’s eligible for bounty?

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Rakesh from comment #24)

Good work. Can anyone confirm if it’s eligible for bounty?

The sec-bounty flag is set to "?" already. That means the bounty committee will make the decision at some point and change the flag. These things may take some time.

See https://wiki.mozilla.org/Security_Severity_Ratings

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 9069728 [details]
Bug 1556230 - Handle Unicode spaces in JS better. r=arai

sec fix for 68.0b10

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2a684be8b912

Comment 28

[Rakesh]

Hi,
As the bug is resolved and the similar bug 1556817 is already public, is it fine if I disclose this bug details?

Comment 29

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

(In reply to Rakesh from comment #28)

Hi,
As the bug is resolved and the similar bug 1556817 is already public, is it fine if I disclose this bug details?

Not until we ship the fix in Firefox 68. We haven't shipped the fix yet. "Fixed" here means fixed in mozilla-central, not fixed in a release that's gone out to our normal users.

Comment 30

[Rakesh]

Okay. Fine. :)

Comment 31

[Maria Berlinger [:mberlinger], Services QA]

I’ve managed to reproduce this issue on an affected Firefox 67.0b12 using Windows 10x64 by accessing the following links the Alert pop-up is displayed:

• http://bounters.team/test.php?payload=<img src=x onerror=񀀍alert񀀍(񀀍document񀀍.domain񀀍)񀀍>
• http://bounters.team/test.php?payload=<img
• data:text/html;charset=utf-8,<script>alert(document%F1%80%80%8D.%F1%80%80%8Ddomain)</script>

This is verified fixed on Firefox 68.0b12 and Firefox 69.0a1 (2019-06-21) on the following OSes: Windows 10x64, Windows 7x86, macOS 10.14, Ubuntu 18.04x64 and the pop-up Alert isn't displayed.

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/f4fb34ec3753e04baf546cefeff21b6161703d59
https://hg.mozilla.org/mozilla-central/rev/f4fb34ec3753