Closed Bug 944851 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Web Audio, defect, P1)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
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

People

(Reporter: jruderman, Assigned: karlt)

References

Details

(Keywords: assertion, sec-moderate, testcase, Whiteboard: [adv-main27+])

Attachments

(4 files)

Attached file testcase
Assertion failure: i < Length() (invalid array index), at nsTArray.h:887
Attached 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
Attached file time-varying testcase
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: 7 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.