Closed Bug 914618 (CVE-2013-5594) Opened 8 years ago Closed 8 years ago

It's possible to modify anonymous content of pluginProblem.xml binding

Categories

(Core :: Security, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox24 --- wontfix
firefox25 + verified
firefox26 + verified
firefox27 + verified
firefox-esr17 --- wontfix
firefox-esr24 25+ verified
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

References

Details

(Keywords: sec-high, Whiteboard: Embargo until b2g18 EOL [adv-main25+][adv-esr24-1+])

Attachments

(4 files, 1 obsolete file)

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.
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.
Assertion failure: handler->isSafeToUnwrap() == AccessCheck::subsumes(target, origin), at /mozilla/js/xpconnect/wrappers/WrapperFactory.cpp:337
Assertion failure: IsBackgroundFinalized(a->tenuredGetAllocKind()) == IsBackgroundFinalized(b->tenuredGetAllocKind()), at /mozilla/js/src/jsobj.cpp:2144
Flags: needinfo?(bobbyholley+bmo)
That GC assert sounds bad, so I'm going to mark this sec-high, though maybe it is sec-critical even.
Keywords: sec-high
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).
Flags: needinfo?(bobbyholley+bmo)
(I filed bug 921454 for this)
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.
Attachment #812100 - Flags: review?(bugs)
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.
Attachment #812100 - Flags: review?(bugs) → review-
(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.
This is much better. Thanks for the tips, Olli.
Attachment #812100 - Attachment is obsolete: true
Attachment #812502 - Flags: review?(bugs)
(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 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.
Attachment #812502 - Flags: review?(bugs) → review+
Assignee: nobody → bobbyholley+bmo
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.
Attachment #812502 - Flags: sec-approval?
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.
Attachment #812502 - Flags: sec-approval? → sec-approval+
Smaug, how different will the event stuff look on the different branches? Will this patch apply on 24-and-up? What about b2g18?
Flags: needinfo?(bugs)
Bug 659350 changed how we store listeners in certain cases. No more JS_DefineProperty etc.
http://hg.mozilla.org/releases/mozilla-esr17/rev/1f58f9ed470c
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/de607a40a7fd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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 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
Attachment #812502 - Flags: approval-mozilla-esr24?
Attachment #812502 - Flags: approval-mozilla-beta?
Attachment #812502 - Flags: approval-mozilla-aurora?
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.
Attachment #812502 - Flags: approval-mozilla-esr24?
Attachment #812502 - Flags: approval-mozilla-esr24+
Attachment #812502 - Flags: approval-mozilla-beta?
Attachment #812502 - Flags: approval-mozilla-beta+
Attachment #812502 - Flags: approval-mozilla-aurora?
Attachment #812502 - Flags: approval-mozilla-aurora+
(In reply to lsblakk@mozilla.com [: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.
Confirmed overlay and crash on FF25, 2013-10-01.
Verified fixed on esr24, FF25, FF26 and FF27, 2013-10-10.
tracking-b2g18: ? → ---
Whiteboard: [adv-main25+][adv-esr24-1+]
Alias: CVE-2013-5594
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.
Flags: needinfo?(abillings)
Depends on: 912322
I need Dan to weigh in her but I expect we'll follow your (wise) request.
Flags: needinfo?(abillings) → needinfo?(dveditz)
Flags: needinfo?(dveditz)
Whiteboard: [adv-main25+][adv-esr24-1+] → Embargo until b2g18 EOL [adv-main25+][adv-esr24-1+]
(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?
(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.
Flags: needinfo?(dveditz)
Group: core-security → core-security-release
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.