Commit 0f6df4fa authored by Darwin Huang's avatar Darwin Huang Committed by Commit Bot

Clipboard: Fix UaP in ClipboardWriter/FileReaderLoader.

Make ClipboardWriter keep FileReaderLoader alive until it's done reading
a Blob, by using SelfKeepAlive<T>.

Previously, ClipboardWriter could be garbage collected unexpectedly
(ex. when the frame detaches). This could cause a use after poison
in the FileReaderLoader, where:
(1) A ClipboardWriter's FileReaderLoader starts reading the input blob
    as an async task.
(2) The ClipboardWriter is destroyed (garbage collected).
(3) The FileReaderLoader completes its async task of reading the input
    blob, and calls ClipboardWriter::StartWrite on the destroyed, owning
    ClipboardWriter.

Additionally, add a "context destroyed" error message when a context
detaches.

Bug: 1142331
Change-Id: I427cd6dc02e773b2d235d45bd9ad8935b575ff71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2509033
Commit-Queue: Darwin Huang <huangdarwin@chromium.org>
Reviewed-by: default avatarMarijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827371}
parent 68ae1beb
......@@ -107,7 +107,7 @@ ScriptPromise ClipboardPromise::CreateForWriteText(ExecutionContext* context,
ClipboardPromise::ClipboardPromise(ExecutionContext* context,
ScriptState* script_state)
: ExecutionContextClient(context),
: ExecutionContextLifecycleObserver(context),
script_state_(script_state),
script_promise_resolver_(
MakeGarbageCollected<ScriptPromiseResolver>(script_state)),
......@@ -496,13 +496,20 @@ scoped_refptr<base::SingleThreadTaskRunner> ClipboardPromise::GetTaskRunner() {
return GetExecutionContext()->GetTaskRunner(TaskType::kUserInteraction);
}
// ExecutionContextLifecycleObserver implementation.
void ClipboardPromise::ContextDestroyed() {
script_promise_resolver_->Reject(MakeGarbageCollected<DOMException>(
DOMExceptionCode::kNotAllowedError, "Document detached."));
clipboard_writer_.Clear();
}
void ClipboardPromise::Trace(Visitor* visitor) const {
visitor->Trace(script_state_);
visitor->Trace(script_promise_resolver_);
visitor->Trace(clipboard_writer_);
visitor->Trace(permission_service_);
visitor->Trace(clipboard_item_data_);
ExecutionContextClient::Trace(visitor);
ExecutionContextLifecycleObserver::Trace(visitor);
}
} // namespace blink
......@@ -27,7 +27,7 @@ class ExecutionContext;
class ClipboardItemOptions;
class ClipboardPromise final : public GarbageCollected<ClipboardPromise>,
public ExecutionContextClient {
public ExecutionContextLifecycleObserver {
public:
// Creates promise to execute Clipboard API functions off the main thread.
static ScriptPromise CreateForRead(ExecutionContext*,
......@@ -86,6 +86,9 @@ class ClipboardPromise final : public GarbageCollected<ClipboardPromise>,
scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner();
// ExecutionContextLifecycleObserver
void ContextDestroyed() override;
Member<ScriptState> script_state_;
Member<ScriptPromiseResolver> script_promise_resolver_;
......
......@@ -298,9 +298,12 @@ ClipboardWriter::ClipboardWriter(SystemClipboard* system_clipboard,
file_reading_task_runner_(promise->GetExecutionContext()->GetTaskRunner(
TaskType::kFileReading)),
system_clipboard_(system_clipboard),
raw_system_clipboard_(raw_system_clipboard) {}
raw_system_clipboard_(raw_system_clipboard),
self_keep_alive_(PERSISTENT_FROM_HERE, this) {}
ClipboardWriter::~ClipboardWriter() = default;
ClipboardWriter::~ClipboardWriter() {
DCHECK(!file_reader_);
}
// static
bool ClipboardWriter::IsValidType(const String& type, bool is_raw) {
......@@ -330,12 +333,16 @@ void ClipboardWriter::DidFinishLoading() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DOMArrayBuffer* array_buffer = file_reader_->ArrayBufferResult();
DCHECK(array_buffer);
file_reader_.reset();
self_keep_alive_.Clear();
StartWrite(array_buffer, clipboard_task_runner_);
}
void ClipboardWriter::DidFail(FileErrorCode error_code) {
file_reader_.reset();
self_keep_alive_.Clear();
promise_->RejectFromReadOrDecodeFailure();
}
......
......@@ -9,6 +9,7 @@
#include "third_party/blink/renderer/core/fileapi/blob.h"
#include "third_party/blink/renderer/core/fileapi/file_reader_loader_client.h"
#include "third_party/blink/renderer/platform/heap/heap.h"
#include "third_party/blink/renderer/platform/heap/self_keep_alive.h"
#include "third_party/skia/include/core/SkImage.h"
namespace blink {
......@@ -27,6 +28,11 @@ class RawSystemClipboard;
// take advantage of vulnerabilities in their decoders. In
// ClipboardRawDataWriter, this decoding is skipped.
// (3) Writing the blob's decoded contents to the system clipboard.
//
// ClipboardWriter is owned only by itself and ClipboardPromise. It keeps
// itself alive for the duration of FileReaderLoader's async operations using
// SelfKeepAlive, and keeps itself alive afterwards during cross-thread
// operations by using WrapCrossThreadPersistent.
class ClipboardWriter : public GarbageCollected<ClipboardWriter>,
public FileReaderLoaderClient {
public:
......@@ -86,6 +92,10 @@ class ClipboardWriter : public GarbageCollected<ClipboardWriter>,
Member<SystemClipboard> system_clipboard_;
// Access to the global unsanitized system clipboard.
Member<RawSystemClipboard> raw_system_clipboard_;
// Oilpan: ClipboardWriter must remain alive until Member<T>::Clear() is
// called, to keep the FileReaderLoader alive and avoid unexpected UaPs.
SelfKeepAlive<ClipboardWriter> self_keep_alive_;
};
} // namespace blink
......
<!doctype html>
<meta charset="utf-8">
<title>
Async Clipboard write garbage collection race condition test
</title>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="../../resources/testdriver.js"></script>
<script src="../../resources/testdriver-vendor.js"></script>
<script>
promise_test(async t => {
await test_driver.set_permission({name: 'clipboard-write'}, 'granted');
const blobText = new Blob(['test text'], {type: 'text/plain'});
const clipboardItemInput = new ClipboardItem(
{'text/plain' : blobText});
const promise = navigator.clipboard.write([clipboardItemInput]);
for (let i = 0; i < 10; ++i) {
// TODO(https://github.com/web-platform-tests/wpt/issues/7899): Support
// cross-browser GC triggers, and move this test to the Web Platform
// Test suite.
if(window.gc) {
window.gc();
}
await new Promise(resolve => setTimeout(resolve, 0));
}
await promise;
}, 'Verify write clipboard avoids garbage collection race condition.');
</script>
\ No newline at end of file
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