995289 - (CVE-2014-1522) Out-of-Bounds Read in mozilla::dom::OscillatorNodeEngine::ComputeCustom

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Holger Fuhrmannek]

Attachment: poc.html

The poc crashes also firefox 28.0.
The asan report was generated with this build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-11-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2

Comment 1

[Holger Fuhrmannek]

Attachment: asan_report.txt

Comment 2

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/annotate/265d82091bce/content/media/webaudio/OscillatorNode.cpp#l399

Comment 3

[Ralph Giles (:rillian)]

Testcase crashes Nightly 31.0a1 (2014-04-14) on OS X. I'll debug.

Comment 4

[Ralph Giles (:rillian)]

Attachment: 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?

Comment 5

[Paul Adenot (:padenot)]

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)

Comment 6

[Paul Adenot (:padenot)]

(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.

Comment 7

[Paul Adenot (:padenot)]

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).

Comment 8

[Karl Tomlinson (:karlt)]

Please don't check in a testcase until fixes are shipped on all affected branches.

Comment 9

[Ralph Giles (:rillian)]

Attachment: Wrap phase with fmod

I guess fmod works. It's a lot slower in the common case, but keeps the change simple.

Comment 10

[Ralph Giles (:rillian)]

Attachment: Wrap phase with fmod

Sorry, wrong patch.

Comment 11

[Ralph Giles (:rillian)]

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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).

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Can we have an evaluation of the risk of this patch?

Comment 14

[Ralph Giles (:rillian)]

(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.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

OK. thanks :)
I will wait for your performance evaluation then.

Comment 16

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 17

[Ralph Giles (:rillian)]

Try push is looking green. https://tbpl.mozilla.org/?tree=Try&rev=a3344d4f501c

Comment 18

[Sylvestre Ledru [:Sylvestre]]

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

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8407161 [details] [diff] [review]
Wrap phase with fmod

sec-approval+ for trunk then.

Comment 20

[Ralph Giles (:rillian)]

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.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

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) ;)

Comment 22

[Ralph Giles (:rillian)]

Thanks Sylvetre, Al. I've pushed to inbound. Will now push to beta and aurora.

https://hg.mozilla.org/integration/mozilla-inbound/rev/abda9ad7e889

Comment 23

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eaa1f0c148ec
https://hg.mozilla.org/releases/mozilla-beta/rev/3b65e1a1c52a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

*sigh* I really hope this sticks given the strongly recommended-against simul-landing...

Comment 25

[Ralph Giles (:rillian)]

Me too! Can you take care of the b2g branches if it sticks?

Comment 26

[Ralph Giles (:rillian)]

Performance issue filed as https://bugzilla.mozilla.org/show_bug.cgi?id=997870

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This is going to stick.

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/0fd9f6fe8832
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/dbf6a7c1e1a3

Comment 28

[Ralph Giles (:rillian)]

Thanks, Ryan.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/abda9ad7e889

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5959dd123953

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5959dd123953