733305 - (CVE-2013-0747) I can confuse gPluginHandler.handleEvent by listening for mutation events

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jesse Ruderman]

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]

Attachment: testcase

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Does this only happen on OS X?  Does the testcase only work there?

Comment 3

[Jesse Ruderman]

Attachment: testcase 2

Comment 4

[Jesse Ruderman]

With testcase 2, I can reproduce on Mac and Windows.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

> 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][[e-mail]]]

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

(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)]

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][[e-mail]]]

Bah, my mistake. Jesse said anonymous, not native anonymous.

Comment 11

[Bobby Holley (:bholley)]

(the div doesn't appear to be a bound node either)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

But where is the div coming from? Missing plugin thingie? Shouldn't that be native anon.

Comment 13

[Benjamin Smedberg]

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][[e-mail]]]

Attachment: wip

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

Comment on attachment 662286 [details] [diff] [review]
wip

Boris, what do you think of this?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 662286 [details] [diff] [review]
wip

Hmm, default content handling is wrong.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Make sure also default content is chrome-only.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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][[e-mail]]]

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][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/24783c876df0

Comment 22

[Alex Keybl [:akeybl]]

(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][[e-mail]]]

Test manually whether the plugin-problem widget works.

Comment 24

[u279076]

(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: [e-mail]]]

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]

Anthony does comment 25 help? Looking for uplift nomination here.

Comment 27

[u279076]

(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

[u279076]

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

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

(and the fact that the security regression is internally reported, and difficult to discover from the patch)

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Yeah, aurora might be enough for this.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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]

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

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][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/24566e7f1b5d

Comment 36

[bhavana bajaj [:bajaj]]

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

(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][[e-mail]]]

The risk is quite high, and the patch does require changing few interfaces.

Comment 39

[Lukas Blakk [:lsblakk] use ?needinfo]

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][[e-mail]]]

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]

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][[e-mail]]]

(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][[e-mail]]]

Attachment: esr17

I need to test this a bit.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/36cc4da3469c