limit ChannelMergerNode output channel count

VERIFIED FIXED in Firefox 30

Status

()

defect
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

({csectype-dos, sec-critical})

Trunk
mozilla32
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main30+])

Attachments

(1 attachment)

Assignee

Description

5 years ago
+++ 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.
Attachment #8400393 - Flags: review?(paul) → review+
Keywords: sec-low
Assignee

Updated

5 years ago
Blocks: 991251
Assignee

Updated

5 years ago
Assignee

Comment 2

5 years ago
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
Flags: in-testsuite?
Assignee

Comment 3

5 years ago
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.
Attachment #8400393 - Flags: approval-mozilla-beta?
Attachment #8400393 - Flags: approval-mozilla-b2g28?
Attachment #8400393 - Flags: approval-mozilla-b2g26?
Attachment #8400393 - Flags: approval-mozilla-aurora?
Assignee

Comment 4

5 years ago
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.
Whiteboard: [don't transplant m-c changeset to branches]
Karl, does it impact ESR? Thanks
Flags: needinfo?(karlt)
Assignee

Comment 6

5 years ago
Only versions 26 and later are affected, so ESR24 is not affected.
Flags: needinfo?(karlt)
Attachment #8400393 - Flags: approval-mozilla-beta?
Attachment #8400393 - Flags: approval-mozilla-beta+
Attachment #8400393 - Flags: approval-mozilla-aurora?
Attachment #8400393 - Flags: approval-mozilla-aurora+
This was merged last Friday.

https://hg.mozilla.org/mozilla-central/rev/28f1a546126f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8400393 [details] [diff] [review]
limit ChannelMergerNode output channel count

sec-lows aren't being approved for the B2G release branches
Attachment #8400393 - Flags: approval-mozilla-b2g28?
Attachment #8400393 - Flags: approval-mozilla-b2g26?
Assignee

Comment 11

5 years ago
Changing to sec-critical as this is part of fixing bug 991533, which is now necessary to avoid sec-critical bug 987679.
Keywords: sec-lowsec-critical
Assignee

Updated

5 years ago
Attachment #8400393 - Flags: approval-mozilla-b2g28?
Attachment #8400393 - Flags: approval-mozilla-b2g26?
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
Attachment #8400393 - Flags: approval-mozilla-b2g28?
Attachment #8400393 - Flags: approval-mozilla-b2g26?
Does anyone have a suggestion on how to verify this sec-critical bug fix? Thanks.
Assignee

Comment 15

5 years ago
(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.)
Assignee

Comment 16

5 years ago
I mean bug 990794.
Whiteboard: [adv-main30+]
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.
(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
Assignee

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ce8894b2876
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.