Closed Bug 1402896 (CVE-2017-7839) Opened 3 years ago Closed 3 years ago

Specially-crafted JavaScript may be pasted into the address bar

Categories

(Firefox :: Address Bar, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- verified
firefox58 --- verified

People

(Reporter: ericlaw1979, Assigned: Gijs)

References

Details

(Keywords: csectype-other, sec-low, Whiteboard: [adv-main57+] self-xss mitigation bypass [fxsearch])

Attachments

(1 file, 1 obsolete file)

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.
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;
}
Component: Untriaged → Address Bar
Attached patch Patch v0.1 (obsolete) — Splinter Review
Attachment #8911858 - Flags: review?(mak77)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.)
Flags: needinfo?(dveditz)
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.)
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).
Group: firefox-core-security
Flags: needinfo?(dveditz)
Whiteboard: self-xss mitigation bypass
(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.
Priority: -- → P1
Whiteboard: self-xss mitigation bypass → self-xss mitigation bypass [fxsearch]
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?
Attachment #8911858 - Flags: review?(mak77)
in particular, maybe we could make uriFixup not remove any control characters but space?
https://en.wikipedia.org/wiki/C0_and_C1_control_codes
(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?
Flags: needinfo?(valentin.gosu)
(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)
(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). :-)
Attachment #8911858 - Attachment is obsolete: true
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.
(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.
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.
(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.
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.)
(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 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
Attachment #8912664 - Flags: review?(mak77) → review+
> The devtools already have a paste warning, yeah.

What I meant was not making them available, similar to stable Safari. Warnings are easily ignored.
>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?
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.
(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
Flags: needinfo?(valentin.gosu)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/638e145f6bba
make the url bar strip javascript even when preceded by control characters, r=mak
https://hg.mozilla.org/mozilla-central/rev/638e145f6bba
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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
Attachment #8912664 - Flags: approval-mozilla-beta?
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
Attachment #8912664 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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"
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: self-xss mitigation bypass [fxsearch] → [adv-main57+] self-xss mitigation bypass [fxsearch]
Alias: CVE-2017-7839
You need to log in before you can comment on or make changes to this bug.