Open
Bug 1307670
Opened 8 years ago
Updated 2 years ago
libVorbis Undefined Behavior (block.c)
Categories
(Core :: Audio/Video: Playback, defect, P5)
Core
Audio/Video: Playback
Tracking
()
UNCONFIRMED
People
(Reporter: vuln-report, Unassigned)
References
Details
Attachments
(1 file)
12.01 KB,
audio/x-vorbis+ogg
|
Details |
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•8 years ago
|
||
:jya, can you take a look?
Group: firefox-core-security → core-security
Component: Untriaged → Audio/Video
Flags: needinfo?(jyavenard)
Product: Firefox → Core
Comment 2•8 years ago
|
||
Isn't that the same as bug 1306476?
Flags: needinfo?(jyavenard) → needinfo?(tterribe)
Reporter | ||
Comment 3•8 years ago
|
||
No, it is not the same bug. 1306476 is an integer overflow in sharedbook.c -- this one is in block.c
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
At least it would prevent future reports like this. I too do think that neither this bug nor 1306476 are security bugs
Updated•8 years ago
|
Group: core-security
Comment 6•8 years ago
|
||
(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.
Updated•8 years ago
|
Component: Audio/Video → Audio/Video: Playback
Signed integer overflow is well defined behaviour so I call this a false positive.
Priority: -- → P5
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•