1811331 - (CVE-2023-25736) cfi-derived-cast: Invalid downcast in GetTableSelectionMode

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[lukas.bernhard]

Steps to reproduce:

When compiling Firefox with cfi-derived-cast, crashtest editor/libeditor/crashtests/1618906.html fails due to the downcasting here: https://searchfox.org/mozilla-central/rev/daf613efc5c358f3a94961d73b90472c00703838/dom/base/Selection.cpp#512
The dynamic type of the casted object is nsHTMLDocument, which is not derived from nsIContent.

This might not be s-s, just flagging as a precaution.

Comment 1

[Andrew McCreight [:mccr8]]

It looks safe-ish outside of undefined behavior kind of issues. The only method called on startContent is IsHTMLElement, which is an inline method that does mNodeInfo->Equals(aTag, kNameSpaceID_XHTML);, and mNodeInfo is a field on nsINode, and nsHTMLDocument is an nsINode. Following the chain of inheritance to nsINode up for both nsHTMLDocument and nsIContent, it always goes via the first superclass, so the cast won't actually do anything at runtime, I'm guessing. It would still be good to fix, of course.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1811331 - remove useless static_cast, r=mccr8

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

remove useless static_cast, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/84398671e7e1872f3ddf17f28b3f87905c6bd3d8
https://hg.mozilla.org/mozilla-central/rev/84398671e7e1

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:smaug, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox110 to wontfix.

For more information, please visit auto_nag documentation.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9313232 [details]
Bug 1811331 - remove useless static_cast, r=mccr8

Beta/Release Uplift Approval Request

• User impact if declined: I think the behavior should be actual fine even without the patch, cfi-derived-cast just complains about this case. But to the safe, worth to land also to beta, maybe.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: NA
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): the patch removes a useless static_cast
• String changes made/needed: NA
• Is Android affected?: Yes

Comment 6

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9313232 [details]
Bug 1811331 - remove useless static_cast, r=mccr8

Approved for 110 beta 5, thanks.

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e41e4da3ecb

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt