Closed Bug 1421420 Opened 3 years ago Closed 3 years ago

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


(Core :: Layout, defect)

50 Branch
Not set



Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed


(Reporter: dholbert, Assigned: bradwerth)



(Keywords: csectype-framepoisoning, sec-other, Whiteboard: [adv-main58-][post-critsmash-triage])


(1 file, 1 obsolete file)

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

nsGridContainerFrame::GetGridFrameWithComputedInfo(nsIFrame* aFrame)

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

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:

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:,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

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?
Closed: 3 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.