914618 - (CVE-2013-5594) It's possible to modify anonymous content of pluginProblem.xml binding

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

This is basically the same as bug 911864, but this does not need document.getAnonymousNodes, so bug 912322 does not fix this.

(In reply to Bobby Holley (:bholley) from bug 911864 comment #10)
> No, this bug is more about pluginProblem, which we can't allow content to
> change due to the threat of clickjacking.
> Modifying pluginProblem AC would indeed be a problem, but AFAICT the exploit
> here only allows it to be read/cloned.

It's possible to modify pluginProblem AC.

Also, it's possible to trigger two types of assertion failures.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - Modifying pluginProblem AC

I don't understand what an attacker can do by performing clickjacking with pluginProblem.  What this testcase does is unlikely to be the clickjacking attack in question.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - Assertion failure

Assertion failure: handler->isSafeToUnwrap() == AccessCheck::subsumes(target, origin), at /mozilla/js/xpconnect/wrappers/WrapperFactory.cpp:337

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3 - Assertion failure

Assertion failure: IsBackgroundFinalized(a->tenuredGetAllocKind()) == IsBackgroundFinalized(b->tenuredGetAllocKind()), at /mozilla/js/src/jsobj.cpp:2144

Comment 4

[Andrew McCreight [:mccr8]]

That GC assert sounds bad, so I'm going to mark this sec-high, though maybe it is sec-critical even.

Comment 5

[Bobby Holley (:bholley)]

So, this is really two bugs:

(1) XML pretty-printing can be exploited to bypass SOWs. This allows clickjacking pluginProblem, among other things.

(2) Touching NAC in causes us to crash in various ways, largely because we have close to no test coverage for it. The two problems are (a) an overzealous assertion in WrapperFactory and (b) a bug in js_TransplantObjectWithWrapper. The former is not dangerous, the latter might be.

I'm going to keep this bug about (1) and file a separate bug for (2).

Comment 6

[Bobby Holley (:bholley)]

(I filed bug 921454 for this)

Comment 7

[Bobby Holley (:bholley)]

Attachment: Reimplement XML pretty printing with events. v1

This plugs the pretty printing hole. I can't think of any more general solution,
but fortunately there aren't all that many XBL bindings exposed to content.

Comment 8

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

Comment on attachment 812100 [details] [diff] [review]
Reimplement XML pretty printing with events. v1


>       </handler>
>+      <handler event="prettyprint-dom-created">
allowuntrusted="false" should be enough to ensure that the handler isn't called with
untrusted event, even when bound to content.

>+        <![CDATA[
>+        if (event.isTrusted && event instanceof CustomEvent)
Then there shouldn't be need for .isTrusted and instanceof check shouldn't be needed either


>+    // Fire an event at the bound element to pass it |resultFragment|.
>+    nsRefPtr<nsPresContext> presContext = aDocument->GetShell()
>+      ? aDocument->GetShell()->GetPresContext()
>+      : nullptr;
>+    nsCOMPtr<nsIDOMEvent> domEvent;
>+    rv = nsEventDispatcher::CreateEvent(rootCont, presContext, nullptr,
>+                                        NS_LITERAL_STRING("customevent"),
>+                                        getter_AddRefs(domEvent));


#include "GeneratedEvents.h"
NS_NewDOMCustomEvent(getter_AddRefs(domEvent), rootCont, nullptr, nullptr);
nsCOMPtr<nsIDOMCustomEvent> event = do_QueryInterface(domEvent);
nsCOMPtr<nsIWritableVariant> resultFragmentVal = new nsVariant();
resultFragmentVal->SetAsISupports(resultFragment);

>+      event->InitCustomEvent(NS_LITERAL_STRING("prettyprint-dom-created"),
>+                             /* bubbles = */ false, /* cancelable = */ false,
>+                             /* detail = */ resultFragmentVal);
>+      customEvent->SetTrusted(true);
>+      bool dummy;
>+      rv = rootCont->DispatchEvent(domEvent, &dummy);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }

Something like that. No JSAPI usage if just possible, please.

And this all is safe just because we don't cache xbl handlers anymore on elements. We used to cache them as onfoo properties.
http://mxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#2037

A new patch would be nice.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 812100 [details] [diff] [review]
> Reimplement XML pretty printing with events. v1
> 
> 
> >       </handler>
> >+      <handler event="prettyprint-dom-created">
> allowuntrusted="false" should be enough to ensure that the handler isn't
> called with
> untrusted event, even when bound to content.

Ah, you're right. I skimmed through nsXBLBinding::InstallEventHandlers too quickly.

> Something like that. No JSAPI usage if just possible, please.

OK.
 
> And this all is safe just because we don't cache xbl handlers anymore on
> elements. We used to cache them as onfoo properties.
> http://mxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#2037

I don't understand how that relates to this. Is it an issue of collisions?

> A new patch would be nice.

Sure. Coming right up.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Reimplement XML pretty printing with events. v2

This is much better. Thanks for the tips, Olli.

Comment 11

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

(In reply to Bobby Holley (:bholley) from comment #9)
 
> > And this all is safe just because we don't cache xbl handlers anymore on
> > elements. We used to cache them as onfoo properties.
> > http://mxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#2037
> 
> I don't understand how that relates to this. Is it an issue of collisions?
content must not be able to modify anon content. So if we exposed the event handlers to content, they
could just call them, similarly to testcase 1.

Comment 12

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

Comment on attachment 812502 [details] [diff] [review]
Reimplement XML pretty printing with events. v2


>+#include "GeneratedEventClasses.h"
You shouldn't need this.

>+    // Fire an event at the bound element to pass it |resultFragment|.
>+    nsRefPtr<nsPresContext> presContext = aDocument->GetShell()
>+      ? aDocument->GetShell()->GetPresContext()
>+      : nullptr;
You don't need this (you aren't even using presContext for anything)


>+    nsCOMPtr<nsIDOMEvent> domEvent;
>+    rv = NS_NewDOMCustomEvent(getter_AddRefs(domEvent), rootCont,
>+                              nullptr, nullptr);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIDOMCustomEvent> customEvent = do_QueryInterface(domEvent);
>+    MOZ_ASSERT(customEvent);
>+    nsCOMPtr<nsIWritableVariant> resultFragmentVariant = new nsVariant();
>+    rv = resultFragmentVariant->SetAsISupports(resultFragment);
>+    MOZ_ASSERT(NS_SUCCEEDED(rv));
>+    CustomEvent* event = static_cast<CustomEvent*>(customEvent.get());
Why this? Just call InitCustomEvent on customEvent.

Comment 13

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a45d3bda5a10

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 812502 [details] [diff] [review]
Reimplement XML pretty printing with events. v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily at all. But it does reveal the fact that we think that XMLPrettyPrint is a security problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Not really. We're just reimplementing something from one way to another way.

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 mostly identical, though there may be some differences in how custom events are dispatched from C++

How likely is this patch to cause regressions; how much testing does it need?

Not all that likely, but we don't have _any_ in-tree tests for XML pretty printing AFAICT. We probably want manual QA.

Comment 15

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

Comment on attachment 812502 [details] [diff] [review]
Reimplement XML pretty printing with events. v2

Sec-approval+ for trunk. You should create and nominate patches for the branches. I'm not sure if we'll take this on ESR17 (though we probably should) but we'll want this on Aurora, Beta, and ESR24 and, I assume, B2G.

Comment 16

[Bobby Holley (:bholley)]

Smaug, how different will the event stuff look on the different branches? Will this patch apply on 24-and-up? What about b2g18?

Comment 17

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de607a40a7fd

Comment 18

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

Bug 659350 changed how we store listeners in certain cases. No more JS_DefineProperty etc.
http://hg.mozilla.org/releases/mozilla-esr17/rev/1f58f9ed470c

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/de607a40a7fd

Comment 20

[Bobby Holley (:bholley)]

Honestly I think we should probably just WONTFIX this for b2g18, because we don't even have XBL scopes there, which means there are probably a jillion other ways that clever people can muck with NAC. And since there are no plugins on b2g18, I'm not worried about the clickjacking.

Flagging for approval for the other branches.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 812502 [details] [diff] [review]
Reimplement XML pretty printing with events. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Longstanding.
User impact if declined: Security problems.
Testing completed (on m-c, etc.): Pushed to m-c.
Risk to taking this patch (and alternatives if risky): Medium risk for messing up XBL pretty printing, which we probably don't care about a huge amount (though I'm not entirely sure). We don't have any tests for that stuff, and I've only smoketested it locally. It's not going to break anything else. 
String or IDL/UUID changes made by this patch: None

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 812502 [details] [diff] [review]
Reimplement XML pretty printing with events. v2

[Triage Comment]

Is it possible to land this as-is (or some reasonable facsimile of the fix) to esr17 and to b2g18 branches?  If a different patch is needed, please request approval, if not and it's not possible to land to those branches we'll want to discuss what the impact is for leaving this unfixed on those branches. With ESR17 we're two cycles away from EOL but with b2g18 it might be a little longer.

Comment 23

[Bobby Holley (:bholley)]

(In reply to [e-mail] [:lsblakk] from comment #22)
> Comment on attachment 812502 [details] [diff] [review]
> Reimplement XML pretty printing with events. v2
> 
> [Triage Comment]
> Is it possible to land this as-is (or some reasonable facsimile of the fix)
> to esr17 and to b2g18 branches?

No, and per comment 20 I don't think it's worth the engineering effort to write one, given that our architecture back then would make this more wack-a-mole than anything else.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6e2c73a4ea6
https://hg.mozilla.org/releases/mozilla-beta/rev/e87b27363b6c
https://hg.mozilla.org/releases/mozilla-esr24/rev/ddaa482cf588

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed overlay and crash on FF25, 2013-10-01.
Verified fixed on esr24, FF25, FF26 and FF27, 2013-10-10.

Comment 26

[Bobby Holley (:bholley)]

We should not be opening this bug up this cycle. In particular, this bug is a workaround to get access to document.getAnonymousNodes, but that won't actually be removed for web content until mozilla 26 (bug 912322). I also think we should keep this closed until bug 914618 is fixed everywhere, since the attack patterns are similar.

In summary, we should embargo this bug until bug 914618 and bug 912322 are both fixed on all supported branches.

Comment 27

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

I need Dan to weigh in her but I expect we'll follow your (wise) request.

Comment 28

[Daniel Veditz [:dveditz]]

(In reply to Bobby Holley (:bholley) from comment #26)
> In summary, we should embargo this bug until bug 914618 and bug 912322 are
> both fixed on all supported branches.

This _is_ bug 914618 -- did you mean to reference another bug there along with 912322?

Comment 29

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #28)
> (In reply to Bobby Holley (:bholley) from comment #26)
> > In summary, we should embargo this bug until bug 914618 and bug 912322 are
> > both fixed on all supported branches.
> 
> This _is_ bug 914618 -- did you mean to reference another bug there along
> with 912322?

Yes, sorry. I meant bug 911864.