1850938 - Crash in [@ JSContext::verifyIsSafeToGC] with ContentEventHandler

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

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

6 crashes from 3 installations of Firefox 118.0 Nightly and Developer Edition (diagnostic assert). Same signature had been fixed in bug 1414762 several years ago.

Crash report: https://crash-stats.mozilla.org/report/index/94d17df9-1d8c-4e86-8d2e-e73360230830

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(!inUnsafeRegion) ([AutoAssertNoGC] possible GC in GC-unsafe region)

Top 10 frames of crashing thread:

0  XUL  JSContext::verifyIsSafeToGC  js/src/vm/JSContext.h:518
0  XUL  js::RunScript  js/src/vm/Interpreter.cpp:419
0  XUL  js::InternalCallOrConstruct  js/src/vm/Interpreter.cpp:612
0  XUL  InternalCall  js/src/vm/Interpreter.cpp:647
0  XUL  js::Call  js/src/vm/Interpreter.cpp:679
1  XUL  JS_CallFunctionValue  js/src/vm/CallAndConstruct.cpp:55
1  XUL  nsXPCWrappedJS::CallMethod  js/xpconnect/src/XPCWrappedJSClass.cpp:918
2  XUL  PrepareAndDispatch  xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_darwin.cpp:117
3  XUL  SharedStub  
4  ?  @0x000000012705135f  

Comment 1

[Andrew McCreight [:mccr8]]

I see ContentEventHandler and IME on the stack, so perhaps this is a regression from bug 1845215.

Comment 2

[Andrew McCreight [:mccr8]]

The stacks are pretty impressive, involving basically every part of Gecko...

Comment 3

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

:masayuki, since you are the author of the regressor, bug 1845215, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: This is a diagnostic assert right now, but it is potentially much worse than that after early beta.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Ah, that could cause loading font files. I think that the method should be marked as MOZ_CAN_RUN_SCRIPT.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Although I'm not 100% sure, this could cause UAF whitch is a traditional bug. It depends on whether the observer causes running content script or never.

Comment 7

[Andrew McCreight [:mccr8]]

I figured it wouldn't be a security issue in builds with the diagnostic asserts but I'm not really sure how that works.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

ContentEventHandler retrieves caret height with an API of nsIFrame. However, the nsIFrame itself can be destroyed during a call of nsIFrame::GetCaretBlockAxisMetrics(). Therefore, for example, here may call aFrame->IsBrFrame() after aFrame is deleted.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Sorting out the fact that.

The crash is a new regression of bug 1845215. I'll post a patch to fix the crash. On the other hand, the root cause of the crash is, The GFX APIs which call gfxFontGroup::GetFirstValidFont() may run scripts of HTTP request open observers. The callers are all nsFontMetrics methods. So, a lot of layout code and DOM code use them with nsIFrames and they don't check whether the frame is destroyed or not with AutoWeakFrame.

I think that I cannot fix all callers of the APIs due to too many callers. However, at least I need to fix the regression part. Currently, I have a draft patch for all callers of nsIFrame::GetCaretBlockAxisMetrics(). I guess that this does not let attackers know the "true" root cause.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Ah, or just changing only in ContentEventHandler could be safer for hiding the root cause.

Comment 11

[Andrew McCreight [:mccr8]]

This is only on Nightly and beta, so we should be able to fix it quickly, so don't worry too much about ensuring that the fix obscures the underlying issue.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Thank you. According to bug 1851118 comment 1, we should consider the GFX API won't cause running script. Therefore, I'll just "fix" the issue in ContentEventHandler.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Bug 1850938 - Make `ContentEventHandler::OnQueryTextRectArray` and `ContentEventHandler::OnQueryTextRect` work with strong pointers again r=smaug!

Only them may run script. Therefore, they need to use PostContentIterator
and SimpleRange instead of UnsafePreContentIterator and UnsafeSimpleRange.

Additionally, they use ContentEventHandler::ConvertFlatTextOffsetToDOMRange
which returns DOMRangeAndAdjustedOffsetInFlattenedText but it stores a range
and a text node with raw pointers. Therefore, it needs to be a template
class to store them with or without strong pointers. Therefore, this patch
makes it and name UnsafeDOMRangeAndAdjustedOffsetInFlattenedText for the
raw pointers version.

Then, ConvertFlatTextOffsetToDOMRange needs to be a template method too
since it returns DOMRangeAndAdjustedOffsetInFlattenedText or
UnsafeDOMRangeAndAdjustedOffsetInFlattenedText.

Finally, GetFirstFrameInRangeForTextRect and GetLastFrameInRangeForTextRect
need to be template methods too because they need to take SimpleRange in
the methods, but UnsafeSimpleRange in the other methods.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 9351522 [details]
Bug 1850938 - Make ContentEventHandler::OnQueryTextRectArray and ContentEventHandler::OnQueryTextRect work with strong pointers again r=smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This patch does not touch around the root cause. So it's hard to investigate which method may run script in the scope of changing some template class types in the methods.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: beta
• If not all supported branches, which bug introduced the flaw?: Bug 1845215
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: All automated tests which run the paths passed on the try sever. (And this patch does not touch the logic.)
• Is Android affected?: Yes

Beta/Release Uplift Approval Request

• User impact if declined: A potential security issue.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch making the paths use strong pointers instead of raw pointers in the template classes.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 15

[Tom Ritter [:tjr]]

Comment on attachment 9351522 [details]
Bug 1850938 - Make ContentEventHandler::OnQueryTextRectArray and ContentEventHandler::OnQueryTextRect work with strong pointers again r=smaug!

Approved to land

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/867a65521beb
Make `ContentEventHandler::OnQueryTextRectArray` and `ContentEventHandler::OnQueryTextRect` work with strong pointers again r=smaug

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/867a65521beb

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9351522 [details]
Bug 1850938 - Make ContentEventHandler::OnQueryTextRectArray and ContentEventHandler::OnQueryTextRect work with strong pointers again r=smaug!

Approved for 118.0b6, thanks.

Comment 19

[Pulsebot]

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