Closed Bug 949940 Opened 6 years ago Closed 6 years ago

Compartment mismatch: Object.create(frameWin).self

Categories

(Core :: DOM: Core & HTML, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- unaffected
firefox28 + verified
firefox29 + verified
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

(4 keywords)

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file testcase
No description provided.
Crash Signature: [@ js::CompartmentChecker::fail(JSCompartment*, JSCompartment*) ]
Looks potentially-related to the recent window changes...
Flags: needinfo?(peterv)
This is exciting.

We land in nsOuterWindowProxy::get with id=="self" (called via js::CrossCompartmentWrapper::get etc).  

That eventually lands us in mozilla::dom::WindowBinding::genericCrossOriginGetter, which ends up throwing because the thisobj is not actually a window.

But genericCrossOriginGetter enters the compartment of uncheckedObj.  And throwing looks like this:

          return ThrowInvalidThis(cx, args, GetInvalidThisErrorForGetter(rv == NS_ERROR_XPC_SECURITY_MANAGER_VETO), "Window");

and ThrowInvalidThis will end up doing:

  JS::Rooted<JSFunction*> func(aCx, JS_ValueToFunction(aCx, aArgs.calleev()));

which is where the compartment check fails.

We need to either reenter the compartment of "obj" before doing ThrowInvalidThis or scope the entering of the compartment of uncheckedObj more narrowly.  In fact, the only reason we need that compartment-entering behavior is for the UnwrapArg<nsGlobalWindow> call.  So maybe we can just scope the autocompartment around that?
Blocks: 946067
(In reply to Boris Zbarsky [:bz] from comment #3)
> In fact, the only reason we need that compartment-entering
> behavior is for the UnwrapArg<nsGlobalWindow> call.  So maybe we can just
> scope the autocompartment around that?

Yes, this sounds like the right thing.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8347365 [details] [diff] [review]
Only enter the uncheckedObj compartment in a crossOriginGetter/Setter/Method around the UnwrapArg call that needs us to be inthat compartment.

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

The generated code looks right, but I'm not up to speed enough on the Codegen to review what look like nontrivial changes. bouncing that part to peter.
Attachment #8347365 - Flags: review?(peterv)
Attachment #8347365 - Flags: review?(bobbyholley+bmo)
Attachment #8347365 - Flags: feedback+
Setting sec-high for compartment mismatch, though I'm not sure how possible it would be to actually exploit this.  Feel free to adjust.
Keywords: sec-high
I don't think it's possible to exploit this, fwiw.
Comment on attachment 8347365 [details] [diff] [review]
Only enter the uncheckedObj compartment in a crossOriginGetter/Setter/Method around the UnwrapArg call that needs us to be inthat compartment.

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

::: dom/bindings/Codegen.py
@@ +2663,5 @@
>  ${codeOnFailure}
>    }
>  }""").substitute(self.substitution, codeOnFailure=codeOnFailure)
>  
> +    def getXPConnectUnwrap(self, allowCrossOriginObj):

Not sure why this needs to be a method, might be better to just store this in a string where you define self.substitution["uncheckedObjDecl"].
Attachment #8347365 - Flags: review?(peterv) → review+
Flags: needinfo?(peterv)
Attachment #8347365 - Attachment is obsolete: true
Comment on attachment 8349031 [details] [diff] [review]
Updated to review comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 946067 
User impact if declined: Can't land fix for bug 946067 on aurora, get web compat
   regressions.
Testing completed (on m-c, etc.): Passes tests
Risk to taking this patch (and alternatives if risky): Low risk, I believe.
String or IDL/UUID changes made by this patch: None
Attachment #8349031 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
Whiteboard: Needs to be uplifted on top of bug 946067
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/88f21d4b4c78
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8349031 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Confirmed crash, compartment mismatch in FF29, 2013-12-13.
Verified fixed in FF28, FF29, 2014-01-18.
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.