QUIC Crypto - return the reasons for reject message. Reject reason

net/quic/crypto/quic_crypto_server_config.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/crypto/quic_crypto_server_config.h"
 
 #include <stdlib.h>
 #include <algorithm>
 
 #include "base/stl_util.h"
@@ skipping 15 matching lines @@
 #include "net/quic/crypto/local_strike_register_client.h"
 #include "net/quic/crypto/p256_key_exchange.h"
 #include "net/quic/crypto/proof_source.h"
 #include "net/quic/crypto/quic_decrypter.h"
 #include "net/quic/crypto/quic_encrypter.h"
 #include "net/quic/crypto/quic_random.h"
 #include "net/quic/crypto/source_address_token.h"
 #include "net/quic/crypto/strike_register.h"
 #include "net/quic/crypto/strike_register_client.h"
 #include "net/quic/quic_clock.h"
+#include "net/quic/quic_flags.h"
 #include "net/quic/quic_protocol.h"
 #include "net/quic/quic_socket_address_coder.h"
 #include "net/quic/quic_utils.h"
 
 using base::StringPiece;
 using crypto::SecureHash;
 using std::map;
 using std::sort;
 using std::string;
 using std::vector;
@@ skipping 28 matching lines @@
   const QuicWallTime now;
 
   // Outputs from EvaluateClientHello.
   bool valid_source_address_token;
   bool client_nonce_well_formed;
   bool unique;
   StringPiece sni;
   StringPiece client_nonce;
   StringPiece server_nonce;
   StringPiece user_agent_id;
+
+  // Errors from EvaluateClientHello.
+  vector<HandshakeFailureReason> reject_reasons;

[wtc, 2014/06/19 00:13:24]

Since reject_reasons is passed to SetVector(), we need to ensure that
sizeof(HandshakeFailureReason) is the same on all platforms.

I recommend declare |reject_reasons| as a vector<QuicTag> (or QuicTagVector?).
QuicTag has a fixed width and it is the type assumed on the client side.

This may require static_cast<QuicTag> when adding a HandshakeFailureReason value
to this vector.

[ramant (doing other things), 2014/06/19 01:57:35]

Done.

[avd, 2014/06/19 02:27:54]

QuicTag has a very specific meaning, and this is not it.  I think you should be
doing static_cast<uint32> if anything.

[ramant (doing other things), 2014/06/19 17:25:48]

Changed it vector<uint32> and added static_cast<uint32>. A compromise.

Done.

 };
 
 struct ValidateClientHelloResultCallback::Result {
   Result(const CryptoHandshakeMessage& in_client_hello,
          IPEndPoint in_client_ip,
          QuicWallTime in_now)
       : client_hello(in_client_hello),
         info(in_client_ip, in_now),
         error_code(QUIC_NO_ERROR) {
   }
@@ skipping 45 matching lines @@
   VerifyNonceIsValidAndUniqueCallback(
       ValidateClientHelloResultCallback::Result* result,
       ValidateClientHelloResultCallback* done_cb)
       : result_(result), done_cb_(done_cb) {
   }
 
  protected:
   virtual void RunImpl(bool nonce_is_valid_and_unique) OVERRIDE {
     DVLOG(1) << "Using client nonce, unique: " << nonce_is_valid_and_unique;
     result_->info.unique = nonce_is_valid_and_unique;
+    // TODO(rtenneti): Implement capturing of error from strike register.
+    // Temporarily treat them as CLIENT_NONCE_UNKNOWN_FAILURE.
+    if (!nonce_is_valid_and_unique) {
+      result_->info.reject_reasons.push_back(CLIENT_NONCE_UNKNOWN_FAILURE);
+    }
     done_cb_->Run(result_);
   }
 
  private:
   ValidateClientHelloResultCallback::Result* result_;
   ValidateClientHelloResultCallback* done_cb_;
 
   DISALLOW_COPY_AND_ASSIGN(VerifyNonceIsValidAndUniqueCallback);
 };
 
@@ skipping 722 matching lines @@
 
   if (client_hello.GetStringPiece(kSNI, &info->sni) &&
       !CryptoUtils::IsValidSNI(info->sni)) {
     helper.ValidationComplete(QUIC_INVALID_CRYPTO_MESSAGE_PARAMETER,
                               "Invalid SNI name");
     return;
   }
 
   client_hello.GetStringPiece(kUAID, &info->user_agent_id);
 
-  StringPiece srct;
-  if (requested_config.get() != NULL &&
-      client_hello.GetStringPiece(kSourceAddressTokenTag, &srct) &&
-      ValidateSourceAddressToken(*requested_config,
-                                 srct,
-                                 info->client_ip,
-                                 info->now)) {
-    info->valid_source_address_token = true;
-  } else {
-    // No server config with the requested ID, or no valid source address token.
+  if (!requested_config.get()) {
+    StringPiece requested_scid;
+    client_hello.GetStringPiece(kSCID, &requested_scid);
+    if (requested_scid.empty()) {
+      info->reject_reasons.push_back(SERVER_CONFIG_INCHOATE_HELLO_FAILURE);
+    } else {
+      info->reject_reasons.push_back(SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE);
+    }

[wtc, 2014/06/19 00:13:24]

Nit: require lines 902-907 as follows:

  if (client_hello.GetStringPiece(kSCID, &requested_scid)) {
    info->reject_reasons.push_back(SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE);
  } else {
    info->reject_reasons.push_back(SERVER_CONFIG_INCHOATE_HELLO_FAILURE);
  }

[ramant (doing other things), 2014/06/19 01:57:34]

Done.

+    // No server config with the requested ID.

[wtc, 2014/06/19 00:13:24]

It is possible to keep going in this case, although the
ValidateSourceAddressToken test, which requirea a non-null requested_config,
must be skipped.

[ramant (doing other things), 2014/06/19 01:57:34]

avd suggested we do an early return. Will submit a separate CL with that change
so that we can discuss in it. Hope you don't mind.

     helper.ValidationComplete(QUIC_NO_ERROR, "");
     return;
   }
 
+  HandshakeFailureReason source_address_token_error;
+  StringPiece srct;
+  DCHECK(requested_config.get());

[wtc, 2014/06/19 00:13:24]

This DCHECK's condition is ensured by the test on line 900 and the return on
line 910. So I would omit this DCHECK. Up to you.

[ramant (doing other things), 2014/06/19 01:57:35]

Done.

+  if (client_hello.GetStringPiece(kSourceAddressTokenTag, &srct)) {
+    source_address_token_error =
+        ValidateSourceAddressToken(*requested_config,
+                                   srct,
+                                   info->client_ip,
+                                   info->now);
+    info->valid_source_address_token =
+        (source_address_token_error == HANDSHAKE_OK);
+  } else {
+    source_address_token_error = SOURCE_ADDRESS_TOKEN_INVALID_FAILURE;
+  }
+
+  bool found_error = false;
+  if (source_address_token_error != HANDSHAKE_OK) {
+    info->reject_reasons.push_back(source_address_token_error);
+    // No valid source address token.
+    if (FLAGS_use_early_return_when_verifying_chlo) {
+      helper.ValidationComplete(QUIC_NO_ERROR, "");
+      return;
+    }
+    found_error = true;
+  }
+
   if (client_hello.GetStringPiece(kNONC, &info->client_nonce) &&
       info->client_nonce.size() == kNonceSize) {
     info->client_nonce_well_formed = true;
   } else {
+    info->reject_reasons.push_back(CLIENT_NONCE_INVALID_FAILURE);
     // Invalid client nonce.
     DVLOG(1) << "Invalid client nonce.";
-    helper.ValidationComplete(QUIC_NO_ERROR, "");
-    return;
+    if (FLAGS_use_early_return_when_verifying_chlo) {
+      helper.ValidationComplete(QUIC_NO_ERROR, "");
+      return;
+    }
+    found_error = true;
   }
 
   if (!replay_protection_) {
     info->unique = true;

[wtc, 2014/06/19 00:13:24]

IMPORTANT: change this line to:

  if (!found_error) {
    info->unique = true;
  }

[ramant (doing other things), 2014/06/19 01:57:34]

Thanks for catching this.

Done.

[avd, 2014/06/19 02:27:53]

In practice it doesn't matter since other failure conditions will be set in the
info object.  I still think it is pretty silly to attempt to continue processing
after some of these failure conditions - I even think there is mutual desire to
restore the early return version once we're done collecting data.

[ramant (doing other things), 2014/06/19 17:25:48]

Acknowledged.

     DVLOG(1) << "No replay protection.";
     helper.ValidationComplete(QUIC_NO_ERROR, "");
     return;
   }
 
   client_hello.GetStringPiece(kServerNonceTag, &info->server_nonce);
   if (!info->server_nonce.empty()) {
     // If the server nonce is present, use it to establish uniqueness.
-    info->unique = ValidateServerNonce(info->server_nonce, info->now);
+    HandshakeFailureReason server_nonce_error =
+        ValidateServerNonce(info->server_nonce, info->now);
+    if (server_nonce_error == HANDSHAKE_OK) {
+      info->unique = true;
+    } else {
+      info->reject_reasons.push_back(server_nonce_error);
+      info->unique = false;
+    }
     DVLOG(1) << "Using server nonce, unique: " << info->unique;
+    if (FLAGS_use_early_return_when_verifying_chlo) {
+      helper.ValidationComplete(QUIC_NO_ERROR, "");
+      return;
+    }
+    found_error = true;

[wtc, 2014/06/19 00:13:24]

IMPORTANT: replace lines 972-976 with:

    helper.ValidationComplete(QUIC_NO_ERROR, "");
    return;

Your new code ends up with the same outcome but is confusing because it sets
|found_error| to true.

[ramant (doing other things), 2014/06/19 01:57:34]

Done.

+  }
+
+  if (found_error) {

[wtc, 2014/06/19 00:13:24]

Please add a comment to explain why you do not keep going at this point. The
reason is not obvious.

[ramant (doing other things), 2014/06/19 01:57:34]

Done.

     helper.ValidationComplete(QUIC_NO_ERROR, "");
     return;
   }
 
   // Use the client nonce to establish uniqueness.
   StrikeRegisterClient* strike_register_client;
   {
     base::AutoLock locked(strike_register_client_lock_);
 
     if (strike_register_client_.get() == NULL) {
@@ skipping 27 matching lines @@
   out->SetStringPiece(kSourceAddressTokenTag,
                       NewSourceAddressToken(
                           config,
                           info.client_ip,
                           rand,
                           info.now));
   if (replay_protection_) {
     out->SetStringPiece(kServerNonceTag, NewServerNonce(rand, info.now));
   }
 
+  if (FLAGS_send_quic_crypto_reject_reason) {
+    // Send client the reject reason for debugging purposes.
+    DCHECK_LT(0u, info.reject_reasons.size());
+    out->SetVector(kRejectReason, info.reject_reasons);
+  }
+
   // The client may have requested a certificate chain.
   const QuicTag* their_proof_demands;
   size_t num_their_proof_demands;
 
   if (proof_source_.get() == NULL ||
       client_hello.GetTaglist(kPDMD, &their_proof_demands,
                               &num_their_proof_demands) !=
           QUIC_NO_ERROR) {
     return;
   }
@@ skipping 297 matching lines @@
   if (ip.GetSockAddrFamily() == AF_INET) {
     ip_address = ConvertIPv4NumberToIPv6Number(ip_address);
   }
   source_address_token.set_ip(IPAddressToPackedString(ip_address));
   source_address_token.set_timestamp(now.ToUNIXSeconds());
 
   return config.source_address_token_boxer->Box(
       rand, source_address_token.SerializeAsString());
 }
 
-bool QuicCryptoServerConfig::ValidateSourceAddressToken(
+HandshakeFailureReason QuicCryptoServerConfig::ValidateSourceAddressToken(
     const Config& config,
     StringPiece token,
     const IPEndPoint& ip,
     QuicWallTime now) const {
   string storage;
   StringPiece plaintext;
   if (!config.source_address_token_boxer->Unbox(token, &storage, &plaintext)) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_DECRYPTION_FAILURE;
   }
 
   SourceAddressToken source_address_token;
   if (!source_address_token.ParseFromArray(plaintext.data(),
                                            plaintext.size())) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_PARSE_FAILURE;
   }
 
   IPAddressNumber ip_address = ip.address();
   if (ip.GetSockAddrFamily() == AF_INET) {
     ip_address = ConvertIPv4NumberToIPv6Number(ip_address);
   }
   if (source_address_token.ip() != IPAddressToPackedString(ip_address)) {
     // It's for a different IP address.
-    return false;
+    return SOURCE_ADDRESS_TOKEN_DIFFERENT_IP_ADDRESS_FAILURE;
   }
 
   const QuicWallTime timestamp(
       QuicWallTime::FromUNIXSeconds(source_address_token.timestamp()));
   const QuicTime::Delta delta(now.AbsoluteDifference(timestamp));
 
   if (now.IsBefore(timestamp) &&
       delta.ToSeconds() > source_address_token_future_secs_) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_CLOCK_SKEW_FAILURE;
   }
 
   if (now.IsAfter(timestamp) &&
       delta.ToSeconds() > source_address_token_lifetime_secs_) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_EXPIRED_FAILURE;
   }
 
-  return true;
+  return HANDSHAKE_OK;
 }
 
 // kServerNoncePlaintextSize is the number of bytes in an unencrypted server
 // nonce.
 static const size_t kServerNoncePlaintextSize =
     4 /* timestamp */ + 20 /* random bytes */;
 
 string QuicCryptoServerConfig::NewServerNonce(QuicRandom* rand,
                                               QuicWallTime now) const {
   const uint32 timestamp = static_cast<uint32>(now.ToUNIXSeconds());
 
   uint8 server_nonce[kServerNoncePlaintextSize];
   COMPILE_ASSERT(sizeof(server_nonce) > sizeof(timestamp), nonce_too_small);
   server_nonce[0] = static_cast<uint8>(timestamp >> 24);
   server_nonce[1] = static_cast<uint8>(timestamp >> 16);
   server_nonce[2] = static_cast<uint8>(timestamp >> 8);
   server_nonce[3] = static_cast<uint8>(timestamp);
   rand->RandBytes(&server_nonce[sizeof(timestamp)],
                   sizeof(server_nonce) - sizeof(timestamp));
 
   return server_nonce_boxer_.Box(
       rand,
       StringPiece(reinterpret_cast<char*>(server_nonce), sizeof(server_nonce)));
 }
 
-bool QuicCryptoServerConfig::ValidateServerNonce(StringPiece token,
-                                                 QuicWallTime now) const {
+HandshakeFailureReason QuicCryptoServerConfig::ValidateServerNonce(
+    StringPiece token,
+    QuicWallTime now) const {
   string storage;
   StringPiece plaintext;
   if (!server_nonce_boxer_.Unbox(token, &storage, &plaintext)) {
-    return false;
+    return SERVER_NONCE_DECRYPTION_FAILURE;
   }
 
   // plaintext contains:
   //   uint32 timestamp
   //   uint8[20] random bytes
 
   if (plaintext.size() != kServerNoncePlaintextSize) {
     // This should never happen because the value decrypted correctly.
     LOG(DFATAL) << "Seemingly valid server nonce had incorrect length.";
-    return false;
+    return SERVER_NONCE_INVALID_FAILURE;
   }
 
   uint8 server_nonce[32];
   memcpy(server_nonce, plaintext.data(), 4);
   memcpy(server_nonce + 4, server_nonce_orbit_, sizeof(server_nonce_orbit_));
   memcpy(server_nonce + 4 + sizeof(server_nonce_orbit_), plaintext.data() + 4,
          20);
   COMPILE_ASSERT(4 + sizeof(server_nonce_orbit_) + 20 == sizeof(server_nonce),
                  bad_nonce_buffer_length);
 
   bool is_unique;
   {
     base::AutoLock auto_lock(server_nonce_strike_register_lock_);
     if (server_nonce_strike_register_.get() == NULL) {
       server_nonce_strike_register_.reset(new StrikeRegister(
           server_nonce_strike_register_max_entries_,
           static_cast<uint32>(now.ToUNIXSeconds()),
           server_nonce_strike_register_window_secs_, server_nonce_orbit_,
           StrikeRegister::NO_STARTUP_PERIOD_NEEDED));
     }
     is_unique = server_nonce_strike_register_->Insert(
         server_nonce, static_cast<uint32>(now.ToUNIXSeconds()));
   }
 
-  return is_unique;
+  return is_unique ? HANDSHAKE_OK : SERVER_NONCE_NOT_UNIQUE_FAILURE;
 }
 
 QuicCryptoServerConfig::Config::Config()
     : channel_id_enabled(false),
       is_primary(false),
       primary_time(QuicWallTime::Zero()),
       priority(0),
       source_address_token_boxer(NULL) {}
 
 QuicCryptoServerConfig::Config::~Config() { STLDeleteElements(&key_exchanges); }
 
 }  // namespace net