1842658 - (CVE-2023-4049) Clean up some manual AddRef/Release implementations

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

While reading over the refcnt implementations in https://phabricator.services.mozilla.com/D182992, I noticed some of the implementations were manual and in some cases a bit sketchy when they didn't need to be.

This patch is to track cleaning them up, and potentially fixing race issues in atomic refcounts which were not being handled correctly.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1842658 - Part 1: Use a refcounting macro for imgCacheEntry, r=emilio!

It appears that when this was originally added in bug 430061, the inline
refcounting macros didn't exist yet.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1842658 - Part 2: Use refcounting macros for nsHtml5OwningUTF16Buffer, r=mccr8!

When the code was originally added in bug 1842658, it was to use non-atomic
refcounting across multiple threads due to some invariants preventing
concurrent access. That was changed in bug 1607762 to fix a race issue.

We should be able to switch to using the macros now, which will simplify the
code here a bit.

Depends on D183203

Comment 3

[Andrew McCreight [:mccr8]]

Nika pointed out that this can lead to double frees (which could lead to UAFs), if the refcount is at 2, then 2 threads decrement, then they both read the refcount as zero, so I'm going to mark this sec-high. I don't know how easily exploitable that is for these particular classes. These don't look directly exposed to web content, but they can certainly be created and destroyed by it.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Specifically the sec-high part is only part 2, part 1 should have no changes due to it never being a threadsafe refcnt.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9343147 [details]
Bug 1842658 - Part 2: Use refcounting macros for nsHtml5OwningUTF16Buffer, r=mccr8!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would be quite difficult to. The issue is due to multiple atomic loads during a Release function of a platform object.
• 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?: Bug 1607762
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I expect that the patch will apply cleanly on all impacted branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions
• Is Android affected?: Yes

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 9343147 [details]
Bug 1842658 - Part 2: Use refcounting macros for nsHtml5OwningUTF16Buffer, r=mccr8!

Approved to land and uplift if desired.

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9343146 [details]
Bug 1842658 - Part 1: Use a refcounting macro for imgCacheEntry, r=emilio!

Approved to land and uplift if desired.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9343147 [details]
Bug 1842658 - Part 2: Use refcounting macros for nsHtml5OwningUTF16Buffer, r=mccr8!

Beta/Release Uplift Approval Request

• User impact if declined: Potential race condition leading to UAF.

Only part 2 could actually lead to a security issue, so part 1 could be not uplifted if desired, but they uplift well as a package.

• Is this code covered by automated tests?: No
• 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): Change to use battle-tested reference counting macros rather than manually implemented reference counting on some objects.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potential race condition leading to UAF.

Only part 2 could actually lead to a security issue, so part 1 could be not uplifted if desired, but they uplift well as a package.

• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Change to use battle-tested reference counting macros rather than manually implemented reference counting on some objects.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/642d09cfa740
Part 1: Use a refcounting macro for imgCacheEntry, r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/509bf5b39d3f
Part 2: Use refcounting macros for nsHtml5OwningUTF16Buffer, r=mccr8

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/642d09cfa740
https://hg.mozilla.org/mozilla-central/rev/509bf5b39d3f

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9343146 [details]
Bug 1842658 - Part 1: Use a refcounting macro for imgCacheEntry, r=emilio!

Approved for 116.0b6

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/46e2e4de8d2b
https://hg.mozilla.org/releases/mozilla-beta/rev/d41ddd1d61e2

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9343147 [details]
Bug 1842658 - Part 2: Use refcounting macros for nsHtml5OwningUTF16Buffer, r=mccr8!

Approved for 115.1esr

Comment 14

[Pulsebot]

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

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9343147 [details]
Bug 1842658 - Part 2: Use refcounting macros for nsHtml5OwningUTF16Buffer, r=mccr8!

Approved for 102.14esr

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/39238cdf16e8

Comment 17

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt