GitLab

MultiThreadedCertVerifier keeps a list of
MultiThreadedCertVerifier::InternalRequests in order to eagerly reset
callbacks passed to Verify() if the MultiThreadedCertVerifier is
itself deleted (CertVerifier contract guarantees this eager reset
behavior).

In ~MultiThreadedCertVerifier we loop through this list and reset the
callbacks, but then delete the InternalRequest from the list. However,
the callbacks are allowed to own the InternalRequest, so this leads
to a UaF.

We don't need to remove the InternalRequest from the list in
~MultiThreadedCertVerifier, because we are not in charge of the
lifetime of the InternalRequest. InternalRequest can remove itself
from the list during ~InternalRequest, or MultiThreadedCertVerifier
can remove it from the list when a CertVerification job is complete.
The former is safe because ~InternalRequest won't remove itself from
the list if the MultiThreadedCertVerifier is already destructed.
The latter is obviously safe because if the request was cancelled,
then InternalRequest::OnJobCompleted will never run, so
|this| is always valid to remove from the list during
InternalRequest::OnJobCompleted.

The added test reproduces the UAF without the fix.

Bug: 1157562
Change-Id: I92d0dc6ca6df084f55ea511ea692853ee63f5033
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587560Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836903}