Last Comment Bug 733305 - (CVE-2013-0747) I can confuse gPluginHandler.handleEvent by listening for mutation events
(CVE-2013-0747)
: I can confuse gPluginHandler.handleEvent by listening for mutation events
Status: RESOLVED FIXED
[adv-main18+][adv-esr17+]
: sec-high
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Olli Pettay [:smaug] (TPAC)
: Matt Wobensmith [:mwobensmith][:matt:]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-06 00:04 PST by Jesse Ruderman
Modified: 2013-04-30 18:41 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed
+
fixed
18+
wontfix
18+
fixed


Attachments
testcase (441 bytes, text/html)
2012-03-06 00:05 PST, Jesse Ruderman
no flags Details
testcase 2 (460 bytes, text/html)
2012-09-16 04:37 PDT, Jesse Ruderman
no flags Details
wip (42.76 KB, patch)
2012-09-18 13:14 PDT, Olli Pettay [:smaug] (TPAC)
bugs: review-
Details | Diff | Splinter Review
testcase (459 bytes, text/html)
2012-09-18 15:04 PDT, Olli Pettay [:smaug] (TPAC)
no flags Details
patch (42.89 KB, patch)
2012-09-18 15:34 PDT, Olli Pettay [:smaug] (TPAC)
bzbarsky: review+
bajaj.bhavana: approval‑mozilla‑aurora+
abillings: sec‑approval+
Details | Diff | Splinter Review
esr17 (35.66 KB, patch)
2012-12-21 05:34 PST, Olli Pettay [:smaug] (TPAC)
akeybl: approval‑mozilla‑esr17+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-03-06 00:04:35 PST
JavaScript error: chrome://browser/content/browser.js, line 10437: iconStatus is null

>          let installStatus = doc.getAnonymousElementByAttribute(plugin, "class", "installStatus");
>          installStatus.setAttribute("status", "ready");
>          let iconStatus = doc.getAnonymousElementByAttribute(plugin, "class", "icon");
>          iconStatus.setAttribute("status", "ready");

The page gets an event whose originalTarget is an anonymous DIV.  I wouldn't expect the page to be able to get a reference to this anonymous content. Related to bug 329813?
Comment 1 Jesse Ruderman 2012-03-06 00:05:03 PST
Created attachment 603180 [details]
testcase
Comment 2 Steven Michaud [:smichaud] (Retired) 2012-03-06 08:06:09 PST
Does this only happen on OS X?  Does the testcase only work there?
Comment 3 Jesse Ruderman 2012-09-16 04:37:04 PDT
Created attachment 661582 [details]
testcase 2
Comment 4 Jesse Ruderman 2012-09-16 04:37:51 PDT
With testcase 2, I can reproduce on Mac and Windows.
Comment 5 Olli Pettay [:smaug] (TPAC) 2012-09-16 07:54:26 PDT
(In reply to Jesse Ruderman from comment #0)
> The page gets an event whose originalTarget is an anonymous DIV.  I wouldn't
> expect the page to be able to get a reference to this anonymous content.
> Related to bug 329813?
What? Can web page access native anon?
Comment 6 Jesse Ruderman 2012-09-16 12:41:16 PDT
> What? Can web page access native anon?

Yep. Stick in "alert(event.originalTarget)" and you'll see a div element.
Comment 7 Olli Pettay [:smaug] (TPAC) 2012-09-16 12:47:00 PDT
Has something regressed then. Content pages shouldn't be able to access native anon content.
IIRC, there used to be an exception if that happened.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-09-16 15:31:37 PDT
(In reply to Olli Pettay [:smaug] from comment #7)
> Has something regressed then. Content pages shouldn't be able to access
> native anon content.

It shouldn't, SOWs should be saving us here. I'll look at this soon.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-09-17 08:00:34 PDT
So, why do we believe that the div is Native Anonymous Content? grabbing it in a debugger, it appears that div->IsInNativeAnonymousSubtree() is false, which explains why SOWs aren't wrapping it...
Comment 10 Olli Pettay [:smaug] (TPAC) 2012-09-17 08:05:59 PDT
Bah, my mistake. Jesse said anonymous, not native anonymous.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-09-17 08:06:42 PDT
(the div doesn't appear to be a bound node either)
Comment 12 Olli Pettay [:smaug] (TPAC) 2012-09-17 08:09:37 PDT
But where is the div coming from? Missing plugin thingie? Shouldn't that be native anon.
Comment 13 Benjamin Smedberg [:bsmedberg] 2012-09-17 08:43:09 PDT
smaug said he'd take this. It's not clear to me whether this is directly exploitable, but looking through browser-plugins.js I'd bet a dedicated attacker could turn it into something pretty serious by rearranging the anonymous DOM and clickjacking plugin install prompts.

The plan we hatched on IRC was to add a <binding native="true"> attribute which would force the pluginProblem XBL subtree to be considered native-anonymous instead of just anonymous, which would prevent access from content script.
Comment 14 Olli Pettay [:smaug] (TPAC) 2012-09-18 13:14:17 PDT
Created attachment 662286 [details] [diff] [review]
wip
Comment 15 Olli Pettay [:smaug] (TPAC) 2012-09-18 15:04:02 PDT
Created attachment 662324 [details]
testcase

So, with the patch we end up throwing when accessing anon inside the binding stuff.
And yes, we should not expose nsIDOMDocumentXBL to web, but that is a separate
bug.
Comment 16 Olli Pettay [:smaug] (TPAC) 2012-09-18 15:17:15 PDT
Comment on attachment 662286 [details] [diff] [review]
wip

Boris, what do you think of this?
Comment 17 Olli Pettay [:smaug] (TPAC) 2012-09-18 15:22:15 PDT
Comment on attachment 662286 [details] [diff] [review]
wip

Hmm, default content handling is wrong.
Comment 18 Olli Pettay [:smaug] (TPAC) 2012-09-18 15:34:39 PDT
Created attachment 662337 [details] [diff] [review]
patch

Make sure also default content is chrome-only.
Comment 19 Boris Zbarsky [:bz] (TPAC) 2012-09-30 08:23:48 PDT
Comment on attachment 662337 [details] [diff] [review]
patch

>+++ b/content/base/public/nsINode.h
>+  // True for native anonymous content and for XBL content if the binging

s/binging/binding/

>+++ b/content/base/src/FragmentOrElement.cpp
>@@ -906,23 +906,24 @@ nsIContent::PreHandleEvent(nsEventChainP
>-                       : (IsInNativeAnonymousSubtree()
>+                       : (ChromeOnlyAccess()
>                            ? "(is in native anonymous subtree)" : ""),

Fix the string there too?

>+                     relatedTarget->ChromeOnlyAccess()
>                        ? "(is in native anonymous subtree)" : "",

And here.

>+++ b/content/xbl/src/nsXBLBinding.h
>+  void InstallAnonymousContent(nsIContent* aAnonParent, nsIContent* aElement,
>+                               bool aNativeAnon);

Fix the arg name, please.

r=me
Comment 20 Olli Pettay [:smaug] (TPAC) 2012-09-30 12:44:21 PDT
Comment on attachment 662337 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Shouldn't be too easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The commit message will just tell what the patch will let us do -
mark XBL hidden from content.

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to create patches for branches.

How likely is this patch to cause regressions; how much testing does it need?
Needs some manual testing for pluginProblem.xml to make sure it still works the
way we want to.
Comment 21 Olli Pettay [:smaug] (TPAC) 2012-10-10 13:39:58 PDT
https://hg.mozilla.org/mozilla-central/rev/24783c876df0
Comment 22 Alex Keybl [:akeybl] 2012-10-11 16:10:40 PDT
(In reply to Olli Pettay [:smaug] from comment #20)
> How likely is this patch to cause regressions; how much testing does it need?
> Needs some manual testing for pluginProblem.xml to make sure it still works
> the
> way we want to.

We're interested in getting QA verification/regression testing before uplifting. Can you advise?
Comment 23 Olli Pettay [:smaug] (TPAC) 2012-10-11 18:11:42 PDT
Test manually whether the plugin-problem widget works.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-15 16:16:30 PDT
(In reply to Olli Pettay [:smaug] from comment #23)
> Test manually whether the plugin-problem widget works.

Can you please be more specific? I don't know where to begin.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-15 23:08:23 PDT
The "plugin-problem widget" is the UI that shows up in content when a plugin crashes, is missing, is blocked, is disabled, needs to be click to played, etc. (there are many possible causes, the same binding is used for all of them). So ideally I guess that the UI (links, clicking behavior, etc.) in all of those cases is still functional. I don't know how much of this is covered by automated tests.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-plugins.js#52 is the relevant code.
Comment 26 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:50:44 PDT
Anthony does comment 25 help? Looking for uplift nomination here.
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-21 16:37:13 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #26)
> Anthony does comment 25 help? Looking for uplift nomination here.

Getting this tested in time for 17.0b3 uplift might be tight. I'll ask Softvision to do some dogfooding along the lines of comment 25 in the latest Nightly. If they don't find any issues, we can move ahead with uplift nomination.
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-22 07:28:22 PDT
No regressions nor crashes were experienced in overnight testing by Softvision.

I had them check the following basic scenarios:
 * loading plugin content when a plugin is not installed
 * loading plugin content when a plugin is blocked
 * loading plugin content when a plugin is disabled
 * loading plugin content when a plugin is click-to-play blocked 

...against the following plug-ins:
 * Adobe Flash Player
 * Adobe Reader
 * Apple QuickTime
 * Microsoft Silverlight
 * Oracle Java, Quicktime

...on the following platforms:
 * Mac OSX 10.7.5
 * Windows 7
 * Ubuntu 12.04

Again, no regressions nor crashes were encountered.
Comment 29 Alex Keybl [:akeybl] 2012-10-25 16:07:08 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #26)
> Anthony does comment 25 help? Looking for uplift nomination here.

Olli - given Anthony's testing, please nominate for uplift as soon as possible. We may not take this fix for beta depending on risk and the size of this patch.
Comment 30 Alex Keybl [:akeybl] 2012-10-25 16:08:34 PDT
(and the fact that the security regression is internally reported, and difficult to discover from the patch)
Comment 31 Olli Pettay [:smaug] (TPAC) 2012-10-25 16:54:02 PDT
Yeah, aurora might be enough for this.
Comment 32 Olli Pettay [:smaug] (TPAC) 2012-10-25 16:56:27 PDT
Comment on attachment 662337 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  NA
User impact if declined: Possible risk for security sensitive crashes
Testing completed (on m-c, etc.): land m-c 2012-10-10
Risk to taking this patch (and alternatives if risky): Shouldn't be too risky.
Affects only pluginerror widget for now 
String or UUID changes made by this patch: NA
Comment 33 Robert Kaiser 2012-10-26 06:44:26 PDT
(In reply to Olli Pettay [:smaug] from comment #31)
> Yeah, aurora might be enough for this.

In this case, should it be marked wontfix for 17?
Comment 34 bhavana bajaj [:bajaj] 2012-10-26 15:27:20 PDT
Comment on attachment 662337 [details] [diff] [review]
patch

Approving it for Aurora as it is low risk and had good bake time on central.
Comment 35 Olli Pettay [:smaug] (TPAC) 2012-10-26 16:09:06 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/24566e7f1b5d
Comment 36 bhavana bajaj [:bajaj] 2012-10-26 19:40:08 PDT
(In reply to Olli Pettay [:smaug] from comment #31)
> Yeah, aurora might be enough for this.

Please feel free to uplift the patch , if you think its a low risk one  & the testing done is sufficient for beta population . Alternatively I can wontfix for FF17 if we are not too comfortable .
Comment 37 Alex Keybl [:akeybl] 2012-11-29 16:27:56 PST
(In reply to Olli Pettay [:smaug] from comment #35)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/24566e7f1b5d

Can we prepare patches for ESR10/17 now? Let us know if the risk to backporting is too great, and we'll reconsider the necessity for ESRs. Thanks!
Comment 38 Olli Pettay [:smaug] (TPAC) 2012-11-29 18:53:32 PST
The risk is quite high, and the patch does require changing few interfaces.
Comment 39 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-11 11:32:40 PST
Olli - we can definitely wontfix this for ESR10 since it's EOL anyway, but are you saying this changes interfaces for ESR17 as well? What would be the fallout if we left this unfixed on ESR17?
Comment 40 Olli Pettay [:smaug] (TPAC) 2012-12-11 12:51:32 PST
This would change interfaces in ESR17.

We don't really know what not-fixing this could cause.
Comment 13 has some guesses.
Comment 41 Benjamin Smedberg [:bsmedberg] 2012-12-11 13:08:23 PST
It looks to me like we can implement this in ESR17 without changing interface IDs. Specifically the only interface change here appears to be:

-  virtual nsIContent* FindFirstNonNativeAnonymous() const;
+  virtual nsIContent* FindFirstNonChromeOnlyAccessContent() const;

Since this is merely a renaming, it doesn't change the vtable layout or even the call signature, so I think we can make an ESR patch which simply doesn't change the nsIContent IID.

The other method on nsIContent

+  bool IsRootOfChromeAccessOnlySubtree() const

is nonvirtual and therefore doesn't affect the vtable/binary layout.
Comment 42 Olli Pettay [:smaug] (TPAC) 2012-12-11 13:15:03 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> It looks to me like we can implement this in ESR17 without changing
> interface IDs. Specifically the only interface change here appears to be:
> 
> -  virtual nsIContent* FindFirstNonNativeAnonymous() const;
> +  virtual nsIContent* FindFirstNonChromeOnlyAccessContent() const;
> 
> Since this is merely a renaming, it doesn't change the vtable layout or even
> the call signature, so I think we can make an ESR patch which simply doesn't
> change the nsIContent IID.
Hmm, true, although we'd change the meaning of an existing method a bit.
But I don't think that matters.
Comment 43 Olli Pettay [:smaug] (TPAC) 2012-12-21 05:34:24 PST
Created attachment 694813 [details] [diff] [review]
esr17

I need to test this a bit.
Comment 44 Olli Pettay [:smaug] (TPAC) 2012-12-21 06:20:58 PST
Comment on attachment 694813 [details] [diff] [review]
esr17

[Approval Request Comment]
User impact if declined: 
possible odd UI issues if evil page plays with XBL. Maybe even crashes.
Fix Landed on Version: FF19, FF18
Risk to taking this patch (and alternatives if risky): just tiny bit
risky if some binary addon uses nsINode bits for something.
String or UUID changes made by this patch: 
NA
Comment 45 Olli Pettay [:smaug] (TPAC) 2012-12-22 12:43:45 PST
https://hg.mozilla.org/releases/mozilla-esr17/rev/36cc4da3469c

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