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....
Yeah, separate tables seems like it might work better.
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.
Created attachment 668953 [details] [diff] [review]
Merging this with bug 793267 will be fun. :(
Created attachment 669504 [details] [diff] [review]
Still working on a testcase
Created attachment 669506 [details] [diff] [review]
Comment on attachment 669506 [details] [diff] [review]
> 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) &&
>+ * 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.
> 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!
(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.
Thanks for the assessment!
Created attachment 669960 [details] [diff] [review]
(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?
> 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.
Created attachment 669964 [details] [diff] [review]
Patch to add test
Comment on attachment 669960 [details] [diff] [review]
[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.
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.
I didn't check in the testcase, how long do we want to wait with that?
Push backed out for compilation failures:
> 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.
(In reply to Ed Morley [:edmorley UTC+1] from comment #19)
> Push backed out for compilation failures:
Not caused by this patch.
(In reply to Ryan VanderMeulen from comment #22)
If we think this is low risk enough to take in FF17, please nominate for uplift asap.
Comment on attachment 669960 [details] [diff] [review]
[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
I backed out just the testcase from the branch while I figure out what changes it needs.
(In reply to Ryan VanderMeulen from comment #27)
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.
(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.