Closed Bug 1111960 (CVE-2015-0821) Opened 5 years ago Closed 5 years ago
Open any pseudo URL (e
.g . chrome://) when manually calling a link in a new tab
153 bytes, text/html
72 bytes, text/html
5.48 KB, patch
|Details | Diff | Splinter Review|
7.36 KB, patch
|Details | Diff | Splinter Review|
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.
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Security
Ever confirmed: true
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.
OS: Linux → All
Hardware: x86_64 → All
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).
(In reply to :Gavin Sharp [email: email@example.com] 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  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.  http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/net/URLLoader.html
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 on attachment 8538023 [details] [diff] [review] potential patch for non-mozilla-central Looks like felipe's on this one.
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 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.
Attachment #8538023 - Flags: feedback?(felipc) → feedback+
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
It's too late to take a speculative fix on 35, this sounds like it needs more time to bake. Wontfixing.
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.
Attachment #8542690 - Flags: review?(gavin.sharp) → review+
Felipe, what are your plans wrt this bug? Thanks
Gavin, do you think we can checkin this code? Or just for 37?
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.
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.
Yeah I mixed the numbers, I meant 35 and 36. I'd like to wait at least until next week to land it though.
Felipe, Are we ready now? Thanks
Sylvestre: yeah, requesting approval
Removed the dead code that fires Content:Click per gavin's request on comment 13.
Attachment #8561448 - Flags: review+
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
Was this going to land on trunk still too (which typically happens before uplift)?
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.
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.
Hi Felipe, can you provide a point value.
Status: NEW → ASSIGNED
Iteration: --- → 39.1 - 9 Mar
Points: --- → 8
This has been fixed on mozilla-central in bug 1163422
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Do you agree with the assessment of this as a sec-moderate?
(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.)
Sorry, I don't know much about other sec bugs to properly compare and assess the level on this one
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 .  Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit
Whiteboard: [adv-main36+] → [adv-main36+][b2g-affected?]
Also verified fixed with ESR 38, across platforms , built from https://hg.mozilla.org/releases/mozilla-esr38/rev/bbac190ad1e7 Version: 38.0.1esrpre Build ID: 20150616140147  Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5
Whiteboard: [adv-main36+][b2g-affected?] → [adv-main36+][b2g-affected?][adv-main39+][adv-esr38.1+]
You need to log in before you can comment on or make changes to this bug.