Closed Bug 1319524 Opened 3 years ago Closed 3 years ago

Compartment mismatch in GetNPObjectWrapper

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 50+ fixed
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: jandem, Assigned: jandem)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(1 file, 1 obsolete file)

This came up on IRC. The shell and SM itself currently call JS_FireOnNewGlobalObject outside AutoCompartment in some places, while some Gecko callers do enter the global's compartment first. Let's enforce we're in the global's compartment to avoid surprises.

I noticed there are more APIs that don't have compartment asserts but probably should have them.
Attached patch Part 2 - Add compartment asserts (obsolete) — Splinter Review
This adds a bunch of assertCompartment calls to various APIs. Appears to be green on Try so far.
Attachment #8813641 - Flags: review?(jcoppeard)
Comment on attachment 8813641 [details] [diff] [review]
Part 2 - Add compartment asserts

r? bholley for the JSAutoCompartment in xpc::CreateSandboxObject.

There's a similar |JSAutoCompartment ac(cx, sandbox)| before it. I could move the JS_FireOnNewGlobalObject call to be in that scope, but that will change the order of the calls.
Attachment #8813641 - Flags: review?(bobbyholley)
On Try I get a compartment mismatch in GetNPObjectWrapper, the JS_GetPrototype call:

http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/dom/plugins/base/nsJSNPRuntime.cpp#1191

That code does use JS_WrapObject, but we should probably enter |obj|'s compartment before calling JS_GetPrototype.
Adding a JSAutoCompartment to GetNPObjectWrapper fixes the orange on Try.
Attachment #8813673 - Flags: review?(bobbyholley)
Attachment #8813641 - Attachment description: Patch → Part 2 - Add compartment asserts
Comment on attachment 8813641 [details] [diff] [review]
Part 2 - Add compartment asserts

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

Nice.

::: js/src/jsapi.cpp
@@ +160,5 @@
>      static_assert(unsigned(OkCode) == unsigned(JSMSG_NOT_AN_ERROR),
>                    "unsigned value of OkCode must not be an error code");
>      MOZ_ASSERT(code_ != Uninitialized);
>      MOZ_ASSERT(!ok());
> +    assertSameCompartment(cx, obj);

I saw this didn't check |id|, but then saw we don't check compartments for IDs in CompartmentChecker anyway.  

Not for this patch, but isn't this something we should check?

If so we should include id in the arguments here in preparation for that.

@@ +2015,5 @@
>  JS_PUBLIC_API(bool)
>  JS_GetPropertyDescriptorById(JSContext* cx, HandleObject obj, HandleId id,
>                               MutableHandle<PropertyDescriptor> desc)
>  {
> +    assertSameCompartment(cx, obj);

Also here.
Attachment #8813641 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #5)
> Not for this patch, but isn't this something we should check?

bz pointed out to me why we don't need to check this.
Comment on attachment 8813641 [details] [diff] [review]
Part 2 - Add compartment asserts

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

r=me on the Sandbox.cpp change.

::: js/xpconnect/src/Sandbox.cpp
@@ +1229,5 @@
>  
> +    {
> +        JSAutoCompartment ac(cx, sandbox);
> +        JS_FireOnNewGlobalObject(cx, sandbox);
> +    }

No need to scope this.
Attachment #8813641 - Flags: review?(bobbyholley) → review+
Attachment #8813673 - Flags: review?(bobbyholley) → review+
Group: javascript-core-security
[Tracking Requested - why for this release]:

We should land the NPAPI oneliner everywhere first.
Comment on attachment 8813673 [details] [diff] [review]
Part 1 - Fix GetNPObjectWrapper

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's unclear. Let's assume it's possible with some effort.

> 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 makes it kinda obvious, it adds a single line.

> Which older supported branches are affected by this flaw?
All.

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

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely, patch is very safe.
Attachment #8813673 - Flags: sec-approval?
Comment on attachment 8813673 [details] [diff] [review]
Part 1 - Fix GetNPObjectWrapper

sec-approval+ for trunk. Please nominate patches for branches so we can ship the fix more quickly.
Attachment #8813673 - Flags: sec-approval? → sec-approval+
Comment on attachment 8813673 [details] [diff] [review]
Part 1 - Fix GetNPObjectWrapper

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Potential security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Very low risk.
[Why is the change risky/not risky?]: It adds a single line/JSAutoCompartment, pretty obvious fix.
[String changes made/needed]: None.
Attachment #8813673 - Flags: approval-mozilla-esr45?
Attachment #8813673 - Flags: approval-mozilla-beta?
Attachment #8813673 - Flags: approval-mozilla-aurora?
Comment on attachment 8813673 [details] [diff] [review]
Part 1 - Fix GetNPObjectWrapper

We should get this on the 50.1 radar as well.
Attachment #8813673 - Flags: approval-mozilla-release?
This landed on m-c yesterday:

https://hg.mozilla.org/mozilla-central/rev/f15f2cb856457694dec7390802ccf5578a13e152

To simplify tracking, let's use this bug only for part 1, and later I'll file a new bug for part 2.
Summary: Add compartment asserts to JS_FireOnNewGlobalObject and other APIs → Compartment mismatch in GetNPObjectWrapper
Target Milestone: --- → mozilla53
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8813673 [details] [diff] [review]
Part 1 - Fix GetNPObjectWrapper

Sec-high, approved for all branches
Attachment #8813673 - Flags: approval-mozilla-release?
Attachment #8813673 - Flags: approval-mozilla-release+
Attachment #8813673 - Flags: approval-mozilla-esr45?
Attachment #8813673 - Flags: approval-mozilla-esr45+
Attachment #8813673 - Flags: approval-mozilla-beta?
Attachment #8813673 - Flags: approval-mozilla-beta+
Attachment #8813673 - Flags: approval-mozilla-aurora?
Attachment #8813673 - Flags: approval-mozilla-aurora+
Attachment #8813641 - Attachment is obsolete: true
This should not have landed in 50.1 since it affects ESR45 and the next ESR45 release isn't until late January.
Flags: needinfo?(abillings)
Ok. We have an ESR45.6 going out with 50.1. This advisory should come out with both releases.
Flags: needinfo?(abillings)
Whiteboard: [adv-main50.1+][adv-esr45.6+]
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.