1224363 - UBSan: index out of bounds [@vp8_loop_filter_row_simple]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

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/common/loopfilter.c#285

vp8/common/vp8_loopfilter.c:285:39: runtime error: index 254 out of bounds for type 'unsigned char [64][16]'
    #0 0x77d6a1 in vp8_loop_filter_row_simple (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x77d6a1)
    #1 0x83f10d in decode_mb_rows (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x83f10d)
    #2 0x837e2d in vp8_decode_frame (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x837e2d)
    #3 0x5d052d in vp8dx_receive_compressed_data (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x5d052d)
    #4 0x5ca52c in vp8_decode (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x5ca52c)
    #5 0x4ecdde in vpx_codec_decode (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x4ecdde)
    #6 0x4eb189 in main /home/user/code/libvpx/examples/simple_decoder.c:135:11
    #7 0x7fb1aca3bec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #8 0x41dad5 in _start (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x41dad5)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.vp9

Comment 2

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

(In reply to Tyson Smith [:tsmith] from comment #1)
> Created attachment 8686822 [details]
> test_case.vp9

Note: Despite what the file extension suggests, the content is actually VP8 (in an IVF container).

Comment 3

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

Attachment: 1224363-gtest.patch

Added vp8/ivf test case

Comment 4

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

Attachment: 1224363-clamp-abs-feature-data.patch

Clamp abs-value segment feature data.

Even when the segment feature data is in absolute mode, it is still read as a
6-bit value with an added sign, so it could have values between -63 and +63.
Later, this signed value is used without checks as a filter level, which is
used to access an entry in an array of size 64.

This is fixed by clamping the segment feature data to 0-63 in absolute mode.

It is best handled there, as these values are read at most once per frame,
but then may be used many times in the filter loop.

Comment 5

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

We probably should get a VPx expert to review this issue and proposed fix.
Ralph, I see in bug 1218124 that you've involved "James Zern from the libvpx upstream team", would he be the right person?

Regarding security risks: The main risk here is reading some data from outside the loop_filter_info struct. It could just crash, otherwise that data would be used to apply wrong filter values to the decoded image, probably just producing a not-so-good-looking image.
I doubt it could be used for evil: An attacker would have to compare the actual output with the expected image, and from there reverse-calculate (if that's even a 1-to-1 function) the filter values and hence whatever was pointed to by the out-of-bound array pointer.
Once again, a VPx expert should look into this and determine the potential threat.

Comment 6

[Ralph Giles (:rillian)]

(In reply to Gerald Squelart [:gerald] from comment #5)

> Ralph, I see in bug 1218124 that you've involved "James Zern from the libvpx
> upstream team", would he be the right person?

Yes, he's a good technical contact. GPG key 0x2C513690771127B1. Or add him to the bug.

You can also now file something on gerrit and set the security group flag. See http://www.webmproject.org/code/contribute/submitting-patches/ for setup.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8690054 [details] [diff] [review]
1224363-gtest.patch

Review of attachment 8690054 [details] [diff] [review]:
-----------------------------------------------------------------

Rather than adding a ivf parser, I'd rather we remuxed the file as webm and uses our existing webm parser, even though it's a bit more code. That makes it easier to use the test case in other contexts, and avoids adding a new code path just for this test.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8690056 [details] [diff] [review]
1224363-clamp-abs-feature-data.patch

Review of attachment 8690056 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libvpx/vp8/decoder/decodeframe.c
@@ +1119,5 @@
>  
>                          if (vp8_read_bit(bc))
>                              xd->segment_feature_data[i][j] = -xd->segment_feature_data[i][j];
> +
> +                        /* clamp to 0-63 if absolute */

Looks like you should clamp to 1<<mb_feature_data_bits[i] - 1?

Not all segment features are 6 bits, I think. https://tools.ietf.org/html/rfc6386#section-9.4

Comment 9

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

Attachment: 1224363-clamp-abs-lvl_seg.patch

Clamp seg_lvl also in abs-value mode.

Even when the segment feature data is in absolute mode, it is still read as a
6-bit value with an added sign, so it could have values between -63 and +63.
Later, this signed value is used without checks as a filter level, which is
used to access an entry in an array of size MAX_LOOP_FILTER+1=64.

This patch just extends the existing clamping (that was done only to relative-
mode data) to absolute mode data, before it is blindly 'memset' in
lfi->lvl[seg][0], which was where the out-of-bound filter_value was read in
subsequent vp8_loop_filter_row_simple.


Note that this is a simpler patch, where I moved a single line to apply to more situations. So please don't complain about the magic value '63', as it was there!
Of course, if you'd like I could change it to a less magic MAX_LOOP_FILTER, but it would be inconsistent with all the other '63's in the file.


(In reply to Ralph Giles (:rillian) from comment #8)
> Comment on attachment 8690056 [details] [diff] [review]
> 1224363-clamp-abs-feature-data.patch
> ...
> Looks like you should clamp to 1<<mb_feature_data_bits[i] - 1?
> Not all segment features are 6 bits, I think.
> https://tools.ietf.org/html/rfc6386#section-9.4

This is the storage size, which could be indeed be 6 or 7 bits (in addition to a sign bit), so it is implicitly clamped to that range by virtue of how it is read:
http://mxr.mozilla.org/mozilla-central/source/media/libvpx/vp8/decoder/decodeframe.c#1118

However the issue here is with how this value is later used as index into an array of size MAX_LOOP_FILTER+1=64. That's why I believe it needs to be clamped to 0-MAX_LOOP_FILTER.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8690606 [details] [diff] [review]
1224363-clamp-abs-lvl_seg.patch

Review of attachment 8690606 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this seems reasonable.

Comment 11

[Ralph Giles (:rillian)]

Note, please also include a copy of all patches you add the libvpx and add them from update.py to simplify porting to new releases.

Comment 12

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

(In reply to Ralph Giles (:rillian) from comment #11)
> Note, please also include a copy of all patches you add the libvpx and add
> them from update.py to simplify porting to new releases.

When I try update.py, it stops at vp9_filter_restore_aligment.patch because the change is already present upstream!
Should I use a specific "--commit" hash from the past, or should I update our patches to match master or 1.5?

Comment 13

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

James, could you please have a look at this?
I'd be interested in your opinion regarding the security risk (comment 5), and if you think my proposed fix makes sense.

Comment 14

[Ralph Giles (:rillian)]

(In reply to Gerald Squelart [:gerald] from comment #12)
> (In reply to Ralph Giles (:rillian) from comment #11)
> > Note, please also include a copy of all patches you add the libvpx and add
> > them from update.py to simplify porting to new releases.
> 
> When I try update.py, it stops at vp9_filter_restore_aligment.patch because
> the change is already present upstream!
> Should I use a specific "--commit" hash from the past, or should I update
> our patches to match master or 1.5?

The simple thing for fixing this issue (and backports if necessary) is to specify --commit with the same commit id the in-tree version uses, which is listed in README_MOZILLA. We should update to 1.5 and/or master, but that's really a separate bug.

Comment 15

[James Zern]

Sorry for the delayed response. The analysis is correct, the value is 6-bits with a trailing sign bit so clamping everywhere is correct. As mentioned this is probably a low security risk, most likely resulting in a crash or corrupted image.

Comment 16

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

Attachment: 1224363-upstream-update-patch.patch

libvpx upstream update patch.

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8694985 [details] [diff] [review]
1224363-upstream-update-patch.patch

Review of attachment 8694985 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, that's the idea. Thanks!

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6b527b5e13
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0cc92be4cd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcca1d97bd32

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f6b527b5e13
https://hg.mozilla.org/mozilla-central/rev/f0cc92be4cd8
https://hg.mozilla.org/mozilla-central/rev/fcca1d97bd32