Closed Bug 1660211 (CVE-2020-15675) Opened 9 months ago Closed 9 months ago

Iterator invalidation potentially causing memory corruption

Categories

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

defect

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 81+ fixed
firefox79 --- wontfix
firefox80 --- wontfix
firefox81 + fixed

People

(Reporter: lukas.bernhard, Assigned: botond)

References

(Regression)

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main81+][adv-esr78.3+])

Attachments

(2 files, 1 obsolete file)

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.

Group: firefox-core-security → gfx-core-security
Component: Untriaged → Panning and Zooming
Product: Firefox → Core

Botond, could you take a look please? Thanks.

Flags: needinfo?(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.

Assignee: nobody → botond
Severity: -- → S2
Flags: needinfo?(botond)
Priority: -- → P2

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

Regressed by: 1617179

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

Flags: needinfo?(botond)
Status: UNCONFIRMED → NEW
Ever confirmed: true

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.
Flags: needinfo?(botond)
Attachment #9171446 - Flags: sec-approval?

(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.

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

(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.

(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)

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.

Keywords: sec-moderate

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.

Attachment #9171446 - Flags: sec-approval?
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Please nominate this for ESR78 when you get a chance.

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
Flags: needinfo?(botond)
Attachment #9171446 - Flags: approval-mozilla-esr78?
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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

Approved for 78.3esr.

Attachment #9171446 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main81+]
Whiteboard: [post-critsmash-triage][adv-main81+] → [post-critsmash-triage][adv-main81+][adv-esr78.3+]
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.