Project Fortify, an experimental security project, has identified the following potential security issue.

Overview: A range-based for loop in ImageDownloaderImpl::ContextDestroyed iterates over a vector of unique_ptrs. Calling Dispose() on the elements synchronously removes them from the vector, leaving stale pointers in the vector’s trailing slots that the loop subsequently dereferences, leading to a Use-After-Free.

Affected files:

• third_party/blink/renderer/modules/image_downloader/image_downloader_impl.cc
• third_party/blink/renderer/modules/image_downloader/multi_resolution_image_resource_fetcher.cc

Estimated timestamp from git blame: 2020-02-16

Overview

There is a potential Use-After-Free (UAF) vulnerability in ImageDownloaderImpl::ContextDestroyed() in the renderer process. The issue stems from a range-based for loop iterating over WTF::Vector<std::unique_ptr<MultiResolutionImageResourceFetcher>> image_fetchers_. The loop body calls a method on the unique_ptr that synchronously mutates the vector by erasing the element. Because WTF::Vector::erase() uses memmove to shift elements but does not clear the trailing slots, the loop continues to iterate (using its cached end iterator) and eventually dereferences a stale unique_ptr in the uncleared memory, causing a UAF when it calls Dispose() on an already-deleted object.

Technical Details

1. The Loop: ImageDownloaderImpl::ContextDestroyed() iterates over the image_fetchers_ vector using a range-based for loop:

   void ImageDownloaderImpl::ContextDestroyed() {
     for (const auto& fetcher : image_fetchers_) {
       fetcher->Dispose();
     }
     image_fetchers_.clear();
   }
   

2. Synchronous Mutation: MultiResolutionImageResourceFetcher::Dispose() synchronously runs its callback: std::move(callback_).Run(...).
3. Vector Erasure: This callback is bound to ImageDownloaderImpl::DidFetchImage(), which finds the corresponding fetcher in image_fetchers_ and erases it: it = image_fetchers_.erase(it);.
4. Stale Pointers: WTF::Vector::erase() calls TypeOperations::MoveOverlapping() (using memmove) to shift the remaining elements left. Crucially, because the default allocator is PartitionAlloc (where kIsGarbageCollected is false), ClearUnusedSlots() is a no-op. The trailing slots of the vector’s backing buffer retain the raw bit patterns of the moved unique_ptrs.
5. The UAF: The range-based for loop caches the end iterator initially. Even though the vector shrinks with each erasure, the loop continues iterating until it reaches the original end. It eventually encounters the uncleared, stale unique_ptrs in the trailing slots. These point to memory that was freed in previous iterations. The loop dereferences the stale pointer and calls Dispose() on the attacker-controlled, freed memory chunk.

Potential Exploitability

This vulnerability could potentially lead to Remote Code Execution (RCE) in the renderer process.

Inside Dispose(), the code accesses the callback_ member, which is a base::OnceCallback. Executing this callback requires reading the bind_state_ pointer (a scoped_refptr<BindStateBase>) to find the polymorphic_invoke_ function pointer.

An attacker could attempt to groom the heap to reclaim the memory chunk of the freed MultiResolutionImageResourceFetcher during the synchronous window provided immediately after the erasure (e.g., during the Mojo IPC allocations in DidDownloadImage, which is called right after DidFetchImage). By controlling the contents of the reclaimed memory, the attacker could forge the BindStateHolder and hijack control flow.

This issue is not protected by MiraclePtr (BackupRefPtr) because the stale pointer is a raw T* stored inside a standard std::unique_ptr within the WTF::Vector’s raw backing store, completely bypassing BRP checks.

Suggested Fix

To fix this, the image_fetchers_ vector should be cleared safely before iterating, for example by swapping it with a local vector or taking ownership using std::exchange:

void ImageDownloaderImpl::ContextDestroyed() {
  auto fetchers = std::move(image_fetchers_);
  for (const auto& fetcher : fetchers) {
    fetcher->Dispose();
  }
}

(Note: These are suggested steps based on code analysis; a working proof of concept has not yet been executed to confirm exact exploitation parameters.)

Evaluated with Chrome root at commit: 876d480da1f794d87813cfa2e6ff4fcf9771e939

Results so far have been promising, but there can be wrong deductions. If this proves to be a false positive, please close as WAI; data from false positives will be used to improve accuracy over time. And please feel free to reach out to me directly if you have concerns or feedback on the project.