1846687 - (CVE-2023-4573) use-after-free in mStream

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[sonakkbi]

Attachment: ASAN.txt

While graphics rendering, |mStream| is used to receive rendering data1. |mStream| could be destroyed when initialized. However, there is no check in init function2 and it could initialize twice. This could lead to use-after-free when |mStream| is destroyed during accessing rendering data.

REPRODUCE
Operating System: All
1.apply *patch1.diff and patch2.diff
2.visit index.html

*: patch1.diff is emulating a compromised content process. patch2.diff is patching the Graphic Process to improve the success rate (just for reproducing easily)

Crash State: see asan file

Comment 1

[sonakkbi]

Attachment: index.html

Comment 2

[sonakkbi]

Attachment: patch1.diff

Comment 3

[sonakkbi]

Attachment: patch2.diff

Comment 4

[Tyson Smith [:tsmith]]

I was unable to reproduce this on Linux.

Comment 5

[sonakkbi]

I'm sorry to hear that it's not reproducible on Linux. The vulnerability itself still exists in Linux, but I overlooked that the function used in the patch.diff is not called in Linux. Could you test it on Windows?

Comment 6

[Tyson Smith [:tsmith]]

(In reply to sonakkbi from comment #5)

I'm sorry to hear that it's not reproducible on Linux. The vulnerability itself still exists in Linux, but I overlooked that the function used in the patch.diff is not called in Linux. Could you test it on Windows?

I did modify the patch to work on Linux, but perhaps I missed something. I was trying to get a rr/Pernosco session (Linux only) to hopefully save them some work. I'll leave it do a developer with more knowledge of this component to try.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: svg.html

Tyson, it might be possible to reproduce this with this test case on Linux instead of the attached one.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Bob, can you take a look at this?

Comment 9

[Tyson Smith [:tsmith]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)

Tyson, it might be possible to reproduce this with this test case on Linux instead of the attached one.

I'd be happy to give it a try.

I also had issues with the patches not applying cleanly and targeting Windows. Could you have a look at them as well please?

Comment 10

[Bob Owen (:bobowen)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)

Bob, can you take a look at this?

The patches were a bit mangled for me as well, so I applied them by hand.
I could see the issue either way from the description.
Pretty sure it will only affect Windows.

Comment 11

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #10)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)

Bob, can you take a look at this?

The patches were a bit mangled for me as well, so I applied them by hand.
I could see the issue either way from the description.
Pretty sure it will only affect Windows.

The crash I was getting turned out to be in webrender code and not a UAF.
By modifying the patch a bit I think I've now reproduced the issue raised here.
Once I've got something for this, I'll see if I can still reproduce the crash in webrender and file a separate bug.

Comment 12

[Bob Owen (:bobowen)]

Attachment: Force CanvasTranslator to reinitialize while processing.

Here's my reproduction patch that I've rolled into one.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Bug 1846687: Don't allow CanvasTranslator to be initialized twice. r=jrmuizel!

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 9347656 [details]
Bug 1846687: Don't allow CanvasTranslator to be initialized twice. r=jrmuizel!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Assuming they knew it was a sec bug, working out the problem from the patch is probably fairly easy, but not trivial.
  However, there isn't a known exploit of the vulnerability currently.
• 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?: 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?: (Patch grafts onto all required branches.)
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, it adds an extra check that should never be triggered without an exploited content process.
• Is Android affected?: No

Comment 15

[Tom Ritter [:tjr]]

Comment on attachment 9347656 [details]
Bug 1846687: Don't allow CanvasTranslator to be initialized twice. r=jrmuizel!

Approved to land and uplift

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d548ab29d331
Don't allow CanvasTranslator to be initialized twice. r=jrmuizel

Comment 17

[Bob Owen (:bobowen)]

Comment on attachment 9347656 [details]
Bug 1846687: Don't allow CanvasTranslator to be initialized twice. r=jrmuizel!

Beta/Release Uplift Approval Request

• User impact if declined: Vulnerability that can be triggered in the GPU process from a compromised content process, which could potentially be used as part of a sandbox escape.
• 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): Simple is null check added that should never fail without a compromised content process.
• String changes made/needed: None
• Is Android affected?: No

Comment 18

[Bob Owen (:bobowen)]

Comment on attachment 9347656 [details]
Bug 1846687: Don't allow CanvasTranslator to be initialized twice. 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: Vulnerability that can be triggered in the GPU process from a compromised content process, which could potentially be used as part of a sandbox escape.
• Fix Landed on Version: 118
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple is null check added that should never fail without a compromised content process.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/d548ab29d331

Comment 20

[Dianna Smith [:diannaS]]

Comment on attachment 9347656 [details]
Bug 1846687: Don't allow CanvasTranslator to be initialized twice. r=jrmuizel!

Approved for 117.0b6

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/ae4bf01c1157

Comment 22

[Dianna Smith [:diannaS]]

Comment on attachment 9347656 [details]
Bug 1846687: Don't allow CanvasTranslator to be initialized twice. r=jrmuizel!

This will be roll out with 117 and will not be included in the next 116 dot release. If for any reason we do need to take this in the dot release, please feel free to NI me.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9347656 [details]
Bug 1846687: Don't allow CanvasTranslator to be initialized twice. r=jrmuizel!

Approved for 115.2esr and 102.15esr.

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/9fa8de7e28de

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/594fcde70067

Comment 26

[June Wilde (she/her) [:jewilde]]

Attachment: advisory.txt