Commit d3cf8403 authored by Ken Rockot's avatar Ken Rockot Committed by Chromium LUCI CQ

Fix potential UAF in Session Storage

This was fixed for StorageAreaImpl usage in the Local Storage
implementation, but Session Storage was overlooked.

It turns out that Session Storage namespaces also own StorageAreaImpl
objects, and these too can outlive their backing database. This ensures
that the map of namespaces is cleared before SessionStorageImpl resets
its database, avoiding the UAF.

Bug: 1152800
Change-Id: I946841b20fa73754e8b8ac611b352db0319ce4d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617239
Commit-Queue: Ken Rockot <rockot@google.com>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: default avatarVictor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841363}
parent 5719e284
...@@ -995,19 +995,21 @@ void SessionStorageImpl::OnConnectionFinished() { ...@@ -995,19 +995,21 @@ void SessionStorageImpl::OnConnectionFinished() {
std::move(callbacks[i]).Run(); std::move(callbacks[i]).Run();
} }
void SessionStorageImpl::PurgeAllNamespaces() {
for (const auto& it : data_maps_)
it.second->storage_area()->CancelAllPendingRequests();
for (const auto& namespace_pair : namespaces_)
namespace_pair.second->Reset();
DCHECK(data_maps_.empty());
}
void SessionStorageImpl::DeleteAndRecreateDatabase(const char* histogram_name) { void SessionStorageImpl::DeleteAndRecreateDatabase(const char* histogram_name) {
if (connection_state_ == CONNECTION_SHUTDOWN) if (connection_state_ == CONNECTION_SHUTDOWN)
return; return;
// We're about to set database_ to null, so delete the StorageAreas // We're about to set database_ to null, so delete the StorageAreas
// that might still be using the old database. // that might still be using the old database.
for (const auto& it : data_maps_) PurgeAllNamespaces();
it.second->storage_area()->CancelAllPendingRequests();
for (const auto& namespace_pair : namespaces_) {
namespace_pair.second->Reset();
}
DCHECK(data_maps_.empty());
// Reset state to be in process of connecting. This will cause requests for // Reset state to be in process of connecting. This will cause requests for
// StorageAreas to be queued until the connection is complete. // StorageAreas to be queued until the connection is complete.
...@@ -1059,6 +1061,7 @@ void SessionStorageImpl::OnDBDestroyed(bool recreate_in_memory, ...@@ -1059,6 +1061,7 @@ void SessionStorageImpl::OnDBDestroyed(bool recreate_in_memory,
void SessionStorageImpl::OnShutdownComplete() { void SessionStorageImpl::OnShutdownComplete() {
DCHECK(shutdown_complete_callback_); DCHECK(shutdown_complete_callback_);
// Flush any final tasks on the DB task runner before invoking the callback. // Flush any final tasks on the DB task runner before invoking the callback.
PurgeAllNamespaces();
database_.reset(); database_.reset();
leveldb_task_runner_->PostTaskAndReply( leveldb_task_runner_->PostTaskAndReply(
FROM_HERE, base::DoNothing(), std::move(shutdown_complete_callback_)); FROM_HERE, base::DoNothing(), std::move(shutdown_complete_callback_));
......
...@@ -221,6 +221,7 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider, ...@@ -221,6 +221,7 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider,
MetadataParseResult ParseNextMapId(ValueAndStatus next_map_id); MetadataParseResult ParseNextMapId(ValueAndStatus next_map_id);
void OnConnectionFinished(); void OnConnectionFinished();
void PurgeAllNamespaces();
void DeleteAndRecreateDatabase(const char* histogram_name); void DeleteAndRecreateDatabase(const char* histogram_name);
void OnDBDestroyed(bool recreate_in_memory, leveldb::Status status); void OnDBDestroyed(bool recreate_in_memory, leveldb::Status status);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment