734076 - (CVE-2012-1966) XSS with context menu

Status: VERIFIED FIXED

(Firefox :: Security, defect)

Description

[moz_bug_r_a4]

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.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - showOnlyThisFrame

This tries to get cookies for www.mozilla.org.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - viewMedia

This tries to get cookies for www.mozilla.org.

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3 - viewBGImage

This tries to get cookies for www.mozilla.org.

Comment 4

[Daniel Veditz [:dveditz]]

This is probably in the context-menu code. Toolkit?

Comment 5

[Dietrich Ayala (:dietrich)]

Drew, can you look at this asap?

Comment 6

[Dietrich Ayala (:dietrich)]

Drew is on PTO this week. Re-assigning to David.

Comment 7

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

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

[David Dahl :ddahl]

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

[David Dahl :ddahl]

Attachment: Patch 1 - Easy Fix - Does not work

All test cases still work with this "easy fix" patch applied

Comment 10

[David Dahl :ddahl]

(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.

Comment 11

[David Dahl :ddahl]

(In reply to Gavin Sharp (use [e-mail] 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 ?

Comment 12

[David Dahl :ddahl]

(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...

Comment 13

[David Dahl :ddahl]

Attachment: Patch 2 - targeting is incorrect

This patch fixes the cookie value snooping, but targets things to new tabs or a new window.

Comment 14

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

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.

Comment 15

[David Dahl :ddahl]

Attachment: Patch 3 - working - needs tests

Ok, this works... also my targeting was wrong in the prev. patch as I had the tabs pinned, whoops.

Comment 16

[David Dahl :ddahl]

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?

Comment 17

[Dão Gottwald [:dao]]

"current" doesn't actually maintain the current behavior. What openUILink does depends on the event.

Comment 18

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

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.

Comment 19

[David Dahl :ddahl]

Attachment: Patch 4

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?

Comment 20

[Dão Gottwald [:dao]]

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".

Comment 21

[David Dahl :ddahl]

Attachment: Patch 5

Comment 22

[David Dahl :ddahl]

(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.

Comment 23

[David Dahl :ddahl]

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;
>

Comment 24

[David Dahl :ddahl]

Attachment: Patch 6

Sorry about the spam

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 607623 [details] [diff] [review]
Patch 6

openUILink's second argument needs to be the event object

Comment 26

[David Dahl :ddahl]

Attachment: Patch 7

It seems like 'this' is the event for 2 of the methods, but one of them is passed in 'e', correct?

Comment 27

[Dão Gottwald [: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.

Comment 28

[David Dahl :ddahl]

Attachment: Patch 8

Comment 29

[Dão Gottwald [: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

Comment 30

[David Dahl :ddahl]

(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.

Comment 31

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

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).

Comment 32

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

(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?

Comment 33

[David Dahl :ddahl]

(In reply to Gavin Sharp (use [e-mail] 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

Comment 34

[David Dahl :ddahl]

(In reply to Gavin Sharp (use [e-mail] 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...

Comment 35

[David Dahl :ddahl]

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?

Comment 36

[Boris Zbarsky [:bzbarsky]]

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.

Comment 37

[David Dahl :ddahl]

Attachment: Patch 9 Test problem

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.)

Comment 38

[Dão Gottwald [:dao]]

urlSecurityCheck (called by viewBGImage) apparently throws this exception.

Comment 39

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

You can just use expectUncaughtException.

Comment 40

[David Dahl :ddahl]

(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?

Comment 41

[Dão Gottwald [:dao]]

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 42

[Dão Gottwald [:dao]]

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.

Comment 43

[David Dahl :ddahl]

(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?

Comment 44

[Dão Gottwald [:dao]]

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.

Comment 45

[Johnathan Nightingale [:johnath]]

Can we get an updated patch here, David? I do so hate sg:highs in the Firefox component.

Comment 46

[David Dahl :ddahl]

(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.

Comment 47

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

Attachment: patch with tests

Comment 48

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

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).

Comment 49

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

We probably also need to fix openLinkInCurrent. I forgot to audit for other places where we load things into the current page.

Comment 50

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #48)
> This code does actually look bogus

(filed bug 746781)

Comment 51

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

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 52

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

(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 53

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/78e5d85cb6ac

Comment 54

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

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

Comment 55

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

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.

Comment 56

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78e5d85cb6ac

Comment 57

[Alex Keybl [:akeybl]]

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.

Comment 58

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

Yes, ESR is also affected. I can just look into also backporting bug 631500.

Comment 59

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

Verified fixed in 5/13 Nightly trunk build.

Why did we check the tests in on an SG:High bug before release?

Comment 60

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

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...

Comment 61

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

Attachment: ESR-10 patch

[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

Comment 62

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

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.

Comment 63

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

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 64

[Alex Keybl [:akeybl]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #63)
> (In reply to :Gavin Sharp (use [e-mail] 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?

Comment 65

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

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 66

[Alex Keybl [:akeybl]]

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.

Comment 67

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

https://hg.mozilla.org/releases/mozilla-beta/rev/a017dff24aa4

Comment 68

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

https://hg.mozilla.org/releases/mozilla-esr10/rev/a7b0106943ba

Comment 69

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

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.

Comment 70

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

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.

Comment 71

[u279076]

Al, where is the test you used to verify this? I'd like to test against ESR and 15.

Comment 72

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

There are three tests attached to this bug. AFAIK, that's what I used.

Comment 73

[u279076]

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".