Add a `allowWaivers` option to JS sandboxes

RESOLVED FIXED in Firefox 42

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: paul, Assigned: bholley)

Tracking

unspecified
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
We want to be able to disable waivers when we use xrays in js sandboxes.

See relevant discussion in bug 1174733 comment 24.
(Reporter)

Updated

3 years ago
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 1

3 years ago
Created attachment 8633865 [details] [diff] [review]
Add 'allowWaivers' sandbox option. v1
Attachment #8633865 - Flags: review?(gkrizsanits)
Just a few questions that came up while reading the patch:
- do we want to let this option to be set for system principal? (kind of meaningless since one can create a sandbox without the flag from it and eval in that scope)
- it's a bit paranoid, and I don't think there is a use case here, but if the sandbox gets an already waived reference to an object form another scope without this flag, wouldn't that be an issue? (so shouldn't we have to deal with ShouldWaiveXray in PrepareForWrapping?)
(Assignee)

Comment 4

3 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> - do we want to let this option to be set for system principal? (kind of
> meaningless since one can create a sandbox without the flag from it and eval
> in that scope)

I don't think it matters. This isn't really a security mechanism so much as it is a mechanism to avoid compat problems. I don't think we should clutter the code by checking for system principal.

> - it's a bit paranoid, and I don't think there is a use case here, but if
> the sandbox gets an already waived reference to an object form another scope
> without this flag, wouldn't that be an issue? (so shouldn't we have to deal
> with ShouldWaiveXray in PrepareForWrapping?)

The tests explicitly test for that case (see the 'friend' part, which is necessary because waivers are only transferred same-origin).

The fundamental check here is the one in WrapperFactory::Rewrap. Everything else is just sugar, really.
(Assignee)

Comment 6

3 years ago
Created attachment 8634207 [details] [diff] [review]
Add 'allowWaivers' sandbox option. v2
Attachment #8633865 - Attachment is obsolete: true
Attachment #8633865 - Flags: review?(gkrizsanits)
Attachment #8634207 - Flags: review?(gkrizsanits)
Comment on attachment 8634207 [details] [diff] [review]
Add 'allowWaivers' sandbox option. v2

Review of attachment 8634207 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bobby Holley (:bholley) from comment #4)
> I don't think it matters. This isn't really a security mechanism so much as
> it is a mechanism to avoid compat problems.

Right, makes sense.

> The tests explicitly test for that case (see the 'friend' part, which is
> necessary because waivers are only transferred same-origin).

I was talking about the case when the rewrap is not even called [1], but I
guess those special cases when we return something meaningful from PrepareForWrapping
for some wn-s are not that important here.

[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jscompartment.cpp#435
Attachment #8634207 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 8

3 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)

> I was talking about the case when the rewrap is not even called [1], but I
> guess those special cases when we return something meaningful from
> PrepareForWrapping
> for some wn-s are not that important here.

That is a very good thought - though I agree that in this case it shouldn't matter.
https://hg.mozilla.org/mozilla-central/rev/eca18bb1b558
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.