Closed Bug 874083 Opened 9 years ago Closed 9 years ago

subsumes-conditional prototype remapping in PrepareForWrapping can cause crashes during document.domain remapping

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 + fixed
firefox23 + fixed
firefox24 + fixed
firefox-esr17 22+ fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main22-][adv-esr1707-])

Attachments

(1 file)

This is a regression from bug 866823. It doesn't affect release, because we didn't backport this there. It was discovered when trying to backport that bug to esr17, where various incidental factors caused us to trigger an assertion.

The basic problem is that RemapWrappers can't handle a cross-compartment wrapper transforming into a regular same-compartment objects (consumers wishing to do that need to use JS_TransplantObject). So PrepareForWrapping needs to be consistent over time about whether it tells the caller to create a wrapper or whether it just returns a new object for the given scope. Since document.domain can cause security relationships between principals to mutate over time, we need to ignore domain here.
Attachment #751723 - Flags: review?(wmccloskey)
Attachment #751723 - Flags: review?(gkrizsanits)
Group: core-security
Group: dom-core-security
Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

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

::: js/src/jswrapper.cpp
@@ +980,5 @@
>      JS_ASSERT(Wrapper::wrappedObject(wobj) == newTarget);
>  
>      // Update the entry in the compartment's wrapper map to point to the old
>      // wrapper, which has now been updated (via reuse or swap).
> +    JS_ASSERT(wobj->isWrapper());

I only looked at the assertion, but it seems fine. We're already asserting this in the Wrapper::wrappedObject(wobj) call above it.
Attachment #751723 - Flags: review?(wmccloskey) → review+
Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

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

Looks good.
Attachment #751723 - Flags: review?(gkrizsanits) → review+
I'm going to go with sec-high - it's a GC hazard, but probably very hard to exploit.
Keywords: sec-high
Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The browser can easily be made to assert (indeed, the test does just that). Constructing an exploit would probably be pretty hard, though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes

Which older supported branches are affected by this flaw?

Everywhere we backported bug 866823, so aurora, beta, and b2g18.

If not all supported branches, which bug introduced the flaw?

Bug 866823.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

trivial.

How likely is this patch to cause regressions; how much testing does it need?

Not likely to cause regressions, but we should get this in right away, because we just introduced the regression on the branches a few days ago.
Attachment #751723 - Flags: sec-approval?
Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

sec-approval+ for m-c. Please prepare branch patches and nominate there ASAP when m-c is in. We should try to avoid shipping the security issue.
Attachment #751723 - Flags: sec-approval? → sec-approval+
Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

Flagging for uplift. There might be minor context differences on the branches, but all this patch does is to call subsumesIgnoringDomain instead of subsumes, so it should be trivial to apply everywhere.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 866823
User impact if declined: sec
Testing completed: just pushed to m-i
Risk to taking this patch (and alternatives if risky): not risky
String or UUID changes made by this patch: none
Attachment #751723 - Flags: approval-mozilla-beta?
Attachment #751723 - Flags: approval-mozilla-b2g18?
Attachment #751723 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/d6dc6f1460f0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

Approving for landing to esr17 as well, please carry forward if you need to rework the patch (without significant risk) for landing to that branch.
Attachment #751723 - Flags: approval-mozilla-esr17+
Attachment #751723 - Flags: approval-mozilla-beta?
Attachment #751723 - Flags: approval-mozilla-beta+
Attachment #751723 - Flags: approval-mozilla-aurora?
Attachment #751723 - Flags: approval-mozilla-aurora+
Attachment #751723 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Whiteboard: [adv-main22-]
I'm assuming no test case here. For QA - based on that and comment 5 - I'd assume it's not worth verification.
Whiteboard: [adv-main22-] → [adv-main22-][adv-esr1707-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.