1846197 - (CVE-2024-2616) Crash in [@ icu_73::umtx_atomic_dec]

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

Most of these reports are showing as UAFs.

Crash report: https://crash-stats.mozilla.org/report/index/7be9b933-bd0f-438f-a4c6-aad480230730

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  std::_Atomic_integral<int, 4>::fetch_add  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/atomic:1536
0  xul.dll  std::_Atomic_integral_facade<int>::fetch_sub  /builds/worker/fetches/vs/VC/Tools/MSVC/14.29.30133/include/atomic:1755
0  xul.dll  icu_73::umtx_atomic_dec  intl/icu/source/common/umutex.h:96
0  xul.dll  icu_73::UnicodeString::removeRef  intl/icu/source/common/unistr.cpp:128
0  xul.dll  icu_73::UnicodeString::releaseArray  intl/icu/source/common/unistr.cpp:138
0  xul.dll  icu_73::UnicodeString::~UnicodeString  intl/icu/source/common/unistr.cpp:439
1  xul.dll  icu_73::DecimalFormatSymbols::~DecimalFormatSymbols  intl/icu/source/i18n/dcfmtsym.cpp:139
2  xul.dll  icu_73::DecimalFormatSymbols::~DecimalFormatSymbols  intl/icu/source/i18n/dcfmtsym.cpp:138
3  xul.dll  icu_73::LocalPointer<const icu_73::DecimalFormatSymbols>::adoptInstead  intl/icu/source/common/unicode/localpointer.h:301
3  xul.dll  icu_73::DecimalFormat::touch  intl/icu/source/i18n/decimfmt.cpp:1639

Comment 1

[Daniel Veditz [:dveditz]]

First crash was in 115rc1, but it's low volume even in release so who knows when during 115 it was introduced. Two crashes in 117 nightly. A disproportionate number of crashes are in Windows 7, but that's probably because we shunted those folks onto the esr branch and iirc the esr does not throttle crash reports like the main release channel does.

Nearly all UAFs. All are releasing strings, but they're coming from different parts of ICU. Date formatting was common, for instance.

Comment 2

[Jan de Mooij [:jandem]]

ICU was updated to version 73 in Firefox 115 (bug 1824744).

Before that, we had a similar icu_72::umtx_atomic_dec signature (crash-stats).

Comment 3

[Jan de Mooij [:jandem]]

André or Dan, since you're more familiar with ICU, does anything stand out to you in these stack traces?

Comment 4

[Dan Minor [:dminor]]

I'm on PTO this week and next, I'll have a look when I'm back.

Comment 5

[André Bargull [:anba]]

I've spent some time going through the ICU source code to look for obvious mistakes, but I didn't spot anything suspicious. There could be an issue in ICU internal error handling, because ICU has a very brittle approach to check for errors. For example this code seems broken, because UErrorCode isn't passed as a reference. And if we run out-of-memory and ICU doesn't handle OOM errors correctly (bug 1552900), we could end up in an inconsistent state, where UnicodeString::fUnion::fFields::fArray has been deallocated, but the state isn't propagated to all UnicodeString instances using the shared reference. (The reference counter is stored as a std::atomic<int32_t> before UnicodeString::fUnion::fFields::fArray.)

In the browser we're calling JS_SetICUMemoryFunctions to set ICU's memory allocation functions, but we don't do that in shell builds, so shell fuzzing is unlikely to hit this bug. And we can't call JS_SetICUMemoryFunctions for shell builds, because oomTest will hit ICU OOM bugs [1, 2] right away, so we'd create ourselves a fuzz-blocker.

To rule out ICU OOM bugs, we could change ICUReporter to crash on OOM.

[1] https://unicode-org.atlassian.net/browse/ICU-20367
[2] https://unicode-org.atlassian.net/browse/ICU-22057

Comment 6

[Dan Minor [:dminor]]

I spent this morning looking through the ICU code. The recent crashes are in DateFormatSymbols and DecimalFormatSymbols. Like André said, both of these rely upon UnicodeString instances that point to shared data. In the DateFormatSymbols crashes, we're crashing in dispose but not consistently in the same place. I agree with André's analysis, it really looks like we've gotten into an inconsistent state due to an earlier error.

Unfortunately, looking at the comments on the ICU bugs linked above, it doesn't look like they see fixing error handling / OOM handling as a high priority. We could (and I will) spend some time auditing the error handling in the code around DateFormatSymbols and DecimalFormatSymbols but I'm not optimistic that this will lead to a fix. This seems to be a general problem in ICU. It's also not guaranteed we'd be able to upstream any fixes, it seems like in some cases, properly fixing things would require API changes, which seems to be in part why the two ICU bugs are stalled.

Like Jan pointed out, this isn't really a new bug, it's just that the crash signature changes with new versions of ICU, and this isn't really a new class of problem either, it seems like the situation has not improved since Bug 1552900 was filed.

I think it would be interesting to try switching ICUReporter to crash on OOM. We'd have to monitor to make sure this doesn't lead to a spike in new crash reports. Since the volume of crashes on existing bugs is small, hopefully that wouldn't be the case. Unfortunately, that would also mean we'd be waiting awhile to see if it had any impact on bugs like this one.

Unless anyone has any other suggestions, I can put together a Nightly only patch to change the ICUReporter behaviour over on Bug 1552900, and we can go from there.

Comment 7

[Dan Minor [:dminor]]

The fix on Bug 1552900 landed in Firefox 118, and so far there's no crash reports for 118, but since this is a low frequency crash, it could just be there's not enough Nightly users for this to show up. I'm continuing to check crash reports.

Comment 8

[Dan Minor [:dminor]]

Still no results for 118.

Comment 9

[Dan Minor [:dminor]]

118 still seems unaffected, I'll wait until 119 hits release before marking this resolved.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Looks like Fenix 118 has a couple reports, like https://crash-stats.mozilla.org/report/index/2c9f00ff-b0ce-4316-8f14-9e2cf0231003?

Comment 11

[Dan Minor [:dminor]]

Oops, I just noticed that the fix from Bug 1552900 was nightly only :( I was worried that the fix could cause problems elsewhere in ICU. Since that doesn't seem to be the case, I guess we can let it ride to release, and then see if it fixes this problem. Sorry for the oversight.

Comment 12

[Dan Minor [:dminor]]

I filed Bug 1858496.

Comment 13

[Dan Minor [:dminor]]

The change in Bug 1858496 has hit release. The OOM crash rates seem relatively low, 37 as of right for Firefox 121. The results for the icu_73::umtx_atomic_dec signature end at Firefox 120. We had to backout the ICU74 update because of a web compat problem (see Bug 1859752), so ICU73 is still current.

Unless we see a big spike in crashes for ICU OOM now that 121 is in release, I think we can safely call this bug fixed by Bug 1858496.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Did we want to backport the OOM behavior change to ESR? The reports from 122 don't look like UAFs at least!

Comment 15

[Dan Minor [:dminor]]

Yes, I that makes sense to me. I requested ESR uplift on Bug 1858496. Crashing isn't great, but it's better than UAF.

Comment 16

[Tom Ritter [:tjr] (OOTO until 9/2)]

Attachment: advisory.txt

Comment 17

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter