1402896 - (CVE-2017-7839) Specially-crafted JavaScript may be pasted into the address bar

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Eric Lawrence (@ericlaw)]

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

Steps to reproduce:

1. Visit https://www.bayden.com/test/controlchars.html
2. Select all of the text in the box on the page *except* the first character
3. Copy
4. Go to Address bar.
5. Hit paste.


Actual results:

JavaScript is pasted and executes if enter is hit.


Expected results:

Text up to and including "JavaScript:" should be stripped.

Firefox, like Chrome and IE, attempts to prevent the user from pasting JavaScript into the address bar.

Unfortunately, as discovered in https://crbug.com/766010, these protections can be circumvented by introducing leading control characters into the string.

Comment 1

[Eric Lawrence (@ericlaw)]

I believe the incomplete regular expression can be found here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5951

function stripUnsafeProtocolOnPaste(pasteData) {
  // Don't allow pasting javascript URIs since we don't support
  // LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL for those.
  let changed = false;
  let pasteDataNoJS = pasteData.replace(/\r?\n/g, "")
                               .replace(/^(?:\s*javascript:)+/i,
                                        () => {
                                                changed = true;
                                                return "";
                                              });
  return changed ? pasteDataNoJS : pasteData;
}

Comment 2

[:Gijs (he/him)]

Attachment: Patch v0.1

Comment 3

[:Gijs (he/him)]

Eric: thanks for the report!

I would like to mark this sec-low and open it up, given the chrome bug items are all public and that this isn't *much* better for a malicious actor than saying something like "copy-paste 'iavascript:alert(...)' and replace the first 'i' with 'j' in the location bar".

(It's obviously a little better, and we should fix it, but the control characters sure look... odd... and this is all just a best-effort type thing anyway.)

Comment 4

[Eric Lawrence (@ericlaw)]

Public is fine to me. (FWIW, when I was a PM on IE and we did the lockdown, a popular social network reported the JavaScript-paste vector was getting exploited hundreds of thousands of times per hour.)

Comment 5

[Daniel Veditz [:dveditz]]

Why is this even a valid URL? Sure, we strip whitespace and a couple of these count but we're left with what should be an invalid scheme <junk>javascript: or treating them as two tokens in which case it should be a search.

The patch here looks fine and works, just wondering if we have a separate issue with the Address Bar. If you create a link with that text as the "src" it's an invalid link and does nothing. Something in the Address Bar is parsing that out and throwing it away before handing the URL part off. Probably should not be that helpful -- let the user clean up the junk, or trigger a search (which is what happens if I put a non-CTRL char in the middle of the junk).

Comment 6

[Marco Bonardo [:mak]]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Why is this even a valid URL? Sure, we strip whitespace and a couple of
> these count but we're left with what should be an invalid scheme
> <junk>javascript: or treating them as two tokens in which case it should be
> a search.

I think this is a VERY GOOD question!
And I agree that the patch doesn't fully cover the problem.

I think it all boils down to this:

Services.uriFixup.getFixupURIInfo(` javascript:alert("Malicious. " + document.cookie)//https://fake`, Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS |
          Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP).preferredURI.spec

In practice, the Location Bar trusts uriFixup to make proper choices, looks like in this case it's indeed doing a nice fixup, but it's excessive.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8911858 [details] [diff] [review]
Patch v0.1

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

may we get a test in browser_removeUnsafeProtocolsFromURLBarPaste.js?

Also, you have touched urifixup quite a bit in the recent past, any idea how to solve the more global problem with it being too "liberal" into cleaning up special chars?

Comment 8

[Marco Bonardo [:mak]]

in particular, maybe we could make uriFixup not remove any control characters but space?
https://en.wikipedia.org/wiki/C0_and_C1_control_codes

Comment 9

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #8)
> in particular, maybe we could make uriFixup not remove any control
> characters but space?
> https://en.wikipedia.org/wiki/C0_and_C1_control_codes


Uhh, so:

makeURI("	javascript:alert(document.cookie)")

also produces the correct URI.

So I don't think this is URI fixup as much as it is URI parsing. In fact, reading:

https://url.spec.whatwg.org/#concept-basic-url-parser

I think this is correct per-spec. In particular:

> If url is not given:
>   1) Set url to a new URL.
>   2) If input contains any leading or trailing C0 control or space, validation error.
>   3) Remove any leading and trailing C0 control or space from input.

Note that "validation error" does not mean parsing terminates, per https://url.spec.whatwg.org/#validation-error .

Valentin or Anne, can you confirm my spec-reading?

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs from comment #9)
> (In reply to Marco Bonardo [::mak] from comment #8)
> > in particular, maybe we could make uriFixup not remove any control
> > characters but space?
> > https://en.wikipedia.org/wiki/C0_and_C1_control_codes
> 
> 
> Uhh, so:
> 
> makeURI("	javascript:alert(document.cookie)")

Bugzilla is being "helpful" and sanitizing input, but this should be something along the lines of:

makeURI("\u0001\u0002\u0003\u0004\u0005\u0006javascript:alert(document.cookie)")

(where makeURI is for all intents and purposes an alias of Services.io.newURI)

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1402896 - make the url bar strip javascript even when preceded by control characters,

Review commit: https://reviewboard.mozilla.org/r/183994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183994/

Comment 12

[:Gijs (he/him)]

Comment on attachment 8912664 [details]
Bug 1402896 - make the url bar strip javascript even when preceded by control characters,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183994/diff/1-2/

Comment 13

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Why is this even a valid URL? Sure, we strip whitespace and a couple of
> these count but we're left with what should be an invalid scheme
> <junk>javascript: or treating them as two tokens in which case it should be
> a search.
> 
> The patch here looks fine and works, just wondering if we have a separate
> issue with the Address Bar. If you create a link with that text as the "src"
> it's an invalid link and does nothing.

This doesn't match what I'm seeing on 57:

data:text/html,<a href="%01%02%03%04%05%06%07%08%10%11%12%13%14%15%16%17%18%19javascript:alert(location.href);">Click</a>

works if clicked. I think we're dropping stuff after %00 at some point, presumably prior to it arriving at the URI parser, which is presumably also why comment #0 suggests selecting everything except the first character (which is a null byte instead of all the non-zero C0 control characters). :-)

Comment 14

[Anne (:annevk)]

Per the standard these are removed by the URL parser as you found in comment 9. I'll leave the needinfo for Valentin since I don't know where we implement that in our stack. Note that per the URL Standard U+0000 NULL would be removed too, that not happening here is strange and maybe indicative of another subtle issue.

Furthermore, if there is any address bar sanitization, that should take place after parsing the URL into an object, not before. Only at that point can you trust the definitive scheme.

Comment 15

[:Gijs (he/him)]

(In reply to Anne (:annevk) from comment #14)
> Furthermore, if there is any address bar sanitization, that should take
> place after parsing the URL into an object, not before.

We don't parse as a URL at the point of pasting (only when the user hits go/enter/whatever to load the final URL), and we don't want unsanitized content to enter the url bar, so the only way to do this is to duplicate the exact URI fixup that the url bar's autocomplete/navigation code uses later on, and do the exact same thing at the point of pasting, which is also fragile. :-(

>  Only at that point
> can you trust the definitive scheme.

Not really... URI fixup might either change the scheme of a valid URL, or come up with a valid URL (which may be a search URL) even if URL parsing itself failed.

Comment 16

[Anne (:annevk)]

Fixup makes sense, but to figure out if it's scheme is X you should really consider roundtripping through the parser. Or maybe add some very visible comment to point out which pieces of code need to be kept in sync for security purposes and hope that folks pay attention... Or alternatively add some logic to the navigation code that enforces the restrictions the address bar wants to keep.

Comment 17

[:Gijs (he/him)]

(In reply to Anne (:annevk) from comment #16)
> Fixup makes sense, but to figure out if it's scheme is X you should really
> consider roundtripping through the parser. Or maybe add some very visible
> comment to point out which pieces of code need to be kept in sync for
> security purposes and hope that folks pay attention... Or alternatively add
> some logic to the navigation code that enforces the restrictions the address
> bar wants to keep.

Unfortunately we still want to allow people to do user-initiated toplevel js: navigation because bookmarklets and the like (obviously pages do lots of this stuff with <a href="javascript:..."> anyway, but if we wanted to we could batten down the URL bar / bookmarks hatches - we just don't (want to close them completely)). So instead we just try to make it harder for malicious actors.

TBH, I wonder if the time has come to stick that type of thing behind a pref (browser.urlbar.javascript.enabled ?) and force a dialog if the user tries to navigate to a JS URL (with either the url bar or bookmarks or drag/drop) without that pref flipped.

Comment 18

[Anne (:annevk)]

Separate bug? Seems reasonable for copy-and-paste or user-entered javascript URLs. (Though we should also consider restricting the developer tools on release in that case if we haven't already as it's pretty similar attack-wise.)

Comment 19

[:Gijs (he/him)]

(In reply to Anne (:annevk) from comment #18)
> Separate bug? 

Yep, bug 371923 and bug 305692 and friends.

> Seems reasonable for copy-and-paste or user-entered javascript
> URLs. (Though we should also consider restricting the developer tools on
> release in that case if we haven't already as it's pretty similar
> attack-wise.)

The devtools already have a paste warning, yeah.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8912664 [details]
Bug 1402896 - make the url bar strip javascript even when preceded by control characters,

https://reviewboard.mozilla.org/r/183994/#review189252

Comment 21

[Anne (:annevk)]

> The devtools already have a paste warning, yeah.

What I meant was not making them available, similar to stable Safari. Warnings are easily ignored.

Comment 22

[Eric Lawrence (@ericlaw)]

>we should also consider restricting the developer tools

Restricting the Developer Tools console so that all forms of pasted JavaScript won't run seems like an idea likely to provoke outrage. 

Or is something else being suggested here?

Comment 23

[Anne (:annevk)]

I think if we clearly communicate why we'd make such a change and what steps can be taken as workaround (e.g., use the Developer's Edition) it might turn out okay. As I said we wouldn't be the first browser to do it and it's an attack vector that's being used against end users.

Comment 24

[Valentin Gosu [:valentin] (he/him)]

(In reply to Anne (:annevk) from comment #14)
> Per the standard these are removed by the URL parser as you found in comment
> 9. I'll leave the needinfo for Valentin since I don't know where we
> implement that in our stack.

This is done in nsSimpleURI/nsStandardURL::SetSpec [1][2] by calling net_FilterAndEscapeURI/net_FilterURIString [3] which strips whitespace at the beginning and end of the string.

> Note that per the URL Standard U+0000 NULL
> would be removed too, that not happening here is strange and maybe
> indicative of another subtle issue.

We do strip out the leading U+0000 too for example this:
var url = new URL("\u0000\u0001\u0002javascript:alert('hello')"); url.href

It's possible some calls pass around regular C strings when creating URLs, which causes stuff after \0 to be ignored.

[1] https://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/netwerk/base/nsSimpleURI.cpp#301
[2] https://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/netwerk/base/nsStandardURL.cpp#1705
[3] https://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/netwerk/base/nsURLHelper.cpp#594,633

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/638e145f6bba
make the url bar strip javascript even when preceded by control characters, r=mak

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/638e145f6bba

Comment 27

[:Gijs (he/him)]

Comment on attachment 8912664 [details]
Bug 1402896 - make the url bar strip javascript even when preceded by control characters,

Approval Request Comment
[Feature/Bug causing the regression]: fixing a sec-low self-xss-mitigation bypass
[User impact if declined]: increased risk that users fall victim to self-xss issues
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: very small, defensive change to a regular expression, has (increased) test coverage
[String changes made/needed]: nope

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912664 [details]
Bug 1402896 - make the url bar strip javascript even when preceded by control characters,

Sure, let's block that.
Should be in 57b5

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/04e56dc5714e

Comment 30

[roxleitan]

Verified fixed using the latest Nightly (2017-10-09) and Firefox Beta 57.0b6 on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.

Pasting the text in the address bar displays : " alert("Malicious. " + document.cookie)//https://fake"