The default bug view has changed. See this FAQ.
Bug 879924 (CVE-2013-1708)

Non-null crash at nsCString::CharAt

RESOLVED FIXED in Firefox 23

Status

()

Core
Audio/Video
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Aki Helin, Assigned: rillian)

Tracking

({sec-low})

23 Branch
mozilla24
x86_64
Linux
sec-low
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox21- wontfix, firefox22- wontfix, firefox23- fixed, firefox24- fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [asan][adv-main23+], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Created attachment 758723 [details]
repro

adding repro again, because bugzilla eated the first one

Updated

4 years ago
Duplicate of this bug: 879923

Comment 3

4 years ago
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];
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsCString::CharAt() ]
Ever confirmed: true
Created attachment 760656 [details] [diff] [review]
fix

We were trying check for null-termination of a zero-length string, reading off the beginning of the array.
Assignee: nobody → giles
Status: NEW → ASSIGNED
Attachment #760656 - Flags: review?(tterribe)
Comment on attachment 760656 [details] [diff] [review]
fix

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

LGTM.
Attachment #760656 - Flags: review?(tterribe) → review+
(Reporter)

Comment 6

4 years ago
Patch looks good here against the attached repro and others.
Thanks. Adding tracking flags.

The original code landed as part of Bug 778053.
status-b2g18: --- → unaffected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
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 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.
Attachment #760656 - Flags: sec-approval?
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.
Attachment #760656 - Flags: sec-approval? → sec-approval+
Thanks. I'll check in for trunk as soon as m-i reopens.
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.
Attachment #760656 - Flags: approval-mozilla-aurora?
Pushed directly to m-c for quicker integration feedback.

https://hg.mozilla.org/mozilla-central/rev/291207393608
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.
Attachment #760656 - Flags: approval-mozilla-beta?
status-firefox24: affected → fixed
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:
Attachment #760656 - Flags: approval-mozilla-release?
landed on m-c = fixed, btw
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(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, ..?
(Reporter)

Comment 18

4 years ago
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.
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.
status-firefox-esr17: --- → unaffected

Updated

4 years ago
status-firefox21: affected → wontfix
status-firefox22: affected → wontfix
tracking-firefox21: ? → -
tracking-firefox22: ? → -
tracking-firefox23: ? → -
tracking-firefox24: ? → -

Updated

4 years ago
Keywords: sec-low

Updated

4 years ago
Attachment #760656 - Flags: approval-mozilla-release?
Attachment #760656 - Flags: approval-mozilla-release-
Attachment #760656 - Flags: approval-mozilla-beta?
Attachment #760656 - Flags: approval-mozilla-beta-
Attachment #760656 - Flags: approval-mozilla-aurora?
Attachment #760656 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7cae220f4d2f
status-firefox23: affected → fixed
Thanks Alex, Ryan.
Whiteboard: [adv-main23+]
Whiteboard: [adv-main23+] → [asan][adv-main23+]
Alias: CVE-2013-1708
Group: core-security
You need to log in before you can comment on or make changes to this bug.