1439396 - A specially-crafted javascript: URL may be pasted into the Addressbar leading to Self-XSS Attack (similar to bug 1402896)

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Jordi Chancel]

Attachment: Firefox self XSS.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180215111455

Steps to reproduce:

1. Go to the testcase in Attachments with Firefox Beta update (59.0b10) or Firefox Nightly 60.0a1
2. Click on the "Copy malicious javascript: URL" link to copy the crafted javascript: URL
3. Make a right-click into the Addressbar and click on "Paste & Go"

This bug can be used in Self-XSS attack because the JavaScript: URL scheme is not deleted when it is pasted into the Addressbar.


Actual results:

JavaScript URL is executed leading to Self-XSS vulnerability (the JavaScript: URL scheme is not deleted when it is pasted into the Addressbar).


Expected results:

The JavaScript: URL scheme should be deleted when it is pasted into the Addressbar

Comment 1

[Jordi Chancel]

Attachment: Firefox self XSS Testcase2.html

This testcase is for nightly:
In Firefox Nightly 60.0a1 you can only executed the JavaScript code on the same tab where the testcase webpage is loaded. 

In this 2nd testcase, after that the specially-crafted JavaScript: link is copied, a targeted website is loaded (in this case: https://www.google.com/), and when you are on the google website you can make a right-click into the Addressbar and click on "Paste and Go".

STR:

-1) Click on the "Copy malicious javascript: URL" link to copy the specially-crafted javascript: URL (you will redirected to https://www.google.com/)

-2) Make a right-click into the Addressbar and click on "Paste & Go" into the context-menue

Results: javascript: URL can be pasted into the Addressbar allowing the JavaScript code to be executed on the targeted website leading to Self-XSS Attack

Comment 2

[Daniel Veditz [:dveditz]]

Gijs: you worked on bug 1402896. any ideas here? Looks like the stripping function is looking for case-sensitive 'javascript' from the URL, and this is pasting mixed-case text (not a URL) that is then, after maybe-stripping, converted into a URL and loaded.

Comment 3

[:Gijs (he/him)]

Jordi, does this reproduce on Firefox 58 (current release) ? Asking because comment #0 specifically references 59 (which is beta right now) and 60, and I don't have 58 to hand myself (plus, I'd like to be sure I'm seeing the same things as you).

Off-hand, I think this is a regression from bug 1422643, which tried to use our builtin URL parsing to detect JS URIs. Prior to that code, we used regular expressions, which were case insensitive, e.g. see release code here:

https://dxr.mozilla.org/mozilla-release/rev/0a0a804a5b6f252f12d9808b54ed2a7f6ada27e3/browser/base/content/browser.js#6142-6147

which I would expect to work 'against' the testcase in this bug.


Using the builtin parsing was meant to avoid all these issues. Services.io.newURI("JavaScript:Foo").scheme is all-lowercase 'javascript', but it seems 'extractScheme' doesn't lowercase the scheme it extracts? That is unfortunate, to say the least. :-\

Comment 4

[:Gijs (he/him)]

Attachment: Patch v0.1

Comment 5

[Anne (:annevk)]

Shouldn't we fix this in extractScheme instead?

Comment 6

[Marco Bonardo [:mak]]

well, the scheme is case-insensitive per spec, right? So it may be worth fixing extractScheme.
I see that net_ExtractURLScheme is also used in a lot of other places where it is compared in a case sensitive manner, for example net_ParseFileURL compares it with "file". Same in SubstitutingURL::EnsureFile, ExtensionProtocolHandler::NewStream, and so on.
Leaving it as-is sounds like a footgun.

Comment 7

[:Gijs (he/him)]

(In reply to Anne (:annevk) from comment #5)
> Shouldn't we fix this in extractScheme instead?

We can, but that is not my bailiwick, would probably be trickier to uplift, and I don't know if there are cases that expect it not to lowercase things. There's several dozen callers through both the IO service and the net_* prefixed version, and I gave up trying to look at whether that was always going to be safe.

Comment 8

[:Gijs (he/him)]

While the issue in the networking code may be older, I'm not convinced the issue as reported exists on ESR or 58.

I'm happy for us to try to fix the issue in the networking code, but I'd prefer to do that in a separate (non-sec-sensitive) bug.

Comment 9

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #6)
> net_ParseFileURL
> compares it with "file". Same in SubstitutingURL::EnsureFile,
> ExtensionProtocolHandler::NewStream, and so on.

These cases sound like they 'fail safe', though (ie the behaviour consumers want won't happen if passed "File", but it's not a bypass of a safety check).

Comment 10

[Marco Bonardo [:mak]]

Fair enough, it's hard to follow all the code running after an extractScheme or net_extractURLScheme to be sure if it has security implications. For the same reason, it's also hard to figure out if the change may cause regressions (better safe than sorry).

I'd suggest to file a bug in netwerk to fix that util (possibly soon, at least in 60 and maybe uplift to 59).
And file a bug depending on the above network bug to remove the .toLowerCase() once extractScheme() is fixed.

Comment 11

[Marco Bonardo [:mak]]

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

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

r=me with the other bugs filed, thanks!

Comment 12

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #10)
> Fair enough, it's hard to follow all the code running after an extractScheme
> or net_extractURLScheme to be sure if it has security implications. For the
> same reason, it's also hard to figure out if the change may cause
> regressions (better safe than sorry).
> 
> I'd suggest to file a bug in netwerk to fix that util (possibly soon, at
> least in 60 and maybe uplift to 59).
> And file a bug depending on the above network bug to remove the
> .toLowerCase() once extractScheme() is fixed.

I filed bug 1439931 and bug 1439944 . Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c38198fec8a

(FWIW, given this is sec-low, I think we could open it up.)

Comment 13

[:Gijs (he/him)]

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

Approval Request Comment
[Feature/Bug causing the regression]: I think bug 1422643, but no confirmation from the reporter yet.
[User impact if declined]: potential for self-xss
[Is this code covered by automated tests?]: yep
[Has the fix been verified in Nightly?]: not manually, but see previous question
[Needs manual test from QE? If yes, steps to reproduce]:  I don't think so.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: one-line fix, has good automated test coverage
[String changes made/needed]: nope

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Low-risk fix with automated test coverage. Taking for 59b12.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c38198fec8a7654862e491884db41b852ac367c
https://hg.mozilla.org/mozilla-central/rev/2c38198fec8a

Comment 16

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

https://hg.mozilla.org/releases/mozilla-beta/rev/5abddba6d262

Comment 17

[Jordi Chancel]

This Self-XSS issue was valid only in Beta 59 and in 60 Nightly.

Thank you to have patched this security bug.