879924 - (CVE-2013-1708) Non-null crash at nsCString::CharAt

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Aki Helin]

Opening the attached page with broken WAV data causes Firefox release-, beta- and aurora channels to crash at least on Linux (64-bit Debian wheezy) due to an invalid but heapish address. Filed promptly without much further research since this looks like a potential security issue in release version.

$ opt/firefox-aurora-asan/firefox ff-crash-nsCString.html 2>&1 | symbolize | c++filt
ASAN:SIGSEGV
=================================================================
==3779== ERROR: AddressSanitizer crashed on unknown address 0x7f69c31e8b87 (pc 0x7f68e00348e8 sp 0x7f68b04fb160 bp 0x7f68b04fb3f0 T20)
AddressSanitizer can not provide additional info.
    #0 0x7f68e00348e7 in nsCString::CharAt(unsigned int) const /home/aki/src/mozilla-aurora/../../../dist/include/nsTString.h:90
    #1 0x7f68e0032102 in mozilla::WaveReader::LoadAllChunks(nsAutoPtr<nsDataHashtable<nsCStringHashKey, nsCString> >&) /home/aki/src/mozilla-aurora/content/media/wave/WaveReader.cpp:642
    #2 0x7f68e0031616 in mozilla::WaveReader::ReadMetadata(mozilla::VideoInfo*, nsDataHashtable<nsCStringHashKey, nsCString>**) /home/aki/src/mozilla-aurora/content/media/wave/WaveReader.cpp:145
    #3 0x7f68df7266af in mozilla::MediaDecoderStateMachine::DecodeMetadata() /home/aki/src/mozilla-aurora/content/media/MediaDecoderStateMachine.cpp:1816
    #4 0x7f68df72627f in mozilla::MediaDecoderStateMachine::DecodeThreadRun() /home/aki/src/mozilla-aurora/content/media/MediaDecoderStateMachine.cpp:498
Thread T20 created by T19 here:
    #0 0x439d84 in pthread_create ??:0
    #1 0x7f68e8120176 in _PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:444
    #2 0x7f68e811fc77 in PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:527
Thread T19 created by T0 here:
    #0 0x439d84 in pthread_create ??:0
    #1 0x7f68e8120176 in _PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:444
    #2 0x7f68e811fc77 in PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:527
Stats: 247M malloced (251M for red zones) by 343876 calls
Stats: 50M realloced by 16371 calls
Stats: 215M freed by 207394 calls
Stats: 78M really freed by 149442 calls
Stats: 440M (112721 full pages) mmaped in 110 calls
  mmaps   by size class: 8:229362; 9:32764; 10:12285; 11:16376; 12:2048; 13:1536; 14:1280; 15:384; 16:704; 17:1344; 18:48; 19:40; 20:24; 21:2;
  mallocs by size class: 8:259684; 9:42719; 10:11536; 11:21111; 12:2363; 13:1984; 14:1570; 15:454; 16:896; 17:1423; 18:70; 19:42; 20:22; 21:2;
  frees   by size class: 8:141180; 9:31851; 10:8122; 11:19392; 12:1538; 13:1339; 14:1380; 15:326; 16:744; 17:1401; 18:61; 19:38; 20:21; 21:1;
  rfrees  by size class: 8:110450; 9:17331; 10:5579; 11:13200; 12:779; 13:636; 14:452; 15:202; 16:531; 17:248; 18:29; 19:3; 20:1; 21:1;
Stats: malloc large: 1559 small slow: 2115
==3779== ABORTING

Comment 1

[Aki Helin]

Attachment: repro

adding repro again, because bugzilla eated the first one

Comment 3

[Bob Clary [:bc] (inactive)]

debug nightly shows

###!!! ASSERTION: index exceeds allowable range: 'i <= mLength', file ../../dist/include/nsTString.h, line 89

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffdcf2c700 (LWP 28816)]
0x00007ffff1633a37 in nsCString::CharAt (this=0x7fffdcf2b960, i=4294967295) at ../../dist/include/nsTString.h:90
90	          return mData[i];

Comment 4

[Ralph Giles (:rillian)]

Attachment: fix

We were trying check for null-termination of a zero-length string, reading off the beginning of the array.

Comment 5

[Timothy B. Terriberry (:derf)]

Comment on attachment 760656 [details] [diff] [review]
fix

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

LGTM.

Comment 6

[Aki Helin]

Patch looks good here against the attached repro and others.

Comment 7

[Ralph Giles (:rillian)]

Thanks. Adding tracking flags.

The original code landed as part of Bug 778053.

Comment 8

[Ralph Giles (:rillian)]

This is a pretty guaranteed crash, but looks hard to exploit beyond denial of service.

The problem is we index an nsCString at offset ((uint32_t)0 - 1), or 4GB. That is very likely to segfault. If it doesn't segfault and the value is non-zero we do nothing. If it is zero, we call the SetLength((uint32_t)-1) which would fail to resize the string to beyond 2GB with the check on https://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsTSubstring.cpp#56 which calls NS_RUNTIMEABORT("OOM");

So we leak whether a byte at a 4GB offset is either unmapped, or mapped and zero through the browser crashing. That's a pretty slow attack vector.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 760656 [details] [diff] [review]
fix

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

It's a trivial crash from web content; see the repro attachment. It looks hard to exploit as an invalid read. See comment above for analysis.

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

I tried to be circumspect in the commit message. I mention shortening strings, but not the crash.

> Which older supported branches are affected by this flaw?

Aurora, Beta and Release are all affected. b2g18 and esr17 are not.

> 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?

The patch should apply to all affected branches; the code hasn't been touched since initial landing.

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

Regression risk is low; the patch adds an additional guard to code of limited scope. Wave playback isn't a widely used feature because of the size of the associated files, and they must be corrupt to trigger the affected code.

I would like to add a regression test for this and related bounds-checking issues, to land on nightly only.

Comment 10

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

Comment on attachment 760656 [details] [diff] [review]
fix

sec-approval+ for trunk. Please check it in and nominate for branches. Beta patches need to go in *today* to make cut off for Firefox 22.

Comment 11

[Ralph Giles (:rillian)]

Thanks. I'll check in for trunk as soon as m-i reopens.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 760656 [details] [diff] [review]
fix

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

User impact if declined: Corrupt or malicious content can crash the browser.

Testing completed (on m-c, etc.): Tested locally; landing on m-c asap.

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

Risk is low. One line fix to wave file metadata parsing. Wave file playback is not widely used, and valid files are handled identically.

String or IDL/UUID changes made by this patch: None.

Comment 13

[Ralph Giles (:rillian)]

Pushed directly to m-c for quicker integration feedback.

https://hg.mozilla.org/mozilla-central/rev/291207393608

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 760656 [details] [diff] [review]
fix

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

User impact if declined: Corrupt or malicious content can crash the browser.

Testing completed (on m-c, etc.): Tested locally. Landed on m-c today.

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

Risk is low. One line guard added to WAVE metadata parsing. Wave playback is a little used feature and there is no change to valid file handling.

String or IDL/UUID changes made by this patch: None.

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 760656 [details] [diff] [review]
fix

[Approval Request Comment]
Regression caused by (bug #): Bug 778053

User impact if declined: Corrupt or malicious content could crash the browser.

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

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

Risk is low. One line fix to wave file metadata handling. Wave file playback is not widely used and there is no change to valid file handling.

The alternative is to not fix the issue, leaving the crash vulnerability in the current release.

String or IDL/UUID changes made by this patch:

Comment 16

[Ryan VanderMeulen [:RyanVM]]

landed on m-c = fixed, btw

Comment 17

[Alex Keybl [:akeybl]]

(In reply to Ralph Giles (:rillian) from comment #15)
> Comment on attachment 760656 [details] [diff] [review]
> fix
> 
> [Approval Request Comment]
> Regression caused by (bug #): Bug 778053
> 
> User impact if declined: Corrupt or malicious content could crash the
> browser.

In an exploitable fashion? Is this sec-dos, sec-critical, ..?

Comment 18

[Aki Helin]

This seems to be a sec-dos. See comment 8. The ASan error report suggested an integer error, which are usually sec-critical, but this one lacks control and is safe even if it hits heap.

Comment 19

[Ralph Giles (:rillian)]

Yes. There's a very slow read-only channel, but you have to crash the browser for each bit, so DOS is the significant attack.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7cae220f4d2f

Comment 21

[Ralph Giles (:rillian)]

Thanks Alex, Ryan.