Bug 1100409 (CVE-2014-8640)

Crash in mozilla::dom::AudioParamTimeline::AudioNodeInputValue

VERIFIED FIXED in Firefox 35

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: hofusec, Assigned: karlt)

Tracking

(5 keywords)

33 Branch
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox33 wontfix, firefox34- wontfix, firefox35+ verified, firefox36+ verified, firefox-esr31- wontfix, b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main35+][b2g-adv-main2.2+], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
Posted file testcase.html
FF Version: https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1416229949/firefox-36.0a1.en-US.linux-x86_64-asan.tar.bz2
Assertion + Stacktrace:
Assertion failure: aIndex < Length() (invalid array index), at ../../../../dist/include/nsTArray.h:946

#0  0x00007fffe8459974 in nsTArray_Impl<mozilla::AudioChunk, nsTArrayInfallibleAllocator>::ElementAt (this=<optimized out>, aIndex=<optimized out>)
    at ../../../../dist/include/nsTArray.h:946
#1  0x00007fffea3e10bc in mozilla::dom::AudioParamTimeline::AudioNodeInputValue (this=<optimized out>, aCounter=0)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/dom/media/webaudio/AudioParam.cpp:135
#2  0x00007fffea3ecbed in mozilla::dom::AudioParamTimeline::GetValueAtTime<long> (this=0x6110005b4448, aTime=<optimized out>, aCounter=0)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/dom/media/webaudio/AudioParamTimeline.h:93
#3  0x00007fffea417580 in mozilla::dom::DelayNodeEngine::UpdateOutputBlock (
    this=0x6110005b4400, aOutput=<optimized out>, minDelay=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/dom/media/webaudio/DelayNode.cpp:142
...

With a release build of firefox the testcase gives a invalid read at 0x5a5a5a62 looks like poisoned memory.

Comment 1

5 years ago
It crashes FF33 on Win 7 too.
Severity: normal → critical
Crash Signature: [@ mozilla::dom::AudioParamTimeline::AudioNodeInputValue(unsigned int) ]
Keywords: crash, reproducible
Summary: SEGV in AudioParamTimeline::AudioNodeInputValue → Crash in mozilla::dom::AudioParamTimeline::AudioNodeInputValue

Updated

5 years ago
Keywords: testcase

Comment 2

5 years ago
FF36 CR (with e10s):
https://crash-stats.mozilla.com/report/index/1c73a631-ff20-40af-b5df-899522141117

Regression range:
good=2014-07-18
bad=2014-07-19
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=99f694d1b50c&tochange=35f3fa435d2c

Suspected nug:
Karl Tomlinson — b=932400 change stream ordering to get feedback DelayNode output before supplying input r=roc Previously downstream nodes from DelayNodes in cycles sometimes received stale output from the previous MSG iteration. Also, if two cycles share a common path, they will now *both* be treated as cycles, either by muting or by enforcing minimum delay. Previously, marking one cycle first could prevent detection of other cycles in the same SCC.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 36 Branch → 33 Branch

Updated

5 years ago
Blocks: 932400
Comment hidden (me-too)
Assignee

Comment 4

5 years ago
This permits a read of 512kB block of memory with an uninitialized pointer.
Assignee: nobody → karlt
Keywords: sec-high
Assignee

Comment 5

5 years ago
AudioNodeEngine::mOutputCount is const so the array length can be set early.

There is a deeper issue here that I can file once this is fixed on branches:

A delay node, as currently spec'd can't produce output before it has input on
its parameter, to know how much delay to apply.

Either the delay node algorithm could be changed to apply the delay to the
input instead of to the output, or cycles involving the delay node parameter
could be muted.
Attachment #8524276 - Flags: review?(padenot)
Assignee

Comment 6

5 years ago
Given the pointer is near nsTArrayHeader::sEmptyHdr, I doubt content can control the read.
Keywords: sec-highsec-moderate
Attachment #8524276 - Flags: review?(padenot) → review+
Overall stability of 34 is in an acceptable range. Given that this is sec-moderate and we don't need to take further stability fixes, I'm marking this as wontfix for 34.
Assignee

Comment 8

5 years ago
The previous patch failed to compile in non-unified builds due to
mEngine->OutputCount() requiring the definition of nsAudioNodeEngine.
Attachment #8526403 - Flags: review?(padenot)
Assignee

Updated

5 years ago
Attachment #8524276 - Attachment is obsolete: true
Attachment #8526403 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/bbe24bcf277e
https://hg.mozilla.org/mozilla-central/rev/874e07bec1cc

Please nominate this for Aurora and b2g34 approval when you get a chance.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee

Comment 12

5 years ago
Comment on attachment 8526403 [details] [diff] [review]
uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h

Please consider this an approval request for both patches here.

Approval Request Comment
[Feature/regressing bug #]:
The particular testcase here didn't trigger the problem until changes from bug
932400, but I expect slightly different web audio graphs would produce the
same crash even in earlier releases, as far back as enabling webaudio for 25. 
[User impact if declined]:
sec-moderate crash.
[Describe test coverage new/current, TBPL]:
This particular crash is tested only against the testcase in this bug.
Delaying landing this test until the security flaw is patched on release
branches.
The code is widely tested in automated tests (but they don't cover this case).
[Risks and why]: 
Some risks associated with array manipulation, but existing testing provides
assurances.
[String/UUID change made/needed]:
None.
Attachment #8526403 - Flags: approval-mozilla-b2g34?
Attachment #8526403 - Flags: approval-mozilla-aurora?
Assignee

Comment 13

5 years ago
Comment on attachment 8526403 [details] [diff] [review]
uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h

The graph ordering was changed in bug 932400, which means this testcase doesn't affect earlier versions and can't be used to verify, but the underlying bug was still present before changes from bug 932400.
Attachment #8526403 - Flags: approval-mozilla-b2g32?
Attachment #8526403 - Flags: approval-mozilla-b2g30?
Assignee

Comment 14

5 years ago
[Tracking Requested - why for this release]:
sec-moderate.
Comment on attachment 8526403 [details] [diff] [review]
uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h

we merged in between. cf comment #12 for uplift the request.
Attachment #8526403 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8526403 [details] [diff] [review]
uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h

Approving for Beta uplift, wontfix for ESR31 as it doesn't meet the landing criteria nor does it appear to be a highly requested fix from that user base.
Attachment #8526403 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8526403 [details] [diff] [review]
uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h

Approving on 2.1 as this is the release version in flight where we could avoid this bug by this uplift.

For other branches we are only uplifting sec-crit's alone at this time and this will be a wontfix there.
Attachment #8526403 - Flags: approval-mozilla-b2g34?
Attachment #8526403 - Flags: approval-mozilla-b2g34+
Attachment #8526403 - Flags: approval-mozilla-b2g32?
Attachment #8526403 - Flags: approval-mozilla-b2g32-
Attachment #8526403 - Flags: approval-mozilla-b2g30?
Attachment #8526403 - Flags: approval-mozilla-b2g30-
Group: media-core-security
Whiteboard: [adv-main35+]
Alias: CVE-2014-8640
Whiteboard: [adv-main35+] → [adv-main35+][b2g-adv-main2.2+]

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.