Bug 733305 (CVE-2013-0747)

I can confuse gPluginHandler.handleEvent by listening for mutation events

RESOLVED FIXED in Firefox 18

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

({sec-high})

Trunk
mozilla18
x86
Mac OS X
sec-high
Points:
---

Firefox Tracking Flags

(firefox17+ wontfix, firefox18+ fixed, firefox19+ fixed, firefox-esr1018+ wontfix, firefox-esr1718+ fixed)

Details

(Whiteboard: [adv-main18+][adv-esr17+])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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?
(Reporter)

Comment 1

5 years ago
Created attachment 603180 [details]
testcase
Does this only happen on OS X?  Does the testcase only work there?
(Reporter)

Comment 3

5 years ago
Created attachment 661582 [details]
testcase 2
Attachment #603180 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
With testcase 2, I can reproduce on Mac and Windows.
(Assignee)

Comment 5

5 years ago
(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?
(Reporter)

Comment 6

5 years ago
> What? Can web page access native anon?

Yep. Stick in "alert(event.originalTarget)" and you'll see a div element.
(Assignee)

Comment 7

5 years ago
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.
Group: core-security
(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.
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...
(Assignee)

Comment 10

5 years ago
Bah, my mistake. Jesse said anonymous, not native anonymous.
(the div doesn't appear to be a bound node either)
(Assignee)

Comment 12

5 years ago
But where is the div coming from? Missing plugin thingie? Shouldn't that be native anon.
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.
Assignee: nobody → bugs
Keywords: sec-high
(Assignee)

Comment 14

5 years ago
Created attachment 662286 [details] [diff] [review]
wip
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Comment on attachment 662286 [details] [diff] [review]
wip

Boris, what do you think of this?
Attachment #662286 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

5 years ago
Comment on attachment 662286 [details] [diff] [review]
wip

Hmm, default content handling is wrong.
Attachment #662286 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 18

5 years ago
Created attachment 662337 [details] [diff] [review]
patch

Make sure also default content is chrome-only.
Attachment #662286 - Attachment is obsolete: true
Attachment #662337 - Flags: review?(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
Attachment #662337 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

5 years ago
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.
Attachment #662337 - Flags: sec-approval?
Attachment #662337 - Flags: sec-approval? → sec-approval+
status-firefox-esr10: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/mozilla-central/rev/24783c876df0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla18
(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?
Keywords: qawanted, verifyme
QA Contact: mwobensmith
(Assignee)

Comment 23

5 years ago
Test manually whether the plugin-problem widget works.
(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.
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.
Anthony does comment 25 help? Looking for uplift nomination here.
(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.
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.
Keywords: qawanted, verifyme
(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.
(and the fact that the security regression is internally reported, and difficult to discover from the patch)
(Assignee)

Comment 31

5 years ago
Yeah, aurora might be enough for this.
(Assignee)

Comment 32

5 years ago
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
Attachment #662337 - Flags: approval-mozilla-aurora?

Comment 33

5 years ago
(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 on attachment 662337 [details] [diff] [review]
patch

Approving it for Aurora as it is low risk and had good bake time on central.
Attachment #662337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 35

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/24566e7f1b5d
status-firefox18: affected → fixed
status-firefox19: affected → fixed
(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 .
status-firefox17: affected → wontfix

Updated

5 years ago
tracking-firefox-esr10: ? → 18+
tracking-firefox-esr17: --- → 18+
(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!
status-firefox-esr17: --- → affected
(Assignee)

Comment 38

5 years ago
The risk is quite high, and the patch does require changing few interfaces.
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?
status-firefox-esr10: affected → wontfix
(Assignee)

Comment 40

5 years ago
This would change interfaces in ESR17.

We don't really know what not-fixing this could cause.
Comment 13 has some guesses.
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.
(Assignee)

Comment 42

5 years ago
(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.
(Assignee)

Comment 43

5 years ago
Created attachment 694813 [details] [diff] [review]
esr17

I need to test this a bit.
(Assignee)

Comment 44

5 years ago
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
Attachment #694813 - Flags: approval-mozilla-esr17?

Updated

5 years ago
Attachment #694813 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
(Assignee)

Comment 45

5 years ago
https://hg.mozilla.org/releases/mozilla-esr17/rev/36cc4da3469c
status-firefox-esr17: affected → fixed
Whiteboard: [adv-main18+][adv-esr17+]
Alias: CVE-2013-0747
Group: core-security
You need to log in before you can comment on or make changes to this bug.