Closed Bug 821760 Opened 7 years ago Closed 7 years ago

Static operations called via Xrays end up with compartment mismatches

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 + fixed
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: sec-critical, Whiteboard: [adv-main19-])

Attachments

(1 file)

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.
Keywords: sec-high
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Attachment #692337 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/040f5d35fa6a
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla20
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.
Attachment #692337 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/040f5d35fa6a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
There seem to be no crashes with this stack with a build of 12-16 or later. Hooray.
Comment on attachment 692337 [details] [diff] [review]
Properly handle compartments in WebIDL static operation bindings.

Hooray indeed, please go ahead with uplift.
Attachment #692337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Duplicate of this bug: 827962
Whiteboard: [adv-main19-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.