1811464 - (CVE-2023-25737) cfi-derived-cast: Invalid downcast in SVGUtils::SetupStrokeGeometry

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

Steps to reproduce:

When compiling Firefox with cfi-derived-cast, crashtest layout/svg/crashtests/267650-1.svg fails due to the downcasting here:
https://searchfox.org/mozilla-central/rev/daf613efc5c358f3a94961d73b90472c00703838/layout/svg/SVGUtils.cpp#1525

The dynamic type of aFrame->GetContent() is nsTextNode, which is not derived from SVGElement.

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

Comment 1

[Robert Longson [:longsonr]]

Attachment: Bug 1811464 - Use SVGElement::FromNode more r=emilio

Comment 2

[Robert Longson [:longsonr]]

I believe this is security sensitive, we end up in layout/style with an nsIContent where we should have an Element and we do some Element specific things. While I could fix it without the MOZ_ASSERT, I don't think that makes it any more obvious. The main problem with this bug is how easy it is to trigger it. We really should get this into a Firefox release and into ESR too.

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 9313377 [details]
Bug 1811464 - Use SVGElement::FromNode more r=emilio

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Any SVG with text in it is a problem so not very hard.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This patch should apply to older branches.
• How likely is this patch to cause regressions; how much testing does it need?: Should be safe it's really a one line change in SVGTextFrame.cpp the rest is clean up and may serve to obfuscate the real issue very slightly.
• Is Android affected?: Yes

Comment 4

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

Comment on attachment 9313377 [details]
Bug 1811464 - Use SVGElement::FromNode more r=emilio

Approved to land and request uplift

Comment 5

[Andrew McCreight [:mccr8]]

I guess I'll mark this sec-high based on comment 2 and comment 3.

Comment 6

[Daniel Holbert [:dholbert]]

Landed via Lando: https://hg.mozilla.org/integration/autoland/rev/03880b3839c906d2d5b806a0a60a2ba5e243f951

Comment 7

[Daniel Holbert [:dholbert]]

(It's late in longsonr's timezone, so I landed it at his request, and am tagging him with needinfo as a reminder to do the beta-uplift-request dance when he's awake.)

Comment 8

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

Use SVGElement::FromNode more r=emilio
https://hg.mozilla.org/integration/autoland/rev/03880b3839c906d2d5b806a0a60a2ba5e243f951
https://hg.mozilla.org/mozilla-central/rev/03880b3839c9

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 9313377 [details]
Bug 1811464 - Use SVGElement::FromNode more r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: invalid pointer dereferences, possibly exploitable although there are no known exploits.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: Existing crashtests should now crash in debug as there's a new MOZ_ASSERT in the patch.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch is fairly small and includes an assert.
• String changes made/needed: none
• Is Android affected?: Unknown

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's sec:high currently.
• User impact if declined: invalid pointer dereferences, possibly exploitable although there are no known exploits.
• Fix Landed on Version: 111
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch is fairly small and includes an assert.

Comment 10

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

Comment on attachment 9313377 [details]
Bug 1811464 - Use SVGElement::FromNode more r=emilio

Approved for 110 beta 5, thanks.

Comment 11

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

https://hg.mozilla.org/releases/mozilla-beta/rev/68f4be2517e3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9313377 [details]
Bug 1811464 - Use SVGElement::FromNode more r=emilio

Approved for 102.8esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/53574fbd4e07

Comment 14

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

Attachment: advisory.txt