881066 - heap-buffer-overflow (read) at mozilla::(anonymous namespace)::ReadUint8

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Aki Helin]

Attachment: repro

Opening the attached page causes ASan build of Firefox to report an out-of-bounds read. This happens at least in aurora built today and beta built a few days ago. The read happens near the site of Bug 879924 but with a different trace. ASan typically catches segfaults caused by buffer overflows at the time of the overflow, so filing this separately in case it is a distinct issue.

$ opt/firefox-aurora-asan/firefox ff-bofr-readuint8.html 2>&1 | symbolize | c++filt
=================================================================
==15293== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f2a97ee6c81 at pc 0x7f2ab53bf36c bp 0x7f2a8559b4f0 sp 0x7f2a8559b4e8
READ of size 1 at 0x7f2a97ee6c81 thread T22
    #0 0x7f2ab53bf36b in mozilla::(anonymous namespace)::ReadUint8(char const**) /home/aki/src/mozilla-aurora/content/media/wave/WaveReader.cpp:111
0x7f2a97ee6c81 is located 0 bytes to the right of 1-byte region [0x7f2a97ee6c80,0x7f2a97ee6c81)
allocated by thread T22 here:
    #0 0x43db50 in __interceptor_malloc ??:0
    #1 0x7f2abb90f54b in moz_xmalloc /home/aki/src/mozilla-aurora/memory/mozalloc/mozalloc.cpp:54
Thread T22 created by T21 here:
    #0 0x439d84 in pthread_create ??:0
    #1 0x7f2abd4ac176 in _PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:444
    #2 0x7f2abd4abc77 in PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:527
Thread T21 created by T0 here:
    #0 0x439d84 in pthread_create ??:0
    #1 0x7f2abd4ac176 in _PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:444
    #2 0x7f2abd4abc77 in PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:527
Shadow byte and word:
  0x1fe552fdcd90: 1
  0x1fe552fdcd90: 01 fb fb fb fb fb fb fb
More shadow bytes:
  0x1fe552fdcd70: 00 fb fb fb fb fb fb fb
  0x1fe552fdcd78: fb fb fb fb fb fb fb fb
  0x1fe552fdcd80: fa fa fa fa fa fa fa fa
  0x1fe552fdcd88: fa fa fa fa fa fa fa fa
=>0x1fe552fdcd90: 01 fb fb fb fb fb fb fb
  0x1fe552fdcd98: fb fb fb fb fb fb fb fb
  0x1fe552fdcda0: fa fa fa fa fa fa fa fa
  0x1fe552fdcda8: fa fa fa fa fa fa fa fa
  0x1fe552fdcdb0: fa fa fa fa fa fa fa fa
[...]

Comment 1

[Aki Helin]

Seems to be distinct from Bug 879924, because this reproduces after applying the patch for it.

Comment 2

[Ralph Giles (:rillian)]

We're not correctly validating the header correctly. It claims to be 8-bit stereo with 1-byte frames. We believe the 1-byte part, call ReadAll to fill the buffer with 1 byte of data, and then try to read two bytes of sample data from it.

Comment 3

[Daniel Veditz [:dveditz]]

do we have two bytes of space to put them in? If so then this would just be a potential DoS crash.

Could there be other frame sizes where we might read more data? If we didn't crash on an access violation reading bogus data what would we do with it?

Comment 4

[Ralph Giles (:rillian)]

We multiply by the number of channels before allocating the destination buffer, so we have allocated space to store the bogus read result. If we don't crash it could be returned to web content.

I don't think we can read more than one byte. The loop is over frames and then channels. We'll reject the file in the fmt chunk validation if there are more than two channels, or if the file lies about the framesize with a value different from 1. For values greater than one, the division in the calculation of the 'frames' loop bound will take the floor correctly.

Comment 6

[Ralph Giles (:rillian)]

This was resolved by the fix for bug 785967. We instead reject the file when we parse the format chunk. I can no longer reproduce with an asan build.

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Ralph Giles (:rillian) from comment #6)
> This was resolved by the fix for bug 785967.

How is that possible? That bug was fixed in Sept 2012 and this one wasn't filed until nine months later in 2013.

Comment 8

[Ralph Giles (:rillian)]

Sorry, I must have meant bug 966311.

I get an asan crash  the asan report in ReadUint8 called from DecodeAudioData with the attached testcase in the commit immediately prior to that fix landing, and I cannot reproduce once the patch is applied.