UBSan: index out of bounds [@vp8_mb_init_dequantizer]

RESOLVED FIXED in Firefox 45



4 years ago
3 years ago


(Reporter: tsmith, Assigned: gerald)


(Blocks 1 bug, {csectype-bounds, sec-low, testcase})

Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, b2g-master fixed)


(Whiteboard: [adv-main45+][post-critsmash-triage])


(4 attachments)



4 years ago
Posted file test_case.vp8
This was found by fuzzing libvpx (commit c6641709a707ccb98cbdf785428659e44d4f2c8b) and it appears to be in our branch.

vp8/decoder/decodeframe.c:86:25: runtime error: index -127 out of bounds for type 'short [128][2]'
    #0 0x831de5 in vp8_mb_init_dequantizer (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x831de5)
    #1 0x836f5d in vp8_decode_frame (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x836f5d)
    #2 0x5d052d in vp8dx_receive_compressed_data (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x5d052d)
    #3 0x5ca52c in vp8_decode (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x5ca52c)
    #4 0x4ecdde in vpx_codec_decode (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x4ecdde)
    #5 0x4eb189 in main /home/user/code/libvpx/examples/simple_decoder.c:135:11
    #6 0x7f6d49488ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #7 0x41dad5 in _start (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x41dad5


4 years ago
Blocks: 1224362
Do you want to take a look, Gerald?
Assignee: nobody → gsquelart
Priority: -- → P1

Comment 2

4 years ago
Added vp8/ivf test case.

If you also insert:
> if (QIndex < MINQ || QIndex > MAXQ) { fprintf(stderr, "QIndex=%d\n", QIndex); }
in vp8/decoder/decodeframe.c before line 84, you will see it trigger with -127 values.
Attachment #8693383 - Flags: review?(giles)

Comment 3

4 years ago
Clamp QIndex also in abs-value mode.

Similar to bug 1224363, just moving a clamping line down, to cover more cases.

This silences the printf suggested in comment 2.
Attachment #8693384 - Flags: review?(giles)

Comment 4

4 years ago
And like bug 1224363, I don't believe this is a security risk: At worst either FF will crash, or the wrong dequant constant will be used.

Comment 5

4 years ago
James, could you please have a look at this as well?
I'd be interested in your opinion regarding the security risk (comment 4), and if you think my proposed fix makes sense.
Attachment #8693383 - Flags: review?(giles) → review+
Attachment #8693384 - Flags: review?(giles) → review+

Comment 6

4 years ago
Like the other bug this is correct, quantizer is 7-bits + sign. Risk is probably low as mentioned as it results in a table lookup.
Thanks for looking at these Ralph, vp8 has been ignored. It has some asan fuzzing coverage, but still an open bug for races in the segment-level threading code [1]. vp9 is clear on asan, but could use some ubsan treatment like this.


Comment 7

4 years ago
libvpx upstream update patch.
Attachment #8694988 - Flags: review?(giles)
Attachment #8694988 - Flags: review?(giles) → review+

Comment 8

4 years ago

As with bug 1224369, I might have been to eager to check-in, as I believe this bug should be sec-low or even no-sec, confirmed by a libvpx expert in comment 6. And it's very similar to the issues found in bug 1224363 and bug 1224369.

But sorry if I should have asked for official approval first.
Could you please mark it sec-low or lower?
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Keywords: sec-other
Keywords: sec-othersec-low
Group: media-core-security → core-security-release
Since this is sec-low and we've landed our fix, I told James he could publish the testcase in the libvpx regression suite.
Whiteboard: [adv-main45+]
Whiteboard: [adv-main45+] → [adv-main45+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.