1447969 - (CVE-2018-5167) Old console and current debugger linkify javascript: URLs

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Wladimir Palant]

A response returning application/json will be displayed in the JSON Viewer and links in the data will be linkified. While this might be convenient, javascript: links will also be linkified. Web services don't expect that JSON output needs to be sanitized, so injecting a javascript: URL into the output is often trivial. See for example this Mozilla URL:

> https://token.services.mozilla.com/1.0/sync/javascript:alert(JSON.stringify(JSONView.headers.request%29%29

If the user somehow clicks value of the errors.0.name property an alert message will show up - JavaScript code executed successfully. And while the principal is set to the null principal so that the code cannot access document.cookies for example, it can access the JSONView object injected by the JSON Viewer. That object contains among other things all request headers - meaning also cookies and authorization tokens.

This JavaScript code can also trigger saving of arbitrary content, though that shouldn't be as critical:

> https://token.services.mozilla.com/1.0/sync/javascript:void(window.dispatchEvent(new%2520CustomEvent(%2527contentMessage%2527,%7Bdetail:%7Btype:%2527save%2527,value:%2527data:,foobar%2527%7D%7D%29%29%29

Clearly, JSON Viewer should only linkify HTTP and HTTPS URLs. For reference, the problematic code line is https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/devtools/client/shared/components/reps/reps.js#88. While this also allows data: and chrome: URLs, browser security mechanisms will no longer allow navigating to those.

Comment 1

[Wladimir Palant]

After updating Nightly this appears to be fixed, I guess that this is a duplicate? In Firefox 59.0.1 this issue is still reproducible.

Comment 2

[Wladimir Palant]

It seems that https://hg.mozilla.org/mozilla-central/rev/aa46f0522cfd (meaning bug 1440388) removed javascript from validProtocols variable, a change not visible in DXR yet for some reason. The same code exists in two other locations in the Firefox codebase however and hasn't been updated there. The old console frontend doesn't appear to be used any more, but the JavaScript debugger still uses the problematic code. You can see by adding "javascript:alert(1)" as watch expression in the debugger - clicking the value will show an alert.

There is another issue here: both console and debugger will still linkify chrome:// URLs. So if a website logs "chrome://global/content/" to console or if you have it as a value in a debugger watch expression, this link will work. A while back websites were forbidden from linking to chrome:// because it could introduce security issues because browser UI didn't expect untrusted parameters. Here Firefox still allows websites to link to chrome:// and potentially exploit security issues, even if users are far less likely to click the links.

Comment 3

[Frederik Braun [:freddy]]

Nominating this for bounty consideration.
Can you submit some easier STR for a test case that isn't in the JSON viewer? :)
Whichever is easiest to help reproducing and understanding this issue better.
Also, can you come up with a list of all the affected code? Comment 2 seems a bit incomprehensive.

Thank you!

Comment 4

[:Gijs (he/him)]

(In reply to Wladimir Palant (for Adblock Plus info Cc [e-mail]) from comment #1)
> After updating Nightly this appears to be fixed, I guess that this is a
> duplicate?

Yes, bug 1442840.

(In reply to Wladimir Palant (for Adblock Plus info Cc [e-mail]) from comment #2)
> It seems that https://hg.mozilla.org/mozilla-central/rev/aa46f0522cfd
> (meaning bug 1440388) removed javascript from validProtocols variable, a
> change not visible in DXR yet for some reason.

bug 1446660

> The same code exists in two
> other locations in the Firefox codebase however and hasn't been updated
> there.

https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/debugger/new/debugger.js#l1762
https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/webconsole/console-output.js#l31

:nchevobbe, can we also update these with the new reps bundle? Though see below.

> There is another issue here: both console and debugger will still linkify
> chrome:// URLs. So if a website logs "chrome://global/content/" to console
> or if you have it as a value in a debugger watch expression, this link will
> work. A while back websites were forbidden from linking to chrome:// because
> it could introduce security issues because browser UI didn't expect
> untrusted parameters. Here Firefox still allows websites to link to
> chrome:// and potentially exploit security issues, even if users are far
> less likely to click the links.

I think we don't want to break this outright because those links should work in the browser console and debugger.

It seems more likely that we should alter the openLinks parameter for those tools so that it doesn't trigger opening those links if not loaded in the browser toolbox / console.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

So, javascript: URLs should not be linkified in the console nor browser console anymore, since they both use the reps bundle.

https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/webconsole/console-output.js#l31 is the old console frontend (which do not use reps), which when Bug 1347127 land won't be used anywhere. We can still remove the "javascript" element from the valid protocol list for the old frontend and uplift it if you want.

The debugger was updated to use the latest Reps version this week (see https://github.com/devtools-html/debugger.html/pull/5714), and the debugger bundle containing this should land soon in mozilla central.

Regarding chrome: and data: urls, what is the risk here ?

Comment 7

[Wladimir Palant]

data: URIs present no risk from what I can tell. When in content, they won't load (top navigation to data: URIs is forbidden now). In developer tools these will open in a new tab, without inheriting any security context.

As to chrome: URLs, I cannot find the bug where linking from content to chrome: was disallowed, that was a long time ago. I only remember that it was about a Firefox vulnerability where opening a chrome: URL with particular parameters resulted in execution of arbitrary code with chrome privileges. It was deemed that allowing webpages to trigger arbitrary chrome: URLs is too dangerous.

Comment 9

[:Gijs (he/him)]

(In reply to Wladimir Palant from bug 1442840 comment #38)
> there are more aspects here that
> haven't been addressed yet:
> 
> * Same javascript: linkification happens in a few other places
> (https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/debugger/
> new/debugger.js#l1762 and
> https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/webconsole/
> console-output.js#l31). So a javascript: URL in the debugger's watch
> expression will still be linkified - less critical here but still not nice.

Looks to me like this is addressed in comment #6. My understanding is that both cases are now fixed on nightly with the new reps bundle, and the fact that the old console is dead (and has been dead for content consoles since 57, though AIUI the new frontend will be vulnerable in 59/60 given it doesn't have the new reps bundle). :nchevobbe and Wladimir, can you confirm? :nchevobbe: How upliftable is the reps bundle change? Do we need to uplift spot-fixes for the console and/or debugger to 60? If so we can consider doing that either here or in a separate sec-sensitive bug?

> * chrome: URLs are also being linkified. So a website could for example do
> console.log("chrome://global/content/") and that URL will be linkified in
> the console. I think the consensus is that websites linking to chrome: URLs
> is too risky, it shouldn't be possible here either.

Preventing this is part of our defense-in-depth strategy, yes. However, given that the console and debugger cases require the user to manually open those devtools on an attacking page (as opposed to the json viewer which can open in-content and is therefore more vulnerable to clickjacking techniques etc.), an attack that uses chrome: URLs seems somewhat implausible to me (nor do I know of any current issues where just loading a chrome URI causes issues - if such issues exist they ought to be fixed separately, so please report in a new bug). Not significantly more likely than, say, putting the link in the clipboard and telling the user to click 'paste and go' from the location bar context menu (which will work, IIRC).

I still think we should fix it, but as I said in comment #4, it's not as straightforward as just disallowing them - frontend developers still need to be able to click links in the browser console to open the relevant source (which I noticed was broken on Nightly recently, I need to check if it's fixed by now...).

Given all of that, I would like to open a separate (public, sec-want/sec-audit) bug for disallowing chrome: and resource: links (really, anything non-http/https, maybe non-protocol-of-debuggee so that we don't kill file: links for users debugging file: pages) for content console/debugger clients.

Comment 10

[:Gijs (he/him)]

(Note that the fixes in bug 1442840 will mean chrome: URIs should not be opened anymore for links in the JSON viewer, only http/https links, even if they still 'look' linkified, they have a click handler that will prevent opening them)

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

> My understanding is that both cases are now fixed on nightly with the new reps bundle, and the fact that the old console is dead (and has been dead for content consoles since 57, though AIUI the new frontend will be vulnerable in 59/60 given it doesn't have the new reps bundle). :nchevobbe and Wladimir, can you confirm? :nchevobbe: How upliftable is the reps bundle change? Do we need to uplift spot-fixes for the console and/or debugger to 60? If so we can consider doing that either here or in a separate sec-sensitive bug?

It's not easily upliftable as is, there are many other changes that needs different call sites options in the tools that use them. What we could do is simply alter the valid protocol array in 59 and 60, bot in reps.js and in debugger-bundle (which includes reps). I never done anything like that so I don't know what would be the proper way to handle this.

Comment 12

[Wladimir Palant]

(In reply to :Gijs (out until 26th) from comment #9)
> Looks to me like this is addressed in comment #6. My understanding is that
> both cases are now fixed on nightly with the new reps bundle, and the fact
> that the old console is dead (and has been dead for content consoles since
> 57, though AIUI the new frontend will be vulnerable in 59/60 given it
> doesn't have the new reps bundle).

Yes, I can confirm that javascript: URLs are no longer linkified in the debugger. The problematic code is still there for the old console but no longer used from what I can tell.


> I still think we should fix it, but as I said in comment #4, it's not as
> straightforward as just disallowing them - frontend developers still need to
> be able to click links in the browser console to open the relevant source
> (which I noticed was broken on Nightly recently, I need to check if it's
> fixed by now...).

Well, source links should open in the debugger view rather than a new tab but I see your point.

> Given all of that, I would like to open a separate (public,
> sec-want/sec-audit) bug for disallowing chrome: and resource: links (really,
> anything non-http/https, maybe non-protocol-of-debuggee so that we don't
> kill file: links for users debugging file: pages) for content
> console/debugger clients.

Fine with me. I merely didn't want this aspect of the issue to be forgotten.

Comment 13

[:Gijs (he/him)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> What we could do is
> simply alter the valid protocol array in 59 and 60, bot in reps.js and in
> debugger-bundle (which includes reps).

This sgtm as a simple way of mitigating the JS angle here. I'm reopening this so we can track this fix here. Self-needinfo'ing so I can get a patch together + file a follow-up for chrome/resource: once I plow through my remaining bugmail/requests from being away for a few days - if someone beats me to it, happy to have someone else write it...

Comment 14

[:Gijs (he/him)]

Attachment: Patch v1

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: minor security issues in old console (should be unused except for browser console, which has to be preffed on and hopefully has clue-ful users) and js debugger (requiring significant user interaction)
[Is this code covered by automated tests?]: generally yes
[Has the fix been verified in Nightly?]: a different fix was applied in nightly, so no.
[Needs manual test from QE? If yes, steps to reproduce]: not really
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: this breaks linking javascript: URIs for the old console and debugger. Nothing and nobody should be relying on that working.
[String changes made/needed]: no

Comment 15

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

Comment on attachment 8963273 [details] [diff] [review]
Patch v1

Bet approval (as a security issue).

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8963273 [details] [diff] [review]
Patch v1

Looks good to me

Comment 17

[:Gijs (he/him)]

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

Wladimir, once this hits beta 8 (should be released tomorrow), can you verify this? Thanks!

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Sorry I missed something during the review: why the `data` URLs were removed as well here ?
There are still stringified in the reps bundle on mozilla-central, so I was wondering if I should remove them as well.
Are they a target for attacks ?

Comment 19

[:Gijs (he/him)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #18)
> Sorry I missed something during the review: why the `data` URLs were removed
> as well here ?
> There are still stringified in the reps bundle on mozilla-central, so I was
> wondering if I should remove them as well.
> Are they a target for attacks ?

For the JSON Viewer, the patch in the other bug only linkified http/https, and it enforces opening the links in a new tab.

I don't know what codepath the console and debugger use to open links, and if they could ever be convinced to load them in the same tab. As Wladimir said elsewhere in this bug (I think?) navigation to toplevel data: URIs is now prevented anyway ( https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-59/ ) -- but coincidentally, I think that shipped a release later than planned, and there's still an about:config pref to turn it off.

So I was mostly being cautious. If we're confident that they can't be used for an attack I'm fine leaving the reps bundle to link them, also because I understand that they can be useful.

Out of curiosity, how do we deal with links in the inspector? Like image data: URIs in the style sidebar for list-style-image and background-image and the like? I assume they don't use either the debugger or the console codepath here...

Do you want us to land a follow-up patch to unblock data: URIs on 60 for the old console frontend + debugger? Personally, I think the risk+effort/reward calculus means we shouldn't, but I could be persuaded otherwise...

Comment 20

[Wladimir Palant]

As I mentioned, debugger opens data: links in a new tab - it ignores that "no top-level navigation to data:" pref but otherwise it's unproblematic.

Comment 21

[Wladimir Palant]

(In reply to :Gijs (back Tuesday April 3rd) from comment #17)
> https://hg.mozilla.org/releases/mozilla-beta/rev/
> 2bb8a74aa36cfb0430bfdab09adb7fcce0dab1c7
> 
> Wladimir, once this hits beta 8 (should be released tomorrow), can you
> verify this? Thanks!

I tried Firefox 60.0b8 today and I see something weird. javascript: URLs in the JSON Viewer are linkified but clicks are ignored - that's fine. javascript: URLs in watch expression in the JS Debugger aren't linkified, same goes for the Browser Console - also fine. However, the console in the Web Developer tools will still linkify javascript: URLs, e.g. if the page does console.log("javascript:alert('oops')"). These links open in a new tab and the code executes in an unprivileged context (about:blank), but this still shouldn't be.

Comment 22

[Wladimir Palant]

Actually, my findings above aren't weird at all seeing that the instance under https://hg.mozilla.org/releases/mozilla-beta/file/85da1c81b9f4/devtools/client/shared/components/reps/reps.js#l88 hasn't been changed. While it doesn't matter for the JSON Viewer (clicks on javascript: links aren't processed due to changes in bug 1442840), this code is also used by the new console fronten.

Comment 23

[:Gijs (he/him)]

(In reply to Wladimir Palant (for Adblock Plus info Cc [e-mail]) from comment #21)
> However, the console in the
> Web Developer tools will still linkify javascript: URLs, e.g. if the page
> does console.log("javascript:alert('oops')"). These links open in a new tab
> and the code executes in an unprivileged context (about:blank), but this
> still shouldn't be.

Right... Nicholas, we could uplift a 1-line fix to the reps bundle for this, or uplift the reps bundle change, or do nothing. Given there isn't a concrete security impact for 60 anymore, I could live with the last option - but on the other hand, this will be an ESR release and it may be nice to have that change for 60 anyway. Up to you...

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

It would be nice to uplift the 1-line fix in the Reps bundle IMO

Comment 25

[:Gijs (he/him)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #24)
> It would be nice to uplift the 1-line fix in the Reps bundle IMO

Filed bug 1451502 for this. Going to mark this one verified per Wladimir's comments.