If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

libVorbis Undefined Behavior (block.c)

UNCONFIRMED
Unassigned

Status

()

Core
Audio/Video: Playback
P5
normal
UNCONFIRMED
a year ago
11 months ago

People

(Reporter: Craig Young, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Created attachment 8797881 [details]
id:000698,src:000108+000246,op:splice,rep:2,+cov

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

The issue reported here was found by creating a corpus of malformed input with AFL and then running the input through a libvorbis example app with UBSAN sanitization enabled.

Here are steps to reproduce the test harness:
1) Download libvorbis-1.3.5.tar.gz from http://downloads.xiph.org/releases/vorbis/libvorbis-1.3.5.tar.gz
2) Extract and build the code using clang with UBSAN support (i.e. CFLAGS="-fsanitize=undefined -fno-omit-frame-pointer -g")
3) Make the examples (i.e. cd examples && make)
4) Run the attached AFL produced test case through the decoder sample (i.e. cat $FILE | decoder_example)

To be clear, I have not verified this problem with a proper Firefox build, but I believe the affected code is similar enough that FF's custom version of libVorbis could be affected as well.

The vulnerable line appears in the gecko-dev git here: https://github.com/mozilla/gecko-dev/blob/233f30f2377f3df0f3388721901681f432b813fb/media/libtremor/lib/tremor_block.c#L442


Actual results:

UBSAN produces some runtime errors including the following signed integer overflow:
block.c:902:33: runtime error: signed integer overflow: 32896 - -9223372036854743040 cannot be represented in type 'long'

(NOTE: block.c:902 in my source lines up with tremor_block.c:442 as shown in the gecko-dev git here: https://github.com/mozilla/gecko-dev/blob/233f30f2377f3df0f3388721901681f432b813fb/media/libtremor/lib/tremor_block.c#L442)

Note that this happens after several errors about left shift operations which cannot be represented as int values.


Expected results:

Values should not overflow their data type permissible ranges.

Comment 1

a year ago
:jya, can you take a look?
Group: firefox-core-security → core-security
Component: Untriaged → Audio/Video
Flags: needinfo?(jyavenard)
Product: Firefox → Core
Isn't that the same as bug 1306476?
Flags: needinfo?(jyavenard) → needinfo?(tterribe)
See Also: → bug 1306476
(Reporter)

Comment 3

a year ago
No, it is not the same bug.  1306476 is an integer overflow in sharedbook.c -- this one is in block.c
This is an overflow in comparing the difference between two timestamps. If the concern is that this is technically undefined behavior, okay, but I don't think this is a security bug. I doubt any compiler would compile this in a way to produce anything other than the obvious behavior. The result is carefully checked against crazy values.

It *is* possible to sanitize timestamp handling to avoid any undefined behavior (see op_granpos_diff() at <https://git.xiph.org/?p=opusfile.git;a=blob;f=src/opusfile.c;h=7a697be32556#l665> for an example). Do people think that is worth doing here?
Flags: needinfo?(tterribe)
At least it would prevent future reports like this.

I too do think that neither this bug nor 1306476 are security bugs
Group: core-security
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> At least it would prevent future reports like this.

Okay. If you want to have a go at adapting the code linked above, I'm happy to review.
Component: Audio/Video → Audio/Video: Playback
Signed integer overflow is well defined behaviour so I call this a false positive.
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.