Last Comment Bug 734076 - (CVE-2012-1966) XSS with context menu
(CVE-2012-1966)
: XSS with context menu
Status: VERIFIED FIXED
[sg:high][advisory-tracking+]
: sec-high
Product: Firefox
Classification: Client Software
Component: Security (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 15
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on: 631500
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 07:25 PST by moz_bug_r_a4
Modified: 2012-12-01 06:32 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
fixed
14+
fixed


Attachments
Patch 1 - Easy Fix - Does not work (10.22 KB, patch)
2012-03-15 22:00 PDT, David Dahl :ddahl
no flags Details | Diff | Review
Patch 2 - targeting is incorrect (4.71 KB, patch)
2012-03-16 13:41 PDT, David Dahl :ddahl
no flags Details | Diff | Review
Patch 3 - working - needs tests (3.02 KB, patch)
2012-03-16 15:54 PDT, David Dahl :ddahl
no flags Details | Diff | Review
Patch 4 (3.04 KB, patch)
2012-03-20 08:16 PDT, David Dahl :ddahl
no flags Details | Diff | Review
Patch 5 (3.02 KB, patch)
2012-03-20 10:51 PDT, David Dahl :ddahl
no flags Details | Diff | Review
Patch 6 (2.65 KB, patch)
2012-03-20 10:59 PDT, David Dahl :ddahl
dao+bmo: feedback-
Details | Diff | Review
Patch 7 (2.61 KB, patch)
2012-03-20 12:12 PDT, David Dahl :ddahl
no flags Details | Diff | Review
Patch 8 (2.62 KB, patch)
2012-03-20 13:34 PDT, David Dahl :ddahl
no flags Details | Diff | Review
Patch 9 Test problem (6.40 KB, patch)
2012-03-27 08:36 PDT, David Dahl :ddahl
no flags Details | Diff | Review
patch with tests (6.85 KB, patch)
2012-04-18 15:46 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
dao+bmo: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
ESR-10 patch (6.89 KB, patch)
2012-06-22 19:31 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description moz_bug_r_a4 2012-03-08 07:25:37 PST
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 2012-03-08 07:27:04 PST
Created attachment 604053 [details]
testcase 1 - showOnlyThisFrame

This tries to get cookies for www.mozilla.org.
Comment 2 moz_bug_r_a4 2012-03-08 07:28:14 PST
Created attachment 604054 [details]
testcase 2 - viewMedia

This tries to get cookies for www.mozilla.org.
Comment 3 moz_bug_r_a4 2012-03-08 07:29:19 PST
Created attachment 604057 [details]
testcase 3 - viewBGImage

This tries to get cookies for www.mozilla.org.
Comment 4 Daniel Veditz [:dveditz] 2012-03-14 17:08:19 PDT
This is probably in the context-menu code. Toolkit?
Comment 5 Dietrich Ayala (:dietrich) 2012-03-15 16:11:43 PDT
Drew, can you look at this asap?
Comment 6 Dietrich Ayala (:dietrich) 2012-03-15 16:13:07 PDT
Drew is on PTO this week. Re-assigning to David.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-15 16:58:22 PDT
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 2012-03-15 19:51:15 PDT
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 2012-03-15 22:00:53 PDT
Created attachment 606458 [details] [diff] [review]
Patch 1 - Easy Fix - Does not work

All test cases still work with this "easy fix" patch applied
Comment 10 David Dahl :ddahl 2012-03-15 22:02:26 PDT
(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 2012-03-16 09:31:03 PDT
(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 ?
Comment 12 David Dahl :ddahl 2012-03-16 13:09:51 PDT
(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 2012-03-16 13:41:06 PDT
Created attachment 606715 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-03-16 14:45:35 PDT
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 2012-03-16 15:54:28 PDT
Created attachment 606775 [details] [diff] [review]
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 2012-03-16 16:27:08 PDT
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] 2012-03-17 05:07:26 PDT
"current" doesn't actually maintain the current behavior. What openUILink does depends on the event.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-19 15:19:20 PDT
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 2012-03-20 08:16:48 PDT
Created attachment 607560 [details] [diff] [review]
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] 2012-03-20 08:28:55 PDT
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 2012-03-20 10:51:21 PDT
Created attachment 607621 [details] [diff] [review]
Patch 5
Comment 22 David Dahl :ddahl 2012-03-20 10:52:07 PDT
(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 2012-03-20 10:54:21 PDT
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 2012-03-20 10:59:22 PDT
Created attachment 607623 [details] [diff] [review]
Patch 6

Sorry about the spam
Comment 25 Dão Gottwald [:dao] 2012-03-20 11:53:28 PDT
Comment on attachment 607623 [details] [diff] [review]
Patch 6

openUILink's second argument needs to be the event object
Comment 26 David Dahl :ddahl 2012-03-20 12:12:14 PDT
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?
Comment 27 Dão Gottwald [:dao] 2012-03-20 12:15:29 PDT
(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 2012-03-20 13:34:42 PDT
Created attachment 607691 [details] [diff] [review]
Patch 8
Comment 29 Dão Gottwald [:dao] 2012-03-20 13:51:31 PDT
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 2012-03-20 14:18:36 PDT
(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: gavin@gavinsharp.com] 2012-03-20 14:18:51 PDT
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: gavin@gavinsharp.com] 2012-03-20 14:22:26 PDT
(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 2012-03-20 14:34:06 PDT
(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
Comment 34 David Dahl :ddahl 2012-03-21 12:02:34 PDT
(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...
Comment 35 David Dahl :ddahl 2012-03-26 10:29:44 PDT
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 [:bz] (Out June 25-July 6) 2012-03-26 11:09:19 PDT
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 2012-03-27 08:36:12 PDT
Created attachment 609720 [details] [diff] [review]
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] 2012-03-27 08:43:40 PDT
urlSecurityCheck (called by viewBGImage) apparently throws this exception.
Comment 39 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-27 10:39:08 PDT
You can just use expectUncaughtException.
Comment 40 David Dahl :ddahl 2012-03-27 11:11:20 PDT
(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] 2012-03-27 11:14:58 PDT
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] 2012-03-27 11:16:35 PDT
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 2012-03-27 14:16:44 PDT
(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] 2012-03-28 11:18:32 PDT
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] 2012-04-16 06:45:23 PDT
Can we get an updated patch here, David? I do so hate sg:highs in the Firefox component.
Comment 46 David Dahl :ddahl 2012-04-16 10:52:36 PDT
(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: gavin@gavinsharp.com] 2012-04-18 15:46:04 PDT
Created attachment 616324 [details] [diff] [review]
patch with tests
Comment 48 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-18 15:50:54 PDT
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: gavin@gavinsharp.com] 2012-04-18 15:53:47 PDT
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: gavin@gavinsharp.com] 2012-04-18 15:56:39 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #48)
> This code does actually look bogus

(filed bug 746781)
Comment 51 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-07 15:33:22 PDT
(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.
Comment 52 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-07 15:35:46 PDT
(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: gavin@gavinsharp.com] 2012-05-07 16:52:17 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e5d85cb6ac
Comment 54 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-07 16:57:09 PDT
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: gavin@gavinsharp.com] 2012-05-07 16:57:54 PDT
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] 2012-05-08 03:21:51 PDT
https://hg.mozilla.org/mozilla-central/rev/78e5d85cb6ac
Comment 57 Alex Keybl [:akeybl] 2012-05-09 16:17:24 PDT
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: gavin@gavinsharp.com] 2012-05-09 16:24:39 PDT
Yes, ESR is also affected. I can just look into also backporting bug 631500.
Comment 59 [On PTO until 6/29] 2012-05-14 14:52:36 PDT
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: gavin@gavinsharp.com] 2012-06-21 16:20:59 PDT
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: gavin@gavinsharp.com] 2012-06-22 19:31:37 PDT
Created attachment 636003 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-06-22 19:37:25 PDT
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: gavin@gavinsharp.com] 2012-06-22 19:41:17 PDT
(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.
Comment 64 Alex Keybl [:akeybl] 2012-06-24 12:14:11 PDT
(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?
Comment 65 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-24 14:54:51 PDT
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] 2012-06-26 09:37:31 PDT
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: gavin@gavinsharp.com] 2012-06-26 17:16:49 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/a017dff24aa4
Comment 68 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-29 09:33:31 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/a7b0106943ba
Comment 69 [On PTO until 6/29] 2012-07-16 14:39:50 PDT
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 [On PTO until 6/29] 2012-07-16 15:10:51 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-13 15:25:41 PDT
Al, where is the test you used to verify this? I'd like to test against ESR and 15.
Comment 72 [On PTO until 6/29] 2012-09-14 16:25:47 PDT
There are three tests attached to this bug. AFAIK, that's what I used.
Comment 73 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-14 16:58:33 PDT
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".

Note You need to log in before you can comment on or make changes to this bug.