Closed
Bug 1444231
Opened 6 years ago
Closed 6 years ago
FragmentOrElement should not QI to Element
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
(Keywords: sec-audit, Whiteboard: [adv-main60-][adv-esr52.8-])
Attachments
(2 files)
2.94 KB,
patch
|
Details | Diff | Splinter Review | |
1.97 KB,
patch
|
mccr8
:
review+
jcristau
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: 8ZAmv43anue
Attachment #8957353 -
Flags: review?(continuation)
Assignee | ||
Updated•6 years ago
|
Attachment #8957351 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8957351 -
Attachment is obsolete: false
Comment 3•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8957351 -
Flags: review+
Updated•6 years ago
|
Attachment #8957353 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 4•6 years ago
|
||
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?
Assignee | ||
Comment 5•6 years ago
|
||
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?
Updated•6 years ago
|
status-firefox58:
--- → wontfix
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
status-firefox-esr52:
--- → affected
Comment 6•6 years ago
|
||
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?
Updated•6 years ago
|
status-firefox61:
--- → affected
tracking-firefox-esr52:
--- → 60+
Assignee | ||
Comment 7•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/060024fa93121d878d27f329c959757e6f71017a
Updated•6 years ago
|
Group: core-security
Comment 8•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/060024fa9312
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 9•6 years ago
|
||
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+
Comment 10•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/925d5693c5a1
Updated•6 years ago
|
Group: dom-core-security → core-security-release
Comment 11•6 years ago
|
||
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+
Updated•6 years ago
|
Whiteboard: [adv-main60-][adv-esr52.8-]
Comment 13•6 years ago
|
||
(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-
Updated•6 years ago
|
Group: core-security-release
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•