Simplify [[DefaultValue]] security wrapping setup

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Now that we have SecurityWrapper, our situation for the [[DefaultValue]] trap can be simplified a lot.

Given that we chemspilled on this code in October, I'm going to get several reviews here. :-)
(Assignee)

Comment 1

5 years ago
Created attachment 735334 [details] [diff] [review]
Simplify [[DefaultValue]] security wrapping setup. v1
Attachment #735334 - Flags: review?(mrbkap)
Attachment #735334 - Flags: review?(gkrizsanits)
Attachment #735334 - Flags: review?(ejpbruel)
Before I r+ I must ask this since I don't remember the history of this chemspill. Are you sure we have tests for all the cases that is mentioned in the removed comment in and around Wrapper::defaultValue? (A assume you are just want to double check it since I see no tests here)

So if I get it correctly in case of same compartment transparent wrapper the extra AutoCompartment call(cx, wrappedObject(wrapper)); that we stop doing post-patch is not doing anything interesting anyway, right?

There is one thing that might be a good idea, since most of the explanation comment is gone, a warning comment at setSafeToUnwrap that if this flag is set to false on a wrapper class that is not derived from SecurityWrapper, one must keep an eye open for defaultValue implementation. (pre-patch this was handled at wrapper level, post-patch it is not)
(Assignee)

Comment 4

5 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> Before I r+ I must ask this since I don't remember the history of this
> chemspill. Are you sure we have tests for all the cases that is mentioned in
> the removed comment in and around Wrapper::defaultValue? (A assume you are
> just want to double check it since I see no tests here)

I sure hope so. We've got test_bug802557.html, test_bug720619.html, and test_sameOriginPolicy.html, as well as some others. Hopefully that's enough.

> So if I get it correctly in case of same compartment transparent wrapper the
> extra AutoCompartment call(cx, wrappedObject(wrapper)); that we stop doing
> post-patch is not doing anything interesting anyway, right?

In the case of a same-compartment transparent wrapper, the AutoCompartment was a no-op. In the case of CrossCompartmentWrapper, it now happens in PIERCE.
 
> There is one thing that might be a good idea, since most of the explanation
> comment is gone, a warning comment at setSafeToUnwrap that if this flag is
> set to false on a wrapper class that is not derived from SecurityWrapper,
> one must keep an eye open for defaultValue implementation. (pre-patch this
> was handled at wrapper level, post-patch it is not)

Well, we depend on the fact that SecurityWrappers have !isSafeToUnwrap() for pretty much everything these days (that's the basis of our security wrapper computation in WrapperFactory::Rewrap). safeToUnwrap basically means "the wrapper is not hiding anything from a security perspective". DefaultValue is only one of many things that are affected by this, so I don't think it makes sense to talk about it explicitly here. The fact that it was previously in Wrapper was purely an implementation / historical quirk.
Attachment #735334 - Flags: review?(gkrizsanits) → review+
Attachment #735334 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #735334 - Flags: review?(ejpbruel) → review+
https://hg.mozilla.org/mozilla-central/rev/270ad24960d3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.