Closed Bug 1224371 Opened 7 years ago Closed 7 years ago

UBSan: bad left shift in [@vp9_parse_superframe_index]

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed

People

(Reporter: tsmith, Assigned: gerald)

References

Details

(Keywords: sec-other, testcase, Whiteboard: [adv-main46-])

Attachments

(3 files)

Attached file 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)
See bug 939157 for information on where undefined behavior in shifts was causing crashes in the JS engine.
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?
Flags: needinfo?(giles)
Passing to Gerald.
Flags: needinfo?(gsquelart)
Priority: -- → P1
Flags: needinfo?(giles)
(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.
See Also: → 1224365
s/255<<8/255<<24/
(I wish we could just fix typos in comments)
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.
Flags: needinfo?(gsquelart)
Priority: P1 → P5
Keywords: sec-other
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?
Assignee: nobody → gsquelart
Attachment #8702797 - Flags: feedback?(twsmith)
Comment on attachment 8702797 [details] [diff] [review]
1224371-cast-before-shift.patch

That removes the UBSan error.
Attachment #8702797 - Flags: feedback?(twsmith) → feedback+
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.
Attachment #8702797 - Flags: review?(jyavenard)
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?
Attachment #8702797 - Flags: review?(jyavenard) → review+
Upstream update patch (for local repo).

I'll also look into submitting a patch to libvpx.
Attachment #8703382 - Flags: review?(jyavenard)
Attachment #8703382 - Flags: review?(jyavenard) → review+
Target Milestone: --- → mozilla46
Group: media-core-security → core-security-release
Whiteboard: [adv-main46-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.