Array out-of-bounds [@ mozilla::dom::AudioParamTimeline::AudioNodeInputValue]

RESOLVED FIXED in Firefox 27

Status

()

defect
P1
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jruderman, Assigned: karlt)

Tracking

(Blocks 2 bugs, {assertion, sec-moderate, testcase})

Trunk
mozilla29
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 wontfix, firefox27+ verified, firefox28+ verified, firefox29 verified, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [adv-main27+])

Attachments

(4 attachments)

Posted file testcase
Assertion failure: i < Length() (invalid array index), at nsTArray.h:887
Posted file stack
This is an invalid read of an uninitialized AudioChunk from an nsAutoTArray in an AudioNodeStream.  AudioChunk contains a ThreadSharedObject, which has virtual functions, but these are not executed, so I don't think arbitrary code execution is possible.  However, the chunk contains uninitialized pointers to 512 block chunks of memory that could be read.  It may be possible, though likely difficult, for content to influence those pointers.
Assignee: nobody → karlt
Keywords: sec-moderate
Priority: -- → P1
We shouldn't be locking to include data from the audio thread here, and trying
to use the current value from the audio thread will only introduce races.  I
guess we could get the audio thread to send its latest value to the main
thread, but that is expensive - I don't want to do that.

This testcase demonstrates that Blink fails to even try to generate reasonable
results if anything is time-varying.
http://src.chromium.org/viewvc/blink/trunk/Source/modules/webaudio/AudioParam.cpp?revision=159093#l116

Gecko gives the expected result:

Defaults:
magnitude = 0.9565200209617615, phase = -1.5707964897155762
With parameter timeline:
magnitude = 0.9565200209617615, phase = -1.5707964897155762

Blink produces incorrect results, which may be dependent on uninitialized
values:

Defaults:
magnitude = 0.9565200209617615, phase = -1.5707964897155762
With parameter timeline:
magnitude = 0, phase = 0

Defaults:
magnitude = 0.9565200209617615, phase = -1.5707964897155762
With parameter timeline:
magnitude = 0, phase = -3.1415927410125732
Blocks: 893020, 866434
Attachment #8351280 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/f4da19ec3e3b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8351280 [details] [diff] [review]
don't consider AudioNode input when getting AudioParam values on the main thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 866434
User impact if declined: moderate security risk
Testing completed (on m-c, etc.): on m-c.
Manually tested that the code behaves as expected.
Risk to taking this patch (and alternatives if risky): 
Small.  The common paths have no change in behavior.  There is a behavior change in an unusual situation.  I don't expect this to cause any problems because Chrome does not produce reasonable results in this situation.
String or IDL/UUID changes made by this patch: none.
Attachment #8351280 - Flags: approval-mozilla-beta?
Attachment #8351280 - Flags: approval-mozilla-aurora?
Attachment #8351280 - Flags: approval-mozilla-beta?
Attachment #8351280 - Flags: approval-mozilla-beta+
Attachment #8351280 - Flags: approval-mozilla-aurora?
Attachment #8351280 - Flags: approval-mozilla-aurora+
Flags: needinfo?(karlt)
Comment on attachment 8351280 [details] [diff] [review]
don't consider AudioNode input when getting AudioParam values on the main thread

Please see comment 7.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> Is this something we need on b2g26 (v1.2)?

Yes, it is, thank you.
Attachment #8351280 - Flags: approval-mozilla-b2g26?
Flags: needinfo?(karlt)
Confirmed crash in FF27, 2013-11-22.
Verified fixed in FF27, FF28 and FF29, 2014-01-14.
Whiteboard: [adv-main27+]
Comment on attachment 8351280 [details] [diff] [review]
don't consider AudioNode input when getting AudioParam values on the main thread

plus on approval‑mozilla‑b2g26: for sec-moderate
Attachment #8351280 - Flags: approval-mozilla-b2g26? → approval-mozilla-b2g26+
Group: core-security
You need to log in before you can comment on or make changes to this bug.