Closed Bug 969382 Opened 8 years ago Closed 8 years ago

Assertion failure: isString(),


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox27 --- unaffected
firefox28 + fixed
firefox29 + verified
firefox30 + verified
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed


(Reporter: cbook, Assigned: Waldo)




(Keywords: assertion, crash, sec-high)


(5 files, 3 obsolete files)

Attached file bughunter stack
found via bughunter a crash on (some kind of kids site it seems with sound). Loading this site crashes opt and debug builds 

Debug builds fail with Assertion failure: isString(), at c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\include\js/Value.h:1119

Opt Builds fail with

According to bughunter this affects trunk and debug builds 

will see for testcase etc
I can reproduce this on OS X, also happens with the JITs disabled.
Component: JavaScript Engine: JIT → JavaScript Engine
OK, so we're calling a setTimeout function.  Specifically, the function is this one:

  var timer = setInterval(function() {
    if((document.getElementsByTagName("body")[0] != null || document.body != null)) {
      //GA updates
      MATTEL.tracker.Tracker.ready = true;
  }, 250);

This function throws.  We go to report it:

#1  0x0000000102a7c422 in js::ErrorObject::fileName (this=0x126991340) at ErrorObject.h:89
#2  0x0000000102c0b429 in js::ErrorObject::getOrCreateErrorReport (this=0x126991340, cx=0x1157e4e30) at ErrorObject.cpp:128
#3  0x0000000102a44d80 in js_ErrorFromException (cx=0x1157e4e30, objArg={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfbf38}) at jsexn.cpp:285
#4  0x0000000102a4403e in js_ReportUncaughtException (cx=0x1157e4e30) at jsexn.cpp:761

(gdb) frame 1
#1  0x0000000102a7c422 in js::ErrorObject::fileName (this=0x126991340) at ErrorObject.h:89
89              return getReservedSlot(FILENAME_SLOT).toString();

but this->getReservedSlot(FILENAME_SLOT) is an object value.  Specifically, it's an object with getClass()->name == "Location".

So the toString() there asserts fatally in debug builds and presumably in opt builds we crash when we try to use the "string data".
This is a quite curious error object.  It certainly claims to have the "Error" class, but seems to have the following slot layout:

Slot 0: Int32Value(0).  This could be JSEXN_ERR, so ok so far.
Slot 1: Value with payload and tag both equal to 0.  Could be PrivateValue(nullptr), so ok.
Slot 2: ObjectValue(*locationObject).  Expected a StringValue for filename.
Slot 3: NullValue().  Expected an Int32Value for column number.
Slot 4: Int32Value(0).  Could be a line number.
Slot 5: StringValue().  This should be the stack.
Slot 6: UndefinedValue().  This is normal; we may have no message string.

Slot 5 is in fact this:


so that part is fine.

So something happened to slots 2 and 3....
Oh, this is lovely.  Looking at that top MATTEL.util.throwError function, it's doing this:

  var error = new Error();
  error.lineNumber = null;
  error.fileName = window.location;
  error.message = errorMessage;
  error.description = errorMessage;
  throw error;

so it looks like that's directly writing slots that we then read with getReservedSlot()???
So js::ErrorObject::assignInitialShape does things like:

  obj->addDataProperty(cx, cx->names().fileName, FILENAME_SLOT, 0)
Looks like this code was added in bug 724768?
Blocks: 724768
Flags: needinfo?(jwalden+bmo)
So looking at other callers of addDataProperty with fixed indices, since now I'm scared of that function:

1) StringObject::assignInitialShape uses JSPROP_PERMANENT | JSPROP_READONLY, so that's ok.

2) RegExpObject::assignInitialShape uses JSPROP_PERMANENT | JSPROP_READONLY except for lastIndex, which is writable.  But the only user that reads it checks isInt32() and if not calls ToInteger(), so seems to be safe.

Still sorting out the GlobalObject uses.
GlobalObject::setConstructorPropertySlot() is unused in practice, so I _think_ we're OK there.
Another question.  Are these new getters used for anything other than js_CopyErrorObject?
Attached patch Likely patch (obsolete) — Splinter Review
Sigh, not sure how I missed this.  Here's a reasonably backportable hackaround that likely addresses things.

As far as I can tell, this issue wasn't web-visible/exploitable prior to the introduction of ErrorObject::getOrCreateErrorReport, without the user invoking the new debugger stuff.  The only other caller was the new-debugger's ErrorCopier stuff, which requires the Debugger API to be in use.  We can imagine a site getting someone to run that stuff, of course, but it's definitely a mitigating factor here.
Assignee: nobody → jwalden+bmo
Attachment #8372402 - Flags: review?(jorendorff)
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8372402 [details] [diff] [review]
Likely patch

Review of attachment 8372402 [details] [diff] [review]:


Yes, this is good stuff. Clearing r? flag waiting for a test or two.
Attachment #8372402 - Flags: review?(jorendorff)
reviewer d'oh
Attached patch Test (obsolete) — Splinter Review
Attachment #8372402 - Attachment is obsolete: true
Attachment #8372593 - Flags: review?(jorendorff)
Attached patch Actual patch (obsolete) — Splinter Review
Okay, previous patch was busted in so many ways it's not even funny.  This actually compiles with every file, and it makes the previous test pass (it fails without this).
Attachment #8372594 - Flags: review?(jorendorff)
Just curious, why the jsapi-test when a jit-test/jstest also works?

js> var e = Error(); e.fileName = null; throw e;
Assertion failure: isString(), at ../../../dist/include/js/Value.h:1119
(In reply to Jan de Mooij [:jandem] from comment #15)
> Why the jsapi-test when a jit-test/jstest also works?

jstests' expected-failure mechanism is pretty crude, limited just to the exit code.  And I don't *think* you get the uncaught-exception error code invoked unless the entire test fails, here -- evaluate() doesn't cut it.  I'd rather we be precise about invoking the uncaught-exception code and do that narrowly, rather than depending too much on the shell mechanics.  jit-tests let you be more precise about the error that's throw, but they still have the not-doing-exactly-the-necessary-steps thing to worry about.
Keywords: sec-high
...but for this much code you could've added a reportException(exc) function to the shell. :(

You can use JS_SetPendingException followed by JS_ReportPendingException, rather than relying on the JSAPI's "uncaught exception handling" nonsense. Eliminate the JSNative, at least.
Er, I didn't mean that as an imperative. I meant "That would eliminate the JSNative, at least."
Comment on attachment 8372593 [details] [diff] [review]

Review of attachment 8372593 [details] [diff] [review]:

Yeah. I would prefer to see a reportException shell builtin and a few JS tests, but r=me if you'd rather just land this.
Attachment #8372593 - Flags: review?(jorendorff) → review+
Comment on attachment 8372594 [details] [diff] [review]
Actual patch

Review of attachment 8372594 [details] [diff] [review]:

::: js/src/vm/ObjectImpl.h
@@ +1326,4 @@
>          return slots + (slot - fixed);
>      }
> +    const HeapSlot *getSlotAddressUnchecked(uint32_t slot) const {

Please have the non-const version call this one and const_cast the result instead of duplicating the code. (The others below are fine as you've written them.)
Attachment #8372594 - Flags: review?(jorendorff) → review+
Attached patch Test, updatedSplinter Review
Used JS_ReportPendingException, simplified slightly.

This patch doesn't need to land to fix things here, and in theory we shouldn't land it because it clarifies the bug.  But reading the actual patch will expose the problem pretty trivially anyway, so there's not much point in holding off on landing this patch.  I'll fill out the s-a bits on the actual patch, for both the patch and this one.
Attachment #8372593 - Attachment is obsolete: true
Attachment #8381144 - Flags: sec-approval?
Attachment #8381144 - Flags: review+
Attached patch Patch, updatedSplinter Review
Assuming this needs trunk landing before aurora/beta as usual (although I think this should land immediately everywhere, no delay for baking that's going to do nothing more than mark a checkbox on a list), else I'd request everywhere...

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's easy to take things "off the rails" and into undefined behaviors and pointer type-punning, and probably trivial for someone knowledgeable to take the next step to a full exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The code changes are incriminating enough.

Which older supported branches are affected by this flaw?
Aurora/beta, since bug 724768's landing.  Pounce now and we prevent this from hitting release.  ;-)

If not all supported branches, which bug introduced the flaw?
Bug 724768.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Haven't created them, should be very easy, not risky to create.

How likely is this patch to cause regressions; how much testing does it need?
Pretty unlikely, it's very straightforward and clearly fixes the issue in question.  We should land this everywhere, fast.
Attachment #8372594 - Attachment is obsolete: true
Attachment #8381146 - Flags: sec-approval?
Attachment #8381146 - Flags: review+
Comment on attachment 8381146 [details] [diff] [review]
Patch, updated

sec-approval+ for trunk. Please create your Aurora and Beta backports and nominate them.
Attachment #8381146 - Flags: sec-approval? → sec-approval+
Attachment #8381144 - Flags: sec-approval? → sec-approval+
FWIW, this applies cleanly to Aurora and has some rebase issues on beta due to the lack of bug 872273.
waldo, this needs to land now or ?
Flags: needinfo?(jwalden+bmo)

Still going to need a rebased patch for beta before this can be uplifted to the release branches.
Flags: in-testsuite+
Keywords: checkin-needed
Stupid issue responding to review comments, fixed that, landed on trunk:

Aurora/beta backports may or may not be coming, I don't have those trees around and don't have good Internet access to procure them any more.  :-(  Will see what I can do.
Flags: needinfo?(jwalden+bmo)
It is too late for Beta this release but Aurora would be nice.
Attached patch Aurora patchSplinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 724768
User impact if declined: sec-critical exploits
Testing completed (on m-c, etc.): landed on m-c, contains a test
Risk to taking this patch (and alternatives if risky): very low, pretty straightforward
String or IDL/UUID changes made by this patch: N/A
Attachment #8384740 - Flags: review+
Attachment #8384740 - Flags: approval-mozilla-aurora?
Attachment #8384740 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Al Billings [:abillings] from comment #29)
> It is too late for Beta this release but Aurora would be nice.

I'm not sure if it's too late to fix sec-crit bugs on Beta, esp. when risk is "very low" as per comment #30. Release Management, what do you say?
Flags: needinfo?(release-mgmt)
This is listed as a sec-high but I've heard rumblings that it might actually be a sec-critical. Is that true?
Note: This bug is marked sec-high, but it is probably sec-critical in FF29 and FF30.

Jeff and I think this can be used to read arbitrary memory *and* to discover the addresses of strings and objects. That's already as bad as a sec-high bug can get.

It might further be possible to write any memory that looks sufficiently like a Rope. In mozilla-central, js::ErrorObject::getOrCreateErrorReport calls message->ensureFlat(cx) where message is a pointer whose bits are chosen by the attacker. If that gun can be pointed at the stack, it's arbitrary code execution. However, that code is not in FF28 (current mozilla-beta).

Our best guess is that this bug is still "only" sec-high in current beta.
We still have a couple more betas for 28 so if it's not a risky landing I'd be fine with taking it. Otherwise if the critical part isn't as pressing until 29 and landing is too risky, we should wait.
Flags: needinfo?(release-mgmt)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Hm, I wish we'd gone the route of just doubling up the slots. We're going to need to do that for Xrays anyway.
Attached patch Beta patchSplinter Review
I had to rip out the existing test and write one targeted at the debugger aspects of this (I'll land it on trunk at some point, no rush now, tho).  Other than that, and removing changes to code that didn't land in beta, this was a straightforward backport.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 724768
User impact if declined: debugging the wrong code leads to sec-high or maybe sec-critical issues
Testing completed (on m-c, etc.): original patch landed on m-c and aurora, included a test
Risk to taking this patch (and alternatives if risky): should be safe, this is simple enough
String or IDL/UUID changes made by this patch: N/A
Attachment #8385738 - Flags: review+
Attachment #8385738 - Flags: approval-mozilla-beta?
Attachment #8385738 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Confirmed crash on Fx29, 2014-02-10.
Verified fixed on Fx29, 2014-03-25.
Verified fixed on Fx30, 2014-03-24.
Group: core-security
You need to log in before you can comment on or make changes to this bug.