[PM] Add WebMeasureMemorySecurityChecker

Adds a browser-side security check that the frame making a
performance.measureMemory request is allowed to use the API, to guard
against compromised renderers skipping the renderer-side security check.
Currently it only checks that the frame is same-origin with the page's
main frame. Once an accessor for crossOriginIsolated status is added on
the browser side, the class can check that too.

R=fdoray

Bug: 1085129
Change-Id: I73b1955dafa0dec248e86783aee4e8d8e4cdcda1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2588249Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837160}

components/performance_manager/graph/frame_node_impl.cc

@@ -12,7 +12,6 @@
 #include "components/performance_manager/graph/process_node_impl.h"
 #include "components/performance_manager/graph/worker_node_impl.h"
 #include "components/performance_manager/public/v8_memory/web_memory.h"
-#include "third_party/blink/public/common/features.h"
 
 namespace performance_manager {
 
@@ -684,9 +683,9 @@ void FrameNodeImpl::DocumentProperties::Reset(FrameNodeImpl* frame_node,
 void FrameNodeImpl::OnWebMemoryMeasurementRequested(
     mojom::WebMemoryMeasurement::Mode mode,
     OnWebMemoryMeasurementRequestedCallback callback) {
-  CHECK(base::FeatureList::IsEnabled(
-      blink::features::kWebMeasureMemoryViaPerformanceManager));
-  v8_memory::WebMeasureMemory(this, mode, std::move(callback));
+  v8_memory::WebMeasureMemory(
+      this, mode, v8_memory::WebMeasureMemorySecurityChecker::Create(),
+      std::move(callback), mojo::GetBadMessageCallback());
 }
 
 }  // namespace performance_manager

components/performance_manager/public/v8_memory/web_memory.h

@@ -19,12 +19,43 @@ class FrameNode;
 
 namespace v8_memory {
 
-// Implements mojom::DocumentCoordinationUnit::OnWebMemoryMeasurementRequest.
-// Measures memory usage of each frame in the browsing context group of the
-// given frame and invokes the given callback with the result.
-void WebMeasureMemory(const FrameNode*,
-                      mojom::WebMemoryMeasurement::Mode,
-                      base::OnceCallback<void(mojom::WebMemoryMeasurementPtr)>);
+// Verifies that a frame is allowed to call WebMeasureMemory.
+//
+// The production implementation repeats the checks in the
+// performance.measureMemory spec (see the comments on WebMeasureMemory for the
+// link and version). These checks are performed first on the renderer side but
+// repeated in the browser to guard against a compromised renderer invoking
+// performance.measureMemory without them.
+class WebMeasureMemorySecurityChecker {
+ public:
+  virtual ~WebMeasureMemorySecurityChecker() = default;
+
+  // Creates a WebMeasureMemorySecurityChecker for production use.
+  static std::unique_ptr<WebMeasureMemorySecurityChecker> Create();
+
+  // Invokes |measure_memory_closure| on the PM sequence if |frame| is allowed
+  // to call WebMeasureMemory, |bad_message_callback| otherwise.
+  virtual void CheckMeasureMemoryIsAllowed(
+      const FrameNode* frame,
+      base::OnceClosure measure_memory_closure,
+      mojo::ReportBadMessageCallback bad_message_callback) const = 0;
+};
+
+// Implements mojom::DocumentCoordinationUnit::OnWebMemoryMeasurementRequest to
+// perform a memory measurement as defined in the performance.measureMemory
+// spec at https://wicg.github.io/performance-measure-memory (this
+// implementation targets the draft of 20 October 2020.)
+//
+// Verifies that |frame_node| is allowed to measure memory using
+// |security_checker|. If so, measures memory usage of each frame in
+// |frame_node|'s browsing context group and invokes |result_callback| with the
+// result; if not, invokes |bad_message_callback|.
+void WebMeasureMemory(
+    const FrameNode* frame_node,
+    mojom::WebMemoryMeasurement::Mode mode,
+    std::unique_ptr<WebMeasureMemorySecurityChecker> security_checker,
+    base::OnceCallback<void(mojom::WebMemoryMeasurementPtr)> result_callback,
+    mojo::ReportBadMessageCallback bad_message_callback);
 
 }  // namespace v8_memory
 

components/performance_manager/v8_memory/web_memory_aggregator.cc

@@ -88,6 +88,8 @@ WebMemoryAggregator::~WebMemoryAggregator() = default;
 
 WebMemoryAggregator::NodeAggregationType
 WebMemoryAggregator::FindNodeAggregationType(const FrameNode* frame_node) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
 #if DCHECK_IS_ON()
   auto* node = frame_node;
   while (node && node != aggregation_start_node_) {
@@ -126,6 +128,7 @@ WebMemoryAggregator::FindNodeAggregationType(const FrameNode* frame_node) {
 
 mojom::WebMemoryMeasurementPtr
 WebMemoryAggregator::AggregateMeasureMemoryResult() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   aggregation_result_ = mojom::WebMemoryMeasurement::New();
   VisitFrame(nullptr, aggregation_start_node_);
 
@@ -141,6 +144,7 @@ WebMemoryAggregator::AggregateMeasureMemoryResult() {
 bool WebMemoryAggregator::VisitFrame(
     mojom::WebMemoryBreakdownEntry* enclosing_aggregation_point,
     const FrameNode* frame_node) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(aggregation_result_);
   DCHECK(enclosing_aggregation_point || frame_node == aggregation_start_node_);
   DCHECK(frame_node);
@@ -224,6 +228,7 @@ bool WebMemoryAggregator::VisitFrame(
 bool WebMemoryAggregator::VisitOpenedPage(
     mojom::WebMemoryBreakdownEntry* enclosing_aggregation_point,
     const PageNode* page_node) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (ShouldFollowOpenerLink(page_node)) {
     // Visit only the "current" main frame instead of all of the main frames
     // (non-current ones are either about to die, or represent an ongoing

components/performance_manager/v8_memory/web_memory_aggregator.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/optional.h"
+#include "base/sequence_checker.h"
 #include "components/performance_manager/public/mojom/web_memory.mojom.h"
 #include "url/origin.h"
 
@@ -19,9 +20,9 @@ class PageNode;
 namespace v8_memory {
 
 // Traverses the graph of execution contexts to find the results of the last
-// memory measurement and aggregates them according to the rules defined in
-// https://wicg.github.io/performance-measure-memory. (This implements the
-// draft of 20 October 2020.)
+// memory measurement and aggregates them according to the rules defined in the
+// performance.measureMemory spec. (See public/v8_memory/web_memory.h for the
+// link and spec version.)
 class WebMemoryAggregator {
  public:
   // Constructs an aggregator for the results of a memory request from
@@ -94,7 +95,10 @@ class WebMemoryAggregator {
 
   // Stores the result of the aggregation. This is populated by
   // AggregateMeasureMemoryResult.
-  mojom::WebMemoryMeasurementPtr aggregation_result_;
+  mojom::WebMemoryMeasurementPtr aggregation_result_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 namespace internal {

components/performance_manager/v8_memory/web_memory_impl.cc

@@ -10,9 +10,13 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/check.h"
+#include "base/memory/ptr_util.h"
 #include "components/performance_manager/public/graph/frame_node.h"
+#include "components/performance_manager/public/graph/graph.h"
+#include "components/performance_manager/public/graph/page_node.h"
 #include "components/performance_manager/public/graph/process_node.h"
 #include "components/performance_manager/public/v8_memory/web_memory.h"
+#include "third_party/blink/public/common/features.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -95,6 +99,26 @@ WebMemoryMeasurer::WebMemoryMeasurer(
 
 WebMemoryMeasurer::~WebMemoryMeasurer() = default;
 
+// static
+void WebMemoryMeasurer::MeasureMemory(const FrameNode* frame_node,
+                                      mojom::WebMemoryMeasurement::Mode mode,
+                                      MeasurementCallback callback) {
+  // Can't use make_unique with a private constructor.
+  auto measurer = base::WrapUnique(new WebMemoryMeasurer(
+      frame_node->GetFrameToken(),
+      WebMeasurementModeToRequestMeasurementMode(mode), std::move(callback)));
+
+  // Create a measurement complete callback to own |measurer|. It
+  // will be deleted when the callback is executed or dropped.
+  V8DetailedMemoryRequestOneShot* request = measurer->request();
+  auto measurement_complete_callback = base::BindOnce(
+      &WebMemoryMeasurer::MeasurementComplete, std::move(measurer));
+
+  // Start memory measurement for the process of the given frame.
+  request->StartMeasurement(frame_node->GetProcessNode(),
+                            std::move(measurement_complete_callback));
+}
+
 void WebMemoryMeasurer::MeasurementComplete(
     const ProcessNode* process_node,
     const V8DetailedMemoryProcessData*) {
@@ -103,6 +127,49 @@ void WebMemoryMeasurer::MeasurementComplete(
   std::move(callback_).Run(BuildMemoryUsageResult(frame_token_, process_node));
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// WebMeasureMemorySecurityCheckerImpl
+
+// Implements the public function in public/v8_memory/web_memory.h
+std::unique_ptr<WebMeasureMemorySecurityChecker>
+WebMeasureMemorySecurityChecker::Create() {
+  return std::make_unique<WebMeasureMemorySecurityCheckerImpl>();
+}
+
+void WebMeasureMemorySecurityCheckerImpl::CheckMeasureMemoryIsAllowed(
+    const FrameNode* frame,
+    base::OnceClosure measure_memory_closure,
+    mojo::ReportBadMessageCallback bad_message_callback) const {
+  DCHECK(frame);
+  DCHECK_ON_GRAPH_SEQUENCE(frame->GetGraph());
+
+  // TODO(crbug/1085129): The frame may have navigated since it sent the
+  // measureMemory request. We could return true if the new document is allowed
+  // to measure memory, but the actual document that sent the request is not.
+  // If that happens the DocumentCoordinationUnit mojo interface is reset so
+  // the measurement result will be thrown away, so this is not a security
+  // issue, but it does mean doing extra work.
+
+  if (!base::FeatureList::IsEnabled(
+          blink::features::kWebMeasureMemoryViaPerformanceManager)) {
+    std::move(bad_message_callback)
+        .Run("WebMeasureMemoryViaPerformanceManager feature is disabled");
+    return;
+  }
+  // "Memory measurement allowed" predicate from
+  // https://wicg.github.io/performance-measure-memory/ section 3.2.
+  if (url::Origin::Create(frame->GetURL()) !=
+      url::Origin::Create(frame->GetPageNode()->GetMainFrameNode()->GetURL())) {
+    std::move(bad_message_callback)
+        .Run("performance.measureMemory called from cross-origin subframe");
+    return;
+  }
+  // TODO(crbug/1085129): Check crossOriginIsolated once this is available in
+  // the browser. This will need to be done on the UI sequence, and return the
+  // result to the PM sequence to run the closure.
+  std::move(measure_memory_closure).Run();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Free functions
 
@@ -110,20 +177,19 @@ void WebMemoryMeasurer::MeasurementComplete(
 void WebMeasureMemory(
     const FrameNode* frame_node,
     mojom::WebMemoryMeasurement::Mode mode,
-    base::OnceCallback<void(mojom::WebMemoryMeasurementPtr)> callback) {
-  auto measurer = std::make_unique<WebMemoryMeasurer>(
-      frame_node->GetFrameToken(),
-      WebMeasurementModeToRequestMeasurementMode(mode), std::move(callback));
-
-  // Create a measurement complete callback to own |measurer|. It
-  // will be deleted when the callback is executed or dropped.
-  V8DetailedMemoryRequestOneShot* request = measurer->request();
-  auto measurement_complete_callback = base::BindOnce(
-      &WebMemoryMeasurer::MeasurementComplete, std::move(measurer));
-
-  // Start memory measurement for the process of the given frame.
-  request->StartMeasurement(frame_node->GetProcessNode(),
-                            std::move(measurement_complete_callback));
+    std::unique_ptr<WebMeasureMemorySecurityChecker> security_checker,
+    base::OnceCallback<void(mojom::WebMemoryMeasurementPtr)> result_callback,
+    mojo::ReportBadMessageCallback bad_message_callback) {
+  DCHECK(frame_node);
+  DCHECK(security_checker);
+
+  // Validate that |frame_node| is allowed to measure memory, then start the
+  // measurement.
+  security_checker->CheckMeasureMemoryIsAllowed(
+      frame_node,
+      base::BindOnce(&WebMemoryMeasurer::MeasureMemory, frame_node, mode,
+                     std::move(result_callback)),
+      std::move(bad_message_callback));
 }
 
 }  // namespace v8_memory

components/performance_manager/v8_memory/web_memory_impl.h

@@ -8,12 +8,15 @@
 #include <memory>
 
 #include "base/callback.h"
+#include "base/sequence_checker.h"
 #include "components/performance_manager/public/mojom/web_memory.mojom.h"
 #include "components/performance_manager/public/v8_memory/v8_detailed_memory.h"
+#include "components/performance_manager/public/v8_memory/web_memory.h"
 #include "third_party/blink/public/common/tokens/tokens.h"
 
 namespace performance_manager {
 
+class FrameNode;
 class ProcessNode;
 
 namespace v8_memory {
@@ -28,24 +31,52 @@ class WebMemoryMeasurer {
   using MeasurementCallback =
       base::OnceCallback<void(mojom::WebMemoryMeasurementPtr)>;
 
-  WebMemoryMeasurer(const blink::LocalFrameToken&,
-                    V8DetailedMemoryRequest::MeasurementMode,
-                    MeasurementCallback);
+  // Implements WebMeasureMemory (from public/v8_memory/web_memory.h) by
+  // instantiating a WebMemoryMeasurer.
+  static void MeasureMemory(const FrameNode* frame_node,
+                            mojom::WebMemoryMeasurement::Mode mode,
+                            MeasurementCallback callback);
+
   ~WebMemoryMeasurer();
 
   WebMemoryMeasurer(const WebMemoryMeasurer& other) = delete;
   WebMemoryMeasurer& operator=(const WebMemoryMeasurer& other) = delete;
 
-  V8DetailedMemoryRequestOneShot* request() const { return request_.get(); }
+  V8DetailedMemoryRequestOneShot* request() const {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    return request_.get();
+  }
 
   // A callback for V8DetailedMemoryRequestOneShot.
   void MeasurementComplete(const ProcessNode*,
                            const V8DetailedMemoryProcessData*);
 
  private:
+  friend class WebMemoryImplTest;
+
+  WebMemoryMeasurer(const blink::LocalFrameToken&,
+                    V8DetailedMemoryRequest::MeasurementMode,
+                    MeasurementCallback);
+
   blink::LocalFrameToken frame_token_;
   MeasurementCallback callback_;
-  std::unique_ptr<V8DetailedMemoryRequestOneShot> request_;
+  std::unique_ptr<V8DetailedMemoryRequestOneShot> request_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+
+  SEQUENCE_CHECKER(sequence_checker_);
+};
+
+// The default implementation of WebMeasureMemorySecurityChecker.
+class WebMeasureMemorySecurityCheckerImpl
+    : public WebMeasureMemorySecurityChecker {
+ public:
+  WebMeasureMemorySecurityCheckerImpl() = default;
+  ~WebMeasureMemorySecurityCheckerImpl() override = default;
+
+  void CheckMeasureMemoryIsAllowed(
+      const FrameNode* frame,
+      base::OnceClosure measure_memory_closure,
+      mojo::ReportBadMessageCallback bad_message_callback) const override;
 };
 
 }  // namespace v8_memory

components/performance_manager/v8_memory/web_memory_impl_unittest.cc

@@ -31,8 +31,6 @@ namespace performance_manager {
 
 namespace v8_memory {
 
-namespace {
-
 using WebMemoryImplPMTest = V8MemoryPerformanceManagerTestHarness;
 
 class WebMemoryImplTest : public WebMemoryTestHarness {
@@ -41,6 +39,25 @@ class WebMemoryImplTest : public WebMemoryTestHarness {
                         base::flat_map<std::string, Bytes> expected);
 };
 
+class FakeSecurityChecker : public WebMeasureMemorySecurityChecker {
+ public:
+  explicit FakeSecurityChecker(bool allowed) : allowed_(allowed) {}
+
+  void CheckMeasureMemoryIsAllowed(
+      const FrameNode* frame_node,
+      base::OnceClosure measure_memory_closure,
+      mojo::ReportBadMessageCallback bad_message_callback) const override {
+    if (allowed_) {
+      std::move(measure_memory_closure).Run();
+    } else {
+      std::move(bad_message_callback).Run("disallowed");
+    }
+  }
+
+ private:
+  bool allowed_;
+};
+
 void WebMemoryImplTest::MeasureAndVerify(
     FrameNodeImpl* frame,
     base::flat_map<std::string, Bytes> expected) {
@@ -110,6 +127,11 @@ TEST_F(WebMemoryImplPMTest, WebMeasureMemory) {
         EXPECT_EQ(1001u, entry->bytes);
         run_loop.Quit();
       });
+  auto bad_message_callback =
+      base::BindLambdaForTesting([&](const std::string& error) {
+        ADD_FAILURE() << error;
+        run_loop.Quit();
+      });
 
   base::WeakPtr<FrameNode> frame_node_wrapper =
       PerformanceManager::GetFrameNodeForRenderFrameHost(main_frame());
@@ -117,9 +139,10 @@ TEST_F(WebMemoryImplPMTest, WebMeasureMemory) {
       FROM_HERE, base::BindLambdaForTesting([&]() {
         ASSERT_TRUE(frame_node_wrapper);
         FrameNode* frame_node = frame_node_wrapper.get();
-        WebMeasureMemory(frame_node,
-                         mojom::WebMemoryMeasurement::Mode::kDefault,
-                         std::move(measurement_callback));
+        WebMeasureMemory(
+            frame_node, mojom::WebMemoryMeasurement::Mode::kDefault,
+            std::make_unique<FakeSecurityChecker>(true),
+            std::move(measurement_callback), std::move(bad_message_callback));
       }));
 
   // Set up and bind the mock reporter.
@@ -148,6 +171,8 @@ TEST_F(WebMemoryImplPMTest, MeasurementInterrupted) {
       base::BindOnce([](mojom::WebMemoryMeasurementPtr result) {
         FAIL() << "Measurement callback ran unexpectedly";
       });
+  auto bad_message_callback =
+      base::BindOnce([](const std::string& error) { FAIL() << error; });
 
   base::WeakPtr<FrameNode> frame_node_wrapper =
       PerformanceManager::GetFrameNodeForRenderFrameHost(child_frame());
@@ -155,9 +180,10 @@ TEST_F(WebMemoryImplPMTest, MeasurementInterrupted) {
       FROM_HERE, base::BindLambdaForTesting([&]() {
         ASSERT_TRUE(frame_node_wrapper);
         FrameNode* frame_node = frame_node_wrapper.get();
-        WebMeasureMemory(frame_node,
-                         mojom::WebMemoryMeasurement::Mode::kDefault,
-                         std::move(measurement_callback));
+        WebMeasureMemory(
+            frame_node, mojom::WebMemoryMeasurement::Mode::kDefault,
+            std::make_unique<FakeSecurityChecker>(true),
+            std::move(measurement_callback), std::move(bad_message_callback));
       }));
 
   // Set up and bind the mock reporter.
@@ -183,7 +209,35 @@ TEST_F(WebMemoryImplPMTest, MeasurementInterrupted) {
   task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(5));
 }
 
-}  // namespace
+TEST_F(WebMemoryImplPMTest, MeasurementDisallowed) {
+  // Call WebMeasureMemory on the performance manager sequence but expect the
+  // mojo ReportBadMessage callback to be called.
+  base::RunLoop run_loop;
+  auto measurement_callback =
+      base::BindLambdaForTesting([&](mojom::WebMemoryMeasurementPtr result) {
+        ADD_FAILURE() << "Measurement callback ran unexpectedly.";
+        run_loop.Quit();
+      });
+  auto bad_message_callback =
+      base::BindLambdaForTesting([&](const std::string& error) {
+        SUCCEED() << error;
+        run_loop.Quit();
+      });
+
+  base::WeakPtr<FrameNode> frame_node_wrapper =
+      PerformanceManager::GetFrameNodeForRenderFrameHost(main_frame());
+  PerformanceManager::CallOnGraph(
+      FROM_HERE, base::BindLambdaForTesting([&]() {
+        ASSERT_TRUE(frame_node_wrapper);
+        FrameNode* frame_node = frame_node_wrapper.get();
+        WebMeasureMemory(
+            frame_node, mojom::WebMemoryMeasurement::Mode::kDefault,
+            std::make_unique<FakeSecurityChecker>(false),
+            std::move(measurement_callback), std::move(bad_message_callback));
+      }));
+
+  run_loop.Run();
+}
 
 }  // namespace v8_memory