1538758 - Potential Use After Free in layout code need help verifying

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

Sorry to place this under the bug bounty section I wasn't sure how to file a security report.

I discovered the following potential vulnerability by code inspection. I wanted to report it as soon as possible to get the opinions of Mozilla developers and security team to see if this is worth investigating.

In the file /layout/base/presshell.cpp the function bool PresShell::ScrollFrameRectIntoView(nsIFrame* aFrame, const nsRect& aRect, nsIPresShell::ScrollAxis aVertical, nsIPresShell::ScrollAxis aHorizontal, uint32_t aFlags)

gets two raw pointers:

  bool didScroll = false;
  // This function needs to work even if rect has a width or height of 0.
  nsRect rect = aRect;
  nsIFrame* container = aFrame; //raw pointer to a frame
  // Walk up the frame hierarchy scrolling the rect into view and
  // keeping rect relative to container
  do {
    nsIScrollableFrame* sf = do_QueryFrame(container); //raw pointer to scrollable frame
    if (sf) {
      nsPoint oldPosition = sf->GetScrollPosition();
      nsRect targetRect = rect;

...SNIP...

Later on the function calls ScrollToShowRect(sf, targetRect - sf->GetScrolledFrame()->GetPosition(), aVertical, aHorizontal, aFlags);
ScrollToShowRect calls
aFrameAsScrollable->ScrollTo(scrollPt, scrollMode, &allowedRange);

The header file nsIScrollableFrame.h /layout/generic/nsIScrollableFrame.h has the following comment and function definition:

/**
   * @note This method might destroy the frame, pres shell and other objects.
   * Clamps aScrollPosition to GetScrollRange and sets the scroll position
   * to that value.
   * @param aRange If non-null, specifies area which contains aScrollPosition
   * and can be used for choosing a performance-optimized scroll position.
   * Any point within this area can be chosen.
   * The choosen point will be as close as possible to aScrollPosition.
   */
  virtual void ScrollTo(nsPoint aScrollPosition, ScrollMode aMode,
                        const nsRect* aRange = nullptr,
                        nsIScrollbarMediator::ScrollSnapMode aSnap =
                            nsIScrollbarMediator::DISABLE_SNAP) = 0;

The comment says that "this method might destroy the frame, presshell and other objects."

Back in our original function the raw pointers are used after the call to ScrollToShowRect
...SNIP...

      nsPoint newPosition = sf->LastScrollDestination();

...SNIP...

    if (container->IsTransformed()) { //

So basically the full snippet is this:

bool PresShell::ScrollFrameRectIntoView(nsIFrame* aFrame, const nsRect& aRect,
                                        nsIPresShell::ScrollAxis aVertical,
                                        nsIPresShell::ScrollAxis aHorizontal,
                                        uint32_t aFlags) {
  bool didScroll = false;
  // This function needs to work even if rect has a width or height of 0.
  nsRect rect = aRect;

  nsIFrame* container = aFrame; //raw pointer here
  // Walk up the frame hierarchy scrolling the rect into view and
  // keeping rect relative to container
  do {
    nsIScrollableFrame* sf = do_QueryFrame(container); //raw pointer here
    if (sf) {
      nsPoint oldPosition = sf->GetScrollPosition();
      nsRect targetRect = rect;
      // Inflate the scrolled rect by the container's padding in each dimension,
      // unless we have 'overflow-clip-box-*: content-box' in that dimension.
      auto* disp = container->StyleDisplay();
      if (disp->mOverflowClipBoxBlock == StyleOverflowClipBox::ContentBox ||
          disp->mOverflowClipBoxInline == StyleOverflowClipBox::ContentBox) {
        WritingMode wm = container->GetWritingMode();
        bool cbH = (wm.IsVertical() ? disp->mOverflowClipBoxBlock
                                    : disp->mOverflowClipBoxInline) ==
                   StyleOverflowClipBox::ContentBox;
        bool cbV = (wm.IsVertical() ? disp->mOverflowClipBoxInline
                                    : disp->mOverflowClipBoxBlock) ==
                   StyleOverflowClipBox::ContentBox;
        nsMargin padding = container->GetUsedPadding();
        if (!cbH) {
          padding.left = padding.right = nscoord(0);
        }
        if (!cbV) {
          padding.top = padding.bottom = nscoord(0);
        }
        targetRect.Inflate(padding);
      }
      ScrollToShowRect(sf, targetRect - sf->GetScrolledFrame()->GetPosition(),
                       aVertical, aHorizontal, aFlags);       //Frames and other objects can be deleted Maybe a UAF with the sf argument. 
      nsPoint newPosition = sf->LastScrollDestination(); //Potential UAF
      // If the scroll position increased, that means our content moved up,
      // so our rect's offset should decrease
      rect += oldPosition - newPosition;

      if (oldPosition != newPosition) {
        didScroll = true;
      }

      // only scroll one container when this flag is set
      if (aFlags & nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY) {
        break;
      }
    }
    nsIFrame* parent;
    if (container->IsTransformed()) //Potential UAF{
      container->GetTransformMatrix(nullptr, &parent);

I haven't tried to exploit this yet but I think there are a few entry points that would lead to the vulnerable call. I think anywhere the SCROLL_SYNCHRONOUS flag passed into a ScrollSelectionIntoView function would be potential vulnerable call.

  /**
   * ScrollSelectionIntoView scrolls a region of the selection,
   * so that it is visible in the scrolled view.
   *
   * @param aSelectionType the selection to scroll into view.
   *
   * @param aRegion the region inside the selection to scroll into view.
   *
   * @param aFlags the scroll flags.  Valid bits include:
   *   * SCROLL_SYNCHRONOUS: when set, scrolls the selection into view 
   *     before returning. If not set, posts a request which is processed
   *     at some point after the method returns.
   *   * SCROLL_FIRST_ANCESTOR_ONLY: if set, only the first ancestor will be
   *     scrolled into view.
   */
  /*unsafe*/
  nsresult ScrollSelectionIntoView(mozilla::SelectionType aSelectionType,
                                   SelectionRegion aRegion,
                                   int16_t aFlags) const;

Does anyone have any tips on how I could debug this or verify the issue?

Comment 1

[:Gijs (he/him)]

(I attempted to make comment #0 more readable, please see https://guides.github.com/features/mastering-markdown/ for some tips on how to do this yourself for the next item you file.)

Comment 2

[Brandon Wieser]

Nice looks good. I'll try and remember that for next time.

Comment 3

[Daniel Holbert [:dholbert]]

Mats, it looks like you added the comments about the PresShell being possibly destroyed. Do you know if that's still accurate?

I tried to come up with a local crashtest with an iframe whose presShell gets destroyed, and I couldn't get it to happen synchronously. My crashtest has this as the outer file:

<!DOCTYPE html>
<html class="reftest-wait">
<script>
  function boom() {
    myIframe.style.display = "none";
    document.documentElement.offsetTop; // flush layout
    // GC/CC for good measure, now that we've clobbered the iframe:
    SpecialPowers.forceGC();
    SpecialPowers.forceCC();
    SpecialPowers.forceGC();
    SpecialPowers.forceCC();
    document.documentElement.removeAttribute("class");
  }
</script>
<body>
  <iframe id="myIframe" src="helper.html"></iframe>
</body>

And here's the helper.html file (in the iframe):

<!DOCTYPE html>
<script>
  function setupHandler() {
    window.addEventListener('scroll', window.parent.boom);
    setTimeout(go, 500);
  }
  function go() {
    myInput.focus();
  }
</script>
<body onload="setupHandler()">
  <div style="height: 99999px; border: 3px solid teal">Tall div.</div>
  <input id="myInput" value="Text in textbox">
</body>

This crashtest completes successfully for me right now, with no trouble, apparently because the scroll event gets handled after the ScrollTo(...) call has completed. We dispatch the event to the RefreshDriver via mHelper->mOuter->PresContext()->RefreshDriver()->PostScrollEvent(this, false) in the ScrollEvent constructor.

So there's no synchronous PresShell destruction during these scroll APIs, at least not in my testcase. Mats, can you think of another scenario where we would be vulnerable?

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: strawman crashtest that does not trigger any badness from this bug (run via `./mach crashtest layout/generic/crashtests/test.html`)

Here's my strawman testcase (quoted in my previous comment) which doesn't trigger any problems right now.

I wrote it as a crashtest so that I could use SpecialPowers.forceGC/forceCC to prevent the possibility of things being kept alive via delayed-cleanup-cycles. (Though I don't think that actually happens or matters here -- so if others wnt to take this test & iterate on it, it should be fine to remove those lines.)

Comment 5

[Daniel Holbert [:dholbert]]

Also notable: the PresShell destruction happens in a slightly-delayed manner, via a nsHideViewer runnable, which gets created & posted when the outer document's nsSubDocumentFrame is destroyed. It's not part of what saves us, though, because this nsHideViewer does gets dispatched [and destroys the PresShell] synchronously, towards the end of the scroll handler's .offsetTop layout flush in my testcase -- so that bit of delay (from nsHideViewer being a posted runnable) isn't part of helping us delay PresShell destruction here.

So at the moment, the key bit of async-ness that's saving us (in my testcase at least) is the fact that we invoke the scroll handler via PostScrollEvent and don't run its JS until we hit nsRefreshDriver::DispatchScrollEvents, which is after comment 0's Scroll*** APIs have completed.

Comment 6

[Brandon Wieser]

Would these be valid entry points? They appear to lead to the vulnerable sink and call scroll sync
`
PresShell.cpp

NS_IMETHODIMP
PresShell::PageMove(bool aForward, bool aExtend) {
printf("PresShell::PageMove step 1\n");
nsIFrame* frame = nullptr;
if (!aExtend) {
frame = do_QueryFrame(GetScrollableFrameToScroll(nsIPresShell::eVertical));
// If there is no scrollable frame, get the frame to move caret instead.
}
if (!frame) {
frame = mSelection->GetFrameToPageSelect();
if (!frame) {
return NS_OK;
}
}
RefPtr<nsFrameSelection> frameSelection = mSelection;
frameSelection->CommonPageMove(aForward, aExtend, frame);
// After ScrollSelectionIntoView(), the pending notifications might be
// flushed and PresShell/PresContext/Frames may be dead. See bug 418470.
return ScrollSelectionIntoView(
nsISelectionController::SELECTION_NORMAL,
nsISelectionController::SELECTION_FOCUS_REGION,
nsISelectionController::SCROLL_SYNCHRONOUS |
nsISelectionController::SCROLL_FOR_CARET_MOVE); //this call it
}

nsTypeAheadFind.cpp
nsresult nsTypeAheadFind::FindItNow(nsIPresShell* aPresShell, bool aIsLinksOnly,
bool aIsFirstVisiblePreferred,
bool aFindPrev, uint16_t* aResult) {
...SNIP...

  // Change selection color to ATTENTION and scroll to it.  Careful: we
  // must wait until after we goof with focus above before changing to
  // ATTENTION, or when we MoveFocus() and the selection is not on a
  // link, we'll blur, which will lose the ATTENTION.
  if (selectionController) {
    // Beware! This may flush notifications via synchronous
    // ScrollSelectionIntoView.
    SetSelectionModeAndRepaint(nsISelectionController::SELECTION_ATTENTION);
    selectionController->ScrollSelectionIntoView(
        nsISelectionController::SELECTION_NORMAL,
        nsISelectionController::SELECTION_WHOLE_SELECTION,
        nsISelectionController::SCROLL_CENTER_VERTICALLY |
            nsISelectionController::SCROLL_SYNCHRONOUS);
  }

...SNIP...
`

I think there might be others too.

`

Comment 7

[Brandon Wieser]

Would these be valid entry points? They appear to lead to the vulnerable sink and call scroll sync

PresShell.cpp

NS_IMETHODIMP
PresShell::PageMove(bool aForward, bool aExtend) {
printf("PresShell::PageMove step 1\n");
nsIFrame* frame = nullptr;
if (!aExtend) {
frame = do_QueryFrame(GetScrollableFrameToScroll(nsIPresShell::eVertical));
// If there is no scrollable frame, get the frame to move caret instead.
}
if (!frame) {
frame = mSelection->GetFrameToPageSelect();
if (!frame) {
return NS_OK;
}
}
RefPtr<nsFrameSelection> frameSelection = mSelection;
frameSelection->CommonPageMove(aForward, aExtend, frame);
// After ScrollSelectionIntoView(), the pending notifications might be
// flushed and PresShell/PresContext/Frames may be dead. See bug 418470.
return ScrollSelectionIntoView(
nsISelectionController::SELECTION_NORMAL,
nsISelectionController::SELECTION_FOCUS_REGION,
nsISelectionController::SCROLL_SYNCHRONOUS |
nsISelectionController::SCROLL_FOR_CARET_MOVE); //this call it
}

nsTypeAheadFind.cpp

nsresult nsTypeAheadFind::FindItNow(nsIPresShell* aPresShell, bool aIsLinksOnly,
bool aIsFirstVisiblePreferred,
bool aFindPrev, uint16_t* aResult) {
...SNIP...

  // Change selection color to ATTENTION and scroll to it.  Careful: we
  // must wait until after we goof with focus above before changing to
  // ATTENTION, or when we MoveFocus() and the selection is not on a
  // link, we'll blur, which will lose the ATTENTION.
  if (selectionController) {
    // Beware! This may flush notifications via synchronous
    // ScrollSelectionIntoView.
    SetSelectionModeAndRepaint(nsISelectionController::SELECTION_ATTENTION);
    selectionController->ScrollSelectionIntoView(
        nsISelectionController::SELECTION_NORMAL,
        nsISelectionController::SELECTION_WHOLE_SELECTION,
        nsISelectionController::SCROLL_CENTER_VERTICALLY |
            nsISelectionController::SCROLL_SYNCHRONOUS);
  }

...SNIP...

I think there might be others too.

Fixed formating hopefully... Sorry for double post

Comment 8

[Daniel Holbert [:dholbert]]

(RE formatting + double-posting: there's a pencil icon that you might be able to use to edit your comments, at the top-right of each comment. Though you might need "editbugs" permissions to have that available -- if you don't see that pencil icon, see https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html on steps to get that permission if you like. For now, I tagged your first duplicate comment as "obsolete" which should autohide it.)

Anyway - you can try to come up with a testcase using those entry points, but I don't immediately see a reason why they'd be more effective than the entry point I'm using (input.focus, which triggers a call to nsHTMLScrollFrame::ScrollTo with aMode=nsIScrollableFrame::INSTANT)

Comment 9

[Brandon Wieser]

you said in the previous comment that "at the moment, the key bit of async-ness that's saving us (in my testcase at least) is the fact that we invoke the scroll handler via PostScrollEvent and don't run its JS until we hit nsRefreshDriver::DispatchScrollEvents, which is after comment 0's Scroll*** APIs have completed."

So the functions i posted above dispatch scrollto sync instead of async. At least that what it looks like to me. If you follow
selectionController->ScrollSelectionIntoView you get sel->scrollintoview(sync flags here) which call PostScrollSelectionIntoViewEvent(sync flags) which end up calling the vulnerable code listed above. At least I think I'm currently at work so I could be following the code wrong though. Either way I'll you all look it over. Maybe it's nothing i just noticed the comment in the code saying that it could destroy the frame and other objects and traced it backwards.

Comment 10

[Daniel Holbert [:dholbert]]

My testcase dispatches ScrollTo synchronously, too. It triggers a call to ScrollTo with nsIScrollableFrame::INSTANT, and I think that's the key thing here for making the scrolling happen synchronously, via this check:
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/layout/generic/nsGfxScrollFrame.cpp#2259-2263

That flag makes us complete the scroll right away -- though any scroll-triggered JS still happens in a delayed way (which I hand-wavily referred to as "async-ness") via "PostScrollEvent()", as I discussed above, which is why my testcase doesn't explode. The scroll itself is still sync, though.

(Perhaps SCROLL_SYNCHRONOUS is just one way of getting that ::INSTANT flag to be set. I'm not sure. In any case, though, even though the scroll itself is sync, the scroll event is async in the sense that its handler's JS doesn't get run immediately during the C++ ScrollTo call, based on my observations during my debugging session just now.)

Anyway -- the code-comments about PresShell-destruction-during-scrolling seem to date back to this comment and its attachment: bug 898871 comment 2 -- and the attachment there does seem to have PresShell destruction during ScrollTo (though it's from ~6 years ago so things may have changed, e.g. the patches that landed as part of that old bug). Anyway, hopefully Mats remembers enough context around that bug to be able to weigh in here.

Comment 11

[Mats Palmgren (inactive)]

In general, the PresShell destruction comes from setting display:none on an <iframe> or some such.
That is, if you can run script or flush pending style changes then you can also destroy PresShells.
IIRC, the reason our scroll code has been fragile is that it fires events, notifies DOM changes, etc,
which may trigger script.

I think we've fixed most of those issues, by triggering the events from the RefreshDriver,
adding nsAutoScriptBlocker on the stack, making more stuff async, Flushing upfront etc,
but it's been a while since I looked at the scroll code so I'm not sure if there are
remaining issues.

A quick peek shows that ScrollFrameHelper::SetCoordAttribute still calls SetAttr with
aNotify=true, so any callers (recursively) still needs to be prepared for that...
(e.g. ScrollToImpl->UpdateScrollbarPosition->SetCoordAttribute ... ->FlushPendingNotifications)

(The culprit in the stack in bug 898871 comment 2 was the sync FireResizeEvent call in
FlushPendingNotifications which we now do async from the RefreshDriver instead.)

Comment 12

[Mats Palmgren (inactive)]

BTW, SpecialPowers should also allow you to dispatch kbd events to trigger
the PresShell::PageMove code etc. The editor code also expose a bunch
of those through its "cmd_*" command API:
https://searchfox.org/mozilla-central/source/editor/libeditor/EditorCommands.cpp#741
which you can invoke through SpecialPowers.doCommand, see for example:
https://searchfox.org/mozilla-central/source/editor/libeditor/tests/test_selection_move_commands.html

Comment 13

[Mats Palmgren (inactive)]

(In reply to Brandon Wieser from comment #0)

In the file /layout/base/presshell.cpp the function bool PresShell::ScrollFrameRectIntoView(nsIFrame* aFrame, const nsRect& aRect, nsIPresShell::ScrollAxis aVertical, nsIPresShell::ScrollAxis aHorizontal, uint32_t aFlags)

gets two raw pointers:

  nsIFrame* container = aFrame; //raw pointer to a frame
  ...
    nsIScrollableFrame* sf = do_QueryFrame(container); //raw pointer to scrollable frame

Regarding these pointers, they both point to a nsIFrame-derived instance.
Those are specially allocated from an arena buffer owned by the presshell
itself. When a frame is "deallocated" its area in the arena is "poisoned"
but the pointer itself remains valid, in the C++ sense, since the memory
area is still alive. So as long as the caller of ScrollFrameRectIntoView
(or any of its callers) holds a strong ref on the presshell object, uses
of the above pointers, even after the frame is destroyed, are non-exploitable.
It's only if the presshell is deallocated (and thus its arena) that it
becomes potentially exploitable. Usually, callers to PresShell methods
always holds a strong ref on the stack to prevent that.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #11)

A quick peek shows that ScrollFrameHelper::SetCoordAttribute still calls SetAttr with
aNotify=true, so any callers (recursively) still needs to be prepared for that...
(e.g. ScrollToImpl->UpdateScrollbarPosition->SetCoordAttribute ... ->FlushPendingNotifications)

That SetAttr call is on the anonymous XUL content that represents the scrollbars, and I don't think (?) web content can register a mutation observer on that content, so I don't think that scenario should be exploitable (as long as we don't e.g. use privileged frontend JS to register an observer).

As a sanity-check, I just tried viewing my helper.html file with a mutation observer on <html> (registered with subtree:true, attributes:true), and I verified that the observer didn't fire for scrollbar position changes.

Comment 15

[Mats Palmgren (inactive)]

True, but there were no content mutation observes involved in bug 898871.
It's a potential call to FlushPendingNotifications that worries me:
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/layout/base/nsIPresShell.h#559-563
Not sure if that comment is actually true anymore though. As I said,
we've fixed some of those issues there through the years.

I guess we should fix it pro-actively, adding a AutoWeakFrame(container)
on the stack and checking IsAlive() after the ScrollToShowRect call.

Comment 16

[Daniel Veditz [:dveditz]]

Calling this "sec-audit" based on comment 15 ("fix is pro-actively") implying you don't know a specific triggerable vulnerability, but if you find one we can re-rate this.

Comment 17

[Mats Palmgren (inactive)]

Emilio fixed that (comment 15) independently in bug 1549812, so there's nothing more to do here.

Comment 18

[Daniel Veditz [:dveditz]]

Unfortunately we have to decline the bounty for this one. We currently consider framepoisoning bugs unexploitable and not eligible.