926401 - ASan heap-buffer-overflow with BinaryData

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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});

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

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.  :-\

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Niko Matsakis [:nmatsakis]]

The code in question is dead, removed in later patches to typed objects that were blocked on 914220 (but are not now, horray)

Comment 4

[Niko Matsakis [:nmatsakis]]

Note: the code in question is limited to nightly builds only.

Comment 5

[Andrew McCreight [:mccr8]]

Niko, does that mean this test case is fixed now by bug 914220?  The test should be landed then.

Comment 6

[Niko Matsakis [:nmatsakis]]

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.

Comment 7

[Niko Matsakis [:nmatsakis]]

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.

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug926401.diff

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.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 825989 [details] [diff] [review]
Bug926401.diff

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.  :-)

Comment 10

[Niko Matsakis [:nmatsakis]]

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.

Comment 11

[Daniel Veditz [:dveditz]]

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?

Comment 12

[Niko Matsakis [:nmatsakis]]

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.

Comment 13

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=e457606d58cb
(Contains patches for bug 898359, bug 926401, and bug 930974)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ea9176d5e7e

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

Decoder, can you verify that this has been fixed in FF28? Thank you. :)