GetGridFrameWithComputedInfo probably needs to be more careful about data being destroyed across its shell->FlushPendingNotifications call

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: dholbert, Assigned: bradwerth)

Tracking

({csectype-framepoisoning, sec-other})

50 Branch
mozilla59
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Details

(Whiteboard: [adv-main58-][post-critsmash-triage])

Attachments

(1 attachment, 1 obsolete attachment)

Right now, nsGridContainerFrame::GetGridFrameWithComputedInfo has the following code:

==========
nsGridContainerFrame*
nsGridContainerFrame::GetGridFrameWithComputedInfo(nsIFrame* aFrame)
{
  [...]
      shell->FlushPendingNotifications(FlushType::Layout);

      // Since the reflow may have side effects, get the grid frame again.
      gridFrame = GetGridContainerFrame(aFrame);

https://dxr.mozilla.org/mozilla-central/rev/cad9c9573579698c223b4b6cb53ca723cd930ad2/layout/generic/nsGridContainerFrame.cpp#6931-6934
==========

This pattern is problematic & potentially dangerous.  FlushPendingNotifications causes events to be flushed, which means it can cause script to synchronously run and e.g. tear down the document or a piece of the frame tree.  So, by the time we get to the  "GetGridContainerFrame(aFrame)" call, aFrame may be pointing to bogus/freed memory -- so GetGridContainerFrame's usages of aFrame are potentially-unsafe in that scenario.

Fortunately in this case, frame poisoning should make any resulting crash a "safe"/non-exploitable crash. But, still worth avoiding & worrying about.

This is probably a case where we want to use AutoWeakFrame:
https://dxr.mozilla.org/mozilla-central/rev/cad9c9573579698c223b4b6cb53ca723cd930ad2/layout/generic/nsIFrame.h#4571

Or possibly a persistent nsCOMPtr<nsIContent*> which we'd use to re-discover the frame after the layout flush, as it looks like Brad's doing in the flexbox equivalent function over in bug 1409083 (modulo my nsCOMPtr review-feedback over there).
Brad, could you take this?
Flags: needinfo?(bwerth)
Thanks for finding this. I was thinking of this same issue after seeing your comments on the similar flex computed info.
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
Sure!  Yeah, I wanted to be sure my comments on the flex bug wouldn't cause a zero-day for the similar grid code. :) Was happy to see that this version *looks* like a safe frame-poisoning crash, at worst.
Comment on attachment 8932643 [details] [diff] [review]
Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflows.

Review of attachment 8932643 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me, just one commit message nit:
> Hold onto a weak reference to the grid container frame across reflows

s/reflows/grid-devtools reflow flush/

(or something like that. The current text sounds like it's doing this for all or most reflows in general.)
Attachment #8932643 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
This needs a sec rating (and possibly sec-approval depending on said rating) before it can land.
Keywords: checkin-needed
This is a good fix, but I think the issue is mostly theoretical since
this method is only called from DoGridTemplateColumns/Rows and those
properties are marked as needing a layout flush:
https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/layout/style/nsCSSPropList.h#2279,2292
so by the time this method is called all layout data is already
up-to-date.  So this (2nd) flush should be a NOP except for forcing
another reflow on the grid frame [to collect the requested data].
Keywords: sec-other
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9982f241548d0a1745e7625a4554cb76362151d1

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Blocks: 1241932
Flags: needinfo?(bwerth)
Keywords: checkin-needed
Version: Trunk → 50 Branch
Comment on attachment 8932660 [details] [diff] [review]
Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflow flushes triggered by devtools.

Approval Request Comment
[Feature/Bug causing the regression]: 1241932
[User impact if declined]: Future patches may introduce security issues if they add more triggers for this code. Current invocation patterns appear safe.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only triggered by use of specific features in devtools. 
[String changes made/needed]: None
Flags: needinfo?(bwerth)
Attachment #8932660 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/9982f241548d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8932660 [details] [diff] [review]
Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflow flushes triggered by devtools.

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