1843847 - clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Timothy Nikkel (:tnikkel)]

We store a few raw pointers to ASRs that are kept around from one paint to the next. All of them but mCurrentContainerASR are saved and restored to their previous value using a RAII class (since they are init'ed to null this clears them by the time the paint is complete). mCurrentContainerASR however is set to the result of a call to ActiveScrolledRoot::PickAncestor in the destructor of the RAII class that manages it.

https://searchfox.org/mozilla-central/rev/7a4c08f2c3a895c9dc064734ada320f920250c1f/layout/painting/nsDisplayList.h#1233

And there is another places in the code that assigns directly to it without a RAII class that I can see, although I think this code is currently not active due to the value of a pref.

We should just clear it at the end of a paint.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1843847. Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r?mstange,#gfx-reviewers

Comment 2

[Daniel Veditz [:dveditz]]

Not sure if you're describing an independent "sec-high" bug from bug 1840362, or more of a "sec-audit" latent bug. Not marked as a "task" so "sec-other" seems wrong.

Comment 3

[Timothy Nikkel (:tnikkel)]

Bug 1840362 seems to be a pretty broad bug dealing with ActiveScrolledRoot related crashes that could be sec issues, some of them look like cpu bugs, some look like bit flips, some can't be categorized as a cpu bug or bit flip.

This bug could be a UAF relating to ASRs (and hence could show up as one of the crashes from bug 1840362), so sec-high seems reasonable for this bug (erring on the side of caution).

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9344179 [details]
Bug 1843847. Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r?mstange,#gfx-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: patch pretty clearly identifies a field that needs to be cleared. an attacker would have to get that field to be deleted. which is not trivial, but probably possible.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: trivial
• How likely is this patch to cause regressions; how much testing does it need?: very unlikely
• Is Android affected?: Yes

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 9344179 [details]
Bug 1843847. Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r?mstange,#gfx-reviewers

sec-approval+ = dveditz

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9344179 [details]
Bug 1843847. Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r?mstange,#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: possible uaf
• Is this code covered by automated tests?: Yes
• 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): clearing a variable that needs to be initialized fresh at the start of any paint and should not have been kept around
• String changes made/needed: none
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: potential uaf
• Fix Landed on Version: 117
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): clearing a variable that needs to be initialized fresh at the start of any paint and should not have been kept around

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30bca7d40376
Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r=mstange

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/30bca7d40376

Comment 9

[Dianna Smith [:diannaS]]

Comment on attachment 9344179 [details]
Bug 1843847. Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r?mstange,#gfx-reviewers

Approved for 116.0b7

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/releases/mozilla-beta/rev/3c22e85e8da2
Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r=mstange,a=dsmith

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9344179 [details]
Bug 1843847. Clear nsDisplayListBuilder::mCurrentContainerASR at the end of a paint. r?mstange,#gfx-reviewers

Approved for 102.14esr
Approved for 115.1esr

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/469951a8bde7

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/bfcbc3a31178