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

RESOLVED FIXED in Firefox 22

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-high})

unspecified
mozilla24
sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ fixed, firefox23+ fixed, firefox24+ fixed, firefox-esr1722+ fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

Details

(Whiteboard: [adv-main22-][adv-esr1707-])

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1
Attachment #751723 - Flags: review?(wmccloskey)
Attachment #751723 - Flags: review?(gkrizsanits)
(Assignee)

Updated

5 years ago
status-b2g18: --- → affected
status-firefox21: --- → unaffected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox-esr17: --- → affected
Group: core-security
(Assignee)

Updated

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

Comment 5

5 years ago
I'm going to go with sec-high - it's a GC hazard, but probably very hard to exploit.
Keywords: sec-high
(Assignee)

Comment 6

5 years ago
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+
tracking-b2g18: --- → +
tracking-firefox22: --- → +
tracking-firefox23: --- → +
tracking-firefox24: --- → +
tracking-firefox-esr17: --- → +
(Assignee)

Comment 9

5 years ago
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
Last Resolved: 5 years ago
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
status-firefox24: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
tracking-firefox-esr17: + → 22+
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.