1224365 - UBSan: bad left shift in [@mem_get_le32_as_int]

Status: RESOLVED FIXED

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

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/vpx_ports/mem_ops.h#128

vpx_ports/mem_ops.h:128:16: runtime error: left shift of 129 by 24 places cannot be represented in type 'int'
    #0 0x4e6ba6 in mem_get_le32_as_int /home/user/code/libvpx/./vpx_ports/mem_ops.h:128:16
    #1 0x4e6ccd in ivf_read_frame /home/user/code/libvpx/ivfdec.c:81:18
    #2 0x4ea7e3 in vpx_video_reader_read_frame /home/user/code/libvpx/video_reader.c:68:11
    #3 0x4eb28e in main /home/user/code/libvpx/examples/simple_decoder.c:128:12
    #4 0x7f5967a39ec4 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

[Tyson Smith [:tsmith]]

Attachment: call_stack_2.txt

Comment 2

[Daniel Veditz [:dveditz]]

glandium: do we need a cast or something here? I would have expected a shift into a temporary the size of val but maybe it's shifting into a temporary the size of mem[3] ?

Comment 3

[Mike Hommey [:glandium]]

I don't remember the C legalese detail, but what happens there, according to the UBSan report, is that mem[3] is promoted to int, then shifted, and the result can't hold in a temporary (signed) int before being assigned to the unsigned int val. C++ apparently has different rules here, because the same code doesn't fail when compiled as C++. So yeah, you'd need a case of mem[3] before the shift.

Comment 4

[Mike Hommey [:glandium]]

s/case/cast/

Comment 5

[Ralph Giles (:rillian)]

NB, we also need to work out whether these are fixed in upstream's new 1.5 release, and either upstream our fix, or update our code with upstream's fix.

Comment 6

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

Strictly speaking, this bug should probably be closed as "invalid", because it reveals an issue in the IVF parser, which was not imported in our repository as Firefox does not support IVF (a simple VPx container).

However, another mem_ops.h function is used in media/libvpx/vp9/decoder/vp9_decodeframe.c:
> size = mem_get_be32(be_data);
And mem_get_be32 suffers from the same potential scary-shift-into-sign-bit issue.
So we might as well fix all the shifts as part of this bug, and share with Google.

Note: The 'size' read from mem_get_be32() is then sanity-checked against the total data size; so in the worst case, an evil 'be_data' and a bad 'mem_get_be32' couldn't possibly create a security risk.
Lowering bug priority.

Also I think we can remove the security flag. Daniel, would you agree?

Comment 7

[Daniel Veditz [:dveditz]]

If this is potentially a security bug in Google's version we should keep this hidden until they've fixed it or said that it's not.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Do we know if this ever got fixed upstream?

Comment 9

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

Yes, it was fixed on 2016-05-27:
https://chromium.googlesource.com/webm/libvpx/+/f1de6226176ff90552ebe7473d8a76e388685053

However the associated Chromium bug is not public (yet?), so we may still want to keep this bug hidden:
https://bugs.chromium.org/p/chromium/issues/detail?id=614648

The fix is present in published version 1.6.0, which we plan to import: Bug 1223692.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Gerald Squelart [:gerald] from comment #9)
> Yes, it was fixed on 2016-05-27:
> https://chromium.googlesource.com/webm/libvpx/+/
> f1de6226176ff90552ebe7473d8a76e388685053
> 
> However the associated Chromium bug is not public (yet?), so we may still
> want to keep this bug hidden:
> https://bugs.chromium.org/p/chromium/issues/detail?id=614648
> 
> The fix is present in published version 1.6.0, which we plan to import: Bug
> 1223692.

The upstream bug is public now and we've been shipping libvpx 1.6.0+ since Firefox 53. I think we can call this fixed. Can we open this bug now too since the upstream bug is public and ESR52 is unaffected anyway?