Closed Bug 966311 (CVE-2014-1497) Opened 10 years ago Closed 10 years ago

Out-of-bounds read in mozilla::WaveReader::DecodeAudioData()

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla30
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 + verified
firefox29 + verified
firefox30 + verified
firefox-esr24 + verified
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: attekett, Assigned: rillian)

Details

(4 keywords, Whiteboard: [adv-main28+][adv-esr24.4+])

Attachments

(2 files)

repro-file
172.45 KB, audio/wav
Details
proposed fix
1.05 KB, patch
cajbir
: review+
abillings
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Sylvestre
: approval-mozilla-esr24+
abillings
: sec-approval+
Details | Diff | Splinter Review
Attached audio repro-file — 
Tested on:

OS: Ubuntu  12.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1391177067/

Repro-file as attachment. You might need to load the wav-file with a small html wrapper like this:

<html>
<audio src="./repro-file.wav">
</html>

ASAN-report:

==20648==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6210005bdd00 at pc 0x7f84654da535 bp 0x7f843c70dc40 sp 0x7f843c70dc38
READ of size 1 at 0x6210005bdd00 thread T37 (Media Decode)
    #0 0x7f84654da534 in mozilla::(anonymous namespace)::ReadUint8(char const**) /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/wave/WaveReader.cpp:106:0
    #1 0x7f84654d9f0b in mozilla::WaveReader::DecodeAudioData() /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/wave/WaveReader.cpp:219:0
    #2 0x7f84653d35d2 in mozilla::MediaDecoderReader::DecodeToFirstAudioData() /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/MediaDecoderReader.cpp:449:0
    #3 0x7f84653d3a91 in mozilla::MediaDecoderReader::FindStartTime(long&) /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/MediaDecoderReader.cpp:474:0
    #4 0x7f84653f452f in mozilla::MediaDecoderStateMachine::FindStartTime() /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/MediaDecoderStateMachine.cpp:2653:0
    #5 0x7f84653e6e56 in mozilla::MediaDecoderStateMachine::DecodeMetadata() /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/MediaDecoderStateMachine.cpp:1949:0
    #6 0x7f84653e64c4 in mozilla::MediaDecoderStateMachine::DecodeThreadRun() /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/MediaDecoderStateMachine.cpp:508:0
    .
    .
    .
0x6210005bdd00 is located 0 bytes to the right of 4096-byte region [0x6210005bcd00,0x6210005bdd00)
allocated by thread T37 (Media Decode) here:
    #0 0x4463c5 in malloc _asan_rtl_:0
    #1 0x7f846c8d876d in moz_xmalloc /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/memory/mozalloc/mozalloc.cpp:52:0
    #2 0x7f84654d9dd7 in operator new[](unsigned long) /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/obj-firefox/content/media/wave/../../../dist/include/mozilla/mozalloc.h:213:0
    #3 0x7f84654d9dd7 in mozilla::WaveReader::DecodeAudioData() /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/wave/WaveReader.cpp:207:0
    #4 0x7f84653d35d2 in mozilla::MediaDecoderReader::DecodeToFirstAudioData() /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/MediaDecoderReader.cpp:449:0
    #5 0x7f84653d3a91 in mozilla::MediaDecoderReader::FindStartTime(long&) /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/content/media/MediaDecoderReader.cpp:474:0
    .
    .
    .
This code is really wrong. :(

Working on a fix.
Attached patch proposed fix — — Splinter Review 
Proposed fix.

This looks like a precedence error, but I vaguely remember thinking 8-bit wav had to be mono, so it's possible this will break more file than enforcing that would have. OTOH this looks correct by http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/WAVE.html
Assignee: nobody → giles
Attachment #8368842 - Flags: review?(chris.double)
Paul, feel free to steal the review if you can get to it before Sunday.
Attachment #8368842 - Flags: review?(chris.double) → review+
Comment on attachment 8368842 [details] [diff] [review]
proposed fix

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

I wouldn't say it's obvious, but it does point to what field to mess with to get past the header validation.

This is 4k invalid read with heap data accessible to content if it's doesn't crash.

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

No. I only mention the stereo size calculation being wrong.

> Which older supported branches are affected by this flaw?

All of them, I think. The code has been wrong since August 2012.

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

The same patch will apply to older branches.

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

We now reject more invalid files. Ideally I'd like a few weeks of testing on Nightly/Aurora before landing everywhere, but I think the risk of regressions is low.
Attachment #8368842 - Flags: sec-approval?
Comment on attachment 8368842 [details] [diff] [review]
proposed fix

sec-approval+ for trunk. Please create and nominate branch patches when you're ready for it to go in there.
Attachment #8368842 - Flags: sec-approval? → sec-approval+
landed on central https://hg.mozilla.org/mozilla-central/rev/b3579b7bd49e
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox30: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
This backports trivially to all affected branches. Ralph, please request approval for Aurora, Beta, and ESR24 (the B2G backports have auto-approval since this is sec-high).
Flags: needinfo?(giles)
Comment on attachment 8368842 [details] [diff] [review]
proposed fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):

Wave playback feature.

User impact if declined:

Crafted web content can crash browser or obtain heap data.

Testing completed (on m-c, etc.):

Landed on m-c.

Risk to taking this patch (and alternatives if risky):

We reject more invalid wave files than we do without this patch. While it's possible this will affect currently "working" sites with such invalid files, the risk of this is low.

String or IDL/UUID changes made by this patch:

None.
Attachment #8368842 - Flags: approval-mozilla-aurora?
Flags: needinfo?(giles)
Comment on attachment 8368842 [details] [diff] [review]
proposed fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):

Wave file decoding/playback.

User impact if declined:

Crafted web content can crash browser or obtain heap data.

Testing completed (on m-c, etc.): 

Landed on m-c.

Risk to taking this patch (and alternatives if risky):


We reject invalid audio files we previously tried to play. It's possible this will affect currently "working" sites, but the risk is low.

String or IDL/UUID changes made by this patch:

None.
Attachment #8368842 - Flags: approval-mozilla-beta?
Comment on attachment 8368842 [details] [diff] [review]
proposed fix

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 

Bug is sec-high.

> Fix Landed on Version:

Firefox 30.

> Risk to taking this patch (and alternatives if risky): 

We reject invalid audio files we previously tried to play. It's possible this will affect currently "working" sites, but the risk is low.

> String or UUID changes made by this patch: 

none

> See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8368842 - Flags: approval-mozilla-esr24?
Comment on attachment 8368842 [details] [diff] [review]
proposed fix

Approving for Aurora but the note was about wanting some testing there before adding it to Beta or ESR24.
Attachment #8368842 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8368842 - Flags: approval-mozilla-esr24?
Attachment #8368842 - Flags: approval-mozilla-esr24+
Attachment #8368842 - Flags: approval-mozilla-beta?
Attachment #8368842 - Flags: approval-mozilla-beta+
No time like the present, I guess. Thanks, Ryan, for handling the landings.
Is this really sec-high? Worst case you've incorporated some heap data into the wave reader. Can this actually be recovered and interpreted by a malicious page?
Flags: needinfo?(attekett)
The Web Audio API lets any page read decoded audio data, so yes.
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(attekett)
Whiteboard: [adv-main28+][adv-esr24.4+]
Alias: CVE-2014-1497
Confirmed crash on Fx27 release.
Verified fixed on Fx28, 2014-03-13.
Verified fixed on Fx24esrpre, 2014-02-26.
status-firefox28: fixed → verified
status-firefox-esr24: fixed → verified
Verified fixed on Fx29, 2014-03-13.
Verified fixed on Fx30, 2014-03-13.
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Summary: Heap-buffer-overflow in mozilla::WaveReader::DecodeAudioData() → Out-of-bounds read in mozilla::WaveReader::DecodeAudioData()
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: