1224371 - UBSan: bad left shift in [@vp9_parse_superframe_index]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.vp9

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/vp9/decoder/vp9_decoder.c#502

vp9/decoder/vp9_decoder.c:504:29: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'
    #0 0x712dae in vp9_parse_superframe_index (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x712dae)
    #1 0x6c74fd in decoder_decode (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x6c74fd)
    #2 0x4ecdde in vpx_codec_decode (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x4ecdde)
    #3 0x4eb189 in main /home/user/code/libvpx/examples/simple_decoder.c:135:11
    #4 0x7fbfe019aec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #5 0x41dad5 in _start (/home/user/Desktop/libvpx/simple_decoder_ub_asan+0x41dad5)

Comment 1

[Andrew McCreight [:mccr8]]

See bug 939157 for information on where undefined behavior in shifts was causing crashes in the JS engine.

Comment 2

[Daniel Veditz [:dveditz]]

rgiles: can you take a look at this? Shifting by a loop index limited by a variable seems like a reasonable number of places for wrong values to creep in. Are we intentionally throwing away data (ok)? Is it a potential security bug (index to be used later) or just a display bug?

Comment 3

[Ralph Giles (:rillian)]

Passing to Gerald.

Comment 4

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

(In reply to Daniel Veditz [:dveditz] from comment #2)
> rgiles: can you take a look at this? Shifting by a loop index limited by a
> variable seems like a reasonable number of places for wrong values to creep
> in. Are we intentionally throwing away data (ok)? Is it a potential security
> bug (index to be used later) or just a display bug?

The shift is by up to (mag-1)*8, where 'mag' is previously calculated to be:
> const uint32_t mag = ((marker >> 3) & 0x3) + 1;
Therefore 1<=mag<=4, the loop index j<mag, and the maximum shift would be by 3*8=24.
So I don't think there is any actual security risk here.

The issue itself looks exactly like the one in bug 1224365: A uint8_t is promoted to an int because of the shift, but the value 255<<8 overwrites the sign bit of the signed int, though it doesn't actually matter as the value is then OR'd into a uint32_t. A cast (before the shift) should resolve the compiler complaint.

Comment 5

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

s/255<<8/255<<24/
(I wish we could just fix typos in comments)

Comment 6

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

Just like bug 1224356: I don't think it's a security risk, so I'm lowering the priority, and will work on it later on unless someone wants to take it in the meantime.

Comment 7

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

Attachment: 1224371-cast-before-shift.patch

When trying to test with the bug's test case, I got an OOM return before the faulty code was hit.
Tyson, could you please try your test again with this patch, to see if it fixes the TSAN warning?

Comment 8

[Tyson Smith [:tsmith]]

Comment on attachment 8702797 [details] [diff] [review]
1224371-cast-before-shift.patch

That removes the UBSan error.

Comment 9

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

Comment on attachment 8702797 [details] [diff] [review]
1224371-cast-before-shift.patch

Thanks Tyson for confirming that this fixes the UBSAN issue.

Up for review now.
Please note that this is C code, hence the ugly C-style cast.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8702797 [details] [diff] [review]
1224371-cast-before-shift.patch

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

shouldn't this be reported upstream?

Comment 11

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

Attachment: 1224371-upstream-update-patch.patch

Upstream update patch (for local repo).

I'll also look into submitting a patch to libvpx.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e68d3d1ab00c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e497a0b15cd

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e497a0b15cd
https://hg.mozilla.org/mozilla-central/rev/e68d3d1ab00c