Bug 798264 (CVE-2012-4208)

Xrays for new DOM bindings need to filter properties based on their compartment

RESOLVED FIXED in Firefox 17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

({regression, sec-high})

Trunk
mozilla19
regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16 wontfix, firefox17+ fixed, firefox18+ fixed, firefox19+ fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [adv-track-main17+] (sec-moderate in Fx14-16, could exploit an unwise add-on))

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 2

5 years ago
Yeah, separate tables seems like it might work better.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
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.
status-firefox-esr10: --- → unaffected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
(Assignee)

Comment 4

5 years ago
Created attachment 668953 [details] [diff] [review]
v1
Merging this with bug 793267 will be fun.  :(
(Assignee)

Comment 6

5 years ago
Created attachment 669504 [details] [diff] [review]
v1.1

Still working on a testcase
(Assignee)

Comment 7

5 years ago
Created attachment 669506 [details] [diff] [review]
v1.1
Attachment #668953 - Attachment is obsolete: true
Attachment #669504 - Attachment is obsolete: true
Attachment #669506 - Flags: review?(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!
Attachment #669506 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

5 years ago
(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.
(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.
Keywords: sec-high
Thanks for the assessment!
status-firefox19: --- → affected
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
(Assignee)

Comment 12

5 years ago
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.
Attachment #669506 - Attachment is obsolete: true
Attachment #669960 - Flags: review+
(Assignee)

Comment 13

5 years ago
Created attachment 669964 [details] [diff] [review]
Patch to add test
(Assignee)

Comment 14

5 years ago
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.
Attachment #669960 - Flags: sec-approval?
(Assignee)

Comment 15

5 years ago
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.
> but the chrome check is different depending on whether we're in workers

Yeah, indeed.  Hence me not asking you to move it....  ;)
Probably would be nice to get this into an early Firefox 17 beta since the patch is somewhat large, if straightforward.
Blocks: 774245, 740069
status-firefox16: affected → wontfix
tracking-firefox17: ? → +
tracking-firefox18: ? → +
tracking-firefox19: ? → +
Keywords: regression
Whiteboard: (sec-moderate in Fx14-16, could exploit an unwise add-on)
Attachment #669960 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 18

5 years ago
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?
Target Milestone: --- → mozilla19
Flags: in-testsuite?
Push backed out for compilation failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2dbcd6d16b43

https://hg.mozilla.org/integration/mozilla-inbound/rev/5540b310d435
Target Milestone: mozilla19 → ---
> 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.
(Assignee)

Comment 21

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/d178bdc3a557
status-firefox19: affected → fixed
Target Milestone: --- → mozilla19
(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.
(Assignee)

Comment 24

5 years ago
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
Attachment #669960 - Flags: approval-mozilla-beta?
Attachment #669960 - Flags: approval-mozilla-aurora?
Attachment #669960 - Flags: approval-mozilla-beta?
Attachment #669960 - Flags: approval-mozilla-beta+
Attachment #669960 - Flags: approval-mozilla-aurora?
Attachment #669960 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/a11b57d12975
https://hg.mozilla.org/releases/mozilla-beta/rev/d1a798296564
status-firefox17: affected → fixed
status-firefox18: affected → fixed
(Assignee)

Comment 26

5 years ago
I backed out just the testcase from the branch while I figure out what changes it needs.
Test:
https://hg.mozilla.org/mozilla-central/rev/911fd57f1db2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
(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.
Whiteboard: (sec-moderate in Fx14-16, could exploit an unwise add-on) → [adv-track-main17+] (sec-moderate in Fx14-16, could exploit an unwise add-on)
(Assignee)

Comment 29

5 years ago
(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.
Alias: CVE-2012-4208
Group: core-security
You need to log in before you can comment on or make changes to this bug.