From f1a26f7d2328aae5a500fc2a2de2a80899fc6fa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Br=C3=BCning?= Date: Thu, 5 Dec 2019 22:31:34 +0100 Subject: [Backport] CVE-2019-13693 IndexedDB: Unregister cursors from transactions more consistently. IndexedDBCursor now calls IndexedDBTransaction::UnregisterOpenCursor() in Close(), which is called by the destructor. The previous setup missed an edge case where calling IndexedDBCursor::Close() directly would not unregister the cursor. This behavior was relied upon in IndexedDBTransaction::CloseOpenCursors(), but was not intended at other callsites. (cherry picked from commit 23303e6f178ca2993bfde7114e6bdf48dd0eff9d) Bug: 1005753 Reviewed-by: Joshua Bell Commit-Queue: Victor Costan Cr-Original-Commit-Position: refs/heads/master@{#699937} Reviewed-by: Victor Costan Cr-Commit-Position: refs/branch-heads/3865@{#872} Cr-Branched-From: 0cdcc6158160790658d1f033d3db873603250124-refs/heads/master@{#681094} Reviewed-by: Michal Klocek Change-Id: I9a16b513715e05b3c85e80f9103305f771590a45 --- .../browser/indexed_db/indexed_db_cursor.cc | 35 +++++++++++++++++++++- .../browser/indexed_db/indexed_db_transaction.cc | 8 +++-- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/chromium/content/browser/indexed_db/indexed_db_cursor.cc b/chromium/content/browser/indexed_db/indexed_db_cursor.cc index a029b08242f..915c4276543 100644 --- a/chromium/content/browser/indexed_db/indexed_db_cursor.cc +++ b/chromium/content/browser/indexed_db/indexed_db_cursor.cc @@ -15,8 +15,21 @@ #include "content/browser/indexed_db/indexed_db_tracing.h" #include "content/browser/indexed_db/indexed_db_transaction.h" #include "content/browser/indexed_db/indexed_db_value.h" +#include "third_party/WebKit/public/platform/modules/indexeddb/WebIDBDatabaseException.h" namespace content { +namespace { +// This should never be script visible: the cursor should either be closed when +// it hits the end of the range (and script throws an error before the call +// could be made), if the transaction has finished (ditto), or if there's an +// incoming request from the front end but the transaction has aborted on the +// back end; in that case the tx will already have sent an abort to the request +// so this would be ignored. +IndexedDBDatabaseError CreateCursorClosedError() { + return IndexedDBDatabaseError(blink::WebIDBDatabaseExceptionUnknownError, + "The cursor has been closed."); +} +} // namespace IndexedDBCursor::IndexedDBCursor( std::unique_ptr cursor, @@ -32,7 +45,8 @@ IndexedDBCursor::IndexedDBCursor( } IndexedDBCursor::~IndexedDBCursor() { - transaction_->UnregisterOpenCursor(this); + // Call to make sure we complete our lifetime trace. + Close(); } void IndexedDBCursor::Continue(std::unique_ptr key, @@ -40,6 +54,11 @@ void IndexedDBCursor::Continue(std::unique_ptr key, scoped_refptr callbacks) { IDB_TRACE("IndexedDBCursor::Continue"); + if (closed_) { + callbacks->OnError(CreateCursorClosedError()); + return; + } + transaction_->ScheduleTask( task_type_, base::Bind(&IndexedDBCursor::CursorIterationOperation, @@ -53,6 +72,11 @@ void IndexedDBCursor::Advance(uint32_t count, scoped_refptr callbacks) { IDB_TRACE("IndexedDBCursor::Advance"); + if (closed_) { + callbacks->OnError(CreateCursorClosedError()); + return; + } + transaction_->ScheduleTask( task_type_, base::Bind( @@ -104,6 +128,11 @@ void IndexedDBCursor::PrefetchContinue( scoped_refptr callbacks) { IDB_TRACE("IndexedDBCursor::PrefetchContinue"); + if (closed_) { + callbacks->OnError(CreateCursorClosedError()); + return; + } + transaction_->ScheduleTask( task_type_, base::Bind(&IndexedDBCursor::CursorPrefetchIterationOperation, @@ -198,10 +227,14 @@ leveldb::Status IndexedDBCursor::PrefetchReset(int used_prefetches, } void IndexedDBCursor::Close() { + if (closed_) + return; IDB_TRACE("IndexedDBCursor::Close"); closed_ = true; cursor_.reset(); saved_cursor_.reset(); + if (transaction_) + transaction_->UnregisterOpenCursor(this); } } // namespace content diff --git a/chromium/content/browser/indexed_db/indexed_db_transaction.cc b/chromium/content/browser/indexed_db/indexed_db_transaction.cc index 5040dddf49f..b3867a21b49 100644 --- a/chromium/content/browser/indexed_db/indexed_db_transaction.cc +++ b/chromium/content/browser/indexed_db/indexed_db_transaction.cc @@ -460,9 +460,13 @@ void IndexedDBTransaction::Timeout() { void IndexedDBTransaction::CloseOpenCursors() { IDB_TRACE1("IndexedDBTransaction::CloseOpenCursors", "txn.id", id()); - for (auto* cursor : open_cursors_) - cursor->Close(); + + // IndexedDBCursor::Close() indirectly mutates |open_cursors_|, when it calls + // IndexedDBTransaction::UnregisterOpenCursor(). + std::set open_cursors = std::move(open_cursors_); open_cursors_.clear(); + for (auto* cursor : open_cursors) + cursor->Close(); } void IndexedDBTransaction::AddPendingObserver( -- cgit v1.2.1