1525509 - UAF / hash-table modification during iteration / poisoning from FlushPendingScrollAnchorAdjustments (AccessibleCaretManager flushing on a scroll observer)

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

==24230==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000018 (pc 0x7f3b8d91bdc5 bp 0x7fffe54378f0 sp 0x7fffe5437700 T0)
==24230==The signal is caused by a READ memory access.
==24230==Hint: address points to the zero page.
    #0 0x7f3b8d91bdc4 in get src/obj-firefox/dist/include/nsCOMPtr.h
    #1 0x7f3b8d91bdc4 in operator nsIContent * src/obj-firefox/dist/include/nsCOMPtr.h:831
    #2 0x7f3b8d91bdc4 in GetContent src/layout/generic/nsIFrame.h:715
    #3 0x7f3b8d91bdc4 in mozilla::layout::FindScrollAnchoringBoundingRect(nsIFrame const*, nsIFrame*) src/layout/generic/ScrollAnchorContainer.cpp:100
    #4 0x7f3b8d91a2ac in FindScrollAnchoringBoundingOffset src/layout/generic/ScrollAnchorContainer.cpp:174:7
    #5 0x7f3b8d91a2ac in mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() src/layout/generic/ScrollAnchorContainer.cpp:366
    #6 0x7f3b8d68b6ef in FlushPendingScrollAnchorAdjustments src/layout/base/PresShell.cpp:2589:26
    #7 0x7f3b8d68b6ef in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4190
    #8 0x7f3b86d45fe3 in FlushPendingNotifications src/layout/base/nsIPresShell.h:595:5
    #9 0x7f3b86d45fe3 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) src/dom/base/Document.cpp:7064
    #10 0x7f3b86d9ed6f in FlushPendingNotifications src/dom/base/Document.cpp:6995:3
    #11 0x7f3b86d9ed6f in GetPrimaryFrame src/dom/base/Element.cpp:226
    #12 0x7f3b86d9ed6f in mozilla::dom::Element::GetScrollFrame(nsIFrame**, mozilla::FlushType) src/dom/base/Element.cpp:667
    #13 0x7f3b86da32a6 in mozilla::dom::Element::GetClientAreaRect() src/dom/base/Element.cpp:997:28
    #14 0x7f3b89851793 in ClientHeight src/obj-firefox/dist/include/mozilla/dom/Element.h:1277:50
    #15 0x7f3b89851793 in mozilla::dom::Element_Binding::get_clientHeight(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitGetterCallArgs) src/obj-firefox/dom/bindings/ElementBinding.cpp:3548
    #16 0x7f3b8a0eb064 in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3034:13
    #17 0x7f3b91dcee3d in CallJSNative src/js/src/vm/Interpreter.cpp:441:13
    #18 0x7f3b91dcee3d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:533
    #19 0x7f3b91dd1462 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:604:8
    #20 0x7f3b9295f8e6 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2620:10
    #21 0x7f3b84d37d20 in Call src/obj-firefox/dist/include/jsapi.h:1860:10
    #22 0x7f3b84d37d20 in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const src/js/xpconnect/wrappers/XrayWrapper.cpp:2095
    #23 0x7f3b92a11406 in getInternal src/js/src/proxy/Proxy.cpp:342:19
    #24 0x7f3b92a11406 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:350
    #25 0x7f3b92a11643 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #26 0x7f3b92a11643 in getInternal src/js/src/proxy/Proxy.cpp:338
    #27 0x7f3b92a11643 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:350
    #28 0x7f3b92a11643 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #29 0x7f3b92a11643 in getInternal src/js/src/proxy/Proxy.cpp:338
    #30 0x7f3b92a11643 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:350
    #31 0x7f3b92a11643 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #32 0x7f3b92a11643 in getInternal src/js/src/proxy/Proxy.cpp:338
    #33 0x7f3b92a11643 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:350
    #34 0x7f3b92a12c68 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #35 0x7f3b92a12c68 in getInternal src/js/src/proxy/Proxy.cpp:338
    #36 0x7f3b92a12c68 in js::ProxyGetPropertyByValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:367
    #37 0x1a387e009abf  (<unknown module>)

Comment 1

[Ryan Hunt [:rhunt]]

I wasn't able to reproduce this with that test case, but this is a speculative fix.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1525509 - Don't refresh anchor offset after scrolling if we lost our anchor node. r?dholbert

Comment 3

[Daniel Holbert [:dholbert]]

I can't reproduce either. I tried my local debug build (non-ASAN) as well as an ASAN-enabled opt build that I downloaded from treeherder.

Superficially it seems scary for frame destruction to happen in this scenario... (though maybe it'd be mitigated by frame poisoning).

I came up with some simple testcases locally with custom onscroll handlers that tried to get up to mischief during a scroll anchor adjustment, but AFAICT we fire the anchoring-triggered-scroll-event after the scroll anchoring adjustment has been fully completed. (which is good, I think; I was worried that we might synchronously fire it during our mScrollFrame->ScrollTo(...) call).

Tyson, can you still reproduce, and do you know if there's anything else required to trigger this?

Comment 4

[Tyson Smith [:tsmith]]

Ah yep it requires "layout.accessiblecaret.enabled=true" (/me high fives :emilio)

Comment 5

[Daniel Holbert [:dholbert]]

Thanks. With that, I can repro with your testcase, in a normal nightly build.
crash report: bp-acd2d1e4-b4e6-4e5c-bf80-2b5d00190212

Comment 6

[Daniel Holbert [:dholbert]]

In a debug build, I get a fatal assertion failure before the crash:

Assertion failure: IsIdle(oldState), at ../../../mozilla/xpcom/ds/PLDHashTable.h:137

This is happening during frame destruction, and might be a sign of us working with deleted memory (not sure).

--> Marking this bug as security-sensitive, and withdrawing r+ on the proposed band-aid (which wouldn't help with this assertion failure).

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: backtrace of fatal PLDHash assertion failure

Here's the backtrace of the fatal assertion failure.

Notably, we've got ScrollAnchorContainer::ApplyAdjustments at stacklevel 60, calling ScrollTo() to apply an adjustment, which then triggers nsDocShell::NotifyScrollObserver, which in this case causes us to flush layout & recreate frames for some reason, including calling nsHTMLScrollFrame::DestroyFrom on maybe even the same scrollframe whose adjustment we're still in the process of applying.

So by the time we pop back up to stacklevel 60, 'this' will probably be a deleted pointer anyway. (But we get into trouble before that, as noted by the hashtable assertion failure.)

Comment 8

[Daniel Holbert [:dholbert]]

Aha, so the hashtable assertion is happening because we're modifying the hashtable while we're iterating over it.

Stack level 61 is FlushPendingScrollAnchorAdjustments() which has:

void nsIPresShell::FlushPendingScrollAnchorAdjustments() {
  for (auto iter = mPendingScrollAnchorAdjustment.Iter(); !iter.Done();
       iter.Next()) {
    nsIScrollableFrame* scroll = iter.Get()->GetKey();
    scroll->GetAnchor()->ApplyAdjustments();

And while we're inside of that ApplyAdjustments call, we end up trying to modify the "mPendingScrollAnchorAdjustment" table that our for-loop is currently iterating over. This happens ~55 stack levels deeper, in nsIPresShell::NotifyDestroyingFrame, in this call:

    nsIScrollableFrame* scrollableFrame = do_QueryFrame(aFrame);
    if (scrollableFrame) {
      mPendingScrollAnchorSelection.RemoveEntry(scrollableFrame);
      mPendingScrollAnchorAdjustment.RemoveEntry(scrollableFrame);
    }

(In my backtrace, the nsIScrollableFrame pointers are actually different between these two stack-levels -- i.e. the scrollable frame that we're currently destroying isn't the same one that we're currently doing scroll-anchor adjustments for. But it's still bad, because mutation-during-iteration is unsafe for many data structures, presumably including this hash table.)

One part of the solution might be for FlushPendingScrollAnchorAdjustments() to iterate over a copy of the hashtable.

Tangent: initially, I was going to say that we even transfer the contents of the hashtable to a local variable and iterate over that -- but that's actually not a good idea, because if we have two adjustments queued up and the first adjustment kills off the second adjustment's scrollframe, then we'll end up still calling GetAnchor()->ApplyAdjustments on that second (destroyed!) scrollframe, because NotifyDestroyingFrame won't have been able to remove it.

So: perhaps we want to: [EDIT: never mind, we don't want this. We really need something that prevents layout flushing entirely, per emilio's comment 9]

• copy mPendingScrollAnchorAdjustment and iterate over the copy.
• At the start of each loop, check whether the scrollframe that we're currently iterating over still exists in mPendingScrollAnchorAdjustment (i.e. see if NotifyDestroyingFrame removed it) -- and if it doesn't, then skip this loop iteration.
• And on top of that, I think we also need the change that you've already got in your patch (making ApplyAdjustments() friendly to things having been cleared).

Or alternately/instead, maybe we need to decouple the scrollframe adjustment and the (currently-synchronous) caret layout flush? I don't know enough about the caret & scroll-observers to know how feasible that is though.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

FlushPendingAnchorAdjustments should not flush layout at all, if so we're hosed, the RestyleManager method it's called from relies on it.

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

GetTextFrameForContent() (in stack level #45 in comment 7) has a chance to flush frames even if aFlushLayout=false, which could lead to frame destruction.

Emilio, is that intended to fix bug 1413361?

https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/dom/base/nsRange.cpp#2741

Comment 11

[Daniel Holbert [:dholbert]]

Alternately/also, it looks like AccessibleCaretManager already has a member "mAllowFlushingLayout", added in https://hg.mozilla.org/mozilla-central/rev/267a452439f5 , with commit message "Disallow flushing layout in reflow and scroll related callbacks."

This safety variable gets checked and can trigger an early-return via the FlushLayout() check in DispatchCaretStateChangedEvent, which is stack level #49 in my attached backtrace:

void AccessibleCaretManager::DispatchCaretStateChangedEvent(
    CaretChangedReason aReason) {
  if (!FlushLayout()) {
    return;
  }

Perhaps we can coopt that mechanism (or part of it) to prevent the layout flush here?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #10)

GetTextFrameForContent() (in stack level #45 in comment 7) has a chance to flush frames even if aFlushLayout=false, which could lead to frame destruction.

Emilio, is that intended to fix bug 1413361?

https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/dom/base/nsRange.cpp#2741

Great catch, that clearly shouldn't run either at that point. That can reconstruct all the frame tree basically synchronously, no bueno :(

Note that that happened too before my patch, my patch just made it not absolutely unsound by not using stale style information.

(In reply to Daniel Holbert [:dholbert] from comment #11)

Alternately/also, it looks like AccessibleCaretManager already has a member "mAllowFlushingLayout", added in https://hg.mozilla.org/mozilla-central/rev/267a452439f5 , with commit message "Disallow flushing layout in reflow and scroll related callbacks."
[snip]
Perhaps we can coopt that mechanism (or part of it) to prevent the layout flush here?

Note that that's not enough though, if mAllowFlushingLayout is false we won't run layout, but we'll still return true from FlushLayout().

Comment 13

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: We haven't shipped scroll-anchoring yet and this is a very tricky security issue.

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I think we should fix GetTextFrameForContent() and have it not flush any layout or frame if aFlushLayout=false.

The caller CollectClientRectsAndText up on the stack already flush layout, so maybe this could be fixed by replacing

  if (aFlushLayout) {
    doc->FlushPendingNotifications(FlushType::Layout);
  } else if (frameWillBeUnsuppressed) {
    doc->FlushPendingNotifications(FlushType::Frames);
  }

with something like

  if (aFlushLayout && frameWillBeUnsuppressed) {
    doc->FlushPendingNotifications(FlushType::Frames);
  }

Anyway, nsLayoutUtils::GetSelectionBoundingRect() (called by AccessibleCaretManager) already requests CollectClientRectsAndText to not flush layout (if I read it correctly).

Comment 15

[Emilio Cobos Álvarez (:emilio)]

If we want to go that route (I don't know the implications of collapsible whitespace not showing up there, but it's probably fine... the text frame will not be reflowed anyway if aFlushLayout is false).

We should skip calling EnsureFrameForTextNodeYadaYada entirely.

So the logic should probably look:

if (aFlushLayout) {
  if (EnsureFrameForTextNode..()) {
    doc->FlushPendingNotifications(FlushType::Layout);
  }
}

And just return null otherwise. I can write that patch and see how try looks like.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Taking, we should probably also ensure that this is the only scroll observer that flushes. It'd be bad if we miss some other.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3331b31c994ca4f3ce9aa6896a31b7c2cd07e912

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1525509 - Don't rebuild the frame tree to un-suppress whitespace if we cannot flush layout.

The information we'd get would be meaningless anyway in a fully new frame tree
if we don't run layout.

We should consider whether we need to un-suppress whitespace at all here...

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1525509 - Add release asserts.

Just for my sanity. I think the other scroll observer is sane after a quick
look, but this will ensure we don't ship security issues.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(Didn't request review for the first patch since I don't yet know if it'd be green. I suspect it will be though)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9043749 [details]
Bug 1525509 - Don't rebuild the frame tree to un-suppress whitespace if we cannot flush layout.

Security Approval Request

How easily could an exploit be constructed based on the patch?

Not too easily, but not incredibly hard either. Hard to measure :)

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?

66

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

Bug 1305957

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

Should apply cleanly.

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

Not very. Worst case it causes safe crashes (due to the second patch adding assertions) instead of security issues.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

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

Comment 23

[Emilio Cobos Álvarez (:emilio)]

My assertions uncovered a pre-existing issue with XUL trees, hopefully they're not content-exposed so should be less severe. In any case to land the second patch here I need to land bug 1528199 first.

Landed the first one here:

https://hg.mozilla.org/integration/autoland/rev/15158d921194

I'm going to ask uplift for all three patches because I really think we should put these release assertions in place, and the patch in that bug is really simple and only affects XUL trees.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9043749 [details]
Bug 1525509 - Don't rebuild the frame tree to un-suppress whitespace if we cannot flush layout.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1305957

User impact if declined

Security 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

See comment 0 + toggling accessiblecaret prefs.

List of other uplifts needed

Bug 1528199

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The patch itself is simple. Second patch should cause any other remaining related unsoundness to cause safe crashes instead of security-sensitive crashes. Bug 1528199 is needed to make those assertions hold (gotta love XUL layout), but is also simple.

Alternative would be not to ship scroll anchoring in 66 I guess.

String changes made/needed

none

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/15158d921194

Comment 26

[Emilio Cobos Álvarez (:emilio)]

https://hg.mozilla.org/integration/autoland/rev/95b6997c334a

Comment 27

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/95b6997c334a

Comment 28

[Cristian Craciun]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:67.0) Gecko/20100101 Firefox/67.0
Build ID: 20190218094427

Verified as fixed on the latest Nightly build.

Please note that I didn't managed to reproduce the issue on the Beta build. Also, the testcase file is displayed differently when loaded on the Beta build and on the latest Nighlty build (after the fix).

Comment 29

[Julien Cristau [:jcristau]]

Looks like 1528199 and 1528613 need a beta uplift request for this to go in?

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Yes, want me to do separate requests for them?

Comment 31

[Julien Cristau [:jcristau]]

Please.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9043749 [details]
Bug 1525509 - Don't rebuild the frame tree to un-suppress whitespace if we cannot flush layout.

OK for uplift for beta 10.
I think the patches from bug 1528199 and bug 1528613 need to land first.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d720aecf3197
https://hg.mozilla.org/releases/mozilla-beta/rev/e4f640b123bb

Comment 34

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,

Managed to reproduce this issue with 67.0a1 from 2019-02-05(buildID:20190205215922) on Windows 10x64.

Confirming this issue as verified fixed on 66.0b10(buildID:20190221023827) on Windows 10x64, Ubuntu 18.04 x64 and macOS 10.13.