[Backport] CVE-2022-1305: Use after free in storage

Manual cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/3553304:
Change ownership of BlobBytesProvider.

Rather than immediately passing ownership to a cross-thread
SelfOwnedReceiver while retaining a raw pointer, instead maintain
ownership in a unique_ptr as long as it is needed, only transferring
ownership to a SelfOwnedReceiver when BlobData is done with the
BlobBytesProvider.

Also clean-up/tighten down sequence checks for BlobBytesProvider a bit.

Bug: 1285234
Change-Id: I7273e886a0bab2ae489b680d786991c9e4ff1dbb
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/main@{#986111}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

4 files changed, 85 insertions, 78 deletions

diff --git a/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.cc b/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.cc
index 833971da422..6314228c890 100644
--- a/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.cc
+++ b/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.cc
@@ -29,13 +29,10 @@ class BlobBytesStreamer {
 
  public:
   BlobBytesStreamer(Vector<scoped_refptr<RawData>> data,
-                    mojo::ScopedDataPipeProducerHandle pipe,
-                    scoped_refptr<base::SequencedTaskRunner> task_runner)
+                    mojo::ScopedDataPipeProducerHandle pipe) 
       : data_(std::move(data)),
         pipe_(std::move(pipe)),
-        watcher_(FROM_HERE,
-                 mojo::SimpleWatcher::ArmingPolicy::AUTOMATIC,
-                 std::move(task_runner)) {
+        watcher_(FROM_HERE, mojo::SimpleWatcher::ArmingPolicy::AUTOMATIC) {
     watcher_.Watch(pipe_.get(), MOJO_HANDLE_SIGNAL_WRITABLE,
                    MOJO_WATCH_CONDITION_SATISFIED,
                    WTF::BindRepeating(&BlobBytesStreamer::OnWritable,
@@ -43,6 +40,8 @@ class BlobBytesStreamer {
   }
 
   void OnWritable(MojoResult result, const mojo::HandleSignalsState& state) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
     if (result == MOJO_RESULT_CANCELLED ||
         result == MOJO_RESULT_FAILED_PRECONDITION) {
       delete this;
@@ -82,15 +81,18 @@ class BlobBytesStreamer {
 
  private:
   // The index of the item currently being written.
-  size_t current_item_ = 0;
+  size_t current_item_ GUARDED_BY_CONTEXT(sequence_checker_) = 0;
   // The offset into the current item of the first byte not yet written to the
   // data pipe.
-  size_t current_item_offset_ = 0;
+  size_t current_item_offset_ GUARDED_BY_CONTEXT(sequence_checker_) = 0;
   // The data being written.
-  Vector<scoped_refptr<RawData>> data_;
+  Vector<scoped_refptr<RawData>> data_ GUARDED_BY_CONTEXT(sequence_checker_);
+
+  mojo::ScopedDataPipeProducerHandle pipe_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+  mojo::SimpleWatcher watcher_ GUARDED_BY_CONTEXT(sequence_checker_);
 
-  mojo::ScopedDataPipeProducerHandle pipe_;
-  mojo::SimpleWatcher watcher_;
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 // This keeps the process alive while blobs are being transferred.
@@ -116,31 +118,8 @@ void DecreaseChildProcessRefCount() {
 
 constexpr size_t BlobBytesProvider::kMaxConsolidatedItemSizeInBytes;
 
-// static
-BlobBytesProvider* BlobBytesProvider::CreateAndBind(
-    mojo::PendingReceiver<mojom::blink::BytesProvider> receiver) {
-  auto task_runner = base::ThreadPool::CreateSequencedTaskRunner(
-      {base::MayBlock(), base::TaskPriority::USER_VISIBLE});
-  auto provider = base::WrapUnique(new BlobBytesProvider(task_runner));
-  auto* result = provider.get();
-  // TODO(mek): Consider binding BytesProvider on the IPC thread instead, only
-  // using the MayBlock taskrunner for actual file operations.
-  PostCrossThreadTask(
-      *task_runner, FROM_HERE,
-      CrossThreadBindOnce(
-          [](std::unique_ptr<BlobBytesProvider> provider,
-             mojo::PendingReceiver<mojom::blink::BytesProvider> receiver) {
-            mojo::MakeSelfOwnedReceiver(std::move(provider),
-                                        std::move(receiver));
-          },
-          WTF::Passed(std::move(provider)), WTF::Passed(std::move(receiver))));
-  return result;
-}
-
-// static
-std::unique_ptr<BlobBytesProvider> BlobBytesProvider::CreateForTesting(
-    scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  return base::WrapUnique(new BlobBytesProvider(std::move(task_runner)));
+BlobBytesProvider::BlobBytesProvider() {
+  IncreaseChildProcessRefCount();
 }
 
 BlobBytesProvider::~BlobBytesProvider() {
@@ -148,6 +127,8 @@ BlobBytesProvider::~BlobBytesProvider() {
 }
 
 void BlobBytesProvider::AppendData(scoped_refptr<RawData> data) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!data_.IsEmpty()) {
     uint64_t last_offset = offsets_.IsEmpty() ? 0 : offsets_.back();
     offsets_.push_back(last_offset + data_.back()->length());
@@ -156,6 +137,8 @@ void BlobBytesProvider::AppendData(scoped_refptr<RawData> data) {
 }
 
 void BlobBytesProvider::AppendData(base::span<const char> data) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (data_.IsEmpty() ||
       data_.back()->length() + data.size() > kMaxConsolidatedItemSizeInBytes) {
     AppendData(RawData::Create());
@@ -163,8 +146,31 @@ void BlobBytesProvider::AppendData(base::span<const char> data) {
   data_.back()->MutableData()->Append(data.data(), data.size());
 }
 
+// static
+void BlobBytesProvider::Bind(
+    std::unique_ptr<BlobBytesProvider> provider,
+    mojo::PendingReceiver<mojom::blink::BytesProvider> receiver) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(provider->sequence_checker_);
+  DETACH_FROM_SEQUENCE(provider->sequence_checker_);
+
+  auto task_runner = base::ThreadPool::CreateSequencedTaskRunner(
+      {base::MayBlock(), base::TaskPriority::USER_VISIBLE});
+  // TODO(mek): Consider binding BytesProvider on the IPC thread instead, only
+  // using the MayBlock taskrunner for actual file operations.
+  PostCrossThreadTask(
+      *task_runner, FROM_HERE,
+      CrossThreadBindOnce(
+          [](std::unique_ptr<BlobBytesProvider> provider,
+             mojo::PendingReceiver<mojom::blink::BytesProvider> receiver) {
+            DCHECK_CALLED_ON_VALID_SEQUENCE(provider->sequence_checker_);
+            mojo::MakeSelfOwnedReceiver(std::move(provider),
+                                        std::move(receiver));
+          },
+          std::move(provider), std::move(receiver)));
+}
+
 void BlobBytesProvider::RequestAsReply(RequestAsReplyCallback callback) {
-  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // TODO(mek): Once better metrics are created we could experiment with ways
   // to reduce the number of copies of data that are made here.
   Vector<uint8_t> result;
@@ -175,9 +181,10 @@ void BlobBytesProvider::RequestAsReply(RequestAsReplyCallback callback) {
 
 void BlobBytesProvider::RequestAsStream(
     mojo::ScopedDataPipeProducerHandle pipe) {
-  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // BlobBytesStreamer will self delete when done.
-  new BlobBytesStreamer(std::move(data_), std::move(pipe), task_runner_);
+  new BlobBytesStreamer(std::move(data_), std::move(pipe));
 }
 
 void BlobBytesProvider::RequestAsFile(uint64_t source_offset,
@@ -185,7 +192,7 @@ void BlobBytesProvider::RequestAsFile(uint64_t source_offset,
                                       base::File file,
                                       uint64_t file_offset,
                                       RequestAsFileCallback callback) {
-  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DEFINE_THREAD_SAFE_STATIC_LOCAL(BooleanHistogram, seek_histogram,
                                   ("Storage.Blob.RendererFileSeekFailed"));
   DEFINE_THREAD_SAFE_STATIC_LOCAL(BooleanHistogram, write_histogram,
@@ -256,10 +263,4 @@ void BlobBytesProvider::RequestAsFile(uint64_t source_offset,
   std::move(callback).Run(info.last_modified);
 }
 
-BlobBytesProvider::BlobBytesProvider(
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : task_runner_(std::move(task_runner)) {
-  IncreaseChildProcessRefCount();
-}
-
 }  // namespace blink
diff --git a/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.h b/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.h
index 59b019a6ce9..4d6eea10b6f 100644
--- a/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.h
+++ b/chromium/third_party/blink/renderer/platform/blob/blob_bytes_provider.h
@@ -25,19 +25,17 @@ class PLATFORM_EXPORT BlobBytesProvider : public mojom::blink::BytesProvider {
   // data appended to the same item.
   static constexpr size_t kMaxConsolidatedItemSizeInBytes = 15 * 1024;
 
-  // Creates a new instance, and binds it on a new SequencedTaskRunner. The
-  // returned instance should only be considered valid as long as the request
-  // passed in to this method is still known to be valid.
-  static BlobBytesProvider* CreateAndBind(
-      mojo::PendingReceiver<mojom::blink::BytesProvider>);
-  static std::unique_ptr<BlobBytesProvider> CreateForTesting(
-      scoped_refptr<base::SequencedTaskRunner>);
-
+  BlobBytesProvider();
   ~BlobBytesProvider() override;
 
   void AppendData(scoped_refptr<RawData>);
   void AppendData(base::span<const char>);
 
+  // Binds `provider` to `receiver` on a threadpool task runner, transferring
+  // ownership.
+  static void Bind(std::unique_ptr<BlobBytesProvider> provider,
+                   mojo::PendingReceiver<mojom::blink::BytesProvider> receiver);
+
   // BytesProvider implementation:
   void RequestAsReply(RequestAsReplyCallback) override;
   void RequestAsStream(mojo::ScopedDataPipeProducerHandle) override;
@@ -50,17 +48,13 @@ class PLATFORM_EXPORT BlobBytesProvider : public mojom::blink::BytesProvider {
  private:
   FRIEND_TEST_ALL_PREFIXES(BlobBytesProviderTest, Consolidation);
 
-  BlobBytesProvider(scoped_refptr<base::SequencedTaskRunner>);
-
-  // The task runner this class is bound on, as well as what is used by the
-  // RequestAsStream method to monitor the data pipe.
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
-
-  Vector<scoped_refptr<RawData>> data_;
+  Vector<scoped_refptr<RawData>> data_ GUARDED_BY_CONTEXT(sequence_checker_);
   // |offsets_| always contains exactly one fewer item than |data_| (except when
   // |data_| itself is empty).
   // offsets_[x] is equal to the sum of data_[i].length for all i <= x.
-  Vector<uint64_t> offsets_;
+  Vector<uint64_t> offsets_ GUARDED_BY_CONTEXT(sequence_checker_);
+
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 }  // namespace blink
diff --git a/chromium/third_party/blink/renderer/platform/blob/blob_data.cc b/chromium/third_party/blink/renderer/platform/blob/blob_data.cc
index a381dbc3140..feeae4779a5 100644
--- a/chromium/third_party/blink/renderer/platform/blob/blob_data.cc
+++ b/chromium/third_party/blink/renderer/platform/blob/blob_data.cc
@@ -106,6 +106,12 @@ BlobData::BlobData(FileCompositionStatus composition)
 BlobData::~BlobData() = default;
 
 Vector<mojom::blink::DataElementPtr> BlobData::ReleaseElements() {
+  if (last_bytes_provider_) {
+    DCHECK(last_bytes_provider_receiver_);
+    BlobBytesProvider::Bind(std::move(last_bytes_provider_),
+                            std::move(last_bytes_provider_receiver_));
+  }
+
   return std::move(elements_);
 }
 
@@ -140,16 +146,6 @@ std::unique_ptr<BlobData> BlobData::CreateForFileSystemURLWithUnknownSize(
   return data;
 }
 
-void BlobData::DetachFromCurrentThread() {
-  content_type_ = content_type_.IsolatedCopy();
-  for (auto& element : elements_) {
-    if (element->is_file_filesystem()) {
-      auto& file_element = element->get_file_filesystem();
-      file_element->url = file_element->url.Copy();
-    }
-  }
-}
-
 void BlobData::SetContentType(const String& content_type) {
   if (IsValidBlobType(content_type))
     content_type_ = content_type;
@@ -273,6 +269,7 @@ void BlobData::AppendDataInternal(base::span<const char> data,
   if (!elements_.IsEmpty() && elements_.back()->is_bytes()) {
     // Append bytes to previous element.
     DCHECK(last_bytes_provider_);
+    DCHECK(last_bytes_provider_receiver_);
     const auto& bytes_element = elements_.back()->get_bytes();
     bytes_element->length += data.size();
     if (should_embed_bytes && bytes_element->embedded_data) {
@@ -283,9 +280,18 @@ void BlobData::AppendDataInternal(base::span<const char> data,
       bytes_element->embedded_data = base::nullopt;
     }
   } else {
+    if (last_bytes_provider_) {
+      // If `last_bytes_provider_` is set, but the previous element is not a
+      // bytes element, a new BytesProvider will be created and we need to
+      // make sure to bind the previous one first.
+      DCHECK(last_bytes_provider_receiver_);
+      BlobBytesProvider::Bind(std::move(last_bytes_provider_),
+                              std::move(last_bytes_provider_receiver_));
+    }
     mojo::PendingRemote<BytesProvider> bytes_provider_remote;
-    last_bytes_provider_ = BlobBytesProvider::CreateAndBind(
-        bytes_provider_remote.InitWithNewPipeAndPassReceiver());
+    last_bytes_provider_ = std::make_unique<BlobBytesProvider>();
+    last_bytes_provider_receiver_ =
+        bytes_provider_remote.InitWithNewPipeAndPassReceiver();
 
     auto bytes_element = DataElementBytes::New(
         data.size(), base::nullopt, std::move(bytes_provider_remote));
diff --git a/chromium/third_party/blink/renderer/platform/blob/blob_data.h b/chromium/third_party/blink/renderer/platform/blob/blob_data.h
index 697c6ce501e..864198847d4 100644
--- a/chromium/third_party/blink/renderer/platform/blob/blob_data.h
+++ b/chromium/third_party/blink/renderer/platform/blob/blob_data.h
@@ -43,6 +43,7 @@
 #include "base/thread_annotations.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/struct_ptr.h"
+#include "third_party/blink/public/mojom/blob/data_element.mojom-blink-forward.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
@@ -117,13 +118,10 @@ class PLATFORM_EXPORT BlobData {
       const KURL& file_system_url,
       const base::Optional<base::Time>& expected_modification_time);
 
-  // Detaches from current thread so that it can be passed to another thread.
-  void DetachFromCurrentThread();
-
   const String& ContentType() const { return content_type_; }
   void SetContentType(const String&);
 
-  const Vector<mojom::blink::DataElementPtr>& Elements() const {
+  const Vector<mojom::blink::DataElementPtr>& ElementsForTesting() const {
     return elements_;
   }
   Vector<mojom::blink::DataElementPtr> ReleaseElements();
@@ -165,7 +163,15 @@ class PLATFORM_EXPORT BlobData {
 
   Vector<mojom::blink::DataElementPtr> elements_;
   size_t current_memory_population_ = 0;
-  BlobBytesProvider* last_bytes_provider_ = nullptr;
+
+  // These two members are used to combine multiple consecutive 'bytes' elements
+  // (as created by `AppendBytes`, `AppendData` or `AppendText`) into a single
+  // element. When one has a value the other also has a value. Before using
+  // `elements_` to actually create a blob, `last_bytes_provider_` should be
+  // bound to `last_bytes_provider_receiver_`.
+  std::unique_ptr<BlobBytesProvider> last_bytes_provider_;
+  mojo::PendingReceiver<mojom::blink::BytesProvider>
+      last_bytes_provider_receiver_;
 
   DISALLOW_COPY_AND_ASSIGN(BlobData);
 };