1848833 - Use-after-free crash in [@ nsTArray_Impl<T>::AppendElementInternal<T> | nsTArray<T>::AppendElement | mozilla::net::nsHttpChannel::OnCacheEntryCheck]

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/5c96af1c-f48c-419d-8e0c-c00ac0230809

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsTArray_Impl<nsTString<char>, nsTArrayInfallibleAllocator>::AppendElementInternal<nsTArrayInfallibleAllocator, const nsTString<char>&>  xpcom/ds/nsTArray.h:2693
1  xul.dll  nsTArray<nsTString<char> >::AppendElement  xpcom/ds/nsTArray.h:2829
1  xul.dll  mozilla::net::nsHttpChannel::OnCacheEntryCheck  netwerk/protocol/http/nsHttpChannel.cpp:3978
2  xul.dll  mozilla::net::CacheEntry::InvokeCallback  netwerk/cache2/CacheEntry.cpp:746
3  xul.dll  mozilla::net::CacheEntry::InvokeCallbacks  netwerk/cache2/CacheEntry.cpp:674
4  xul.dll  mozilla::net::CacheEntry::InvokeCallbacks  netwerk/cache2/CacheEntry.cpp:616
5  xul.dll  mozilla::net::CacheEntry::OnFileReady  netwerk/cache2/CacheEntry.cpp:501
6  xul.dll  mozilla::net::CacheFile::OnMetadataRead  netwerk/cache2/CacheFile.cpp:645
7  xul.dll  mozilla::net::CacheFileMetadata::OnDataRead::<lambda_6>::operator const  netwerk/cache2/CacheFileMetadata.cpp:618
7  xul.dll  mozilla::ScopeExit<`lambda at /builds/worker/checkouts/gecko/netwerk/cache2/CacheFileMetadata.cpp:616:59'>::~ScopeExit  mfbt/ScopeExit.h:106

17 crashes like this in the last 6 months. They look like they are on the cache thread. Maybe there's a missing mutex, or this callback needs a stronger reference? I haven't looked at the code at all. It seems a bit suspicious that in this gigantic function the UAFs are happening on the access to this specific field mRedirectedCachekeys.

Comment 1

[Andrew McCreight [:mccr8]]

Much lower volume, but the stack looks similar for this signature [@ nsTArray_base<T>::Length | nsTArray_Impl<T>::AppendElementInternal<T> | nsTArray<T>::AppendElement | mozilla::net::nsHttpChannel::OnCacheEntryCheck ]: bp-6624433a-01ed-47ba-aec6-8e3680230803

Comment 2

[Andrew McCreight [:mccr8]]

Another rarer variation [@ nsTArray_Impl<T>::AppendElementInternal<T> | nsTArray<T>::AppendElement<T> | mozilla::net::nsHttpChannel::OnCacheEntryCheck ]: bp-e903b4ce-d7e7-41db-8972-9870b0230810

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Given:

    if (!mRedirectedCachekeys) {
      mRedirectedCachekeys = MakeUnique<nsTArray<nsCString>>();
    } else if (mRedirectedCachekeys->Contains(cacheKey)) {
      doValidation = true;
    }

    // Append cacheKey if not in the chain already
    if (!doValidation) {
      mRedirectedCachekeys->AppendElement(cacheKey); *** crash address

We must have had an mRedirectedCachekeys, and it didn't contain the new cacheKey. However, mRedirectedCachekeys is a UniquePtr<>, and so far as I can tell it can only get created modified in this code (which seems good), or when we do SetupReplacementChannel, which transfers the chain over from another HttpChannel:

      rv = httpInternal->SetCacheKeysRedirectChain(
          mRedirectedCachekeys.release());

and we can clear it:

  inline void CleanRedirectCacheChainIfNecessary() {
    mRedirectedCachekeys = nullptr;
  }

So how does it end up pointing to a released nsTArray? I don't see any obvious multithread access here. If something were to delete the entire HttpChannel while we're in it (from another thread presumably) maybe it's possible. Maybe. But I would expect a lot of failures in lots of different places in that case, not just here

Comment 4

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

If mRedirectedCachekeys does get released by SetupReplacementChannel, the only way I see us calling OnCacheEntryCheck after that point is via Race-Cache-With-Network. But if that's the case, I would have expected us to exit early here.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

mRedirectedCachekeys can only be set from MakeUnique<>, or from SetCacheKeysRedirectChain(), which is only called via SetCacheKeysRedirectChain(mRedirectedCacheKeys.release()). So there should be no reference to the mRedirectedCacheKeys floating around to let it be deleted.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

So, the problem is almost certainly that we're accessing mRedirectedCacheKeys on both the Cache2 IO thread and on MainThread (for the call to SetCacheKeysRedirectChain()). I'll add a mutex

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1848833: Clean up mRedirectedCacheKeys r=valentin!

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9354169 [details]
Bug 1848833: Clean up mRedirectedCacheKeys r=valentin!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard, but probably not impossible
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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?: No
• If not, how different, hard to create, and risky will they be?: Trivial
• How likely is this patch to cause regressions; how much testing does it need?: Extremely unlikely
• Is Android affected?: Yes

Beta/Release Uplift Approval Request

• User impact if declined: Security risk
• 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): DataMutex locking of a var like this is safe; no possible deadlocks.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9354169 [details]
Bug 1848833: Clean up mRedirectedCacheKeys r=valentin!

Approved to land and uplift

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97dc7be3b93c
Clean up mRedirectedCacheKeys r=valentin,necko-reviewers

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/97dc7be3b93c

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f7bdadffc66

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/6ed5e994ed5e

Comment 14

[Daniel Veditz [:dveditz]]

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