821760 - Static operations called via Xrays end up with compartment mismatches

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Code looks like this:

  if (js::IsWrapper(obj)) {
    obj = XPCWrapper::Unwrap(cx, obj, false);
    if (!obj) {
      return Throw<true>(cx, NS_ERROR_XPC_SECURITY_MANAGER_VETO);
    }
  }
...
    val.setObjectOrNull(JS_GetGlobalForObject(cx, obj));

with nary a compartment-enter in sight.

I just checked, and this is the only consumer of XPCWrapper::Unwrap that doesn't handle compartments in the process.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Properly handle compartments in WebIDL static operation bindings.

Comment 6

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/040f5d35fa6a

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 692337 [details] [diff] [review]
Properly handle compartments in WebIDL static operation bindings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 763643
User impact if declined: Some crashes with the fatal compartment checks, possible
   security issues.
Testing completed (on m-c, etc.): Passes tests.  Should have no behavior impact
   otherwise.
Risk to taking this patch (and alternatives if risky): Should be low risk.  The
   other option is to ship the compartment mismatch, which I think is
   undesirable.
String or UUID changes made by this patch: None.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/040f5d35fa6a

Comment 9

[Andrew McCreight [:mccr8]]

There seem to be no crashes with this stack with a build of 12-16 or later. Hooray.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 692337 [details] [diff] [review]
Properly handle compartments in WebIDL static operation bindings.

Hooray indeed, please go ahead with uplift.

Comment 11

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4c455dd56a1

Comment 12

[Al Billings [:abillings - ex-MoCo]]

As an aside, this should have had sec-approval+ per policy before going into trunk since it affected Aurora.

https://wiki.mozilla.org/Security/Bug_Approval_Process:

"Core-security bug fixes should just be landed by a developer without any explicit approval if:

    The bug has a sec-low, sec-moderate, sec-other, or sec-want rating.
    OR
    The bug is a recent regression on mozilla-central (this means that the specific regressing check-in has been identified on mozilla-central) 

This means that the developer can mark the status flags for ESR, Beta, and Aurora as 'unaffected.' It also means that we haven't shipped anywhere public in an official release yet."

I know that the last comment may seem confusing but it means that it hasn't gone out in an Aurora or Beta build yet.