Closed Bug 1100409 (CVE-2014-8640) Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: Web Audio, defect)

33 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
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

People

(Reporter: hofusec, Assigned: karlt)

References

Details

(5 keywords, Whiteboard: [adv-main35+][b2g-adv-main2.2+])

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached 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.
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
Keywords: testcase
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
Blocks: 932400
This permits a read of 512kB block of memory with an uninitialized pointer.
Assignee: nobody → karlt
Keywords: sec-high
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)
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.
The previous patch failed to compile in non-unified builds due to
mEngine->OutputCount() requiring the definition of nsAudioNodeEngine.
Attachment #8526403 - Flags: review?(padenot)
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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?
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?
[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+]
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.