1655115 - Heap use-after-free in unused method StreamControl::CloseReadStreams()

Status: RESOLVED FIXED

(Core :: Storage: Cache API, defect, P3)

Description

[Maddie Stone]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.89 Safari/537.36

Steps to reproduce:

While doing variant analysis on 0-day exploit CVE-2020-6820 (bug 1626728[1]) I found the similar variant bug 1507180 [2]. This report is for a trivial variant of [2].

[2] demonstrated that there is a heap use-after-free due to StreamControl::CloseAllReadStreams holding a reference to mReadStreamList member of CacheStreamControlParent, but in that loop the CacheStreamControlParent can be freed. When the loop in CloseAllReadStreams checks iter.HasMore() again (now ForwardRange()) after the call to ReadStream::Inner::CloseStream (and thus the call to free the CacheStreamControlParent has occurred), this is a use-after-free of the mReadStreamList. The patch [3] instead saved off a copy of mReadStreamList and used the copy in the iterator.

The same process can occur in StreamControl::CloseReadStreams [4]. CloseReadStreams holds a reference to mReadStreamList, a member of CacheStreamControlParent, but CacheStreamControlParent can be freed within the for loop call to CloseStream();

void StreamControl::CloseReadStreams(const nsID& aId) {
AssertOwningThread();
#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
uint32_t closedCount = 0;
#endif

for (const auto& stream : mReadStreamList.ForwardRange()) {
if (stream->MatchId(aId)) {
stream->CloseStream();
#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
closedCount += 1;
#endif
}
}
MOZ_DIAGNOSTIC_ASSERT(closedCount > 0);
}
MOZ_DIAGNOSTIC_ASSERT(closedCount > 0);
}

I don’t yet have a trigger for this because I’m currently running into the same issues that it seems your engineers also did in [1] and [2], but due to the similarity to an exploited in-the-wild bug (CVE-2020-6820), I wanted to report ASAP since it seems those attackers would have figured out how to trigger these cases reliably.

Not applying a deadline or putting into P0 issue tracker due to not having a working trigger. If I get a trigger working, I'll share with you and apply a 90 day deadline beginning then.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1626728
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1507180
[3] https://hg.mozilla.org/mozilla-central/rev/cdf525897bff
[4] https://hg.mozilla.org/mozilla-central/file/e86ae0d0b516126ed1da35e3fa7349c64fd54dad/dom/cache/StreamControl.cpp#l43

Credit Info: Maddie Stone of Google Project Zero

Comment 1

[:Gijs (he/him)]

I'm about to pumpkin for some PTO - scatter-pinging some people to make sure this is on the radar. 79 ships next week, it might be too late for that unless patches are easy - plus I don't know anything about this code so no idea about severity. Andrew/Dan/Yaron, can you investigate if there's still something resembling a working day where you are?

(I'd ping Ryan or Julien but it seems both have needinfos blocked because one was away last week and one the coming week... )

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Fancy Searchfox "Diagram calls into ...CloseReadStreams"

Thank you Maddie Stone of Google Project Zero!

:ytausky is on leave, I'm here. I'll leave Dan's needinfo up for awareness.

I need to do more investigation, but my current understanding from searchfox analysis and seemingly confirmed by our code coverage data is that StreamControl::CloseReadStreams is (currently) dead code (but would indeed be a potential UAF if called).

• As visualized in the attach "fancy" searchfox diagram, to get to CloseReadStreams() we need either a call to StreamList::Close in the parent or for the parent to call PCacheStreamControlParent::SendClose. The diagram logic gets tricked somehow for SendClose, but manual inspection reveals it gets called by CacheStreamControlParent::Close (after the NotifyClose call that we do see in the diagram in the parent) which gets called by StreamList::Close. No one calls StreamList::Close.
• This seems confirmed by coverage data derived from our tests, noting that of course successful attackers will by definition be doing more exciting things than our tests, but this does help reassure that the lack of callers isn't just a searchfox artifact.
  • Coverage data for StreamControl::CloseReadStreams shows no coverage: https://coverage.moz.tools/#revision=latest&path=dom%2Fcache%2FStreamControl.cpp&view=file&line=39
  • Coverage data for StreamList::Close shows no coverage: https://coverage.moz.tools/#revision=latest&path=dom%2Fcache%2FStreamList.cpp&view=file&line=127

Relatedly, :ssengupta has been doing some cleanup of the code in this area (cc'ed).

• PCacheStreamControl and all of its subclasses included CacheStreamControlParent were made explicitly refcounted in https://phabricator.services.mozilla.com/D78857 landed on m-c in https://hg.mozilla.org/mozilla-central/rev/98476ac20f66 for Fx79 which becomes release this week.
  • Note that this patch itself was is in many ways just a formalization of what recent IPC safety changes have done. (Hand-waving) All IPC actors are refcounted under the hood allowing for greater safety by deferring deallocation until after application logic gets off the stack. I'm not sure that we've added additional refcounted uses of these classes yet.

:ssengupta, can you double-check my analysis here? Thank you!

Comment 3

[Subhamoy [:ssengupta][he/him]]

The analysis is completely correct, and removing StreamList::Close() seems to have no bearing on anything :) I will submit a patch for this. I am wondering if that should be done directly under this bug, or under some other refactoring bug. @dveditz: Do you have any suggestions for me?

Comment 4

[Maddie Stone]

Thanks for looking into this. And thank you for the work that's going into making IPC safer!

Comment 5

[Andrew McCreight [:mccr8]]

Can we unhide this, then?

Comment 6

[Subhamoy [:ssengupta][he/him]]

Attachment: Bug 1655115 - Remove unused code paths leading to dom::cache::StreamControl::CloseReadStreams() r=#dom-workers-and-storage

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0496cf869a5
Remove unused code paths leading to dom::cache::StreamControl::CloseReadStreams() r=dom-workers-and-storage-reviewers,ttung

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c0496cf869a5