Bug 995289 (CVE-2014-1522)

Out-of-Bounds Read in mozilla::dom::OscillatorNodeEngine::ComputeCustom

RESOLVED FIXED in Firefox 29

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hofusec, Assigned: rillian)

Tracking

({csectype-bounds, sec-high})

31 Branch
mozilla31
x86_64
Linux
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox28 wontfix, firefox29 fixed, firefox30 fixed, firefox31 fixed, firefox-esr24 unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [adv-main29+])

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Comment 1

5 years ago
Posted file asan_report.txt
Testcase crashes Nightly 31.0a1 (2014-04-14) on OS X. I'll debug.
This fixes the crash for me. I thought we could only wrap once, but with the high playback frequency here we need to do it 528 times.

Wrapping in this situation doesn't perform well. Is there a faster way to do float addition modulo a power of two?

In any case, we should add some limits to guard against these sort of problems, when the requested frequency is too high to give reliable results. Paul, does the spec say anything about what's expected to work?
Attachment #8406563 - Flags: review?(paul)
Comment on attachment 8406563 [details] [diff] [review]
Properly wrap phase index to lookup table.

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

::: content/media/webaudio/OscillatorNode.cpp
@@ +386,4 @@
>                                                       tableInterpolationFactor);
>        // mPhase runs 0..periodicWaveSize here instead of 0..2*M_PI.
>        mPhase += periodicWaveSize * mFinalFrequency * rate;
> +      while (mPhase >= periodicWaveSize) {

fmod(2)
Attachment #8406563 - Flags: review?(paul)
(In reply to Ralph Giles (:rillian) from comment #4)
> Created attachment 8406563 [details] [diff] [review]
> Properly wrap phase index to lookup table.
> 
> This fixes the crash for me. I thought we could only wrap once, but with the
> high playback frequency here we need to do it 528 times.
> 
> Wrapping in this situation doesn't perform well. Is there a faster way to do
> float addition modulo a power of two?
> 
> In any case, we should add some limits to guard against these sort of
> problems, when the requested frequency is too high to give reliable results.
> Paul, does the spec say anything about what's expected to work?

It does not, but we can certainly clip it to (at least) nyquist behind the scenes.
Also, it would be great to check a variation on the testcase in (we just need to have a super high frequency, no need for the setTargetAtTime part, I think).
Please don't check in a testcase until fixes are shipped on all affected branches.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Posted patch Wrap phase with fmod (obsolete) — Splinter Review
I guess fmod works. It's a lot slower in the common case, but keeps the change simple.
Assignee: nobody → giles
Attachment #8406563 - Attachment is obsolete: true
Attachment #8407158 - Flags: review?(paul)
Sorry, wrong patch.
Attachment #8407158 - Attachment is obsolete: true
Attachment #8407158 - Flags: review?(paul)
Attachment #8407161 - Flags: review?(paul)
Attachment #8407161 - Flags: review?(paul) → review+
Comment on attachment 8407161 [details] [diff] [review]
Wrap phase with fmod

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

It's not completely obvious. 'if' vs 'fmod' is a little obfuscated, and there's indirection through another variable before it's used as an array index. However, the invalid read is only 8 lines away.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

> Which older supported branches are affected by this flaw?

Everything but esr24 and b2g18.

> If not all supported branches, which bug introduced the flaw?

Bug has been present since the feature was added in bug 865256.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Same patch applies to all affected branches.

> How likely is this patch to cause regressions; how much testing does it need?

Fix will be slower on mobile, but for normal pages behaviour is identical.
Attachment #8407161 - Flags: sec-approval?
We need Release Management to weigh in on whether we can get this on Beta this cycle. Otherwise, this is too late and should land next cycle (of six weeks).
Flags: needinfo?(sledru)
Can we have an evaluation of the risk of this patch?
Flags: needinfo?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Can we have an evaluation of the risk of this patch?

Risk for desktop is very low. It's a one line change which behaves identically for valid pages and correctly bounds a table read for invalid pages. The change only affects custom waveforms in the webaudio API, which aren't used very widely.

The new bounds check is somewhat expensive on soft-float devices. I'm doing an Android build now to check how much of a performance regression it will be there.
Flags: needinfo?(sledru)
OK. thanks :)
I will wait for your performance evaluation then.
Posted file osc-bench.html
Ok, the attached benchmark page reports about twice the time on a Nexus 7 compared to without the fix. That is, 120 ms vs 60 ms to generate 10 s of output.

Note that even without the fix my debug build is ~ 10x slower than chome so I don't think it's a problem. We can work on optimizing it separately.
Flags: needinfo?(sledru)
Flags: needinfo?(sledru)
OK. So, I am going to approve it. Could you fill the uplift request (before 13PDT would be great to make sure it gets into 29)?
And also, could report a new bug for the performance aspect?

Thanks
Flags: needinfo?(sledru) → needinfo?(giles)
Comment on attachment 8407161 [details] [diff] [review]
Wrap phase with fmod

sec-approval+ for trunk then.
Attachment #8407161 - Flags: sec-approval? → sec-approval+
Comment on attachment 8407161 [details] [diff] [review]
Wrap phase with fmod

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 865256

User impact if declined: Users exposed to out-of-bounds data reads. Data can be returned to web content if it doesn't cause a crash.

Testing completed (on m-c, etc.): Green on try on top of m-c

Risk to taking this patch (and alternatives if risky): Functionality is identical for valid pages. Feature in question isn't in broad use yet. 2x slowdown on mobile, however.

String or IDL/UUID changes made by this patch: none.
Attachment #8407161 - Flags: approval-mozilla-beta?
Flags: needinfo?(giles)
Comment on attachment 8407161 [details] [diff] [review]
Wrap phase with fmod

Approving also for aurora. I guess we want to have also this patch in this branch. 

Can we get also a checkin in m-c? (quickly) ;)
Attachment #8407161 - Flags: approval-mozilla-beta?
Attachment #8407161 - Flags: approval-mozilla-beta+
Attachment #8407161 - Flags: approval-mozilla-aurora+
Thanks Sylvetre, Al. I've pushed to inbound. Will now push to beta and aurora.

https://hg.mozilla.org/integration/mozilla-inbound/rev/abda9ad7e889
*sigh* I really hope this sticks given the strongly recommended-against simul-landing...
Me too! Can you take care of the b2g branches if it sticks?
Thanks, Ryan.
https://hg.mozilla.org/mozilla-central/rev/abda9ad7e889
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Whiteboard: [adv-main29+]
Alias: CVE-2014-1522
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Flags: in-testsuite?
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.