Last Comment Bug 798264 - (CVE-2012-4208) Xrays for new DOM bindings need to filter properties based on their compartment
(CVE-2012-4208)
: Xrays for new DOM bindings need to filter properties based on their compartment
Status: RESOLVED FIXED
[adv-track-main17+] (sec-moderate in ...
: regression, sec-high
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: 740069 774245
  Show dependency treegraph
 
Reported: 2012-10-05 02:28 PDT by Peter Van der Beken [:peterv]
Modified: 2013-03-18 13:12 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
unaffected


Attachments
v1 (44.19 KB, patch)
2012-10-07 14:57 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v1.1 (43.48 KB, patch)
2012-10-09 06:03 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v1.1 (45.32 KB, patch)
2012-10-09 06:12 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Review
v1.1 (45.83 KB, patch)
2012-10-10 07:19 PDT, Peter Van der Beken [:peterv]
peterv: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
dveditz: sec‑approval+
Details | Diff | Review
Patch to add test (1.39 KB, patch)
2012-10-10 07:20 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review

Description Peter Van der Beken [:peterv] 2012-10-05 02:28:24 PDT
Right now we always expose ChromeOnly properties on Xrays, but we should only do that if the Xray is in a chrome compartment. We should probably do away with the dual property tables and have an additional flag in the table for chrome-only properties. The prototype setup can decide based on the compartment whether to add those or not, and the Xray code can pass in the flag's required value based on the Xray's compartment.
Comment 1 Boris Zbarsky [:bz] 2012-10-05 08:59:04 PDT
Would it be faster to keep two tables?  One table for the properties everyone can see.  One table that only contains ChromeOnly properties.  Then we only have to do the compartment check once during proto setup and once during Xray ops: when deciding whether to even look at the second table....
Comment 2 Peter Van der Beken [:peterv] 2012-10-06 13:55:40 PDT
Yeah, separate tables seems like it might work better.
Comment 3 Andrew McCreight [:mccr8] 2012-10-07 14:54:32 PDT
What security rating would you give this?

I'm marking 10 as unaffected and newer things as affected, because it sounds like it could apply any of the new DOM bindings.
Comment 4 Peter Van der Beken [:peterv] 2012-10-07 14:57:58 PDT
Created attachment 668953 [details] [diff] [review]
v1
Comment 5 Boris Zbarsky [:bz] 2012-10-08 09:23:34 PDT
Merging this with bug 793267 will be fun.  :(
Comment 6 Peter Van der Beken [:peterv] 2012-10-09 06:03:31 PDT
Created attachment 669504 [details] [diff] [review]
v1.1

Still working on a testcase
Comment 7 Peter Van der Beken [:peterv] 2012-10-09 06:12:20 PDT
Created attachment 669506 [details] [diff] [review]
v1.1
Comment 8 Boris Zbarsky [:bz] 2012-10-09 20:27:59 PDT
Comment on attachment 669506 [details] [diff] [review]
v1.1

>+++ b/dom/bindings/BindingUtils.cpp
> XrayResolveProperty(JSContext* cx, JSObject* wrapper, jsid id,
>+            JSFunctionSpec& methodSpec = method->specs[i];

This should be nativeProperties->methodSpecs[i], I believe.


> XrayEnumerateProperties(JS::AutoIdVector& props,
>+          if ((method->specs[i].flags & JSPROP_ENUMERATE) &&

Again, nativeProperties->methodSpecs[i]

>+++ b/dom/bindings/BindingUtils.h
>+ * domClass is the JSClass of instance objects for this class.  This can be null

It's not a JSClass.  It's a DOMClass, no?

>+ * chromeProperties contains the methods, attributes and constants to be defined
>+ *                  on objects in chrome compartments.

That doesn't make it clear that it's the _caller_ who is supposed to check the kind of compartment and pass a null chromeProperties as needed.  Please fix the comment to make this very clear.

>+++ b/dom/bindings/Codegen.py
> class CGCreateInterfaceObjectsMethod(CGAbstractMethod):
>              CGGeneric(call % self.properties.variableNames(False))],

I don't think you need to do substition here anymore.  Just CGGeneric(call) should work.

Given that, I think you can get rid of the variableNames() method altogether.

r=me with those changes.  This looks great!
Comment 9 Peter Van der Beken [:peterv] 2012-10-10 05:40:30 PDT
(In reply to Andrew McCreight [:mccr8] from comment #3)
> What security rating would you give this?
> 
> I'm marking 10 as unaffected and newer things as affected, because it sounds
> like it could apply any of the new DOM bindings.

This only affects interfaces with ChromeOnly properties, which right now means XMLHttpRequest and CanvasRenderingContext.

This is also only a problem for same-origin Xrays. Cross-origin we're saved by the isCrossOriginAccessPermitted check, which filters out all of these properties. However, bholley points out that because we now use same-origin Xrays for implementing Components.lookupMethod (since landing bug 774245) this can be easily done from content. Given that, this might be sec-high.
Comment 10 Bobby Holley (PTO through June 13) 2012-10-10 05:46:42 PDT
(In reply to Peter Van der Beken [:peterv] from comment #9)
> bholley points out that because we now use same-origin
> Xrays for implementing Components.lookupMethod (since landing bug 774245)
> this can be easily done from content. Given that, this might be sec-high.

Note that that code is currently on beta, and hasn't been released.
Comment 11 Andrew McCreight [:mccr8] 2012-10-10 07:03:34 PDT
Thanks for the assessment!
Comment 12 Peter Van der Beken [:peterv] 2012-10-10 07:19:01 PDT
Created attachment 669960 [details] [diff] [review]
v1.1

(In reply to Boris Zbarsky (:bz) from comment #8)
> This should be nativeProperties->methodSpecs[i], I believe.

> Again, nativeProperties->methodSpecs[i]

Wow, good catch.

> It's not a JSClass.  It's a DOMClass, no?

Done.

> That doesn't make it clear that it's the _caller_ who is supposed to check
> the kind of compartment and pass a null chromeProperties as needed.  Please
> fix the comment to make this very clear.

Done. It would have been nice to move the check into the BindingUtils CreateInterfaceObjects, but the chrome check is different depending on whether we're in workers or not :-(.

> I don't think you need to do substition here anymore.  Just CGGeneric(call)
> should work.
> 
> Given that, I think you can get rid of the variableNames() method altogether.

Done.
Comment 13 Peter Van der Beken [:peterv] 2012-10-10 07:20:54 PDT
Created attachment 669964 [details] [diff] [review]
Patch to add test
Comment 14 Peter Van der Beken [:peterv] 2012-10-10 07:32:51 PDT
Comment on attachment 669960 [details] [diff] [review]
v1.1

[Security approval request comment]
How easily can the security issue be deduced from the patch?

The patch adds one security check, but it's mostly burried in refactoring.

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

The patch adds a security check which is fairly obvious. The check-in comment does not describe the exact problem. I have a separate patch with a test that clearly shows the issue. 

Which older supported branches are affected by this flaw?

ESR is not affected (problem appeared in 14), aurora and beta are.

If not all supported branches, which bug introduced the flaw?

The problem started with bug 740069, but it's much harder to exploit (requires an addon running untrusted script in a sandbox). Starting with bug 774245 it is much easier to exploit, that patch is currently in beta.

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

Backporting should be trivial.

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

Patch is fairly straightforward, so pretty confident that it won't cause regressions.
Comment 15 Peter Van der Beken [:peterv] 2012-10-10 07:33:55 PDT
If we want to postpone checking in the fix we could split it up in a refactoring patch that can land early and a one-liner that adds the security check.
Comment 16 Boris Zbarsky [:bz] 2012-10-10 09:39:42 PDT
> but the chrome check is different depending on whether we're in workers

Yeah, indeed.  Hence me not asking you to move it....  ;)
Comment 17 Daniel Veditz [:dveditz] 2012-10-10 12:09:28 PDT
Probably would be nice to get this into an early Firefox 17 beta since the patch is somewhat large, if straightforward.
Comment 18 Peter Van der Beken [:peterv] 2012-10-12 05:55:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dbcd6d16b43

I didn't check in the testcase, how long do we want to wait with that?
Comment 20 Boris Zbarsky [:bz] 2012-10-12 08:10:05 PDT
> how long do we want to wait with that?

Given the recent history with Location, I think we should either not wait at all or make sure we have a tracked bug to land the testcases or something.
Comment 21 Peter Van der Beken [:peterv] 2012-10-14 10:43:32 PDT
(In reply to Ed Morley [:edmorley UTC+1] from comment #19)
> Push backed out for compilation failures:

Not caused by this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d178bdc3a557
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-10-14 14:11:45 PDT
https://hg.mozilla.org/mozilla-central/rev/d178bdc3a557
Comment 23 Alex Keybl [:akeybl] 2012-10-23 16:28:23 PDT
(In reply to Ryan VanderMeulen from comment #22)
> https://hg.mozilla.org/mozilla-central/rev/d178bdc3a557

If we think this is low risk enough to take in FF17, please nominate for uplift asap.
Comment 24 Peter Van der Beken [:peterv] 2012-10-30 07:11:38 PDT
Comment on attachment 669960 [details] [diff] [review]
v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 740069, but made easier to exploit by bug 774245
User impact if declined: we allow content to get properties of DOM objects that are intended for chrome only
Testing completed (on m-c, etc.): has been in central for a couple of weeks
Risk to taking this patch (and alternatives if risky): pretty low-risk, adds a security check (but with some refactoring to facilitate that)
String or UUID changes made by this patch: none
Comment 26 Peter Van der Beken [:peterv] 2012-11-02 13:51:01 PDT
I backed out just the testcase from the branch while I figure out what changes it needs.
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-11-02 17:31:51 PDT
Test:
https://hg.mozilla.org/mozilla-central/rev/911fd57f1db2
Comment 28 Al Billings [:abillings] 2012-11-06 12:30:42 PST
(In reply to Ryan VanderMeulen from comment #27)
> Test:
> https://hg.mozilla.org/mozilla-central/rev/911fd57f1db2

Why are we checking in tests for an unreleased security fix? We normally wait to do that until the fix has been released for a while.
Comment 29 Peter Van der Beken [:peterv] 2012-11-07 00:23:48 PST
(In reply to Al Billings [:abillings] from comment #28)
> Why are we checking in tests for an unreleased security fix? We normally
> wait to do that until the fix has been released for a while.

Given that the testcase doesn't work as is on beta or current release I'm not sure it's useful to hold it back. Someone who can use it to figure out how to exploit it for those releases can just as well do the same from the patch.

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