Closed Bug 1846686 (CVE-2023-5170) Opened 7 months ago Closed 6 months ago

heap memory leak in memory shared with compromised content process due to wrong GetPreparedMap

Categories

(Core :: Graphics, defect)

defect

Tracking

()

RESOLVED FIXED
119 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox-esr115 118+ fixed
firefox117 --- wontfix
firefox118 + fixed
firefox119 + fixed

People

(Reporter: sonakkbi, Assigned: bobowen)

References

Details

(Keywords: csectype-disclosure, csectype-sandbox-escape, sec-high, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main118+])

Attachments

(5 files)

Attached file ASAN.txt

While graphics rendering, |SetPreparedMap()|1, |GetPreparedMap()|2 are used to cache |dataSurface|3. In |SetPreparedMap()|, it assign the |dataSurface| to |mPreparedMap| and set corresponding |mMappedSurface|. In |GetPreparedMap()|, it checks |aSurface| has changed by "MOZ_RELEASE_ASSERT(mMappedSurface == aSurface". However, |aSurface| can be replaced between |SetPreparedMap()| and |GetPreparedMap()| with the same |mMappedSurface|. The non-corresponding |aSurface| is used with |GetPreparedMap()|4. It leads to heap memory leak and the memory is sent to the compromised content process with shared memory5.

It is verified that heap memory leak leads to sandbox escape many times, especially with ipc port leaked. There is also |Port| in firefox6 and I think firefox is not safe from this.

REPRODUCE
Operating System: All
1.apply *patch.diff
2.visit index.html

*: Patch to emulate a compromised content process.

Crash State: see asan file

Flags: sec-bounty?
Attached file index.html
Attached patch patch.diffSplinter Review
Group: firefox-core-security → gfx-core-security
Component: Security → Graphics
Product: Firefox → Core
Summary: heap memory leak due to wrong GetPreparedMap → heap memory leak in memory shared with compromised content process due to wrong GetPreparedMap

How much control do you have over what is actually leaked to the child process? There is certainly information in the parent process that could be used in a sandbox escape, but most information won't. Can this bug leak information that is useful in a sandbox escape, and would the child process be able to tell when that information is there or not?

sec-moderate seems more appropriate because this would not be a sandbox escape on its own, even assuming a compromised child process.

Thanks for your question.
As mentioned above, heap memory leak from a higher privilege process could result in sandbox escape. The well-known exploitation in browser is with leaked |Port|. The first exploitation with leaked |Port| is introduced in 1. The article says that privileged commands can be used with the leaked |Port|. Since then, there have been articles2 and presentations3,4 related to the article. Especially, 3, 4 uses a real-world bug5 to steal the password and download the file from victim6.
With the exploitation well-known, heap memory leak from a higher privilege process have begun to be considered important, from Security_Severity-Medium5 to Security_Severity-High7. 7 is a heap memory leak in GPU Process, which is similar to this vulnerability.
As far as I know, firefox also have the IPC model with |Port|8 which is the same as Chromium's. I carefully think that it has more impact than sec-moderate.

Nika might have some thoughts on the Port aspect of this.

Flags: needinfo?(nika)

Leaking ports of a higher privileged process to a compromised content process essentially allows that content process to act as that higher privileged process. There is no form of authentication other than the ports (which is a pair of uint64_t). So, if ports are leakable through this vulnerability, then it is sec-high.

Flags: needinfo?(nika)

Thank you for pushing back in comment 4, sonakkbi. We agree the bug can be used to bypass the sandbox and should be rated sec-high.

Keywords: sec-high

Bob can you look at this

Flags: needinfo?(bobowencode)
Assignee: nobody → bobowencode
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bobowencode)

(In reply to sonakkbi from comment #0)
...

... However, |aSurface| can be replaced between |SetPreparedMap()| and |GetPreparedMap()| with the same |mMappedSurface|. The non-corresponding |aSurface| is used with |GetPreparedMap()|[4]. It leads to heap memory leak and the memory is sent to the compromised content process with shared memory[5].

I think the root issue here is actually that we don't clear the cached DataSourceSurface when we add a new SourceSurface and it is that old data surface that is picked up in RecordedPrepareDataForSurface::PlayCanvasEvent.

The severity field is not set for this bug.
:bhood, could you have a look please?

For more information, please visit BugBot documentation.

Flags: needinfo?(bhood)

Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: The issue is fairly obvious, but how to trigger it is not as clear and would require a compromised content process.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
  • 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?:
  • How likely is this patch to cause regressions; how much testing does it need?: Fairly unlikely, simple changes to use height from mapped surface with a check against original surface width.
    Also, removes any old cached data surfaces or maps when adding or removing a source surface.
  • Is Android affected?: No
Attachment #9348913 - Flags: sec-approval?

Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!

sec-approval+ = dveditz

Attachment #9348913 - Flags: sec-approval? → sec-approval+
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/65aa391e52d7
Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel
Group: gfx-core-security → core-security-release
Severity: -- → S2
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 119 Branch
Flags: needinfo?(bhood)

The patch landed in nightly and beta is affected.
:bobowen, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(bobowencode)

Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!

Beta/Release Uplift Approval Request

  • User impact if declined: Information leak from gpu process to compromised content process.
    Possibly leading to further exploitation.
  • 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): Low as they are fairly simple changes to use height from mapped surface with a check against original surface width.
    Also, removes any old cached data surfaces or maps when adding or removing a source surface.
  • String changes made/needed: None
  • Is Android affected?: No
Flags: needinfo?(bobowencode)
Attachment #9348913 - Flags: approval-mozilla-beta?

Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!

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: Information leak from gpu process to compromised content process.
    Possibly leading to further exploitation.
  • Fix Landed on Version: 119
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low as they are fairly simple changes to use height from mapped surface with a check against original surface width.
    Also, removes any old cached data surfaces or maps when adding or removing a source surface.
Attachment #9348913 - Flags: approval-mozilla-esr115?
Flags: sec-bounty? → sec-bounty+

Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!

Approved for 118.0b6, thanks.

Attachment #9348913 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!

Approved for ESR 115.3, thanks.

Attachment #9348913 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][adv-main118+]
Attached file advisory.txt
Group: core-security-release
Alias: CVE-2023-5170
You need to log in before you can comment on or make changes to this bug.