Compartment mismatch in js::UnwindIteratorForException

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({regression, sec-high})

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(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)

Details

Attachments

(1 attachment)

Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
This appears to be the only call to JSContext::getException that does not check the return value.
Assignee

Updated

6 years ago
Assignee: nobody → continuation
Assignee

Comment 2

6 years ago
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
Assignee

Comment 3

6 years ago
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+
Assignee

Comment 5

6 years ago
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.