Closed Bug 1224363 Opened 9 years ago Closed 8 years ago

UBSan: index out of bounds [@vp8_loop_filter_row_simple]

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-master --- fixed

People

(Reporter: tsmith, Assigned: mozbugz)

References

Details

(Keywords: csectype-bounds, sec-low, testcase, Whiteboard: [adv-main45+][post-critsmash-triage])

Attachments

(4 files, 1 obsolete file)

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)
Summary: UBSan: index out of bounds [@vp8_mb_init_dequantizer] → UBSan: index out of bounds [@vp8_loop_filter_row_simple]
Attached file test_case.vp9
Flags: needinfo?(gsquelart)
Priority: -- → P1
(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).
Added vp8/ivf test case
Assignee: nobody → gsquelart
Attachment #8690054 - Flags: review?(giles)
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.
Attachment #8690056 - Flags: review?(giles)
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.
Flags: needinfo?(gsquelart)
(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 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.
Attachment #8690054 - Flags: review?(giles) → review+
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
Attachment #8690056 - Flags: review?(giles) → review-
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.
Attachment #8690056 - Attachment is obsolete: true
Attachment #8690606 - Flags: review?(giles)
Comment on attachment 8690606 [details] [diff] [review]
1224363-clamp-abs-lvl_seg.patch

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

Ok, this seems reasonable.
Attachment #8690606 - Flags: review?(giles) → review+
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.
(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?
Flags: needinfo?(giles)
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.
(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.
Flags: needinfo?(giles)
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.
Keywords: sec-low
libvpx upstream update patch.
Attachment #8694985 - Flags: review?(giles)
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!
Attachment #8694985 - Flags: review?(giles) → review+
Group: media-core-security → core-security-release
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.

Attachment

General

Created:
Updated:
Size: