Bug 1402896 (CVE-2017-7839)

Specially-crafted JavaScript may be pasted into the address bar

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: ericlaw1979, Assigned: Gijs)

Tracking

({csectype-other, sec-low})

57 Branch
Firefox 58
csectype-other, sec-low
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 wontfix, firefox56 wontfix, firefox57 verified, firefox58 verified)

Details

(Whiteboard: [adv-main57+] self-xss mitigation bypass [fxsearch])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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;
}
(Reporter)

Updated

2 years ago
Component: Untriaged → Address Bar
(Assignee)

Comment 2

2 years ago
Posted patch Patch v0.1 (obsolete) — Splinter Review
Attachment #8911858 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
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)
(Reporter)

Comment 4

2 years ago
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)
Keywords: csectype-other, sec-low
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
(Assignee)

Comment 9

2 years ago
(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)
(Assignee)

Comment 10

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
(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). :-)
(Assignee)

Updated

2 years ago
Attachment #8911858 - Attachment is obsolete: true

Comment 14

2 years ago
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.
(Assignee)

Comment 15

2 years ago
(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

2 years ago
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.
(Assignee)

Comment 17

2 years ago
(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

2 years ago
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.)
(Assignee)

Comment 19

2 years ago
(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

2 years ago
mozreview-review
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+

Comment 21

2 years ago
> The devtools already have a paste warning, yeah.

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

Comment 22

2 years ago
>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

2 years ago
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)

Comment 25

2 years ago
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

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/638e145f6bba
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Comment 27

2 years ago
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?
(Assignee)

Updated

2 years ago
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
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+

Comment 29

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/04e56dc5714e
status-firefox57: affected → fixed
Flags: in-testsuite+
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
status-firefox57: fixed → verified
status-firefox58: fixed → 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.