Closed Bug 1444231 Opened 2 years ago Closed 2 years ago

FragmentOrElement should not QI to Element

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 60+ fixed
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 + fixed
firefox61 + fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: sec-audit, Whiteboard: [adv-main60-][adv-esr52.8-])

Attachments

(2 files)

This is an issue that was introduced in bug 563659.  The QI to Element should only work on Element, not on DocumentFragment.

I did try just now making this case (non-Element reaching FragmentOrElement::QI with the Element IID) MOZ_CRASH and my try push is green.  So I don't have a practical example of a problem this causes right this second.  This makes it a bit unclear to me what the security rating should be.  If this were being hit, that would be a sec-critical, of course...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: 8ZAmv43anue
Attachment #8957353 - Flags: review?(continuation)
Attachment #8957351 - Attachment is obsolete: true
Attachment #8957351 - Attachment is obsolete: false
Comment on attachment 8957351 [details] [diff] [review]
The thing I tried pushing to try

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

Oh boy...
Attachment #8957351 - Flags: review+
Attachment #8957351 - Flags: review+
Attachment #8957353 - Flags: review?(continuation) → review+
Comment on attachment 8957353 [details] [diff] [review]
Fix QI implementation for FragmentOrElement

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not very easily.  You'd need to figure out how to get a DocumentFragment to a place where we QI a node to Element and then do something with it.  This is not very easy to do.  For example, nothing in our test suite does this.  This is helped by the fact that any time we know we have a node we tend to use IsElement()/AsElement() on it, which doesn't QI.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Not any more than the fix itself.

Which older supported branches are affected by this flaw?  All of them.  This problem was introduced in Firefox 17.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I would expect this patch to apply on branches.  If not, it's easy to backport, and the backports would be very safe.

How likely is this patch to cause regressions; how much testing does it need?  I don't think this is likely to cause regressions at all.  Not much testing needed.
Attachment #8957353 - Flags: sec-approval?
Comment on attachment 8957353 [details] [diff] [review]
Fix QI implementation for FragmentOrElement

Approval Request Comment
[Feature/Bug causing the regression]: Bug 563659.
[User impact if declined]: Latent security problem that could bite us.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: I haven't found a way to actually
   hit the problematic code in a problematic way.  This was found via code
   inspection.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's just moving the QI bit from
   FragmentOrElement to Element, and right now we seem to never hit it
   for the non-Element case anyway.
[String changes made/needed]: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: See above.
Fix Landed on Version: Will be 60 or 61.
Risk to taking this patch (and alternatives if risky): Very low risk.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8957353 - Flags: approval-mozilla-esr52?
Attachment #8957353 - Flags: approval-mozilla-beta?
Keywords: sec-audit
Comment on attachment 8957353 [details] [diff] [review]
Fix QI implementation for FragmentOrElement

We decided this was a sec-audit so I'm clearing sec-approval and you can check into trunk as you wish.
Attachment #8957353 - Flags: sec-approval?
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/060024fa9312
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8957353 [details] [diff] [review]
Fix QI implementation for FragmentOrElement

fix possible security issue, beta60+
Attachment #8957353 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Comment on attachment 8957353 [details] [diff] [review]
Fix QI implementation for FragmentOrElement

Simple fix that avoids us being accidentally exposed to sec-crits down the road. Approved for ESR 52.8.
Attachment #8957353 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main60-][adv-esr52.8-]
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> [Is this code covered by automated tests?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.
> [Is the change risky?]: No.

Does not need manual testing, per Boris.
Flags: qe-verify-
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.