Closed Bug 960768 Opened 6 years ago Closed 6 years ago

Compartment mismatch in js::UnwindIteratorForException

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Keywords: regression, sec-high)

Attachments

(1 file)

Seen on crash-stats:
https://crash-stats.mozilla.com/report/index/4a0c9196-12f9-4e77-953f-f2dd02140111
https://crash-stats.mozilla.com/report/index/afe8401e-8e9a-4cf0-ae10-789332131231

The stack looks like this:
  JSContext::setPendingException(JS::Value)
  js::UnwindIteratorForException
  js::jit::HandleExceptionBaseline

The code in UnwindIteratorForException is:
  cx->getPendingException(&v);
  cx->clearPendingException();
  if (!CloseIterator(cx, obj))
    return false;
  cx->setPendingException(v);

getPendingException has 3 cases:
  1. the JSContext is in the atoms compartment
  2. wrapping fails
  3. we check that the wrapped value is in the right compartment

In case 1, the compartment check should succeed.  In case 3, we've already checked, so we should be okay, also.  Based on elimination, then, I'm guessing that we're failing to wrap for some reason.  Notice that UnwindIteratorForException doesn't check the return value, so assuming a failing wrap leaves the argument alone, then we're ending up setting with an unwrapped v.

Maybe the solution is just to throw away the exception in that case?

I don't know how bad this is, given that we're holding the object alive on the context directly, but maybe something bad could happen later.
This appears to be the only call to JSContext::getException that does not check the return value.
Assignee: nobody → continuation
https://tbpl.mozilla.org/?tree=Try&rev=d0093f2ac3ef

Well, maybe it should be high, because it can obviously be triggered somehow, and I assume once it is on the context, anything could grab it and stick it on the heap somewhere.
Keywords: sec-moderatesec-high
I don't know this code, but I assume we want to clear and close in the case of failure.

Also, let me know if you think the sec rating is reasonable, or if it is too high.
Attachment #8361669 - Flags: review?(luke)
Comment on attachment 8361669 [details] [diff] [review]
Check the result of getPendingException in js::UnwindIteratorForException.

I think your analysis is correct.
Attachment #8361669 - Flags: review?(luke) → review+
This is a regression from bug 951282, so it only affects 29.

Prior to that, getPendingException was infallible, and wrapping occurred in wrapPendingException.  If the wrap failed in the latter, then the exception just got thrown away, so there was no problem.
https://hg.mozilla.org/mozilla-central/rev/648a54eeed19
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Group: core-security
You need to log in before you can comment on or make changes to this bug.