Closed Bug 1846685 (CVE-2023-5169) Opened 1 year ago Closed 1 year ago

Missing buffer size check in NEXT_PARAMS

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-bounds, reporter-external, sec-high, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main118+][adv-esr115.3+])

Attachments

(5 files)

Attached file 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

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

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

Keywords: sec-high

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.

Can you take a look, Bob?

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

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)
Duplicate of this bug: 1850065
Duplicate of this bug: 1850064

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
Attachment #9348888 - Flags: sec-approval?

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

sec-approval+ = dveditz

Attachment #9348888 - Flags: sec-approval? → sec-approval+
Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ee62282ee19c Add PathOps::CheckedStreamToSink and use for event playback. r=jrmuizel
Group: gfx-core-security → core-security-release
Severity: -- → S2
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Flags: needinfo?(bhood)
Resolution: --- → FIXED
Target Milestone: --- → 119 Branch

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 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
Flags: needinfo?(bobowencode)
Attachment #9348888 - Flags: approval-mozilla-beta?

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.
Attachment #9348888 - Flags: approval-mozilla-esr115?
Flags: sec-bounty? → sec-bounty+

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

Approved for 118.0b6, thanks.

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

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

Approved for ESR 115.3, thanks.

Attachment #9348888 - 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
Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main118+] → [reporter-external] [client-bounty-form] [verif?][adv-main118+][adv-esr115.3+]
Group: core-security-release
Alias: CVE-2023-5169
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: