<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Reporter

Comment 2

•

3 years ago

(In reply to Sylvestre Ledru [:Sylvestre] from comment #1)

I can reproduce it on linux:
https://crash-stats.mozilla.org/report/index/4d76a195-4fe5-489f-b28c-7bc8f0211015#tab-details

I think this is a different bug.

Keywords: bugmon

https://crash-stats.mozilla.org/report/index/0699a63a-0a46-4498-9635-a08d90211019

Reporter

Comment 3

•

3 years ago

Here is what I get (again on Windows):

This seems more like what I'd expect since it has a similar stack and the poison value (in rax).

Crash Signature: [@ mozilla::SVGRenderingObserverSet::InvalidateAll ]

https://crash-stats.mozilla.org/report/index/b2c5df1f-59e6-426a-817a-636230210615

Reporter

Comment 4

•

3 years ago

Hmm I also get (on Linux)

This has what appears to be a frame poison value in rax.

Crash Signature: [@ mozilla::SVGRenderingObserverSet::InvalidateAll ] → [@ mozilla::SVGRenderingObserverSet::InvalidateAll ] [@ nsIFrame::SetParent ]

Reporter

Comment 5

•

3 years ago

A Pernosco session is available here: https://pernos.co/debug/-LQDdU8fLTbOpr7b7r96XQ/index.html

Reporter

Updated

•

3 years ago

OS: Windows → Unspecified

Daniel Veditz [:dveditz]

Updated

•

3 years ago

Keywords: sec-high

Updated

•

3 years ago

Flags: needinfo?(dholbert)

Updated

•

3 years ago

Assignee: nobody → dholbert

Comment 6

•

3 years ago

•

Edited

FWIW, when I load the attached testcase on linux (in Nightly with a fresh profile), I get an immediate content-process crash, with signature mozilla::SVGRenderingObserverSet::InvalidateAll (as ~expected, unlike sledru's different-looking crash in comment 1).

Crash report: bp-fed810d5-7560-4495-bc0c-4158d0211118

In my case, it seems to have crashed with a null deref.

Comment 7

•

3 years ago

•

Edited

Side note: I spun off bug 1741956 for crashes in XGetImage (like sledru's in comment 1). I suspect that's not related to this bug at all -- I wonder if it might have been posted here by mistake? I notice that sledru's comment 1 was posted on Oct 18th, but the crash report he linked was processed 3 days earlier (2021-10-15 06:15:08 UTC). So I wonder if that was a copypaste error when copying over the crash report link, just getting the wrong crash ID from about:crashes or something.

sledru: if you can somehow trigger parent-process XGetImage-signature crashes with this bug's testcase, let us know; otherwise, I'm guessing that was a copypaste error. Also: if your about:crashes seems to contain any more-related-looking[1] crash reports from around when you posted comment 1, that would help confirm that this is indeed what happened.)

[1] I'd expect your crash to look similar to my crash from comment 6 or tyson's from comment 4; it should be a content-process crash with SVGTextFrame in the backtrace, probably.

Flags: needinfo?(dholbert) → needinfo?(sledru)

Comment 8

•

3 years ago

Attached file backtrace of fatal assertion failure — Details

In a debug build, the attached testcase aborts with this assertion-failure in `SVGRenderingObserver::DebugObserverSet()`: ``` Assertion failure: inObserverSet == mInObserverSet (failed to track whether we're in our referenced element's observer set!), at layout/svg/SVGObserverUtils.cpp:1115 ``` https://searchfox.org/mozilla-central/rev/3881c4ca80d1b4b2f43be695438ecaf90ee4f86c/layout/svg/SVGObserverUtils.cpp#1113-1115 Backtrace for this assertion-failure attached.

Robert Longson [:longsonr]

Comment 9

•

3 years ago

In another debug-build load of the testcase, I got this additional non-fatal assertion, which appeared just before I hit the same fatal assertion-failure from comment 8:

[Child 62601, Main Thread] ###!!! ASSERTION: removing observer from an element we're not observing?: 'observers->Contains(aObserver)', file layout/svg/SVGObserverUtils.cpp:1625

I captured this run in rr and have submitted it to pernosco (along with the recording that we've already got in comment 5). I'll post the recording link when I've got it.

Comment 10

•

3 years ago

There's an idea from jwatt in bug 1539477 not sure if it would fix this but we could give it a shot.

Sylvestre Ledru [:Sylvestre]

Comment 11

•

3 years ago

Thanks! That's an interesting connection.

Here's the pernosco recording for the run in comment 9, at the moment we print the non-fatal assertion failure:
https://pernos.co/debug/CkryXMa2GCmndAKMKmmkqg/index.html#f{m[Cx0y,AA_,t[hw,9Ik_,f{e[Cx0t,AUjo4w_,s{af3y0oVAA,bDkA,uD/+9RQ,oEAjAXw___/

Comment 12

•

3 years ago

I can still reproduce it:
https://crash-stats.mozilla.org/report/index/571b8e59-fdf8-48cb-9fdb-80eb30211121

Flags: needinfo?(sledru)

Comment 13

•

3 years ago

So working backwards from the non-fatal assertion in comment 9 (where we've gotten a RemoveRenderingObserver call for an element that we're not observing), we get this sequence of calls:
(1) SVGObserverUtils::AddRenderingObserver (to add a rendering observer)
(2) SVGObserverUtils::InvalidateRenderingObservers (which clears the set)
(3) SVGObserverUtils::RemoveRenderingObserver (to remove the rendering observer that we added in (1)).

In call #3 here, we trip over the non-fatal assertion because the rendering observer set is empty (it was emptied in call #2).

Comment 14

•

3 years ago

...and actually, call #3 happens while we are inside of call #2. We are, in fact, doing some reentrant reflow, which is why we're acting on the same data in a reentrant way.

Here's a pruned version of the backtrace, at the moment when the non-fatal assertion fails, in the afore-linked pernosco trace (I'll attach the full backtrace as an attachment):

#0 mozilla::SVGObserverUtils::RemoveRenderingObserver
#1 mozilla::SVGRenderingObserver::StopObserving
#2 mozilla::SVGIDRenderingObserver::~SVGIDRenderingObserver (this=0x7f7caa26a280)
#3 mozilla::SVGRenderingObserverProperty::~SVGRenderingObserverProperty
#6 mozilla::SVGRenderingObserverProperty::Release
[...SNIP...]
#10 mozilla::FrameProperties::RemoveAll
[...SNIP...]
#16 nsInlineFrame::DestroyFrom
#17 nsBlockFrame::DoRemoveFrameInternal
#18 nsBlockFrame::DoRemoveFrame
#19 nsBlockFrame::DeleteNextInFlowChild
#20 nsLineLayout::ReflowFrame
[...SNIP...]
#27 mozilla::SVGTextFrame::DoReflow
#28 mozilla::SVGTextFrame::MaybeReflowAnonymousBlockChild
#29 mozilla::SVGTextFrame::ReflowSVGNonDisplayText 
#30 mozilla::SVGTextPathObserver::OnRenderingChange
#31 mozilla::SVGRenderingObserver::OnNonDOMMutationRenderingChange
#32 mozilla::SVGRenderingObserverSet::InvalidateAll
#33 mozilla::SVGObserverUtils::InvalidateRenderingObservers
#34 mozilla::SVGTextFrame::ReflowSVGNonDisplayText
[...SNIP...]
58 mozilla::PresShell::DoReflow

Updated

•

3 years ago

Attachment #9251427 - Attachment description: backtrace of assertion failure → backtrace of fatal assertion failure

Comment 15

•

3 years ago

Attached file backtrace of non-fatal assertion failure (from pernosco trace) — Details

Comment 16

•

3 years ago

•

Edited

A bit more about what's happening here:

We call ReflowSVGNonDisplayText for the SVG Text frame with id a.
This invalidates its rendering observers (a collection of SVGTextPathObserver objects), pulling them into a local variable like so in SVGRenderingObserverSet::InvalidateAll:

  const auto observers = std::move(mObservers);

  for (const auto& observer : observers) {
    observer->OnNonDOMMutationRenderingChange();
  }

Its first rendering observer is the next SVG text frame. As part of invalidating there, we call ReflowSVGNonDisplayText() on that next text frame.
That next text frame has two lines in its anonymous block. During this reflow, it deletes a no-longer-needed next-continuation for its textPath's inline-frame (deleting the part that was in the second line).
While deleting this frame, we of course have to delete all of its frame-properties, including its SVGTextPathObserver. This is bad since we are currently iterating over observers (many stack levels up) which includes a pointer to this object, which it hasn't yet reached in its iteration.
In the SVGTextPathObserver destructor, SVGTextPathObserver attempts to unregister from the observation target (for safety, so that it doesn't get a callback after it's been deleted). But this fails because the observation target doesn't have the observer in its mObservers collection anymore -- it has moved to the local observers variable, which we don't have access to modify.
Later on, the above-quoted for (const auto& observer : observers) { loop presumably reaches the now-deleted SVGTextPathObserver (which still has a straggling pointer that was left behind in the observers collection). We use that pointer and trigger a use-after-free when using it, and we crash.

Comment 17

•

3 years ago

•

Edited

So, the short version of comment 16 is:

We're looping over some pointers and calling OnNonDOMMutationRenderingChange on each one.
In one of these iterations, the OnNonDOMMutationRenderingChange call causes frame-deletion which deletes one of the objects that we're going to iterate over in a later iteration of the loop.
So when we get to the iteration for that object, we're hosed.

I'm not yet sure what the right fix here is. Assuming that each individual operation is essentially correct here (i.e. our line-breaking is correct, we're correct to do frame deletion, etc), we could potentially work around this by changing how we iterate somehow so that the deleted SVGTextPathObserver is actually successful at unregistering itself. (We could do this by e.g. removing observers from mObservers one at a time as we process them; or by using some sort of weak-pointer setup instead of raw pointers, or maybe something else.)

Comment 18

•

3 years ago

Alternate approach: we could probably just make SVGTextPathObserver (or rather its superclass SVGRenderingObserver) refcounted, so that it can be left alive (in some "inactivated" state) when the frame's property table is torn down.

It looks like a lot of other observer classes in SVGObserverUtils are refcounted (and we have other mObservers; collections there that store RefPtrs rather than raw pointers), so there's some prior-art for this approach being reasonable.

Comment 19

•

3 years ago

Attached patch strawman patch (avoids the crash, though it still triggers an assertion and maybe causes other problems [?]) (obsolete) — Details — Splinter Review

Here's a strawman patch, which
(1) makes us use a RefPtr in mObservers (and the local auto observers var that copies its type). This prevents us from deleting the observer when the aforementioned continuation-frame-destruction happens.

(2) Makes the observer itself use a weak pointer to keep track of the frame, so that we'll safely be able to detect that it should be skipped when we get to it in the observer : observers loop (since its frame is destroyed at that point).

We still fail one assertion (failed to track whether we're in our referenced element's observer set!) for one of the observers, which is normally fatal but which this patch downgrades for convenience.

With this patch, I'm not seeing any crashes with the attached testcase, and I'm able to reload it over and over without issue (aside from the downgraded assertion-failure).

jwatt, I probably need to seek your advice here. There's a large comment above SVGRenderingObserverSet, from bug 1495562:
https://searchfox.org/mozilla-central/rev/080e18fa4748456003164f58b0d925b8c3826a67/layout/svg/SVGObserverUtils.cpp#990-1016
...which says (among other things) that it does not store strong references to the observers, and that becomes false with my attached strawman patch, of course (per (1) above). I wonder if this change risks introducing leaks or other issues somehow, or if we should just redesign how this invalidation works to simplify things somehow per your XXXjwatt comment there?

Flags: needinfo?(jwatt)

Updated

•

3 years ago

Attachment #9259022 - Attachment description: strawman patch → strawman patch (avoids the crash, though it still triggers an assertion and maybe causes other problems [?])

Assignee

Comment 20

•

3 years ago

Pretty sure bug 1756793 is the same bug and the patch is the right way to fix this.

Depends on: 1756793

Comment 21

•

3 years ago

Thanks! That does indeed look like the same bug, yeah.

I'm glad the fix turned out to be simple; I hadn't considered that maybe we could just decline-to-observe-the-referenced-element on continuations, and I was worried that we'd have to rethink how this observer stuff is managed, per comment 17 - 18.

(Coincidentally, I was meaning to circle back to look at this this week, though I'm glad you got to it and found a simpler fix first! :))

Comment 22

•

3 years ago

In a build with bug 1756793's patch (getting the FirstContinuation in GetAndObserveTextPathsPath), I verified that I can load the attached testcase on this bug here, without crashing or asserting.

(vs. in a build without that patch, the content process aborts with the assert mentioned in comment 8.)

Updated

•

3 years ago

Flags: needinfo?(jwatt)

Updated

•

3 years ago

No longer depends on: 1756793

Updated

•

3 years ago

Blocks: 1415551

Assignee

Comment 24

•

3 years ago

Attached file Bug 1736243 - Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=jwatt,jfkthame — Details

Updated

•

3 years ago

Assignee: dholbert → emilio

Comment 25

•

3 years ago

After discussion with emilio & dveditz, I've moved emilio's patch here and marked bug 1756793 as a dupe.

Updated

•

3 years ago

Attachment #9259022 - Attachment is obsolete: true

Comment 26

•

3 years ago

Note: I'll port emilio's sec-approval request from bug 1756793 comment 3 over to this bug, now that I've duped that bug over here.

(Also: bug 1756793 comment 2 has some additional notes -- originally the extended commit message of the patch, later-redacted to avoid giving away too much -- explaining the patch a bit more.)

Ryan VanderMeulen [:RyanVM]

Comment 27

•

3 years ago

Comment on attachment 9265271 [details]
Bug 1736243 - Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=jwatt,jfkthame

Security Approval Request

How easily could an exploit be constructed based on the patch?: Not super-easily, but easily enough for someone with deep layout knowledge.
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 cleanly to all supported branches without needing any changes.
How likely is this patch to cause regressions; how much testing does it need?: not very likely.

Attachment #9265271 - Flags: sec-approval?

Updated

•

3 years ago

status-firefox95: affected → wontfix

status-firefox97: --- → wontfix

status-firefox98: --- → affected

status-firefox99: --- → affected

status-firefox-esr91: --- → affected

tracking-firefox98: --- → +

tracking-firefox99: --- → +

tracking-firefox-esr91: --- → 98+

Ryan VanderMeulen [:RyanVM]

Comment 28

•

3 years ago

Comment on attachment 9265271 [details]
Bug 1736243 - Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=jwatt,jfkthame

Approved to land and uplift

Attachment #9265271 - Flags: sec-approval? → sec-approval+

Comment 29

•

3 years ago

https://hg.mozilla.org/integration/autoland/rev/cf5e6b2865d5e7a370446b0deb00e8ef86489bac

Please go ahead and request Beta/ESR approval on this ASAP so we can get it uplifted before next week's RC builds.

Flags: needinfo?(emilio)

Assignee

Comment 30

•

3 years ago

Comment on attachment 9265271 [details]
Bug 1736243 - Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=jwatt,jfkthame

Beta/Release Uplift Approval Request

User impact if declined: Sec-sensitive crashes.
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: No
Needs manual test from QE?: Yes
If yes, steps to reproduce: Test-case here or on dupe bugs.
List of other uplifts needed: none
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): One-liner
String changes made/needed: none

Flags: needinfo?(emilio)

Attachment #9265271 - Flags: approval-mozilla-release?

Attachment #9265271 - Flags: approval-mozilla-beta?

Assignee

Updated

•

3 years ago

Flags: qe-verify+

Assignee

Comment 31

•

3 years ago

Comment on attachment 9265271 [details]
Bug 1736243 - Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=jwatt,jfkthame

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:
Fix Landed on Version: 98
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): One-liner.

Attachment #9265271 - Flags: approval-mozilla-esr91?

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

Assignee

Comment 32

•

3 years ago

Comment on attachment 9265271 [details]
Bug 1736243 - Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=jwatt,jfkthame

Err, wrong flag.

Attachment #9265271 - Flags: approval-mozilla-release?

Comment 33

•

3 years ago

Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=longsonr
https://hg.mozilla.org/integration/autoland/rev/cf5e6b2865d5e7a370446b0deb00e8ef86489bac
https://hg.mozilla.org/mozilla-central/rev/cf5e6b2865d5

Group: layout-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 3 years ago

status-firefox99: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 99 Branch

https://hg.mozilla.org/releases/mozilla-beta/rev/fa541277dd40

Comment 34

•

3 years ago

uplift

status-firefox98: affected → fixed

Mihai Boldan, Desktop QA [:mboldan]

Updated

•

3 years ago

Attachment #9265271 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

•

3 years ago

QA Whiteboard: [post-critsmash-triage] [qa-triaged]

Ciprian Georgiu, Desktop QA

Comment 35

•

3 years ago

I've reproduced this crash using an affected Nightly asan build (2021-10-17), under Ubuntu 18.04 x64.

The crash is not reproducing anymore on the latest asan builds: Beta 98.0b10 and Nightly 99.0a1.

Status: RESOLVED → VERIFIED

status-firefox98: fixed → verified

status-firefox99: fixed → verified

Flags: qe-verify+

Comment 36

•

3 years ago

Comment on attachment 9265271 [details]
Bug 1736243 - Make SVGObserverUtils::GetAndObserveTextPathsPath not observe on continuations. r=jwatt,jfkthame

Approved for ESR.

Attachment #9265271 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+

https://hg.mozilla.org/releases/mozilla-esr91/rev/3c51a47e58c2

Comment 37

•

3 years ago

uplift

status-firefox-esr91: affected → fixed

Ciprian Georgiu, Desktop QA

Comment 38

•

3 years ago

This is also verified as fixed on 91.7.0esr asan build, with Ubuntu 18.04 x64.

Ciprian Georgiu, Desktop QA

Updated

•

3 years ago

status-firefox-esr91: fixed → verified

BugBot [:suhaib / :marco/ :calixte]

Updated

•

3 years ago

Whiteboard: [adv-main98+r]

Comment 39

•

3 years ago

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(emilio)

Whiteboard: [adv-main98+r] → [adv-main98+r][sec-survey]

Updated

•

3 years ago

Whiteboard: [adv-main98+r][sec-survey] → [adv-main98+r][sec-survey][adv-esr91.7+r]

Comment 40

•

3 years ago

Attached file advisory.txt — Details

Assignee

Updated

•

3 years ago

Flags: needinfo?(emilio)

Updated

•

3 years ago

Alias: CVE-2022-26381

Frederik Braun [:freddy]

Comment 42

•

3 years ago

The ZDI plans to publish a blog post about their discovery of this bug (reported as the duplicate bug 1756793) on April 7th, tomorrow.
Any objections to opening this bug to the public?