1902305 - (CVE-2024-6606) Missing an array index check in GetClipboardCacheIfValid (trusting data from content process)

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P2)

Description

[dalmurino]

Attachment: patch.diff

It is a simple yet critical out-of-bound read. In |GetClipboardCacheIfValid()|, |mCaches| array is accessed without an index check1. Given that the value of |aClipboardType| can be controlled, an attacker could use it to trigger the vulnerability.
Unlike ordinary OOB Read vulnerabilities that involves only a single read, the vulnerability leads to several reads from the out-of-bound memory and, more importantly, it returns the pointer to the out-of-bound memory. Then the caller continues to use the malicious pointer not knowing what has happened.

For a simple patch to address this vulnerability, I suggest using nsTArray instead of a legacy array.

REPRODUCTION CASE
Type of vulnerbility: Browser Process
Operating System: Mac OS (Only Mac OS has the widget.clipboard.use-cached-data.enabled feature enabled by default. If want to test in other OS, the feature should be enabled in about:config)

To reproduce the crash please follow next steps:

1. Apply patch.diff
2. Open test.html in browser
   // patch.diff emulates a compromised content process

When running this PoC on ASAN build, you can get an ASAN report like attached asan.txt

Comment 1

[dalmurino]

Attachment: test.html

Comment 2

[dalmurino]

Attachment: asan.txt

Comment 3

[Tom Schuster]

Attachment: WIP: Bug 1902305 - Make nsIClipboard ClipboardType an enum

Comment 4

[Tom Schuster]

We should probably check IsClipboardTypeSupported in nsBaseClipboard::HasDataMatchingFlavors and switching mCaches to mozilla::Array (which always does bounds checking) probably wouldn't hurt either.

However I think the main problem is that we allow sending an arbitrary ClipboardType over IPC. See my attached POC patch that fixes that. We probably want to change the IDL for getClipboard, emptyClipboard etc. to also use the enum type.

Comment 5

[Edgar Chen [:edgar]]

Attachment: Bug 1902305 - Check if clipboard type is supported in `nsBaseClipboard::HasDataMatchingFlavors`; r?tschuster

Comment 6

[Edgar Chen [:edgar]]

Attachment: Bug 1902305 - Use `mozilla::Array` in ClipboardCache; r?tschuster

Depends on D213766

Comment 7

[Daniel Veditz [:dveditz]]

the main problem is that we allow sending an arbitrary ClipboardType over IPC. [...] We probably want to change the IDL for getClipboard, emptyClipboard etc. to also use the enum type.

+1 to that!

Comment 8

[Daniel Veditz [:dveditz]]

If the malicious index happens to point at valid memory that is shaped right, this could lead to RCE. But could a child process influence (or guess!) what was allocated near the clipboard's mCache? If it's a global service that should have been allocated long before, and is probably in the heap with other global service things allocated around the same time.

Comment 9

[Edgar Chen [:edgar]]

(In reply to Daniel Veditz [:dveditz] from comment #7)

the main problem is that we allow sending an arbitrary ClipboardType over IPC. [...] We probably want to change the IDL for getClipboard, emptyClipboard etc. to also use the enum type.

+1 to that!

It is bug 1809713.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 9407498 [details]
Bug 1902305 - Check if clipboard type is supported in nsBaseClipboard::HasDataMatchingFlavors; r?tschuster

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It shouldn't be too hard since the patch shows the issue is related to arrays and handling invalid types.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 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?: Patches should simply be applied.
• How likely is this patch to cause regressions; how much testing does it need?: Changes are straightforward, should be safe.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 9407499 [details]
Bug 1902305 - Use mozilla::Array in ClipboardCache; r?tschuster

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It shouldn't be too hard since the patch shows the issue is related to arrays and handling invalid types.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 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?: Patches should simply be applied.
• How likely is this patch to cause regressions; how much testing does it need?: Changes are straightforward, should be safe.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 12

[dalmurino]

(In reply to Daniel Veditz [:dveditz] from comment #8)

If the malicious index happens to point at valid memory that is shaped right, this could lead to RCE. But could a child process influence (or guess!) what was allocated near the clipboard's mCache? If it's a global service that should have been allocated long before, and is probably in the heap with other global service things allocated around the same time.

I agree with your opinion. Simply using public heap grooming techniques may not be sufficient to place the necessary exploit data directly adjacent to the |mCaches| array, which is a member variable of the global service |nsClipboard| object.
Instead, the out-of-bound range for this index is not just a few bytes. It can go out-of-bound within the int32_t range, so I think that the potential for exploitation should be considered.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 9407498 [details]
Bug 1902305 - Check if clipboard type is supported in nsBaseClipboard::HasDataMatchingFlavors; r?tschuster

sec-approval+ = dveditz

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a95cff346047
Check if clipboard type is supported in `nsBaseClipboard::HasDataMatchingFlavors`; r=tschuster
https://hg.mozilla.org/integration/autoland/rev/f654d2ab11dd
Use `mozilla::Array` in ClipboardCache; r=tschuster

Comment 15

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

The bug is marked as tracked for firefox128 (beta) and tracked for firefox129 (nightly). However, the bug still has low priority.

:hsinyi, could you please increase the priority for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 16

[Edgar Chen [:edgar]]

I am working on it, I will request uplift once the patches get landed in nightly.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/f654d2ab11dd
https://hg.mozilla.org/mozilla-central/rev/a95cff346047

Comment 18

[Phabricator Automation]

Comment on attachment 9407466 [details]
WIP: Bug 1902305 - Make nsIClipboard ClipboardType an enum

Revision D213742 was moved to bug 1809713. Setting attachment 9407466 [details] to obsolete.

Comment 19

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

The patch landed in nightly and beta is affected.
:edgar, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox128 to wontfix.

For more information, please visit BugBot documentation.

Comment 20

[Edgar Chen [:edgar]]

Attachment: Bug 1902305 - Check if clipboard type is supported in `nsBaseClipboard::HasDataMatchingFlavors`; r?tschuster

Original Revision: https://phabricator.services.mozilla.com/D213766

Comment 21

[Edgar Chen [:edgar]]

Attachment: Bug 1902305 - Use `mozilla::Array` in ClipboardCache; r?tschuster

Original Revision: https://phabricator.services.mozilla.com/D213767

Comment 22

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: malicious content process can pass a malicious index to read out-of-bound memory
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: The changes are straightforward, the risk should be pretty low.
• Explanation of risk level: Low
• String changes made/needed: None
• Is Android affected?: yes

Comment 23

[Edgar Chen [:edgar]]

Attachment: Bug 1902305 - Check if clipboard type is supported in `nsBaseClipboard::HasDataMatchingFlavors`; r?tschuster

Original Revision: https://phabricator.services.mozilla.com/D213766

Comment 24

[Edgar Chen [:edgar]]

Attachment: Bug 1902305 - Use `mozilla::Array` in ClipboardCache; r?tschuster

Original Revision: https://phabricator.services.mozilla.com/D213767

Comment 25

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: malicious content process can pass a malicious index to read out-of-bound memory
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: The changes are straightforward, the risk should be pretty low.
• Explanation of risk level: Low
• String changes made/needed: None
• Is Android affected?: yes

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/957abd738d53
https://hg.mozilla.org/releases/mozilla-beta/rev/91ab421b8b89

Comment 27

[Edgar Chen [:edgar]]

I checked the code again, it turns out esr 115 should not be affected, as this is a sort of regression of bug 1835059 (depends on bug 1810850 as well). And in esr115, we check if the type is valid in each platform's implementation, so we don't need the https://phabricator.services.mozilla.com/D214254 for esr115. And https://phabricator.services.mozilla.com/D214255 is an optional patch to prevent accessing out-of-bound memory, but it should not happen with STR in comment #0 in esr115.

Comment 28

[Simon Friedberger [:simonf]]

Attachment: advisory.txt