Bug 734076 (CVE-2012-1966)

XSS with context menu

VERIFIED FIXED in Firefox 14

Status

()

defect
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: moz_bug_r_a4, Assigned: Gavin)

Tracking

({sec-high})

unspecified
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ verified, firefox15 fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [sg:high][advisory-tracking+])

Attachments

(2 attachments, 9 obsolete attachments)

Reporter

Description

7 years ago
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js

showOnlyThisFrame, viewMedia and viewBGImage disallow javascript: url but allow data: url.  Thus, it's possible to perform an XSS attack using data: url.
Reporter

Comment 1

7 years ago
This tries to get cookies for www.mozilla.org.
Reporter

Comment 2

7 years ago
This tries to get cookies for www.mozilla.org.
Reporter

Comment 3

7 years ago
This tries to get cookies for www.mozilla.org.
This is probably in the context-menu code. Toolkit?
Component: Security → Security
Product: Core → Toolkit
Whiteboard: [sg:high]
Drew, can you look at this asap?
Assignee: nobody → adw
Drew is on PTO this week. Re-assigning to David.
Assignee: adw → ddahl
Component: Security → Security
Product: Toolkit → Firefox
QA Contact: toolkit → firefox
The easy fix is to s/DISALLOW_SCRIPT/DISALLOW_INHERIT_PRINCIPAL/ everywhere in nsContextMenu. That might break some legitimate cases, though (e.g. viewing a data: image) - it might be nicer to use openLinkIn, and pass in disallowInheritPrincipal=true.

Comment 8

7 years ago
I will have an "easy fix" patch after a partial clobber. 

Gavin: I am thinking we will need a full blown browser chrome test with mouse events, right? If so, is there an existing test to model after?

Comment 9

7 years ago
All test cases still work with this "easy fix" patch applied
Attachment #606458 - Flags: feedback?(gavin.sharp)
(In reply to David Dahl :ddahl from comment #9)
> Created attachment 606458 [details] [diff] [review]
> Patch 1 - Easy Fix - Does not work
> 
> All test cases still work with this "easy fix" patch applied

Sorry gavin, my emacs just had to remove some tabs or something in that patch. Sigh.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> it might be nicer to use openLinkIn, and pass in disallowInheritPrincipal=true.

Are you saying that instead of using:

this.browser.loadURI(frameURL, referrer ? makeURI(referrer) : null);

change it to:

    openLinkIn(frameURL, "window", 
               { charset: doc.characterSet,
                 referrerURI: referrer ? makeURI(referrer) : null, disallowInheritPrincipal: true});

inside of the function showOnlyThisFrame, etc ?
(In reply to David Dahl :ddahl from comment #11)

This appears to work inside 'showOnlyThisFrame':

    // this.browser.loadURI(frameURL, referrer ? makeURI(referrer) : null);
    openLinkIn(frameURL, "window",
               { charset: doc.characterSet,
                 referrerURI: referrer ? makeURI(referrer) : null,
                 disallowInheritPrincipal: true });

The script was unable to display the cookie values - it was able to load mozilla.org into the frame

The other areas of concern are all using 'openUILink' which does not have a params argument, should this be changed in utilityOverlay.js?

I'll tinker with it...

Updated

7 years ago
Attachment #606458 - Flags: feedback?(gavin.sharp)
This patch fixes the cookie value snooping, but targets things to new tabs or a new window.
Attachment #606458 - Attachment is obsolete: true
Attachment #606715 - Flags: feedback?(gavin.sharp)
You want "current" to avoid changing the targeting behavior (it's the only case where disallowInheritPrincipal matters).

Rather than changing openUILink/openUILink's signature, you can just switch to using openLinkIn directly. I guess bug 631500 would be useful here.
Attachment #606715 - Flags: feedback?(gavin.sharp)
Ok, this works... also my targeting was wrong in the prev. patch as I had the tabs pinned, whoops.
Attachment #606715 - Attachment is obsolete: true
Attachment #606775 - Flags: feedback?(gavin.sharp)
I am unsure how to test this where we are targeting the iframe. I see this test: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_plainTextLinks.js#17

That approach will work for the background image, correct?
"current" doesn't actually maintain the current behavior. What openUILink does depends on the event.
Depends on: 631500
OS: Windows XP → All
Hardware: x86 → All
Comment on attachment 606775 [details] [diff] [review]
Patch 3 - working - needs tests

Looks like you can make use of the new functionality added in bug 631500 and avoid needing to switch from openUILink.
Attachment #606775 - Flags: feedback?(gavin.sharp)
Posted patch Patch 4 (obsolete) — Splinter Review
I think this patch is correct, the params do leave out the 'charset' property as openUILinkIn does not accept one. 

Should I follow the testing method in this test? mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug424101.js#19

Also, can this test method be used with the "show only this frame" test?
Attachment #606775 - Attachment is obsolete: true
Attachment #607560 - Flags: feedback?
Comment on attachment 607560 [details] [diff] [review]
Patch 4

>-    openUILink(viewURL, e, null, null, null, null, doc.documentURIObject );
>+    openUILinkIn(viewURL, "current", { disallowInheritPrincipal: true,
>+                                       allowThirdPartyFixup: false,
>+                                       postData: null,
>+                                       referrerURI: doc.documentURIObject });

Use openUILink instead of openUILinkIn. Remove "allowThirdPartyFixup: false" and "postData: null".
Posted patch Patch 5 (obsolete) — Splinter Review
Attachment #607560 - Attachment is obsolete: true
Attachment #607621 - Flags: feedback?(dao)
Attachment #607560 - Flags: feedback?
(In reply to Dão Gottwald [:dao] from comment #20)

> Use openUILink instead of openUILinkIn. Remove "allowThirdPartyFixup: false"
> and "postData: null".

Sorry. I am a bit distracted right now by the ID work week. I think I have it now.

Updated

7 years ago
Attachment #607621 - Flags: feedback?(dao)
Comment on attachment 607621 [details] [diff] [review]
Patch 5

># HG changeset patch
># Parent 7bbac97c3ce2e3008e37dea1b2a229b2ebd343f3
>
>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
>--- a/browser/base/content/nsContextMenu.js
>+++ b/browser/base/content/nsContextMenu.js
>@@ -762,17 +762,20 @@ nsContextMenu.prototype = {
>   // Open clicked-in frame in the same window.
>   showOnlyThisFrame: function() {
>     var doc = this.target.ownerDocument;
>     var frameURL = doc.location.href;
> 
>     urlSecurityCheck(frameURL, this.browser.contentPrincipal,
>                      Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
>     var referrer = doc.referrer;
>-    this.browser.loadURI(frameURL, referrer ? makeURI(referrer) : null);
>+    openUILink(frameURL, "current", { disallowInheritPrincipal: true,
>+                                      allowThirdPartyFixup: false,
>+                                      postData: null,
>+                                      referrerURI: doc.documentURIObject });
>   },
> 
>   // View Partial Source
>   viewPartialSource: function(aContext) {
>     var focusedWindow = document.commandDispatcher.focusedWindow;
>     if (focusedWindow == window) 
>       focusedWindow = content;
> 
>@@ -834,17 +837,20 @@ nsContextMenu.prototype = {
>     else {
>       viewURL = this.mediaURL;
>       urlSecurityCheck(viewURL,
>                        this.browser.contentPrincipal,
>                        Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
>     }
> 
>     var doc = this.target.ownerDocument;
>-    openUILink(viewURL, e, null, null, null, null, doc.documentURIObject );
>+    openUILink(viewURL, "current", { disallowInheritPrincipal: true,
>+                                     allowThirdPartyFixup: false,
>+                                     postData: null,
>+                                     referrerURI: doc.documentURIObject });
>   },
> 
>   saveVideoFrameAsImage: function () {
>     urlSecurityCheck(this.mediaURL, this.browser.contentPrincipal,
>                      Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
>     let name = "";
>     try {
>       let uri = makeURI(this.mediaURL);
>@@ -870,17 +876,20 @@ nsContextMenu.prototype = {
>   },
> 
>   // Change current window to the URL of the background image.
>   viewBGImage: function(e) {
>     urlSecurityCheck(this.bgImageURL,
>                      this.browser.contentPrincipal,
>                      Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
>     var doc = this.target.ownerDocument;
>-    openUILink(this.bgImageURL, e, null, null, null, null, doc.documentURIObject );
>+    openUILink(this.bgImageURL, "current", { disallowInheritPrincipal: true,
>+                                             allowThirdPartyFixup: false,
>+                                             postData: null,
>+                                             referrerURI: doc.documentURIObject });
>   },
> 
>   disableSetDesktopBackground: function() {
>     // Disable the Set as Desktop Background menu item if we're still trying
>     // to load the image or the load failed.
>     if (!(this.target instanceof Ci.nsIImageLoadingContent))
>       return true;
>
Attachment #607621 - Attachment is obsolete: true
Posted patch Patch 6 (obsolete) — Splinter Review
Sorry about the spam
Attachment #607623 - Flags: feedback?(dao)
Comment on attachment 607623 [details] [diff] [review]
Patch 6

openUILink's second argument needs to be the event object
Attachment #607623 - Flags: feedback?(dao) → feedback-
Posted patch Patch 7 (obsolete) — Splinter Review
It seems like 'this' is the event for 2 of the methods, but one of them is passed in 'e', correct?
Attachment #607623 - Attachment is obsolete: true
Attachment #607654 - Flags: feedback?(dao)
(In reply to David Dahl :ddahl from comment #26)
> Created attachment 607654 [details] [diff] [review]
> Patch 7
> 
> It seems like 'this' is the event for 2 of the methods, but one of them is
> passed in 'e', correct?

No... The two openUILink calls with the 'e' argument should remain openUILink calls with the 'e' argument. this.browser.loadURI you probably want to replace with openUILinkIn, since you have no event object there.
Posted patch Patch 8 (obsolete) — Splinter Review
Attachment #607654 - Attachment is obsolete: true
Attachment #607691 - Flags: feedback?(dao)
Attachment #607654 - Flags: feedback?(dao)
Comment on attachment 607691 [details] [diff] [review]
Patch 8

>     urlSecurityCheck(frameURL, this.browser.contentPrincipal,
>                      Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);

Do we still need this?

>-    var referrer = doc.referrer;
>-    this.browser.loadURI(frameURL, referrer ? makeURI(referrer) : null);
>+    openUILinkIn(frameURL, "current", { disallowInheritPrincipal: true,
>+                                        referrerURI: doc.documentURIObject });

doc.referrer -> doc.documentURIObject looks like an unintended change
(In reply to Dão Gottwald [:dao] from comment #29)

> Do we still need this?
> 
> >-    var referrer = doc.referrer;
> >-    this.browser.loadURI(frameURL, referrer ? makeURI(referrer) : null);
> >+    openUILinkIn(frameURL, "current", { disallowInheritPrincipal: true,
> >+                                        referrerURI: doc.documentURIObject });
> 
> doc.referrer -> doc.documentURIObject looks like an unintended change

using the referrer variable causes this:

JavaScript error: , line 0: uncaught exception: [Exception... "Could not convert JavaScript argument arg 2 [nsIWebNavigation.loadURI]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: chrome://global/content/bindings/browser.xml :: loadURIWithFlags :: line 193"  data: no]

with doc.documentURIObject, it works.
I think we need to keep the security checks - they serve a purpose beyond simply blocking javascript: (e.g. preventing loads of chrome-privileged pages).
(In reply to David Dahl :ddahl from comment #30)
> > doc.referrer -> doc.documentURIObject looks like an unintended change
> 
> using the referrer variable causes this:

That doesn't really make any sense - in fact openUILinkIn doesn't even use aCharset for the "current" case (this seems like an existing bug). How are you testing exactly?
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #32)
> How are you testing exactly?

I am testing via the test case attached to the bug: 

https://bug734076.bugzilla.mozilla.org/attachment.cgi?id=604053&t=2ZPFzMpYg0
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #32)
> That doesn't really make any sense - in fact openUILinkIn doesn't even use
> aCharset for the "current" case (this seems like an existing bug). How are
> you testing exactly?
So the LOAD_FLAGS used when this error occurs is '262144' - well that is what is dumped to the console in: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#193

I'll keep digging...
Attachment #607691 - Flags: feedback?(dao)
I spent a little time working on browser chrome tests for this bug, but it requires cross domain loading of pages, which seems like something we don't want to do in our automated test suite. Is there an example of this kind of test or should this be manually tested?
The test suite supports cross-domain stuff, as long as you stick to domain names that the test proxy maps to the test http server.  See http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt for the list of supported hostnames and whatnot.
Posted patch Patch 9 Test problem (obsolete) — Splinter Review
Integrating the tests here - the test cases fail onload:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug_734076.js | an unexpected uncaught JS exception reported through window.onerror - uncaught exception: Load of  from http://example.com/browser/browser/base/content/test/test-bug-734076.html denied. at :0

Why would the same script throw like this inside a mochitest, but not in Nightly?

(The page does render, I am unsure what that error means.)
Attachment #607691 - Attachment is obsolete: true
Attachment #609720 - Flags: feedback?(dao)
urlSecurityCheck (called by viewBGImage) apparently throws this exception.
You can just use expectUncaughtException.
(In reply to Dão Gottwald [:dao] from comment #38)
> urlSecurityCheck (called by viewBGImage) apparently throws this exception.

How do you provide the proper event when calling viewBGImage?
You either open the menu for real and synthesize a mouse event or you fake the event object, but this doesn't have much to do with what I wrote, since urlSecurityCheck doesn't look at the event object.
Comment on attachment 609720 [details] [diff] [review]
Patch 9 Test problem

>+++ b/browser/base/content/test/browser_bug_734076.js

>+function tabLoad1(aEvent) {
>+  browser.removeEventListener(aEvent.type, arguments.callee, true);
>+  document.popupNode = document.firstChild;

document.firstChild is a browser.xul node.
(In reply to Dão Gottwald [:dao] from comment #41)
> You either open the menu for real and synthesize a mouse event or you fake
> the event object

When I try to synthesize the event, I am never able to open the contextmenu in content. I am however able to open one for chrome:

EventUtils.synthesizeMouse(tab.linkedBrowser.contentWindow.document.firstChild,
                           10,
                           10,
                           { type: "contextmenu", button: 2},
                           tab.linkedBrowser.contentWindow);

When I attempt to fake it and open the context menu programmatically, the browser opens about:logo

  document.popupNode = tab.linkedBrowser.contentWindow.document.firstChild;
  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");
  let contextMenu = new nsContextMenu(contentAreaContextMenu, gBrowser); // does not seem to open the menu
  expectUncaughtException();

  contextMenu.viewBGImage({ctrlKey: true}); // causes the load of about:logo


Can you explain how you would do this or point me to an example?
contentWindow.document.firstChild is <html> where you set about:logo as the background image, so this seems to be working as expected. If you want to get the body's background, use contentWindow.document.body. However, you need to make sure to do this /after/ the content page sets the background image.
Can we get an updated patch here, David? I do so hate sg:highs in the Firefox component.
(In reply to Johnathan Nightingale [:johnath] from comment #45)
> Can we get an updated patch here, David? I do so hate sg:highs in the
> Firefox component.

I apologize, this bug slipped off of my radar.

I am seeing completely different behavior using the nsContextMenu API vs testing by hand with mouse clicks.

I am quite sure the tests for this bug will take me a long time to write. In the interests of brevity, a more skilled mochitest writer (with EventUtils skillz) might be a better way to get this patch landed quickly.
Assignee: ddahl → gavin.sharp
Attachment #609720 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #616324 - Flags: review?(dao)
Attachment #609720 - Flags: feedback?(dao)
Comment on attachment 616324 [details] [diff] [review]
patch with tests

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>   showOnlyThisFrame: function() {
>     var doc = this.target.ownerDocument;

>     var referrer = doc.referrer;
>+    openUILinkIn(frameURL, "current", { disallowInheritPrincipal: true,
>+                                        referrerURI: referrer ? makeURI(referrer) : null });

This code does actually look bogus (I don't think it makes sense to use the iframe's referrer as a referrer for the new load), but I'll file a separate bug about that (it also applies to the other frame menu items).
We probably also need to fix openLinkInCurrent. I forgot to audit for other places where we load things into the current page.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #48)
> This code does actually look bogus

(filed bug 746781)
Attachment #616324 - Flags: review?(dao) → review+
Keywords: sec-high
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #49)
> We probably also need to fix openLinkInCurrent

This turns out to be unnecessary since the plain text link detection code will only activate for http[s] links. I audited the file for other potential "current" loads and didn't find any.
(We may still want to add some additional belt-and-braces protection to openLinkInCurrent, but I don't think we need to block this on it.)
Comment on attachment 616324 [details] [diff] [review]
patch with tests

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: XSS vulnerability if use can be convinced to use certain context menu functionality ("View Image", "Show only this frame", "View background image")
Testing completed (on m-c, etc.): Landed on inbound May 7th
Risk to taking this patch (and alternatives if risky): low risk - only affects specific context menu functionality, and test coverage is good.
String changes made by this patch: None
Attachment #616324 - Flags: approval-mozilla-aurora?
Given that the patch depends on bug 631500 (which is only on Aurora), backporting to beta would be a bit trickier. I'm not sure whether we should bother.
https://hg.mozilla.org/mozilla-central/rev/78e5d85cb6ac
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Comment on attachment 616324 [details] [diff] [review]
patch with tests

[Triage Comment]
Low risk sg:high fix. Approved for Aurora 14.

Is the ESR affected as well? If so, it sounds like we may decide not to fix there as well given bug 631500 is a dep.
Attachment #616324 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Yes, ESR is also affected. I can just look into also backporting bug 631500.
Verified fixed in 5/13 Nightly trunk build.

Why did we check the tests in on an SG:High bug before release?
Status: RESOLVED → VERIFIED
I don't think the tests reveal much more than the patch does, fwiw (it's certainly not a "just copy this code to own someone" kind of test).

I think I'd like to skip trying to backport a fix for this to ESR. sg:high might overstate the importance of this...
Posted patch ESR-10 patchSplinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: Firefox 15
Risk to taking this patch (and alternatives if risky): some context menu options could break. test coverage here is pretty good, though.
String or UUID changes made by this patch: none
Attachment #636003 - Flags: approval-mozilla-esr10?
Attachment #616324 - Flags: approval-mozilla-beta?
Looks like I forgot to check this in on Aurora 14 when I got that approval :(

So I'm re-requesting approval for Beta 14.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #60)
> I think I'd like to skip trying to backport a fix for this to ESR. sg:high
> might overstate the importance of this...

In case it wasn't clear from the activity, I had a change of heart.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #63)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #60)
> > I think I'd like to skip trying to backport a fix for this to ESR. sg:high
> > might overstate the importance of this...
> 
> In case it wasn't clear from the activity, I had a change of heart.

Do you think this needs to be rushed into beta in that case? Or can we let this ride the trains at this point?
I don't think it needs to be "rushed" any more than any other security bug. I think we should take it on beta, though. We're only halfway through the cycle, so I don't really see any reason not to take it (ESR is a bit more complicated given dependency on bug 631500, but even that is pretty straightforward I think).
Comment on attachment 616324 [details] [diff] [review]
patch with tests

[Triage Comment]
This will go into the fifth beta, so we'll still have one more opportunity to fix any regressions. Approved for Beta.
Attachment #616324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #636003 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Whiteboard: [sg:high] → [sg:high][advisory-tracking+]
Alias: CVE-2012-1966
This is not fixed in 14.0.1. I just tried with the release candidate build and compared it against trunk (7/15) and Firefox 13.0.1 behavior. Firefox 14.0.1 is behaving the same as 13.0.1 and following the various "view..." context menu items in the attached testcases is showing a data:url source on both 13.0.1 and 14.0.1 but not nightly.
All right, there are issues with the PoC's. Gavin whipped up a new one and I reconfirmed that this is properly fixed in 14.0.1 and trunk.
Al, where is the test you used to verify this? I'd like to test against ESR and 15.
There are three tests attached to this bug. AFAIK, that's what I used.
Okay, I'll try using those tests. I guess my confusion was in comment 69 and 70. It wasn't clear to me that those tests were ones which Gavin had "fixed".
Group: core-security
You need to log in before you can comment on or make changes to this bug.