heap memory leak in memory shared with compromised content process due to wrong GetPreparedMap
Categories
(Core :: Graphics, defect)
Tracking
()
People
(Reporter: sonakkbi, Assigned: bobowen)
References
Details
(4 keywords, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main118+])
Attachments
(5 files)
14.52 KB,
text/plain
|
Details | |
328 bytes,
text/html
|
Details | |
1.50 KB,
patch
|
Details | Diff | Splinter Review | |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
pascalc
:
approval-mozilla-esr115+
dveditz
:
sec-approval+
|
Details | Review |
293 bytes,
text/plain
|
Details |
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
Updated•1 year ago
|
Updated•1 year ago
|
Comment 3•1 year ago
|
||
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.
Comment 5•1 year ago
|
||
Nika might have some thoughts on the Port aspect of this.
Comment 6•1 year ago
|
||
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.
Updated•1 year ago
|
Comment hidden (obsolete) |
Comment 8•1 year ago
|
||
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.
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 10•1 year ago
|
||
Assignee | ||
Comment 11•1 year ago
|
||
(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•1 year ago
|
||
The severity field is not set for this bug.
:bhood, could you have a look please?
For more information, please visit BugBot documentation.
Assignee | ||
Comment 13•1 year ago
|
||
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
Updated•1 year ago
|
Comment 14•1 year ago
|
||
Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!
sec-approval+ = dveditz
Comment 15•1 year ago
|
||
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/autoland/rev/65aa391e52d7 Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel
Comment 16•1 year ago
|
||
Updated•1 year ago
|
Comment 17•1 year ago
|
||
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
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 18•1 year ago
|
||
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
Assignee | ||
Comment 19•1 year ago
|
||
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.
Updated•1 year ago
|
Comment 20•1 year ago
|
||
Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!
Approved for 118.0b6, thanks.
Updated•1 year ago
|
Comment 22•1 year ago
|
||
Comment on attachment 9348913 [details]
Bug 1846686: Remove DataSourceSurfaces when adding SourceSurfaces to CanvasTranslator. r=jrmuizel!
Approved for ESR 115.3, thanks.
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 24•1 year ago
|
||
Updated•8 months ago
|
Updated•8 months ago
|
Updated•3 months ago
|
Description
•