Closed Bug 1515113 Opened 2 years ago Closed 1 year ago

use-after-free in [@ nsIPresShell::GetRootScrollFrame]

Categories

(Core :: Layout, defect, P2)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + fixed
firefox66 + fixed

People

(Reporter: tsmith, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(4 files, 1 obsolete file)

Attached file testcase.html
I don't have access to an ASan build on android yet so this will have to do.
Crash|SIGSEGV|0x610072|12
0|libxul.so|nsIPresShell::GetRootScrollFrame() const|hg:hg.mozilla.org/mozilla-central:layout/base/nsFrameManager.h:8f91fb1acea5ed5585a8502599e51adfc1618de2|47|0x0
1|libxul.so|nsIPresShell::GetRootScrollFrameAsScrollable() const|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2423|0xb
2|libxul.so|MobileViewportManager::ShrinkToDisplaySizeIfNeeded(nsViewportInfo&, mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&)|hg:hg.mozilla.org/mozilla-central:layout/base/MobileViewportManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|512|0xb
3|libxul.so|MobileViewportManager::RefreshViewportSize(bool)|hg:hg.mozilla.org/mozilla-central:layout/base/MobileViewportManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|497|0x16
4|libxul.so|MobileViewportManager::RequestReflow()|hg:hg.mozilla.org/mozilla-central:layout/base/MobileViewportManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|122|0xd
5|libxul.so|mozilla::PresShell::ResizeReflow(int, int, int, int, nsIPresShell::ResizeReflowOptions)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1866|0x8
6|libxul.so|nsViewManager::DoSetWindowDimensions(int, int)|hg:hg.mozilla.org/mozilla-central:view/nsViewManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|182|0x17
7|libxul.so|nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned int)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2042|0x14
8|libxul.so|nsDocumentViewer::SetBounds(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2065|0x10
9|libxul.so|nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned int)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2057|0xf
10|libxul.so|nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|5171|0xe
11|libxul.so|non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5
12|libxul.so|nsFrameLoader::UpdateBaseWindowPositionAndSize(nsSubDocumentFrame*)|hg:hg.mozilla.org/mozilla-central:dom/base/nsFrameLoader.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2321|0x29
13|libxul.so|nsFrameLoader::UpdatePositionAndSize(nsSubDocumentFrame*)|hg:hg.mozilla.org/mozilla-central:dom/base/nsFrameLoader.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2295|0xa
14|libxul.so|nsSubDocumentFrame::ReflowFinished()|hg:hg.mozilla.org/mozilla-central:layout/generic/nsSubDocumentFrame.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|818|0xc
15|libxul.so|non-virtual thunk to nsSubDocumentFrame::ReflowFinished()|hg:hg.mozilla.org/mozilla-central:layout/generic/nsSubDocumentFrame.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5
16|libxul.so|mozilla::PresShell::HandlePostedReflowCallbacks(bool)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|3885|0x8
17|libxul.so|mozilla::PresShell::DidDoReflow(bool)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|8350|0xf
18|libxul.so|mozilla::PresShell::ResizeReflowIgnoreOverride(int, int, int, int, nsIPresShell::ResizeReflowOptions)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1995|0x10
19|libxul.so|nsViewManager::DoSetWindowDimensions(int, int)|hg:hg.mozilla.org/mozilla-central:view/nsViewManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|182|0x17
20|libxul.so|nsDocumentViewer::SetBoundsWithFlags(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, unsigned int)|hg:hg.mozilla.org/mozilla-central:layout/base/nsDocumentViewer.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2042|0x14
21|libxul.so|nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|5171|0xe
22|libxul.so|non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)|hg:hg.mozilla.org/mozilla-central:docshell/base/nsDocShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5
23|libxul.so|nsWebShellWindow::WindowResized(nsIWidget*, int, int)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsWebShellWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|290|0x11
24|libxul.so|nsWebShellWindow::WidgetListenerDelegate::WindowResized(nsIWidget*, int, int)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsWebShellWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|740|0x16
25|libxul.so|nsWindow::OnSizeChanged(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&)|hg:hg.mozilla.org/mozilla-central:widget/android/nsWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1817|0xc
26|libxul.so|nsWindow::Resize(double, double, double, double, bool)|hg:hg.mozilla.org/mozilla-central:widget/android/nsWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1620|0xc
27|libxul.so|nsWindow::Resize(double, double, bool)|hg:hg.mozilla.org/mozilla-central:widget/android/nsWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1604|0x28
28|libxul.so|nsXULWindow::SetSize(int, int, bool)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsXULWindow.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|580|0x1c
29|libxul.so|nsContentTreeOwner::SetSize(int, int, bool)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsContentTreeOwner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|512|0x11
30|libxul.so|non-virtual thunk to nsContentTreeOwner::SetSize(int, int, bool)|hg:hg.mozilla.org/mozilla-central:xpfe/appshell/nsContentTreeOwner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|0|0x5
31|libxul.so|nsGlobalWindowOuter::ResizeByOuter(int, int, mozilla::dom::CallerType, mozilla::ErrorResult&)|hg:hg.mozilla.org/mozilla-central:dom/base/nsGlobalWindowOuter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4905|0x18
32|libxul.so|nsGlobalWindowInner::ResizeBy(int, int, mozilla::dom::CallerType, mozilla::ErrorResult&)|hg:hg.mozilla.org/mozilla-central:dom/base/nsGlobalWindowInner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|3480|0x13
33|libxul.so|mozilla::dom::Window_Binding::resizeBy(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&)|s3:gecko-generated-sources:609c24cdbf77eb87c64e5f2116444d78efa5fe9e90b3d5ae351d3b3934a74fa3ac1b3a0c4ab6ed86a1d829abac8bfe58e28e82dc186b3a16ac48868a75285da8/dom/bindings/WindowBinding.cpp:|3919|0x11
34|libxul.so|bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)|hg:hg.mozilla.org/mozilla-central:dom/bindings/BindingUtils.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|3064|0xd
35|libxul.so|CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|443|0xe
36|libxul.so|js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|535|0xb
37|libxul.so|InternalCall(JSContext*, js::AnyInvokeArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|590|0x14
38|libxul.so|Interpret(JSContext*, js::RunState&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|594|0x8
39|libxul.so|js::RunScript(JSContext*, js::RunState&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|423|0x9
40|libxul.so|js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|563|0xd
41|libxul.so|InternalCall(JSContext*, js::AnyInvokeArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|590|0x14
42|libxul.so|<name omitted>|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|606|0x7
43|libxul.so|JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/jsapi.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|2649|0x2a
44|libxul.so|mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&)|s3:gecko-generated-sources:344e7bbc1d2eb535f4d85b157b525fa18eb1e4688bb70bf6e3c51b02fe597784d3fe3e6a2b078d2e5b5c857d2435e216ad8a578cae88f140c6e1c2a969833ac5/dom/bindings/EventHandlerBinding.cpp:|265|0x24
45|libxul.so|void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*)|s3:gecko-generated-sources:3760fb8938acb537e28af191191b3b28eae59c21a2b0dcc49f0990b2613d2b29a3f705864d9a9ad5a160e5faa013e2d0b447215b3fc0b25795d7ebbd0dc4ce29/dist/include/mozilla/dom/EventHandlerBinding.h:|363|0x1e
46|libxul.so|mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*)|hg:hg.mozilla.org/mozilla-central:dom/events/JSEventHandler.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|205|0x36
47|libxul.so|mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*)|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1044|0xc
48|libxul.so|mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)|hg:hg.mozilla.org/mozilla-central:dom/events/EventListenerManager.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1239|0x1d
49|libxul.so|mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|346|0x18
50|libxul.so|mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|548|0x18
51|libxul.so|mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)|hg:hg.mozilla.org/mozilla-central:dom/events/EventDispatcher.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1038|0x23
52|libxul.so|(anonymous namespace)::AsyncTimeEventRunner::Run()|hg:hg.mozilla.org/mozilla-central:dom/smil/nsSMILTimedElement.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|96|0x19
53|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|1157|0x8
54|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|468|0x11
55|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|88|0x10
56|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:8f91fb1acea5ed5585a8502599e51adfc1618de2|314|0xb
57|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:8f91fb1acea5ed5585a8502599e51adfc1618de2|307|0xb
58|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|137|0x9
59|libxul.so|nsAppStartup::Run()|hg:hg.mozilla.org/mozilla-central:toolkit/components/startup/nsAppStartup.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|271|0x9
60|libxul.so|XREMain::XRE_mainRun()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4616|0x8
61|libxul.so|XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4754|0x8
62|libxul.so|XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|4839|0xf
63|libxul.so|GeckoStart|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAndroidStartup.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|47|0xd
64|libxul.so|mozilla::BootstrapImpl::GeckoStart(_JNIEnv*, char**, int, mozilla::StaticXREAppData const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/Bootstrap.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|71|0x11
65|libmozglue.so|Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun|hg:hg.mozilla.org/mozilla-central:mozglue/android/APKOpen.cpp:8f91fb1acea5ed5585a8502599e51adfc1618de2|371|0x18
66|base.odex||||0x8eeab7
67|||||0xed4ec0e4
68|libart.so||||0x735b84
69|||||0xffffffff
70|dalvik-main space (deleted)||||0x1422a0
71|libart.so||||0x313fc3
72|dalvik-LinearAlloc (deleted)||||0x98b8
Flags: in-testsuite?
+ Maire while Sean is out.
The attached test case hits the following assertion but unreduced (and other) versions trigger the above crash at different addresses.

Assertion failure: get() (dereferencing a UniquePtr containing nullptr), at src/obj-firefox/dist/include/mozilla/UniquePtr.h:302
Hi Cameron -- I'm hoping to get this fixed soon.  Thoughts about what's going on here?
Flags: needinfo?(cam)
The testcase didn't seem to crash my Fennec Beta (65). Possible regression?
Severity: normal → critical
Tyson, does the testcase require some non-default prefs?
(we don't allow window.resizeBy by default, do we?)
Flags: needinfo?(twsmith)
Attached file prefs.js
Here is the prefs.js file that was used. FWIW I've only been able to reproduce this with Fennec.
Flags: needinfo?(twsmith)
How do you install a prefs.js file into a Fennec installation?

I opened the testcase with just the "dom.disable_window_move_resize" pref changed manually (which seems to be the relevant one based on a cursory look), but did not reproduce the crash.
Tyson, do you know the answer to Botond's question?
Flags: needinfo?(twsmith)
Given that this requires non-default prefs, tentatively setting to P3, unless anyone thinks otherwise.
Priority: -- → P3
I've verified I can reproduce this with default prefs on the latest available m-c debug build.
BuildID=20190102094850
SourceStamp=5826b2352ac08248205d3b0e29587ab8ad415bfe

I put testcase.html on a web server and opened it on the device. It can take up to a minute to repro but opening it in multiple tabs speeds up the process.
Flags: needinfo?(twsmith)
I was able to reproduce exactly once despite many many efforts.

https://crash-stats.mozilla.org/report/index/6beb5827-d979-4b79-830f-2bb360190103

Looks like mFrameConstructor is null on the presshell (based on the stack and comment 2). Which doesn't make a lot of sense, it is cleared in ~PresShell, and created (as a UniquePtr) in PresShell::Init, which gets called pretty much immediately after we create PresShells at the one location we create PresShells.

The AccessibleCaret code observes Reflow done notifications, and the first thing is does is FlushLayout before updating the caret. Which is silly.

I created a try build here with printf_stderrs if anyone that can reproduce wants to capture logcat

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f482b6c60161a8594aec9bf0cbcc9baa3df83e6
Flags: needinfo?(twsmith)

Thanks to Tyson for running some try builds I was able to figure this out. The ResizeReflowIgnoreOverride call in MobileViewportManager::RefreshViewportSize kills the presshell and MobileViewportManager, so the call into the presshell in ShrinkToDisplaySizeIfNeeded is using deleted memory for mPresShell pointer.

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

The AccessibleCaret code observes Reflow done notifications, and the first
thing is does is FlushLayout before updating the caret. Which is silly.

Looks like that is covered by bug 1445794, but reflow could still potentially kill us via reflow callbacks that ask for a flush.

Flags: needinfo?(twsmith)
Flags: needinfo?(cam)
Attached patch patchSplinter Review
Assignee: nobody → tnikkel
Attachment #9034806 - Flags: review?(kats)
Keywords: csectype-uaf
Summary: crash in [@ nsIPresShell::GetRootScrollFrame] → use-after-free in [@ nsIPresShell::GetRootScrollFrame]

Nice!

Priority: P3 → P2

Comment on attachment 9034806 [details] [diff] [review]
patch

Review of attachment 9034806 [details] [diff] [review]:

LGTM, thanks!

::: layout/base/MobileViewportManager.cpp
@@ +523,5 @@
>    // Update internal state.
>    mMobileViewportSize = viewport;
>  
> +  // ResizeReflowIgnoreOverride can kill us.
> +  RefPtr<MobileViewportManager> strongThis(this);

you don't like the kungFuDeathGrip? :p

Attachment #9034806 - Flags: review?(kats) → review+

Comment on attachment 9034806 [details] [diff] [review]
patch

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I'll strip the commit msg and comment from the patch but not much we can do to hide this, the fix makes it obvious which object dies and what call causes that, they'd need a fuzzer or some smarts/work to trigger it though

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?: probably all of them

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: should be pretty trivial

How likely is this patch to cause regressions; how much testing does it need?: very unlikely

Attachment #9034806 - Flags: sec-approval?

(In reply to Timothy Nikkel (:tnikkel) from comment #12)

The ResizeReflowIgnoreOverride call in MobileViewportManager::RefreshViewportSize kills the presshell and MobileViewportManager, so the call into the presshell in ShrinkToDisplaySizeIfNeeded is using deleted memory for mPresShell pointer.

Oh whoops. My bad for not catching that during review.

(In reply to Timothy Nikkel (:tnikkel) from comment #16)

Which older supported branches are affected by this flaw?: probably all of them

If not all supported branches, which bug introduced the flaw?: None

The problematic call to ShrinkToDisplaySizeIfNeeded() was introduced in bug 1423709, which landed in 65, so branches older than 65 should not be affected.

Blocks: 1423709

sec-approval+ for trunk. Let's get a beta nomination on a patch so we can avoid shipping the issue.

Keywords: sec-critical
Attachment #9034806 - Flags: sec-approval? → sec-approval+
Attached patch beta patch (obsolete) — Splinter Review

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1423709

User impact if declined: sec uaf

Is this code covered by automated tests?: No

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): just holds a strong ref to this to keep it alive

String changes made/needed: none

Attachment #9035125 - Flags: approval-mozilla-beta?
Attached patch beta patchSplinter Review
Attachment #9035125 - Attachment is obsolete: true
Attachment #9035125 - Flags: approval-mozilla-beta?

Comment on attachment 9035131 [details] [diff] [review]
beta patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1423709

User impact if declined: sec uaf

Is this code covered by automated tests?: No

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): just holds a strong ref to this to keep it alive

String changes made/needed: none

Attachment #9035131 - Flags: approval-mozilla-beta?
Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Comment on attachment 9035131 [details] [diff] [review]
beta patch

[Triage Comment]
Fixes a sec-crit UAF. Approved for 65.0b10.

Attachment #9035131 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.