Reject reasons from strike register when nonce validation fails.

Send reject reason to client for debugging purposes.

Merge internal change: 70366491

R=wtc@chromium.org

Review URL: https://codereview.chromium.org/388333005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283692 0039d316-1c4b-4281-b951-d872f2087c98

net/quic/crypto/crypto_handshake.h

@@ -26,49 +26,54 @@ enum HandshakeFailureReason {
 
   // Failure reasons for an invalid client nonce in CHLO.
   //
+  // The default error value for nonce verification failures from strike
+  // register (covers old strike registers and unknown failures).
+  CLIENT_NONCE_UNKNOWN_FAILURE = 1,
   // Client nonce had incorrect length.
-  CLIENT_NONCE_INVALID_FAILURE = 1,
+  CLIENT_NONCE_INVALID_FAILURE = 2,
   // Client nonce is not unique.
-  CLIENT_NONCE_NOT_UNIQUE_FAILURE = 2,
+  CLIENT_NONCE_NOT_UNIQUE_FAILURE = 3,
   // Client orbit is invalid or incorrect.
-  CLIENT_NONCE_INVALID_ORBIT_FAILURE = 3,
+  CLIENT_NONCE_INVALID_ORBIT_FAILURE = 4,
   // Client nonce's timestamp is not in the strike register's valid time range.
-  CLIENT_NONCE_INVALID_TIME_FAILURE = 4,
-  // Client nonce verification has failed because strike register is down.
-  CLIENT_NONCE_NO_STRIKE_REGISTER_FAILURE = 5,
+  CLIENT_NONCE_INVALID_TIME_FAILURE = 5,
+  // Strike register's RPC call timed out, client nonce couldn't be verified.
+  CLIENT_NONCE_STRIKE_REGISTER_TIMEOUT = 6,
+  // Strike register is down, client nonce couldn't be verified.
+  CLIENT_NONCE_STRIKE_REGISTER_FAILURE = 7,
 
   // Failure reasons for an invalid server nonce in CHLO.
   //
   // Unbox of server nonce failed.
-  SERVER_NONCE_DECRYPTION_FAILURE = 6,
+  SERVER_NONCE_DECRYPTION_FAILURE = 8,
   // Decrypted server nonce had incorrect length.
-  SERVER_NONCE_INVALID_FAILURE = 7,
+  SERVER_NONCE_INVALID_FAILURE = 9,
   // Server nonce is not unique.
-  SERVER_NONCE_NOT_UNIQUE_FAILURE = 8,
+  SERVER_NONCE_NOT_UNIQUE_FAILURE = 10,
   // Server nonce's timestamp is not in the strike register's valid time range.
-  SERVER_NONCE_INVALID_TIME_FAILURE = 9,
+  SERVER_NONCE_INVALID_TIME_FAILURE = 11,
 
   // Failure reasons for an invalid server config in CHLO.
   //
   // Missing Server config id (kSCID) tag.
-  SERVER_CONFIG_INCHOATE_HELLO_FAILURE = 10,
+  SERVER_CONFIG_INCHOATE_HELLO_FAILURE = 12,
   // Couldn't find the Server config id (kSCID).
-  SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE = 11,
+  SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE = 13,
 
   // Failure reasons for an invalid source-address token.
   //
   // Missing Source-address token (kSourceAddressTokenTag) tag.
-  SOURCE_ADDRESS_TOKEN_INVALID_FAILURE = 12,
+  SOURCE_ADDRESS_TOKEN_INVALID_FAILURE = 14,
   // Unbox of Source-address token failed.
-  SOURCE_ADDRESS_TOKEN_DECRYPTION_FAILURE = 13,
+  SOURCE_ADDRESS_TOKEN_DECRYPTION_FAILURE = 15,
   // Couldn't parse the unbox'ed Source-address token.
-  SOURCE_ADDRESS_TOKEN_PARSE_FAILURE = 14,
+  SOURCE_ADDRESS_TOKEN_PARSE_FAILURE = 16,
   // Source-address token is for a different IP address.
-  SOURCE_ADDRESS_TOKEN_DIFFERENT_IP_ADDRESS_FAILURE = 15,
+  SOURCE_ADDRESS_TOKEN_DIFFERENT_IP_ADDRESS_FAILURE = 17,
   // The source-address token has a timestamp in the future.
-  SOURCE_ADDRESS_TOKEN_CLOCK_SKEW_FAILURE = 16,
+  SOURCE_ADDRESS_TOKEN_CLOCK_SKEW_FAILURE = 18,
   // The source-address token has expired.
-  SOURCE_ADDRESS_TOKEN_EXPIRED_FAILURE = 17,
+  SOURCE_ADDRESS_TOKEN_EXPIRED_FAILURE = 19,
 
   MAX_FAILURE_REASON,
 };

net/quic/crypto/crypto_server_test.cc

@@ -365,7 +365,7 @@ TEST_F(CryptoServerTest, DISABLED_DefaultCert) {
   EXPECT_NE(0u, cert.size());
   EXPECT_NE(0u, proof.size());
   const HandshakeFailureReason kRejectReasons[] = {
-    CLIENT_NONCE_INVALID_FAILURE
+    CLIENT_NONCE_INVALID_TIME_FAILURE
   };
   CheckRejectReasons(kRejectReasons, arraysize(kRejectReasons));
 }
@@ -551,7 +551,7 @@ TEST_P(CryptoServerTest, ReplayProtection) {
   ASSERT_EQ(kREJ, out_.tag());
 
   const HandshakeFailureReason kRejectReasons[] = {
-    CLIENT_NONCE_INVALID_FAILURE
+    CLIENT_NONCE_INVALID_TIME_FAILURE
   };
   CheckRejectReasons(kRejectReasons, arraysize(kRejectReasons));
 
@@ -652,7 +652,7 @@ TEST_P(CryptoServerTestNoConfig, DontCrash) {
       NULL));
 
   const HandshakeFailureReason kRejectReasons[] = {
-    CLIENT_NONCE_INVALID_FAILURE
+    SERVER_CONFIG_INCHOATE_HELLO_FAILURE
   };
   CheckRejectReasons(kRejectReasons, arraysize(kRejectReasons));
 }

net/quic/crypto/local_strike_register_client.cc

@@ -30,19 +30,21 @@ bool LocalStrikeRegisterClient::IsKnownOrbit(StringPiece orbit) const {
 }
 
 void LocalStrikeRegisterClient::VerifyNonceIsValidAndUnique(
-    StringPiece nonce, QuicWallTime now, ResultCallback* cb) {
-  bool nonce_is_valid_and_unique;
+    StringPiece nonce,
+    QuicWallTime now,
+    ResultCallback* cb) {
+  InsertStatus nonce_error;
   if (nonce.length() != kNonceSize) {
-    nonce_is_valid_and_unique = false;
+    nonce_error = NONCE_INVALID_FAILURE;
   } else {
     base::AutoLock lock(m_);
-    nonce_is_valid_and_unique = strike_register_.Insert(
+    nonce_error = strike_register_.Insert(
         reinterpret_cast<const uint8*>(nonce.data()),
         static_cast<uint32>(now.ToUNIXSeconds()));
   }
 
   // m_ must not be held when the ResultCallback runs.
-  cb->Run(nonce_is_valid_and_unique);
+  cb->Run((nonce_error == NONCE_OK), nonce_error);
 }
 
 }  // namespace net

net/quic/crypto/local_strike_register_client_test.cc

@@ -25,21 +25,27 @@ class RecordResultCallback : public StrikeRegisterClient::ResultCallback {
   // RecordResultCallback stores the argument to RunImpl in
   // |*saved_value| and sets |*called| to true.  The callback is self
   // deleting.
-  RecordResultCallback(bool* called, bool* saved_value)
+  RecordResultCallback(bool* called,
+                       bool* saved_value,
+                       InsertStatus* saved_nonce_error)
       : called_(called),
-        saved_value_(saved_value) {
+        saved_value_(saved_value),
+        saved_nonce_error_(saved_nonce_error) {
     *called_ = false;
   }
 
  protected:
-  virtual void RunImpl(bool nonce_is_valid_and_unique) OVERRIDE {
+  virtual void RunImpl(bool nonce_is_valid_and_unique,
+                       InsertStatus nonce_error) OVERRIDE {
     *called_ = true;
     *saved_value_ = nonce_is_valid_and_unique;
+    *saved_nonce_error_ = nonce_error;
   }
 
  private:
   bool* called_;
   bool* saved_value_;
+  InsertStatus* saved_nonce_error_;
 
   DISALLOW_COPY_AND_ASSIGN(RecordResultCallback);
 };
@@ -85,39 +91,45 @@ TEST_F(LocalStrikeRegisterClientTest, IncorrectNonceLength) {
     // Validation fails if you remove a byte from the nonce.
     bool called = false;
     bool is_valid = false;
+    InsertStatus nonce_error = NONCE_UNKNOWN_FAILURE;
     string short_nonce = valid_nonce.substr(0, valid_nonce.length() - 1);
     strike_register_->VerifyNonceIsValidAndUnique(
         short_nonce,
         QuicWallTime::FromUNIXSeconds(kCurrentTimeExternalSecs),
-        new RecordResultCallback(&called, &is_valid));
+        new RecordResultCallback(&called, &is_valid, &nonce_error));
     EXPECT_TRUE(called);
     EXPECT_FALSE(is_valid);
+    EXPECT_EQ(NONCE_INVALID_FAILURE, nonce_error);
   }
 
   {
     // Validation fails if you add a byte to the nonce.
     bool called = false;
     bool is_valid = false;
+    InsertStatus nonce_error = NONCE_UNKNOWN_FAILURE;
     string long_nonce(valid_nonce);
     long_nonce.append("a");
     strike_register_->VerifyNonceIsValidAndUnique(
         long_nonce,
         QuicWallTime::FromUNIXSeconds(kCurrentTimeExternalSecs),
-        new RecordResultCallback(&called, &is_valid));
+        new RecordResultCallback(&called, &is_valid, &nonce_error));
     EXPECT_TRUE(called);
     EXPECT_FALSE(is_valid);
+    EXPECT_EQ(NONCE_INVALID_FAILURE, nonce_error);
   }
 
   {
     // Verify that the base nonce validates was valid.
     bool called = false;
     bool is_valid = false;
+    InsertStatus nonce_error = NONCE_UNKNOWN_FAILURE;
     strike_register_->VerifyNonceIsValidAndUnique(
         valid_nonce,
         QuicWallTime::FromUNIXSeconds(kCurrentTimeExternalSecs),
-        new RecordResultCallback(&called, &is_valid));
+        new RecordResultCallback(&called, &is_valid, &nonce_error));
     EXPECT_TRUE(called);
     EXPECT_TRUE(is_valid);
+    EXPECT_EQ(NONCE_OK, nonce_error);
   }
 }
 

net/quic/crypto/quic_crypto_server_config.cc

@@ -148,13 +148,40 @@ class VerifyNonceIsValidAndUniqueCallback
   }
 
  protected:
-  virtual void RunImpl(bool nonce_is_valid_and_unique) OVERRIDE {
-    DVLOG(1) << "Using client nonce, unique: " << nonce_is_valid_and_unique;
+  virtual void RunImpl(bool nonce_is_valid_and_unique,
+                       InsertStatus nonce_error) OVERRIDE {
+    DVLOG(1) << "Using client nonce, unique: " << nonce_is_valid_and_unique
+             << " nonce_error: " << nonce_error;
     result_->info.unique = nonce_is_valid_and_unique;
-    // TODO(rtenneti): Implement capturing of error from strike register.
-    // Temporarily treat them as CLIENT_NONCE_INVALID_FAILURE.
     if (!nonce_is_valid_and_unique) {
-      result_->info.reject_reasons.push_back(CLIENT_NONCE_INVALID_FAILURE);
+      HandshakeFailureReason client_nonce_error;
+      switch (nonce_error) {
+        case NONCE_INVALID_FAILURE:
+          client_nonce_error = CLIENT_NONCE_INVALID_FAILURE;
+          break;
+        case NONCE_NOT_UNIQUE_FAILURE:
+          client_nonce_error = CLIENT_NONCE_NOT_UNIQUE_FAILURE;
+          break;
+        case NONCE_INVALID_ORBIT_FAILURE:
+          client_nonce_error = CLIENT_NONCE_INVALID_ORBIT_FAILURE;
+          break;
+        case NONCE_INVALID_TIME_FAILURE:
+          client_nonce_error = CLIENT_NONCE_INVALID_TIME_FAILURE;
+          break;
+        case STRIKE_REGISTER_TIMEOUT:
+          client_nonce_error = CLIENT_NONCE_STRIKE_REGISTER_TIMEOUT;
+          break;
+        case STRIKE_REGISTER_FAILURE:
+          client_nonce_error = CLIENT_NONCE_STRIKE_REGISTER_FAILURE;
+          break;
+        case NONCE_OK:
+        case NONCE_UNKNOWN_FAILURE:
+        default:
+          LOG(WARNING) << "Unexpected nonce error: " << nonce_error;
+          client_nonce_error = CLIENT_NONCE_UNKNOWN_FAILURE;
+          break;
+      }
+      result_->info.reject_reasons.push_back(client_nonce_error);
     }
     done_cb_->Run(result_);
   }
@@ -1440,7 +1467,7 @@ HandshakeFailureReason QuicCryptoServerConfig::ValidateServerNonce(
   COMPILE_ASSERT(4 + sizeof(server_nonce_orbit_) + 20 == sizeof(server_nonce),
                  bad_nonce_buffer_length);
 
-  bool is_unique;
+  InsertStatus nonce_error;
   {
     base::AutoLock auto_lock(server_nonce_strike_register_lock_);
     if (server_nonce_strike_register_.get() == NULL) {
@@ -1450,11 +1477,27 @@ HandshakeFailureReason QuicCryptoServerConfig::ValidateServerNonce(
           server_nonce_strike_register_window_secs_, server_nonce_orbit_,
           StrikeRegister::NO_STARTUP_PERIOD_NEEDED));
     }
-    is_unique = server_nonce_strike_register_->Insert(
+    nonce_error = server_nonce_strike_register_->Insert(
         server_nonce, static_cast<uint32>(now.ToUNIXSeconds()));
   }
 
-  return is_unique ? HANDSHAKE_OK : SERVER_NONCE_NOT_UNIQUE_FAILURE;
+  switch (nonce_error) {
+    case NONCE_OK:
+      return HANDSHAKE_OK;
+    case NONCE_INVALID_FAILURE:
+      return SERVER_NONCE_INVALID_FAILURE;
+    case NONCE_NOT_UNIQUE_FAILURE:
+      return SERVER_NONCE_NOT_UNIQUE_FAILURE;
+    case NONCE_INVALID_TIME_FAILURE:
+      return SERVER_NONCE_INVALID_TIME_FAILURE;
+    case NONCE_UNKNOWN_FAILURE:
+    case NONCE_INVALID_ORBIT_FAILURE:
+    case STRIKE_REGISTER_TIMEOUT:
+    case STRIKE_REGISTER_FAILURE:
+    default:
+      LOG(WARNING) << "Unexpected nonce error: " << nonce_error;
+      return SERVER_NONCE_NOT_UNIQUE_FAILURE;
+  }
 }
 
 QuicCryptoServerConfig::Config::Config()

net/quic/crypto/strike_register.cc

@@ -130,8 +130,8 @@ void StrikeRegister::Reset() {
   internal_node_head_ = kNil;
 }
 
-bool StrikeRegister::Insert(const uint8 nonce[32],
-                            uint32 current_time_external) {
+InsertStatus StrikeRegister::Insert(const uint8 nonce[32],
+                                    uint32 current_time_external) {
   // Make space for the insertion if the strike register is full.
   while (external_node_free_head_ == kNil ||
          internal_node_free_head_ == kNil) {
@@ -142,7 +142,7 @@ bool StrikeRegister::Insert(const uint8 nonce[32],
 
   // Check to see if the orbit is correct.
   if (memcmp(nonce + sizeof(current_time), orbit_, sizeof(orbit_))) {
-    return false;
+    return NONCE_INVALID_ORBIT_FAILURE;
   }
 
   const uint32 nonce_time = ExternalTimeToInternal(TimeFromBytes(nonce));
@@ -151,7 +151,7 @@ bool StrikeRegister::Insert(const uint8 nonce[32],
   pair<uint32, uint32> valid_range =
       StrikeRegister::GetValidRange(current_time);
   if (nonce_time < valid_range.first || nonce_time > valid_range.second) {
-    return false;
+    return NONCE_INVALID_TIME_FAILURE;
   }
 
   // We strip the orbit out of the nonce.
@@ -171,13 +171,13 @@ bool StrikeRegister::Insert(const uint8 nonce[32],
     memcpy(external_node(index), value, sizeof(value));
     internal_node_head_ = (index | kExternalFlag) << 8;
     DCHECK_LE(horizon_, nonce_time);
-    return true;
+    return NONCE_OK;
   }
 
   const uint8* best_match = external_node(best_match_index);
   if (memcmp(best_match, value, sizeof(value)) == 0) {
     // We found the value in the tree.
-    return false;
+    return NONCE_NOT_UNIQUE_FAILURE;
   }
 
   // We are going to insert a new entry into the tree, so get the nodes now.
@@ -263,7 +263,7 @@ bool StrikeRegister::Insert(const uint8 nonce[32],
   *where_index = (*where_index & 0xff) | (internal_node_index << 8);
 
   DCHECK_LE(horizon_, nonce_time);
-  return true;
+  return NONCE_OK;
 }
 
 const uint8* StrikeRegister::orbit() const {

net/quic/crypto/strike_register.h

@@ -15,6 +15,26 @@
 
 namespace net {
 
+// InsertStatus enum values cannot be changed, they need to be stable.
+enum InsertStatus {
+  NONCE_OK = 0,
+  // The default error value for nonce verification failures from strike
+  // register (covers old strike registers and unknown failures).
+  NONCE_UNKNOWN_FAILURE = 1,
+  // Decrypted nonce had incorrect length.
+  NONCE_INVALID_FAILURE = 2,
+  // Nonce is not unique.
+  NONCE_NOT_UNIQUE_FAILURE = 3,
+  // Nonce's orbit is invalid or incorrect.
+  NONCE_INVALID_ORBIT_FAILURE = 4,
+  // Nonce's timestamp is not in the strike register's valid time range.
+  NONCE_INVALID_TIME_FAILURE = 5,
+  // Strike register's RPC call timed out, nonce couldn't be verified.
+  STRIKE_REGISTER_TIMEOUT = 6,
+  // Strike register is down, nonce couldn't be verified.
+  STRIKE_REGISTER_FAILURE = 7,
+};
+
 // A StrikeRegister is critbit tree which stores a set of observed nonces.
 // We use a critbit tree because:
 //   1) It's immune to algorithmic complexity attacks. If we had used a hash
@@ -107,16 +127,17 @@ class NET_EXPORT_PRIVATE StrikeRegister {
   //   b) before the current horizon
   //   c) outside of the valid time window
   //   d) already in the set of observed nonces
-  // and returns false if any of these are true. It is also free to return
-  // false for other reasons as it's always safe to reject an nonce.
+  // and returns the failure reason if any of these are true. It is also free to
+  // return failure reason for other reasons as it's always safe to reject an
+  // nonce.
   //
   // nonces are:
   //   4 bytes of timestamp (UNIX epoch seconds)
   //   8 bytes of orbit value (a cluster id)
   //   20 bytes of random data
   //
-  // Otherwise, it inserts |nonce| into the observed set and returns true.
-  bool Insert(const uint8 nonce[32], uint32 current_time);
+  // Otherwise, it inserts |nonce| into the observed set and returns NONCE_OK.
+  InsertStatus Insert(const uint8 nonce[32], uint32 current_time);
 
   // orbit returns a pointer to the 8-byte orbit value for this
   // strike-register.