1224361 - UBSan: index out of bounds [@vp8_mb_init_dequantizer]

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.vp8

This was found by fuzzing libvpx (commit c6641709a707ccb98cbdf785428659e44d4f2c8b) and it appears to be in our branch.
https://dxr.mozilla.org/mozilla-central/source/media/libvpx/vp8/decoder/decodeframe.c#84

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

Comment 1

[Ralph Giles (:rillian)]

Do you want to take a look, Gerald?

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1224361-gtest.patch

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.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1224361-clamp-abs-QIndex.patch

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.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 6

[James Zern]

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.

[1] https://bugs.chromium.org/p/webm/issues/detail?id=851

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1224361-upstream-update-patch.patch

libvpx upstream update patch.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea96a37f1a43
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca27c3da032
https://hg.mozilla.org/integration/mozilla-inbound/rev/429cd1f9006d

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?

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea96a37f1a43
https://hg.mozilla.org/mozilla-central/rev/9ca27c3da032
https://hg.mozilla.org/mozilla-central/rev/429cd1f9006d

Comment 10

[Ralph Giles (:rillian)]

Since this is sec-low and we've landed our fix, I told James he could publish the testcase in the libvpx regression suite.