1306476 - Signed Integer Overflow in libvorbis

Status: RESOLVED FIXED

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

Description

[Craig Young]

Attachment: Test case produced by AFL that triggers integer overflow

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

Steps to reproduce:

It is my understanding that Firefox is dependent on libvorbis (at least under Linux) so I decided to conduct some testing on the library using AFL and UBSAN.  Here are steps to reproduce a bug I found via UBSAN:

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)


Actual results:

The output of this run should include UBSAN reports similar to the following:
bitwise.c:400:25: runtime error: left shift of 28 by 27 places cannot be represented in type 'int'
    #0 0x7fef958c2d99 in oggpack_read /home/spotless/fuzzing/media/vorbis/libogg-1.3.2/src/bitwise.c:400:16
    #1 0x7fef95f68562 in vorbis_staticbook_unpack /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/codebook.c:233:14
    #2 0x7fef95f0440a in _vorbis_unpack_books /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/info.c:270:23
    #3 0x7fef95f0440a in vorbis_synthesis_headerin /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/info.c:424
    #4 0x42e5b9 in main /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/examples/decoder_example.c:166:20
    #5 0x7fef94c9ca3f in __libc_start_main /build/buildd/glibc-2.21/csu/libc-start.c:289
    #6 0x4169c8 in _start (/home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/examples/.libs/lt-decoder_example+0x4169c8)

sharedbook.c:174:11: runtime error: signed integer overflow: 4611686018427387904 * 2 cannot be represented in type 'long'
    #0 0x7fef95f72a3e in _book_maptype1_quantvals /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/sharedbook.c:174:7
    #1 0x7fef95f68a44 in vorbis_staticbook_unpack /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/codebook.c:243:32
    #2 0x7fef95f0440a in _vorbis_unpack_books /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/info.c:270:23
    #3 0x7fef95f0440a in vorbis_synthesis_headerin /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/info.c:424
    #4 0x42e5b9 in main /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/examples/decoder_example.c:166:20
    #5 0x7fef94c9ca3f in __libc_start_main /build/buildd/glibc-2.21/csu/libc-start.c:289
    #6 0x4169c8 in _start (/home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/examples/.libs/lt-decoder_example+0x4169c8)

sharedbook.c:173:10: runtime error: signed integer overflow: 4611686018427387904 * 2 cannot be represented in type 'long'
    #0 0x7fef95f729c0 in _book_maptype1_quantvals /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/sharedbook.c:173:7
    #1 0x7fef95f68a44 in vorbis_staticbook_unpack /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/codebook.c:243:32
    #2 0x7fef95f0440a in _vorbis_unpack_books /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/info.c:270:23
    #3 0x7fef95f0440a in vorbis_synthesis_headerin /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/lib/info.c:424
    #4 0x42e5b9 in main /home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/examples/decoder_example.c:166:20
    #5 0x7fef94c9ca3f in __libc_start_main /build/buildd/glibc-2.21/csu/libc-start.c:289
    #6 0x4169c8 in _start (/home/spotless/fuzzing/media/vorbis/libvorbis-1.3.5/examples/.libs/lt-decoder_example+0x4169c8)

NOTE: I am uncertain if this is exploitable but as far as I can tell, it doesn't appear to be handled within the code.


Expected results:

The test program should exit without UBSAN reporting errors.

Comment 1

[Jean-Yves Avenard [:jya]]

none of those files exist in our libvorbis code.

is it a problem you actually reproduced using firefox, or just the vorbis example code?

Comment 2

[Jean-Yves Avenard [:jya]]

We're not using libvorbis under linux either.. 
We use our own copy of libvorbis.

Comment 3

[Craig Young]

I did not test it on Firefox because I have not made a UBSAN build of it, but I think the code in question is this line: https://github.com/mozilla/gecko-dev/blob/233f30f2377f3df0f3388721901681f432b813fb/media/libvorbis/lib/vorbis_sharedbook.c#L174

Comment 4

[Jean-Yves Avenard [:jya]]

I see.. thanks for pointing that out.

I don't see how this is a security issue, at worse it will sound bad. There's no memory being accessed here.

Comment 5

[Craig Young]

I was actually wondering if there might be a possibility of influencing the memory allocation here: https://github.com/mozilla/gecko-dev/blob/233f30f2377f3df0f3388721901681f432b813fb/media/libvorbis/lib/vorbis_codebook.c#L253

Comment 6

[Craig Young]

Looking down that code path further, this test case does cause a smaller than intended allocation, but it also appears that you cannot overflow the quantlist on line 255 since the loop is safely bounded by quantvals. The codebook is being returned however with the small quantlist but I have yet to determine if it might be possible to continue on to do something bad with that short buffer.

Comment 7

[Timothy B. Terriberry (:derf)]

I believe the only way this can be triggered is if b->dims is at least 63 and b->entries > 0, forcing vals to be 1 (which is the correct return value of the function). But at this point (vals+1)**63 overflows a long. In that case, acc1 will be less than b->entries (either LONG_MIN or 0), and it will decide to increment vals. This process repeats until vals**b->dim (mod 2**64) is no greater than b->entries and (vals+1)**b->dim (mod 2**64) is greater than b->entries. In this test case that happens on the very next value (vals==2). So the allocation is actually larger than necessary (I don't believe it can ever be smaller). In this testcase, it then reads too much out of the packet, fails to find the syncword for the next codebook, and errors out.

Comment 8

[Timothy B. Terriberry (:derf)]

Attachment: 0001-Fix-signed-overflow-in-_book_maptype1_quantvals.patch

Here is a proposed fix for upstream (with a bit more extra paranoia about the possible results of the floating pointer operations than is probably necessary from my previous comment). Monty, can you take a look? If it looks reasonable, we can make a patch for m-c out of it.

Comment 9

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

Assigning to Timothy, so it doesn't look like nobody is working on it.
(Please reassign if that's incorrect)

Comment 11

[Monty Montgomery (:xiphmont)]

OK, if I read this correctly, the overflow possibility in acc1 is properly accounted for in the line

+      if(LONG_MAX/(vals+1)<acc1)acc1=LONG_MAX;

...and the overflow possibility in acc is handled by the break statement:

+      if(b->entries/vals<acc)break;

The fact that acc1 overflow prevention does not trigger a break is the remaining concern.  It will saturate to acc1 LONG_MAX until the inner for loop terminates when i==b->dim, and the vals of 1 will be returned.  So that's good too,

Logic all looks good to me.  I assume this was tried with the AFL test case already?

Comment 12

[Timothy B. Terriberry (:derf)]

> I assume this was tried with the AFL test case already?

Yes.

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8797328 [details] [diff] [review]
0001-Fix-signed-overflow-in-_book_maptype1_quantvals.patch

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

Propagating r=xiphmont from IRC.

Comment 14

[Ralph Giles (:rillian)]

Merged upstream as https://git.xiph.org/?p=vorbis.git;a=commitdiff;h=e5b1378996dba3ea82fb35403cb1f0bbff19495c

Comment 15

[Ralph Giles (:rillian)]

Attachment: Bug 1306476 - Add testcase as a crashtest.

I don't know if we run the crashtest under ubsan, but this at
least captures the trigging data stream.

Review commit: https://reviewboard.mozilla.org/r/149954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149954/

Comment 16

[Ralph Giles (:rillian)]

Attachment: Bug 1306476 - Update libvorbis.

Update to the latest upstream master of libvorbis (v1.3.5-48-ge5b1378)
to pick up various fixes.

Review commit: https://reviewboard.mozilla.org/r/149956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149956/

Comment 17

[Monty Montgomery (:xiphmont)]

since I wasn't clear and derf asked, confirmed r=xiphmont

Comment 18

[Timothy B. Terriberry (:derf)]

Comment on attachment 8878662 [details]
Bug 1306476 - Update libvorbis.

https://reviewboard.mozilla.org/r/149956/#review154678

Stealing review since it's the weekend in NZ.

Comment 19

[Timothy B. Terriberry (:derf)]

Comment on attachment 8878661 [details]
Bug 1306476 - Add testcase as a crashtest.

https://reviewboard.mozilla.org/r/149954/#review154706

Stealing review since it's the weekend in NZ.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17d4bad7f9fb
Add testcase as a crashtest. r=derf
https://hg.mozilla.org/integration/autoland/rev/6ac27d7a3fe4
Update libvorbis. r=derf

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/17d4bad7f9fb
https://hg.mozilla.org/mozilla-central/rev/6ac27d7a3fe4