Fix H2 push stream matching logic.

When matching pushed streams against incoming requests, SpdySession
attempts to handle aliased streams. Unfortunately, it duplicated the
stream alias matching in SpdySessionPool, instead of using a common
function. As a result, the matching code has become outdated, and
ignores a number of fields (NetworkIsolationKey, socket tags,
is_proxy_session, disable_secure_dns).

This CL fixes the issue by making both aliasing paths use a
helper method of SpdySessionKey, which is hopefully less likely to
become outdated.

Bug: 1105544
Change-Id: I2d88a4a15d0e260fb5049a533ba52693c09bfc0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300270
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788866}

net/http/http_network_transaction_unittest.cc

@@ -11114,7 +11114,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthSpdyProxy) {
 
 // Test that an explicitly trusted SPDY proxy can push a resource from an
 // origin that is different from that of its associated resource.
-TEST_F(HttpNetworkTransactionTest, CrossOriginSPDYProxyPush) {
+TEST_F(HttpNetworkTransactionTest, CrossOriginSpdyProxyPush) {
   // Configure the proxy delegate to allow cross-origin SPDY pushes.
   auto proxy_delegate = std::make_unique<TestProxyDelegate>();
   proxy_delegate->set_trusted_spdy_proxy(net::ProxyServer::FromURI(

net/spdy/spdy_network_transaction_unittest.cc

@@ -6687,6 +6687,127 @@ TEST_F(SpdyNetworkTransactionTest, ServerPushValidCrossOrigin) {
   VerifyStreamsClosed(helper);
 }
 
+// Verify that push does not work cross origin when NetworkIsolationKeys don't
+// match.
+TEST_F(SpdyNetworkTransactionTest,
+       ServerPushCrossOriginNetworkIsolationKeyMistmatch) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeatures(
+      // enabled_features
+      {features::kPartitionHttpServerPropertiesByNetworkIsolationKey,
+       // Need to partition connections by NetworkIsolationKey for
+       // SpdySessionKeys to include NetworkIsolationKeys.
+       features::kPartitionConnectionsByNetworkIsolationKey},
+      // disabled_features
+      {});
+
+  // "spdy_pooling.pem" is valid for both www.example.org and mail.example.org.
+  const char* url_to_fetch = "https://www.example.org";
+  const char* url_to_push = "https://mail.example.org";
+
+  spdy::SpdySerializedFrame headers(
+      spdy_util_.ConstructSpdyGet(url_to_fetch, 1, LOWEST));
+  spdy::SpdySerializedFrame push_priority(
+      spdy_util_.ConstructSpdyPriority(2, 1, IDLE, true));
+  MockWrite writes[] = {
+      CreateMockWrite(headers, 0),
+      CreateMockWrite(push_priority, 3),
+  };
+
+  spdy::SpdySerializedFrame reply(
+      spdy_util_.ConstructSpdyGetReply(nullptr, 0, 1));
+  spdy::SpdySerializedFrame push(
+      spdy_util_.ConstructSpdyPush(nullptr, 0, 2, 1, url_to_push));
+  spdy::SpdySerializedFrame body(spdy_util_.ConstructSpdyDataFrame(1, true));
+  const char kPushedData[] = "pushed";
+  spdy::SpdySerializedFrame pushed_body(
+      spdy_util_.ConstructSpdyDataFrame(2, kPushedData, true));
+  MockRead reads[] = {
+      CreateMockRead(reply, 1),
+      CreateMockRead(push, 2, SYNCHRONOUS),
+      CreateMockRead(body, 4),
+      CreateMockRead(pushed_body, 5, SYNCHRONOUS),
+      MockRead(SYNCHRONOUS, ERR_IO_PENDING, 6),
+  };
+
+  SequencedSocketData data(reads, writes);
+
+  request_.url = GURL(url_to_fetch);
+
+  NormalSpdyTransactionHelper helper(request_, DEFAULT_PRIORITY, log_, nullptr);
+  helper.RunPreTestSetup();
+  helper.AddData(&data);
+
+  SequencedSocketData data2(net::MockConnect(ASYNC, net::ERR_FAILED),
+                            base::span<MockRead>(), base::span<MockWrite>());
+  helper.AddData(&data2);
+
+  HttpNetworkTransaction* trans0 = helper.trans();
+  TestCompletionCallback callback0;
+  int rv = trans0->Start(&request_, callback0.callback(), log_);
+  rv = callback0.GetResult(rv);
+  EXPECT_THAT(rv, IsOk());
+
+  SpdySessionPool* spdy_session_pool = helper.session()->spdy_session_pool();
+  SpdySessionKey key(host_port_pair_, ProxyServer::Direct(),
+                     PRIVACY_MODE_DISABLED,
+                     SpdySessionKey::IsProxySession::kFalse, SocketTag(),
+                     NetworkIsolationKey(), false /* disable_secure_dns */);
+  base::WeakPtr<SpdySession> spdy_session =
+      spdy_session_pool->FindAvailableSession(
+          key, /* enable_ip_based_pooling = */ true,
+          /* is_websocket = */ false, log_);
+
+  EXPECT_EQ(1u, num_unclaimed_pushed_streams(spdy_session));
+  EXPECT_TRUE(
+      has_unclaimed_pushed_stream_for_url(spdy_session, GURL(url_to_push)));
+
+  HttpNetworkTransaction trans1(DEFAULT_PRIORITY, helper.session());
+  HttpRequestInfo push_request;
+  // Use a different NetworkIsolationKey than |spdy_session| (which uses an
+  // empty one).
+  push_request.network_isolation_key = NetworkIsolationKey::CreateTransient();
+  push_request.method = "GET";
+  push_request.url = GURL(url_to_push);
+  push_request.traffic_annotation =
+      net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
+  TestCompletionCallback callback1;
+  rv = trans1.Start(&push_request, callback1.callback(), log_);
+  // This transaction should try and use a new socket, which fails.
+  EXPECT_THAT(callback1.GetResult(rv), IsError(net::ERR_FAILED));
+
+  // The pushed stream should still be pending.
+  EXPECT_EQ(1u, num_unclaimed_pushed_streams(spdy_session));
+
+  // Try again, this time with an empty NetworkIsolationKey, matching the
+  // SpdySession's. This request should successfully get the pushed stream.
+  HttpNetworkTransaction trans2(DEFAULT_PRIORITY, helper.session());
+  push_request.network_isolation_key = NetworkIsolationKey();
+  TestCompletionCallback callback2;
+  rv = trans1.Start(&push_request, callback2.callback(), log_);
+  EXPECT_THAT(callback2.GetResult(rv), IsOk());
+
+  HttpResponseInfo response = *trans0->GetResponseInfo();
+  EXPECT_TRUE(response.headers);
+  EXPECT_EQ("HTTP/1.1 200", response.headers->GetStatusLine());
+
+  std::string result0;
+  ReadResult(trans0, &result0);
+  EXPECT_EQ("hello!", result0);
+
+  HttpResponseInfo push_response = *trans1.GetResponseInfo();
+  EXPECT_TRUE(push_response.headers);
+  EXPECT_EQ("HTTP/1.1 200", push_response.headers->GetStatusLine());
+
+  std::string result1;
+  ReadResult(&trans1, &result1);
+  EXPECT_EQ(kPushedData, result1);
+
+  base::RunLoop().RunUntilIdle();
+  helper.VerifyDataConsumed();
+  VerifyStreamsClosed(helper);
+}
+
 // Regression test for https://crbug.com/832859:  Server push is accepted on a
 // connection with client certificate, as long as SpdySessionKey matches.
 TEST_F(SpdyNetworkTransactionTest, ServerPushWithClientCert) {

net/spdy/spdy_session.cc

@@ -1606,9 +1606,12 @@ bool SpdySession::ValidatePushedStream(spdy::SpdyStreamId stream_id,
                                        const GURL& url,
                                        const HttpRequestInfo& request_info,
                                        const SpdySessionKey& key) const {
-  // Proxy server and privacy mode must match.
-  if (key.proxy_server() != spdy_session_key_.proxy_server() ||
-      key.privacy_mode() != spdy_session_key_.privacy_mode()) {
+  SpdySessionKey::CompareForAliasingResult compare_result =
+      key.CompareForAliasing(spdy_session_key_);
+  // Keys must be aliasable. This code does not support changing the tag of live
+  // sessions, so just fail on a socket tag mismatch.
+  if (!compare_result.is_potentially_aliasable ||
+      !compare_result.is_socket_tag_match) {
     return false;
   }
   // Certificate must match for encrypted schemes only.

net/spdy/spdy_session_key.cc

@@ -70,6 +70,19 @@ bool SpdySessionKey::operator!=(const SpdySessionKey& other) const {
   return !(*this == other);
 }
 
+SpdySessionKey::CompareForAliasingResult SpdySessionKey::CompareForAliasing(
+    const SpdySessionKey& other) const {
+  CompareForAliasingResult result;
+  result.is_potentially_aliasable =
+      (privacy_mode_ == other.privacy_mode_ &&
+       host_port_proxy_pair_.second == other.host_port_proxy_pair_.second &&
+       is_proxy_session_ == other.is_proxy_session_ &&
+       network_isolation_key_ == other.network_isolation_key_ &&
+       disable_secure_dns_ == other.disable_secure_dns_);
+  result.is_socket_tag_match = (socket_tag_ == other.socket_tag_);
+  return result;
+}
+
 size_t SpdySessionKey::EstimateMemoryUsage() const {
   return base::trace_event::EstimateMemoryUsage(host_port_proxy_pair_);
 }

net/spdy/spdy_session_key.h

@@ -26,6 +26,7 @@ class NET_EXPORT_PRIVATE SpdySessionKey {
     // that we're proxying the connection to.
     kTrue,
   };
+
   SpdySessionKey();
 
   SpdySessionKey(const HostPortPair& host_port_pair,
@@ -47,6 +48,26 @@ class NET_EXPORT_PRIVATE SpdySessionKey {
   bool operator==(const SpdySessionKey& other) const;
   bool operator!=(const SpdySessionKey& other) const;
 
+  // Struct returned by CompareForAliasing().
+  struct CompareForAliasingResult {
+    // True if the two SpdySessionKeys match, except possibly for their
+    // |host_port_pair| and |socket_tag|.
+    bool is_potentially_aliasable = false;
+    // True if the |socket_tag| fields match. If this is false, it's up to the
+    // caller to change the tag of the session (if possible) or to not alias the
+    // session, even if |is_potentially_aliasable| is true.
+    bool is_socket_tag_match = false;
+  };
+
+  // Checks if requests using SpdySessionKey can potentially be used to service
+  // requests using another. The caller *MUST* also make sure that the session
+  // associated with one key has been verified for use with the other.
+  //
+  // Note that this method is symmetric, so it doesn't matter which key's method
+  // is called on the other.
+  CompareForAliasingResult CompareForAliasing(
+      const SpdySessionKey& other) const;
+
   const HostPortProxyPair& host_port_proxy_pair() const {
     return host_port_proxy_pair_;
   }

net/spdy/spdy_session_pool.cc

@@ -276,15 +276,11 @@ OnHostResolutionCallbackResult SpdySessionPool::OnHostResolutionComplete(
       // It shouldn't be in the aliases table if it doesn't exist!
       DCHECK(available_session_it != available_sessions_.end());
 
-      // This session can be reused only if the proxy and privacy settings
-      // match, as well as the NetworkIsolationKey.
-      if (!(alias_key.proxy_server() == key.proxy_server()) ||
-          !(alias_key.privacy_mode() == key.privacy_mode()) ||
-          !(alias_key.is_proxy_session() == key.is_proxy_session()) ||
-          !(alias_key.network_isolation_key() == key.network_isolation_key()) ||
-          !(alias_key.disable_secure_dns() == key.disable_secure_dns())) {
+      SpdySessionKey::CompareForAliasingResult compare_result =
+          alias_key.CompareForAliasing(key);
+      // Keys must be aliasable.
+      if (!compare_result.is_potentially_aliasable)
         continue;
-      }
 
       if (is_websocket && !available_session_it->second->support_websocket())
         continue;
@@ -306,7 +302,7 @@ OnHostResolutionCallbackResult SpdySessionPool::OnHostResolutionComplete(
       bool adding_pooled_alias = true;
 
       // If socket tags differ, see if session's socket tag can be changed.
-      if (alias_key.socket_tag() != key.socket_tag()) {
+      if (!compare_result.is_socket_tag_match) {
         SpdySessionKey old_key = available_session->spdy_session_key();
         SpdySessionKey new_key(old_key.host_port_pair(), old_key.proxy_server(),
                                old_key.privacy_mode(),