798264 - (CVE-2012-4208) Xrays for new DOM bindings need to filter properties based on their compartment

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

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 [:bzbarsky]]

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

Yeah, separate tables seems like it might work better.

Comment 3

[Andrew McCreight [:mccr8]]

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

Attachment: v1

Comment 5

[Boris Zbarsky [:bzbarsky]]

Merging this with bug 793267 will be fun.  :(

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Still working on a testcase

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

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

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

Thanks for the assessment!

Comment 12

[Peter Van der Beken [:peterv]]

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

Attachment: Patch to add test

Comment 14

[Peter Van der Beken [:peterv]]

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

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 [:bzbarsky]]

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

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

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 19

[Ed Morley [:emorley]]

Push backed out for compilation failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2dbcd6d16b43

https://hg.mozilla.org/integration/mozilla-inbound/rev/5540b310d435

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

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

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

Comment 23

[Alex Keybl [:akeybl]]

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

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 25

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a11b57d12975
https://hg.mozilla.org/releases/mozilla-beta/rev/d1a798296564

Comment 26

[Peter Van der Beken [:peterv]]

I backed out just the testcase from the branch while I figure out what changes it needs.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Test:
https://hg.mozilla.org/mozilla-central/rev/911fd57f1db2

Comment 28

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

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

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