1660211 - (CVE-2020-15675) Iterator invalidation potentially causing memory corruption

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[lukas.bernhard]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

The function ComputeClippedCompositionBounds in APZCTreeManager.cpp is not following iterator invalidation rules, potentially causing memory corruption.
The return value of the aDestMap.insert holds an iterator to the inserted key.
https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/gfx/layers/apz/src/APZCTreeManager.cpp#902

Recursively calling ComputeClippedCompositionBounds potentially invalidates the iterator, depending on whether the insert caused a rehash of the std::unordered_map.
https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/gfx/layers/apz/src/APZCTreeManager.cpp#929

The (potentially invalid) iterator is used to insert another value.
https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/gfx/layers/apz/src/APZCTreeManager.cpp#953

I'm not sure whether an adversary is able to reach the code path while exercising sufficient control over the input variables. Hence the severity from a security standpoint remains unclear to me, feel free to de-restrict the bug report.

Comment 1

[Andrew McCreight [:mccr8]]

Botond, could you take a look please? Thanks.

Comment 2

[Botond Ballo [:botond]]

Thanks for the report! I'm used to std::map's iterator invalidation requirements and did not realize std::unordered_map had stricter requirements.

For reference, here is a link to the relevant paragraph from the standard ("Unordered associative containers" [unord.req] p14; emphasis mine):

The insert and emplace members shall not affect the validity of references to container elements, but may invalidate all iterators to the container. The erase members shall invalidate only iterators and references to the erased elements, and preserve the relative order of the elements that are not erased.

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1660211 - Respect iterator invalidation rules in ComputeClippedCompositionBounds(). r=kats

Comment 4

[Botond Ballo [:botond]]

The relevant code landed in 76, setting status flags based on that.

Comment 5

[Andrew McCreight [:mccr8]]

Thanks for the patch, Botond. Do you think a malicious web page might be able to reliably cause this invalidation to happen?

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 9171446 [details]
Bug 1660211 - Respect iterator invalidation rules in ComputeClippedCompositionBounds(). r=kats

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The potential misbehaviour being patched is accessing an invalid iterator which has similar characteristics to a UAF.

I don't have a good sense for how likely that is to occur in practice (it's going to vary from one standard library implementation to another) or how exploitable a UAF in this particular place may be.

Note: the commit message mentions "iterator invalidation", I could revise it to say something more neutral if desired.

• 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?: esr78,release,beta
• 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?: The patch should apply cleanly on all supported branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, it's a straightforward risk with very low regression potential.

Comment 7

[Botond Ballo [:botond]]

(In reply to Andrew McCreight [:mccr8] from comment #5)

Thanks for the patch, Botond. Do you think a malicious web page might be able to reliably cause this invalidation to happen?

A page does have some level of control over how many times this function will recurse (a more deeply nested scrollable element that's doing something like a smooth-scroll animation will mean more levels of recursion), and the chances of accessing an invalid iterator increase with more levels.

I'm less sure about what a page would have to do to actually exploit the invalidation.

Comment 8

[Andrew McCreight [:mccr8]]

Can page JS run while one of these iterators is on the stack? Without that, it would likely be harder to exploit.

Comment 9

[Botond Ballo [:botond]]

(In reply to Andrew McCreight [:mccr8] from comment #8)

Can page JS run while one of these iterators is on the stack? Without that, it would likely be harder to exploit.

The recursion happens in the parent process, so I would say no.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #9)

The recursion happens in the parent process, so I would say no.

(or the GPU process, if that is enabled)

Comment 11

[Andrew McCreight [:mccr8]]

Thanks. It seems like that would make it hard for a web page to set up the right conditions to exploit a UAF, so I'll mark it sec-moderate. As a sec-moderate, this won't need sec-approval.

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 9171446 [details]
Bug 1660211 - Respect iterator invalidation rules in ComputeClippedCompositionBounds(). r=kats

Unsetting sec-approval flag as the bug has been classified as sec-moderate.

Comment 13

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

https://hg.mozilla.org/integration/autoland/rev/5718234b1f27780b0d98ca4e6f1a22e1b012205a
https://hg.mozilla.org/mozilla-central/rev/5718234b1f27

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR78 when you get a chance.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 9171446 [details]
Bug 1660211 - Respect iterator invalidation rules in ComputeClippedCompositionBounds(). r=kats

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Small, low-risk fix for a sec-moderate bug
• User impact if declined: Some pages may trigger memory corruption in the parent or GPU process, leading to a crash or other unpredictable behaviour.
• Fix Landed on Version: 81
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is small, targeted, and well understood. Should apply to the esr78 branch cleanly.
• String or UUID changes made by this patch: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9171446 [details]
Bug 1660211 - Respect iterator invalidation rules in ComputeClippedCompositionBounds(). r=kats

Approved for 78.3esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/e22ad6020423

Comment 18

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 19

[Tom Ritter [:tjr]]

Attachment: advisory.txt