viz: Don't generate new nonce in ParentLocalSurfaceIdAllocator

A client can embed every surface of another client forever if it has one valid
LocalSurfaceID from that client, so generating a new nonce for every allocation
does not provide any security benefit. Let's just generate it once and use it
for all LocalSurfaceIds allocated by that particular allocator.

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ie028c8b3b73b80ca5bc3e6c23e439b23c54fefe4
Reviewed-on: https://chromium-review.googlesource.com/931741Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538981}

components/viz/common/surfaces/child_local_surface_id_allocator.cc

@@ -26,10 +26,10 @@ const LocalSurfaceId& ChildLocalSurfaceIdAllocator::UpdateFromParent(
          parent_allocated_local_surface_id.nonce() ==
              last_known_local_surface_id_.nonce());
 
-  last_known_local_surface_id_ =
-      LocalSurfaceId(parent_allocated_local_surface_id.parent_sequence_number(),
-                     last_known_local_surface_id_.child_sequence_number(),
-                     parent_allocated_local_surface_id.nonce());
+  last_known_local_surface_id_.parent_sequence_number_ =
+      parent_allocated_local_surface_id.parent_sequence_number_;
+  last_known_local_surface_id_.nonce_ =
+      parent_allocated_local_surface_id.nonce_;
   return last_known_local_surface_id_;
 }
 
@@ -38,10 +38,7 @@ const LocalSurfaceId& ChildLocalSurfaceIdAllocator::GenerateId() {
   DCHECK_NE(last_known_local_surface_id_.parent_sequence_number(),
             kInvalidParentSequenceNumber);
 
-  last_known_local_surface_id_ =
-      LocalSurfaceId(last_known_local_surface_id_.parent_sequence_number(),
-                     last_known_local_surface_id_.child_sequence_number() + 1,
-                     last_known_local_surface_id_.nonce());
+  ++last_known_local_surface_id_.child_sequence_number_;
   return last_known_local_surface_id_;
 }
 

components/viz/common/surfaces/local_surface_id.h

@@ -21,10 +21,38 @@ namespace mojom {
 class LocalSurfaceIdDataView;
 }
 
+class ParentLocalSurfaceIdAllocator;
+class ChildLocalSurfaceIdAllocator;
+
 constexpr uint32_t kInvalidParentSequenceNumber = 0;
 constexpr uint32_t kInvalidChildSequenceNumber = 0;
 constexpr uint32_t kInitialChildSequenceNumber = 1;
 
+// This struct is the part of SurfaceId that can be modified by the client.
+// LocalSurfaceId uniquely identifies a surface among the surfaces created by a
+// particular client. A SurfaceId, which is FrameSinkId+LocalSurfaceId, uniquely
+// identifies a surface globally across all clients.
+//
+// LocalSurfaceId consists of:
+//
+// - parent_sequence_number: This part is incremented by the embedder of the
+//   client.
+//
+// - child_sequence_number: This part is incremented by the client itself.
+//
+// - nonce: An UnguessableToken generated by the embedder. The purpose of this
+//   value is to make SurfaceIds unguessable, because FrameSinkIds and
+//   LocalSurfaceIds are otherwise predictable and clients might exploit this
+//   fact to embed surfaces they're not allowed to. This value is generated once
+//   by ParentLocalSurfaceIdAllocator and remains constant during the lifetime
+//   of the allocator.
+//
+// The embedder uses ParentLocalSurfaceIdAllocator to generate LocalSurfaceIds
+// for the embedee. If Surface Synchronization is on, the embedee uses
+// ChildLocalSurfaceIdAllocator to generate LocalSurfaceIds for itself. If
+// Surface Synchronization is off, the embedee also uses
+// ParentLocalSurfaceIdAllocator, as the parent doesn't generate LocalSurfaceIds
+// for the child.
 class VIZ_COMMON_EXPORT LocalSurfaceId {
  public:
   constexpr LocalSurfaceId()
@@ -88,6 +116,8 @@ class VIZ_COMMON_EXPORT LocalSurfaceId {
  private:
   friend struct mojo::StructTraits<mojom::LocalSurfaceIdDataView,
                                    LocalSurfaceId>;
+  friend class ParentLocalSurfaceIdAllocator;
+  friend class ChildLocalSurfaceIdAllocator;
 
   friend bool operator<(const LocalSurfaceId& lhs, const LocalSurfaceId& rhs);
 

components/viz/common/surfaces/parent_local_surface_id_allocator.cc

@@ -11,25 +11,20 @@ namespace viz {
 ParentLocalSurfaceIdAllocator::ParentLocalSurfaceIdAllocator()
     : last_known_local_surface_id_(kInvalidParentSequenceNumber,
                                    kInitialChildSequenceNumber,
-                                   base::UnguessableToken()) {}
+                                   base::UnguessableToken::Create()) {}
 
 const LocalSurfaceId& ParentLocalSurfaceIdAllocator::UpdateFromChild(
     const LocalSurfaceId& child_allocated_local_surface_id) {
   DCHECK_GE(child_allocated_local_surface_id.child_sequence_number(),
             last_known_local_surface_id_.child_sequence_number());
 
-  last_known_local_surface_id_ =
-      LocalSurfaceId(last_known_local_surface_id_.parent_sequence_number(),
-                     child_allocated_local_surface_id.child_sequence_number(),
-                     last_known_local_surface_id_.nonce());
+  last_known_local_surface_id_.child_sequence_number_ =
+      child_allocated_local_surface_id.child_sequence_number_;
   return last_known_local_surface_id_;
 }
 
 const LocalSurfaceId& ParentLocalSurfaceIdAllocator::GenerateId() {
-  last_known_local_surface_id_ =
-      LocalSurfaceId(last_known_local_surface_id_.parent_sequence_number() + 1,
-                     last_known_local_surface_id_.child_sequence_number(),
-                     base::UnguessableToken::Create());
+  ++last_known_local_surface_id_.parent_sequence_number_;
   return last_known_local_surface_id_;
 }
 

components/viz/common/surfaces/parent_local_surface_id_allocator_unittest.cc

@@ -28,8 +28,8 @@ ParentLocalSurfaceIdAllocator GetChildUpdatedAllocator();
 
 }  // namespace
 
-// The default constructor should initialize its last-known LocalSurfaceId (and
-// all of its components) to an invalid state.
+// The default constructor should generate a nonce and initialize the sequence
+// number of the last known LocalSurfaceId to an invalid state.
 TEST(ParentLocalSurfaceIdAllocatorTest,
      DefaultConstructorShouldNotSetLocalSurfaceIdComponents) {
   ParentLocalSurfaceIdAllocator default_constructed_parent_allocator;
@@ -39,7 +39,7 @@ TEST(ParentLocalSurfaceIdAllocatorTest,
   EXPECT_FALSE(default_local_surface_id.is_valid());
   EXPECT_TRUE(ParentSequenceNumberIsNotSet(default_local_surface_id));
   EXPECT_TRUE(ChildSequenceNumberIsSet(default_local_surface_id));
-  EXPECT_TRUE(NonceIsEmpty(default_local_surface_id));
+  EXPECT_FALSE(NonceIsEmpty(default_local_surface_id));
 }
 
 // The move constructor should move the last-known LocalSurfaceId.
@@ -108,7 +108,7 @@ TEST(ParentLocalSurfaceIdAllocatorTest,
 }
 
 // GenerateId() on a parent allocator should monotonically increment the parent
-// sequence number and create a new nonce.
+// sequence number and use the previous nonce.
 TEST(ParentLocalSurfaceIdAllocatorTest,
      GenerateIdOnlyUpdatesExpectedLocalSurfaceIdComponents) {
   ParentLocalSurfaceIdAllocator generating_parent_allocator =
@@ -125,7 +125,7 @@ TEST(ParentLocalSurfaceIdAllocatorTest,
             postgenerateid_local_surface_id.parent_sequence_number());
   EXPECT_EQ(pregenerateid_local_surface_id.child_sequence_number(),
             postgenerateid_local_surface_id.child_sequence_number());
-  EXPECT_NE(pregenerateid_local_surface_id.nonce(),
+  EXPECT_EQ(pregenerateid_local_surface_id.nonce(),
             postgenerateid_local_surface_id.nonce());
   EXPECT_EQ(returned_local_surface_id,
             generating_parent_allocator.last_known_local_surface_id());