1010163 - Heap-buffer-overflow in mozilla::WaveReader::LoadListChunk

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Holger Fuhrmannek]

Attachment: testcase.html

The testcase file crashes the current 32bit firefox version.
The embedded wave file has a list chunk where the length field of an entry is set to 6FFFFFFF which causes a overflow.

Stack:

#0  __memcpy_ssse3_rep ()
    at ../sysdeps/i386/i686/multiarch/memcpy-ssse3-rep.S:1290
#1  0xb1aec3a0 in nsCharTraits<char>::copy (
    s1=0x20000008 "cddd", 'Z' <repeats 196 times>..., s2=0xa813eaec "cddd", 
    n=1879048191) at ../../../dist/include/nsCharTraits.h:387
#2  0xb1aea136 in nsACString_internal::Assign (this=0xa9214ec4, 
    data=0xa813eaec "cddd", length=1879048191)
    at /home/ash/Desktop/ffbuild/mozilla-central/xpcom/string/src/nsTSubstring.cpp:325
#3  0xb1aea02a in nsACString_internal::Assign (this=0xa9214ec4, 
    data=0xa813eaec "cddd", length=1879048191)
    at /home/ash/Desktop/ffbuild/mozilla-central/xpcom/string/src/nsTSubstring.cpp:301
#4  0xb1ac899b in nsCString::nsCString (this=0xa9214ec4, 
    data=0xa813eaec "cddd", length=1879048191)
    at ../../dist/include/nsTString.h:39
#5  0xb3764b94 in mozilla::WaveReader::LoadListChunk (this=0x9775daa0, 
    aChunkSize=16, aTags=...)
    at /home/ash/Desktop/ffbuild/mozilla-central/content/media/wave/WaveReader.cpp:586
#6  0xb3764dee in mozilla::WaveReader::LoadAllChunks (this=0x9775daa0, 
    aTags=...)
    at /home/ash/Desktop/ffbuild/mozilla-central/content/media/wave/WaveReader.cpp:647
#7  0xb37638ed in mozilla::WaveReader::ReadMetadata (this=0x9775daa0, 
    aInfo=0xa9215074, aTags=0xa9215054)
    at /home/ash/Desktop/ffbuild/mozilla-central/content/media/wave/WaveReader.cpp:140

Comment 1

[Daniel Veditz [:dveditz]]

Is this reading off the end, or writing? Our string classes should be smart enough to allocate enough space for what it's trying to assign.

Matt's going to try an ASAN build and see what it says.

Comment 2

[Matt Wobensmith [:mwobensmith][:matt:]]

This is a 32-bit build on a 64-bit Linux OS. Unfortunately, I'm having a hard time cross-compiling an ASan build to test this. I'll keep trying.

Can you try this on one of our 64-bit ASan builds?

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/latest/

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: bug1010163_v0.patch

It's most likely reading off the end of the source, since the length check will overflow with 32-bit pointers:

    if (p + length > end) {

Fix attached that uses:

    if (end - p < length) {

I don't have a 32-bit build handy to test this with.

Comment 4

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de0e14a42e47

Comment 5

[Matthew Gregan [:kinetik]]

And a bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72230d707f86

The cast is safe here because the difference between end and p can never be larger than MAX_CHUNK_SIZE, there's a static assert that proves MAX_CHUNK_SIZE is never larger than UINT_MAX, and end can't be smaller than p because that would've caused the allocation of chunk (from which p is derived) to fail.

Comment 6

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/de0e14a42e47
http://hg.mozilla.org/mozilla-central/rev/72230d707f86

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Was this trunk only? Otherwise, we need a rating and may need branch patches (and this should have gotten sec-approval+ to go in).

Comment 8

[Matthew Gregan [:kinetik]]

Sorry, I forgot.  :-(

The busted code has existed for a long time, introduced into Firefox 20 by bug 778053.  So we need to fix this on the branches.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: bug1010163_v1.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? fairly trivial

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? all

If not all supported branches, which bug introduced the flaw? bug 778053

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? trivial

How likely is this patch to cause regressions; how much testing does it need? little to none

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Do you have a suggestion for a rating? How bad is the problem if exploited?

Comment 11

[Daniel Veditz [:dveditz]]

In theory if you didn't crash because you read into some other process's memory then random heap is now incorporated into your wave data and you might be able to recover the data from the audio API. If we successfully read the data then we've created a CString which will safely hold whatever size it's asked to. But the reason this overflows is because the pointer size is only 32-bits so if p+length overflows such that you bypass the check and read too much when you create the CString starting at p and reading to p+length you will also overflow the pointer, and before you get to the end of the machine's memory you will get to the end of the heap if by some miracle your process's memory filled the heap.

I can't see any way that triggering this bug doesn't crash the process before any data can be read, in other words not exploitable. 

Holger: do you want to argue to the contrary before I un-hide this bug?

Comment 12

[Matthew Gregan [:kinetik]]

(In reply to Daniel Veditz [:dveditz] from comment #11)
> I can't see any way that triggering this bug doesn't crash the process
> before any data can be read, in other words not exploitable. 

That's the same conclusion I came to.

Comment 13

[Holger Fuhrmannek]

I'm not sure, wheather the bug is not exploitable:

- if the length is 0xFFFFFFFF the length of the nsCString is automatically determinated as the size between the start and the first null byte. So the interesting data have to be after the allocated chunk, with a terminating nullbyte and later the data have to pass the IsUTF8 check.

- if then val[0xFFFFFFFE] == '\0' is false (can this be possible?), val.SetLength(0xFFFFFFFE) is not triggered which would also crash (which happens in my test with length = 0xFFFFFFFF)

- at the start of the next loop: p = chunkStart + 12 (because the reads of magic,id and length); if chunksize = 16: p + 8 < end is false so the loop ends without a crash

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8425958 [details] [diff] [review]
bug1010163_v1.patch

Clearing sec-approval? since it looks like this isn't a real security issue.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Marking ESR24 and won't fix based on the fact that this isn't a security issue and I'm not aware of any reports of a related crash on the enterprise channel.