1111960 - (CVE-2015-0821) Open any pseudo URL (e.g. chrome://) when manually calling a link in a new tab

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[Armin Ebert [:arminius]]

Attachment: tab_open.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141201171754

Steps to reproduce:

(I admit that this report is a little awkward, but a serious impact is given anyways.)

If you force a link to a new tab or window by holding CTRL or SHIFT, every href url will be openend, regardless of the security context.

Steps:
- Create an <a> link to a chrome:// or file:// url.
- Trigger the link while holding the SHIFT or CTRL key.
- The specified url will be loaded, disregarding all context restrictions.

A sneaky attack could take advantage of the fact that SHIFT+CR is a common user action. (In Facebook's chat for instance, this will expand your input box with a newline char instead of submitting the text.)
So, if you focus() a hidden link on page load, pressing SHIFT + ENTER in any context is sufficient to potentially compromise your system. Besides that, a lot of people should be used to CTRL-open most links anyways and will therefore be an easy target.

The superfluously attached proof of concept will open chrome://browser/content/blockedSite.xhtml.



Expected results:

Links should respect equal context restrictions for every way of invocation.

Comment 1

[Daniel Veditz [:dveditz]]

In the browser console I get:

* Security Error: Content at https://bug1111960.bugzilla.mozilla.org/attachment.cgi?id=8536958&t=JOuyTWLqu3 may not load or link to chrome://browser/content/blockedSite.xhtml.
  
* uncaught exception: Load of chrome://browser/content/blockedSite.xhtml from https://bug1111960.bugzilla.mozilla.org/attachment.cgi?id=8536958&t=JOuyTWLqu3 denied.

==> and then the tab opens on the chrome:// url anyway.

Looks like the DOM/Security code is detecting the error so I presume this is happening in the front end. How far back does this regression go... Australis rewrite maybe?

NB: it's easier to see what's going on with the testcase if you edit the testcase to add text to the link and then just middle-click/Ctrl-click on it.

Comment 2

[Armin Ebert [:arminius]]

Attachment: A stripped-down testcase.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

This looks like a regression from bug 897062. That bug's patch created an e10s-friendly implementation of contentAreaClick by copying over the logic from contentAreaClick, but it looks to have not copied over any of the security checks...

It also looks like the e10s and non-e10s implementations are both running when e10s is disabled, and the only reason that isn't causing problems in practice is that the non-e10s implementation calls preventDefault() when it does something, and the e10s implementation does nothing in that case. In this testcase, the non-e10s implementation is not calling preventDefault (because its security check fails), and so the e10s version happily takes over.

I think this is sec-moderate since as far as I know it's a stepping-stone only, and requires the modified link clicks to be triggered, but we should definitely fix this ASAP.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

We should probably fix this in the short term by ensuring that the e10s implementation isn't used at all when e10s is disabled, and then work on fixing the e10s implementation separately (or just fix bug 903016).

Comment 5

[Armin Ebert [:arminius]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #3)
> I think this is sec-moderate since as far as I know it's a stepping-stone
> only, and requires the modified link clicks to be triggered, but we should
> definitely fix this ASAP.

I don't fully agree with the severity estimate:

The obvious caveat is, that although you can open any resource on the file system with local privileges, you still need to plant, say, a prepared HTML file on the system and know its location to escalate into true FS access.

But this is simple to achieve, since many features and plugins store website controlled data in /tmp.
More precisely, if you load a flash applet that uses the URLLoader API [1] to request a file called exploit.html, you can pretty reliably find it unaltered at /tmp/plugtmp/plugin-exploit.html.

If you want to rely on builtins, chances are you could still use download-triggering features like an addon installation. Before having approved an addon, Firefox will create /tmp/tmp-xxx.xpi, which is the addon package file with xxx being only three random characters. (Is this lack of entropy worth a bug report?)
Obviously, xpi is not a useful format for the attack, but you can use the jar: pseudo protocol to access its archive contents. This will result in a URL similar to jar:file:///tmp/tmp-xxx.xpi!/exploit.html and leads to local access if you can guess the three characters.

Finally you also increase the risk posed by XSS-vulnerable chrome:// and about: urls, that would otherwise be hardly exploitable. For instance, I just came across
> about:cache?storage=memory&context=<script>alert(1)</script>
which does not properly escape special characters in the 34 branch (but apparently is already fixed in succeeding versions).

Wrapping that up, an attacker will likely find a way to place a custom file in a predictable local place (while the most reliable method I currently know is flash). So from my point of view the bug potentially meets the high severity criteria, as it manages to "obtain confidential data from [...] the local machine, or inject data or code into those sites, requiring no more than normal browsing actions" and "can lead to the targeted compromise of a small number of users".

I know that Firefox makes a lot of effort to randomize cache and profile paths, but I don't see how the general problem of planting and predicting files can be avoided for the host as a whole.

[1] http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/net/URLLoader.html

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: potential patch for non-mozilla-central

On mozilla-central we need to fix the core issue, but for other channels (aurora/beta/release) where e10s isn't supported, I think we should do something like this, which boils down to mostly backing out bug 897062.

Tom/Felipe/Mike, can one of you guys take this bug and validate that this approach is feasible? I'm not sure whether there might be other non-e10s dependencies on this code on those earlier branches, for example.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8538023 [details] [diff] [review]
potential patch for non-mozilla-central

Looks like felipe's on this one.

Comment 8

[Tom S]

Sorry, I can't really help either. Just for historically perspective, at the time I was writing this this patch, it was from my understanding impossible to implement those security checks. Of course this should have never started doing something without e10s enabled.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8538023 [details] [diff] [review]
potential patch for non-mozilla-central

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

I tested this with beta and didn't see any problems. There were no further changes in this file so I think no newer dependencies were added in this code. I'll attach an updated patch as this one didn't apply cleanly.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: potential patch for non-mozilla-central, updated (on beta)

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: New patch for beta

Update: so we can't simply backout bug 897062 because bug 989875 removed the non-e10s paths for the about: error pages button handling.

But we can take a simpler patch that only removes the ContentClick bits, but not the ClickEventHandler that handles the about pages. This is also a lot safer to take on beta because this is clearly code that only matters to e10s (and shouldn't be running here in non-e10s in the first place).

I sent it to try (based on beta): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=172be778c9f9
https://tbpl.mozilla.org/?tree=Try&rev=172be778c9f9

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

It's too late to take a speculative fix on 35, this sounds like it needs more time to bake.  Wontfixing.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8542690 [details] [diff] [review]
New patch for beta

Thanks.

For clarity, it seems like a good idea to also remove the code in ClickEventHandler.handleEvent that fires Content:Click, since it is no longer being observed.

(Is it a bug that that function doesn't return after calling onAboutNetError, like we do after calling onAboutBlocked/onAboutCertError)? I guess it might not be, because there might be other links on about:neterror that we need to handle "normally"? If so, a comment would be nice, it kind of looks like a bug at first glance.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Felipe, what are your plans wrt this bug? Thanks

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Gavin, do you think we can checkin this code? Or just for 37?

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Sorry for the delay, Sylvestre, I forgot to write up the details.

When the beta patch for this got finished, it was already too late in the 36 cycle to take it (only RC remaining). So at the time I discussed this with lsblakk and we decided to leave it for 37.
However, we didn't want to land at the beginning of the cycle and disclose the problem, so we agreed to wait until past half of the 37 cycle to land so that the window for release users is reduced.  My idea is to land 2 or 2.5 weeks before the end, and until then I'll finish up a patch for mozilla-central, so that we can land on all trees at the same time.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

I think you are confusing with the release. Lukas was managing 35, 36 is currently in beta.
And we are at half of the beta cycle. So, this should land now or in 37.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Yeah I mixed the numbers, I meant 35 and 36. I'd like to wait at least until next week to land it though.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Felipe, Are we ready now? Thanks

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Sylvestre: yeah, requesting approval

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Attachment: Beta, updated to tip

Removed the dead code that fires Content:Click per gavin's request on comment 13.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8561448 [details] [diff] [review]
Beta, updated to tip

Approval Request Comment
[Feature/regressing bug #]: regression from bug 897062
[User impact if declined]: security issue as described in comment 0 and comment 5
[Describe test coverage new/current, TreeHerder]: Sent to tryserver
[Risks and why]: This patch only removes code that is related to the e10s path but was active in non-e10s too. Should be safe for the non-e10s trees (i.e. non-nightly)
[String/UUID change made/needed]: none

Comment 23

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/70301436eadb
https://hg.mozilla.org/releases/mozilla-beta/rev/5d37d11b64f2

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Was this going to land on trunk still too (which typically happens before uplift)?

Comment 25

[:Felipe Gomes (needinfo for replies!)]

No, at least not in this patch form. I'll fix this on trunk this week, probably through bug 903016 as suggested in comment 4. But I'm leaving here open for a few more days to decide on that. If bug 903016 turns out more complicated than expected, I'll go for a simpler approach for trunk.

Comment 26

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

Was ESR31 affected? I assume so since the called out cause is from 2013. 

This is only a sec-moderate so we probably won't take it there but it would be good to know.

Comment 27

[Marco Mucci [:MarcoM]]

Hi Felipe, can you provide a point value.

Comment 28

[:Felipe Gomes (needinfo for replies!)]

This has been fixed on mozilla-central in bug 1163422

Comment 29

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

Do you agree with the assessment of this as a sec-moderate?

Comment 30

[Armin Ebert [:arminius]]

(I can follow sec-moderate since the user action requirement differentiates from, say, bug 1147597. I'd not claim a sec-high bounty either way.)

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Sorry, I don't know much about other sec bugs to properly compare and assess the level on this one

Comment 32

[Ada [:adalucinet]]

Reproduced with 38.0b1, under Window 7 64-bit - with the attached test case, the tab opens (as mentioned in comment 1 too).
Verified fixed with 39.0b6 (Build ID: 20150615125213), latest Developer Edition (from 2015-06-15) and latest Nightly (from 2015-06-15), across platforms [1].

[1] Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit

Comment 33

[Ada [:adalucinet]]

Also verified fixed with ESR 38, across platforms [1], built from https://hg.mozilla.org/releases/mozilla-esr38/rev/bbac190ad1e7

Version: 38.0.1esrpre
Build ID: 20150616140147

[1] Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5