1837993 - (CVE-2023-37209) AddressSanitizer: heap-use-after-free [@ mozilla::dom::CanonicalBrowsingContext::NotifyOnHistoryReload] with READ of size 8

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 115.0a1-20230509215006-https://hg.mozilla.org/mozilla-central/rev/44770d5c9e91a75746e5d62aa1a933859292b77e.

For detailed crash information, see attachment.

This is an older report from mid-May that I missed earlier. Could be that we fixed this, but I'd like to double-check.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Andrew McCreight [:mccr8]]

Here's the older version of CanonicalBrowsingContext.cpp.

Both the use and the free are in NotifyOnHistoryReload. Based on the line numbers, it looks like this is what is happening:

// 1. We get a reference into mLoadingEntries.
const LoadingSessionHistoryEntry& loadingEntry =
    mLoadingEntries.LastElement();
// 2. This causes us to CreateLoadInfo, appending to mLoadingEntries and causing it to do a reallocation.
aLoadState.emplace(
    WrapMovingNotNull(RefPtr{CreateLoadInfo(loadingEntry.mEntry)})); 
aReloadActiveEntry.emplace(false);
if (aForceReload) {
  // 3. Now we use the dead reference loadingEntry.
  SessionHistoryEntry::LoadingEntry* entry =
      SessionHistoryEntry::GetByLoadId(loadingEntry.mLoadId);

Comment 3

[Andrew McCreight [:mccr8]]

I'm not sure how exploitable this is, given that there's only a few lines of code in between the free and the use. You'd have to be running code on a worker and somehow allocating a new object that was just released on another thread?

I didn't see any crashes on poison values in the last 6 months with RecvNotifyOnHistoryReload in the proto signature, for what it is worth.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1837993, ensure correct loadId is used when removing entry, r=peterv

Comment 6

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

ensure correct loadId is used when removing entry, r=peterv
https://hg.mozilla.org/integration/autoland/rev/0274a8c06c67e5ee1dccdb4e151ccf0ec70af43f
https://hg.mozilla.org/mozilla-central/rev/0274a8c06c67

Comment 7

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

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

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

For more information, please visit BugBot documentation.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9339757 [details]
Bug 1837993, ensure correct loadId is used when removing entry, r=peterv

Beta/Release Uplift Approval Request

• User impact if declined: unsafe crash
• 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): This shouldn't be risky at all, just storing a value on stack before passing it to a method. That way it should be guaranteed that the value is correct.

(Release versions of Fenix are not affect, because it doesn't use Fission/session-history-in-parent)

• String changes made/needed: NA
• Is Android affected?: No

Comment 9

[Donal Meehan [:dmeehan]]

Comment on attachment 9339757 [details]
Bug 1837993, ensure correct loadId is used when removing entry, r=peterv

Approved for 115.0b9.

Comment 10

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cf4e607d5458

Comment 11

[Donal Meehan [:dmeehan]]

:smaug could you add an esr uplift request on this and a patch based on esr?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

This is not needed on esr102. It doesn't have https://searchfox.org/mozilla-central/rev/3424c000a7ff304b2d1efb8561a924232f7f12fc/docshell/base/CanonicalBrowsingContext.cpp#1021-1022

Comment 13

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt