990868 - limit ChannelMergerNode output channel count

Status: VERIFIED FIXED

(Core :: Web Audio, defect, P1)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #990794 +++
If we land a fix for the size calculation in bug 990794, then that leave a DOS bug, which can be fixed by limiting the channel count.

Fixing the DOS gives away more information about how to induce the integer and heap overflow, so we have the option of landing this fix separately.

See also bug 987679 which can be induced through large channel counts.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: limit ChannelMergerNode output channel count

Comment 2

[Karl Tomlinson (:karlt)]

Had to change some uint32_t array sizes/indexes to size_t due to changes from bug 1004098.

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f1a546126f

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8400393 [details] [diff] [review]
limit ChannelMergerNode output channel count

[Approval Request Comment]
Requesting approval for this patch as part of fixing bug 990868.

Bug caused by (feature/regressing bug #): 
The security issue that I'm concerned about here is bug 987679, which could be triggered using WaveShaperNode.oversample since Bug 875277 landed in 26.

User impact if declined: Possible sec-critical.
Testing completed (on m-c, etc.): on m-i, requesting approval to land once merged to m-c.
Risk to taking this patch (and alternatives if risky): low.
String or IDL/UUID changes made by this patch: none.

Comment 4

[Karl Tomlinson (:karlt)]

The version to land on branches is the one reviewed here (attachment 8400393 [details] [diff] [review]), without the size_t changes on m-c/32.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Karl, does it impact ESR? Thanks

Comment 6

[Karl Tomlinson (:karlt)]

Only versions 26 and later are affected, so ESR24 is not affected.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This was merged last Friday.

https://hg.mozilla.org/mozilla-central/rev/28f1a546126f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8400393 [details] [diff] [review]
limit ChannelMergerNode output channel count

sec-lows aren't being approved for the B2G release branches

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e0fc97c0b28
https://hg.mozilla.org/releases/mozilla-beta/rev/20143f06748b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/20143f06748b

Comment 11

[Karl Tomlinson (:karlt)]

Changing to sec-critical as this is part of fixing bug 991533, which is now necessary to avoid sec-critical bug 987679.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Well in that case!

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/fce835f24818
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d5f64dbd1400

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8400393 [details] [diff] [review]
limit ChannelMergerNode output channel count

FWIW, sec-crits don't need approval for uplift to the B2G release branches per the B2G Landing Page.
https://wiki.mozilla.org/Release_Management/B2G_Landing

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

Does anyone have a suggestion on how to verify this sec-critical bug fix? Thanks.

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Matt Wobensmith from comment #14)
> Does anyone have a suggestion on how to verify this sec-critical bug fix?

The testcase in comment 990794 should no longer crash.

(That's not the sec-critical part of this bug but would verify that the patch is doing what it should.)

Comment 16

[Karl Tomlinson (:karlt)]

I mean bug 990794.

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Karl.

On Fx30 from 2014-05-15 (pre-fix) I don't see a crash, but I do get a hang on close that requires a force quit.
On Fx30/Fx31/Fx32 from 2014-06-02, no crash and program exits smoothly. 

I'm going to consider this verified unless there are objections.

Comment 18

[Justin Wood (:Callek)]

fixed on SeaMonkey 2.26.1 (based on Gecko 29)

https://hg.mozilla.org/releases/mozilla-release/rev/7a22ed0a3752

Comment 19

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #18)
> fixed on SeaMonkey 2.26.1 (based on Gecko 29)
> 
> https://hg.mozilla.org/releases/mozilla-release/rev/7a22ed0a3752

err nope, couldn't get this to apply + pass tests, and that rev is a different bug

Comment 20

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce8894b2876