Closed Bug 825288 Opened 8 years ago Closed 8 years ago

MaybeWrapValue doesn't handle SOWs correctly


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 + fixed
firefox20 + fixed


(Reporter: bzbarsky, Assigned: bzbarsky)



(Keywords: regression, sec-high, Whiteboard: [adv-main19-])


(2 files)

Just realized this.  We now do same-compartment wrappers in WrapNewBindingObject, but we don't handle them in MaybeWrapValue.  So if we have nodes coming out via "any" return values, we have a problem.  :(

I hate SOWs.
Attachment #696373 - Flags: review?(peterv)
Comment on attachment 696371 [details] [diff] [review]
Handle SOWs correctly in MaybeWrapValue.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Would need to find a WebIDL API returning "any" values that can include elements that might need SOWs.  Not sure we have any offhand, and rather doubt we do, but would rather not risk it.

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

Which older supported branches are affected by this flaw?  Aurora 19 only.

If not all supported branches, which bug introduced the flaw?  Bug 801712.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Aurora backport attached to this bug.

How likely is this patch to cause regressions; how much testing does it need?
Attachment #696371 - Flags: sec-approval?
Comment on attachment 696373 [details] [diff] [review]
Aurora version of patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 801712.
User impact if declined: Probably none, but possibly a security bug.
Testing completed (on m-c, etc.): Seems to compile and pass tests
Risk to taking this patch (and alternatives if risky): Low risk, I believe.
String or UUID changes made by this patch: None.
Attachment #696373 - Flags: approval-mozilla-aurora?
Whiteboard: [need review]
Marking sec-high. Feel free to raise or lower as you feel is appropriate.
Keywords: sec-high
Attachment #696371 - Flags: review?(peterv) → review+
Attachment #696373 - Flags: review?(peterv) → review+
Hrm.  I thought this had gotten sec-approval already, so I pushed but it looks like I was wrong...  Backing it out now won't really help the problem, sadly.  :(

In any case, I think this should be a no-brainer for Aurora, and is absolutely required on m-c for 20.
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla20
Assignee: nobody → bzbarsky
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 696373 [details] [diff] [review]
Aurora version of patch

Approving for Aurora 19 (knowingly without sec-approval) since this has already landed on m-c.
Attachment #696373 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 696371 [details] [diff] [review]
Handle SOWs correctly in MaybeWrapValue.

Clearing sec-approval flag since this is already in.
Attachment #696371 - Flags: sec-approval?
Whiteboard: [adv-main19-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.