1846686 - (CVE-2023-5170) heap memory leak in memory shared with compromised content process due to wrong GetPreparedMap

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[sonakkbi]

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

Comment 1

[sonakkbi]

Attachment: index.html

Comment 2

[sonakkbi]

Attachment: patch.diff

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[sonakkbi]

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.

Comment 5

[Andrew McCreight [:mccr8]]

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

Comment 6

[Christian Holler (:decoder)]

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.

Comment 7

[Daniel Veditz [:dveditz]]

clearing the rating for re-triage mid-aired w/Christian

Comment 8

[Frederik Braun [:freddy]]

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.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Bob can you look at this

Comment 10

[Bob Owen (:bobowen)]

Attachment: Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!

Comment 11

[Bob Owen (:bobowen)]

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

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit BugBot documentation.

Comment 13

[Bob Owen (:bobowen)]

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

Comment 14

[Daniel Veditz [:dveditz]]

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

sec-approval+ = dveditz

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65aa391e52d7
Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel

Comment 16

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/mozilla-central/rev/65aa391e52d7

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 18

[Bob Owen (:bobowen)]

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

Comment 19

[Bob Owen (:bobowen)]

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.

Comment 20

[Pascal Chevrel:pascalc]

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

Approved for 118.0b6, thanks.

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/49565181b8c4

Comment 22

[Pascal Chevrel:pascalc]

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

Approved for ESR 115.3, thanks.

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/7386264b52d1

Comment 24

[Tom Ritter [:tjr] (OOTO until mid-August)]

Attachment: advisory.txt