1846685 - (CVE-2023-5169) Missing buffer size check in NEXT_PARAMS

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[sonakkbi]

Attachment: ASAN.txt

|PathOps| is used by graphics rendering. In certain situations, |StreamToSink()|, |TransformedCopy()| and |AsCircle()| are called. They all use |NEXT_PARAMS| MACRO to parse |PathOps|1,2,3. There is no buffer size check before reading. It leads to out-of-bound Read4.

REPRODUCE
Operating System: Windows
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]]

If I'm understanding, this bug allows reading unintentional data into the graphics operation, which I presume then gets turned into the image we're creating. Is there any way to get this data back? You can turn a non-tainted canvas back into an image and then try to read it from that. But the images we return have lossy compression -- how well can specific memory values (which might look like image noise) be recoverable?

I'll call it sec-high for now, but with some doubt about whether this is realistic

Comment 4

[sonakkbi]

I believe this is a problem worth contemplating.
Leaked data from |NEXT_PARAMS| is stored in |aPathSink|(|builder|)'s |mSink| and |mCurrentPoint| 1,2. A point to note is that there appears to be a connection between |PathBuilderD2D|'s |mSink| and |mGeometry| 4,5. Furthermore, the |mGeometry| and |mCurrentPoint| are stored in |Path|'s |mGeometry| and |mEndPoint| 6,7.

|Path|'s |mGeometry| is used in various renderings of DrawTargetD2D1 8,9,10, and these outcomes are assumed to be stored in |mBitmap| 11. Additionally, |mBitmap| can be passed to |SourceSurfaceD2D1| 12, and the data of |SourceSurface|, finally, can be sent to the Content Process 13.

This memory leak, unlike bug 1846686, is not straightforward and cannot be executed in a single step. Nevertheless, it's important to carefully consider that this vulnerability doesn't cause an immediate crash upon an out-of-bound read; theoretically, and under some assumptions, I think it's worth paying attention to the fact that it gives information to the content process in any form in the end.

Moreover, since not every step has been validated, please take into account the possibility of inaccuracies.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Can you take a look, Bob?

Comment 6

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Bug 1846685: Add PathOps::CheckedStreamToSink and use for event playback. r=jgilbert!

Comment 7

[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 10

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 9348888 [details]
Bug 1846685: Add PathOps::CheckedStreamToSink and use for event playback. r=jgilbert!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The issue is fairly obvious, although how it might be triggered is less obvious and would require a content process to be compromised.
  Unclear how much useful information might be extracted.
• 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, not a totally trivial patch, but it only adds a relatively small number of extra checks. Most of them in a systematic way though a macro.
• Is Android affected?: Unknown

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 9348888 [details]
Bug 1846685: Add PathOps::CheckedStreamToSink and use for event playback. r=jgilbert!

sec-approval+ = dveditz

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee62282ee19c
Add PathOps::CheckedStreamToSink and use for event playback. r=jrmuizel

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[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 15

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 9348888 [details]
Bug 1846685: Add PathOps::CheckedStreamToSink and use for event playback. r=jgilbert!

Beta/Release Uplift Approval Request

• User impact if declined: Possible information leak from gpu process to compromised content process.
• 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 not a totally trivial patch, but it only adds a relatively small number of extra checks. Most of them in a systematic way though a macro.
• String changes made/needed: None
• Is Android affected?: Unknown

Comment 16

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 9348888 [details]
Bug 1846685: Add PathOps::CheckedStreamToSink and use for event playback. r=jgilbert!

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: Possible information leak from gpu process to compromised content process.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low as not a totally trivial patch, but it only adds a relatively small number of extra checks. Most of them in a systematic way though a macro.

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9348888 [details]
Bug 1846685: Add PathOps::CheckedStreamToSink and use for event playback. r=jgilbert!

Approved for 118.0b6, thanks.

Comment 18

[Pulsebot]

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

Comment 19

[Pascal Chevrel:pascalc]

Comment on attachment 9348888 [details]
Bug 1846685: Add PathOps::CheckedStreamToSink and use for event playback. r=jgilbert!

Approved for ESR 115.3, thanks.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/b793da058736

Comment 21

[Tom Ritter [:tjr]]

Attachment: advisory.txt