Closed Bug 926401 Opened 9 years ago Closed 9 years ago

ASan heap-buffer-overflow with BinaryData


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox25 --- unaffected
firefox26 --- unaffected
firefox27 + disabled
firefox28 + fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected


(Reporter: decoder, Assigned: Waldo)



(4 keywords, Whiteboard: [asan][fixed in bug 898362; this bug has the test])


(2 files, 1 obsolete file)

The following testcase shows a heap-buffer-overflow on mozilla-central revision 211337f7fb83 (run with --fuzzing-safe):

    var Color = new StructType({'r': uint8, 'g': uint8, 'b': uint8});
    var Rainbow = new ArrayType(Color, (0));
    var theOneISawWasJustBlack = Rainbow.repeat({'r': 0, 'g': 0, 'b': 0});
Whiteboard: [asan]
Attached patch Patch (obsolete) — Splinter Review
I saw this in passing in bugmail and wanted to see how we were going awry so badly here.  Obvious patch.

Do we expose this stuff to untrusted content yet?  If we don't, we might be saved here.  Otherwise this is going to be one of those land-at-the-last-minute patches.  :-\
Assignee: general → jwalden+bmo
Attachment #816575 - Flags: review?(nmatsakis)
The code in question is dead, removed in later patches to typed objects that were blocked on 914220 (but are not now, horray)
Note: the code in question is limited to nightly builds only.
Niko, does that mean this test case is fixed now by bug 914220?  The test should be landed then.
Sorry, I was unclear. Not yet, the actual fix is in bug 898362 which I hope to land shortly. I'll add a dependency. I already added the test case to that bug.
Depends on: 898362
No longer depends on: 914220
My mistake, I failed to include the test case in that bug. I am going to double check that the test is fixed, now that bug 898362 has landed (on inbound, at least), and submit a patch here to add the test.
Whiteboard: [asan] → [asan][fixed in bug 898362; this bug has the test]
Attachment #816575 - Flags: review?(nmatsakis)
Attached patch Bug926401.diffSplinter Review
Although the function "repeat" has been removed in more recent patches, by modifying the test I found a similar bug where we weren't checking for a zero-length array. The effect was harmless but did trip an assert.
Attachment #816575 - Attachment is obsolete: true
Attachment #825989 - Flags: review?(jwalden+bmo)
Comment on attachment 825989 [details] [diff] [review]

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

::: js/src/builtin/TypedObject.js
@@ +152,5 @@
>           "moveToElem invoked on non-array");
>    assert(TO_INT32(index) === index,
>           "moveToElem invoked with non-integer index");
>    assert(index >= 0 && index < this.length(),
> +         "moveToElem invoked with out-of-bounds index: " + index);

Uh...I guess this is against some patch that never landed?  Because this.length() is certainly observable, if it were actually like that in the tree, but it looks like it isn't, and REPR_LENGTH looks fine in that regard, so I guess never mind here.  :-)
Attachment #825989 - Flags: review?(jwalden+bmo) → review+
Yes, it is against a patch that has not (yet) landed. (Bug 922115 adds the `length()` method, I haven't even uploaded the patch yet I think because I'm still waiting for reviews elsewhere). I'll rebase though, I think it's mostly independent.
The whiteboard says this is fixed in some other bug, but that bug landed a month ago. What's the status of this security vulnerability, has the bug morphed?
Flags: needinfo?(jwalden+bmo)
Sorry, I need to push that fix. Slipped off my radar. Note that firefox27 and friends should be unaffected since the relevant code is Nightly only.
Flags: needinfo?(jwalden+bmo)
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Decoder, can you verify that this has been fixed in FF28? Thank you. :)
Group: core-security
You need to log in before you can comment on or make changes to this bug.