Don't crash if QuicServerConfigProtobuf is invalid.
Return an error indication (false) if no keys are
passed to CryptoSecretBoxer::SetKeys, rather than corrupting
the state of the boxer, which would later result in a crash.
PiperOrigin-RevId: 416130497
diff --git a/quic/core/crypto/crypto_secret_boxer.cc b/quic/core/crypto/crypto_secret_boxer.cc
index 1826212..a4a7a2a 100644
--- a/quic/core/crypto/crypto_secret_boxer.cc
+++ b/quic/core/crypto/crypto_secret_boxer.cc
@@ -44,8 +44,11 @@
// kAEAD is the AEAD used for boxing: AES-256-GCM-SIV.
static const EVP_AEAD* (*const kAEAD)() = EVP_aead_aes_256_gcm_siv;
-void CryptoSecretBoxer::SetKeys(const std::vector<std::string>& keys) {
- QUICHE_DCHECK(!keys.empty());
+bool CryptoSecretBoxer::SetKeys(const std::vector<std::string>& keys) {
+ if (keys.empty()) {
+ QUIC_LOG(DFATAL) << "No keys supplied!";
+ return false;
+ }
const EVP_AEAD* const aead = kAEAD();
std::unique_ptr<State> new_state(new State);
@@ -57,7 +60,7 @@
if (!ctx) {
ERR_clear_error();
QUIC_LOG(DFATAL) << "EVP_AEAD_CTX_init failed";
- return;
+ return false;
}
new_state->ctxs.push_back(std::move(ctx));
@@ -65,6 +68,7 @@
QuicWriterMutexLock l(&lock_);
state_ = std::move(new_state);
+ return true;
}
std::string CryptoSecretBoxer::Box(QuicRandom* rand,
diff --git a/quic/core/crypto/crypto_secret_boxer.h b/quic/core/crypto/crypto_secret_boxer.h
index 5dd5cb2..e2cd037 100644
--- a/quic/core/crypto/crypto_secret_boxer.h
+++ b/quic/core/crypto/crypto_secret_boxer.h
@@ -34,13 +34,15 @@
// SetKeys sets a list of encryption keys. The first key in the list will be
// used by |Box|, but all supplied keys will be tried by |Unbox|, to handle
// key skew across the fleet. This must be called before |Box| or |Unbox|.
- // Keys must be |GetKeySize()| bytes long.
- void SetKeys(const std::vector<std::string>& keys);
+ // Keys must be |GetKeySize()| bytes long. No change is made if any key is
+ // invalid, or if there are no keys supplied.
+ bool SetKeys(const std::vector<std::string>& keys);
// Box encrypts |plaintext| using a random nonce generated from |rand| and
// returns the resulting ciphertext. Since an authenticator and nonce are
// included, the result will be slightly larger than |plaintext|. The first
- // key in the vector supplied to |SetKeys| will be used.
+ // key in the vector supplied to |SetKeys| will be used. |SetKeys| must be
+ // called before calling this method.
std::string Box(QuicRandom* rand, absl::string_view plaintext) const;
// Unbox takes the result of a previous call to |Box| in |ciphertext| and
diff --git a/quic/core/crypto/crypto_secret_boxer_test.cc b/quic/core/crypto/crypto_secret_boxer_test.cc
index 225bfc8..7b07d96 100644
--- a/quic/core/crypto/crypto_secret_boxer_test.cc
+++ b/quic/core/crypto/crypto_secret_boxer_test.cc
@@ -56,9 +56,9 @@
std::string key_12(CryptoSecretBoxer::GetKeySize(), 0x12);
CryptoSecretBoxer boxer_11, boxer_12, boxer;
- boxer_11.SetKeys({key_11});
- boxer_12.SetKeys({key_12});
- boxer.SetKeys({key_12, key_11});
+ EXPECT_TRUE(boxer_11.SetKeys({key_11}));
+ EXPECT_TRUE(boxer_12.SetKeys({key_12}));
+ EXPECT_TRUE(boxer.SetKeys({key_12, key_11}));
// Neither single-key boxer can decode the other's tokens.
EXPECT_FALSE(CanDecode(boxer_11, boxer_12));
@@ -74,7 +74,7 @@
// After we flush key_11 from |boxer|, it can no longer decode tokens from
// |boxer_11|.
- boxer.SetKeys({key_12});
+ EXPECT_TRUE(boxer.SetKeys({key_12}));
EXPECT_FALSE(CanDecode(boxer, boxer_11));
}
diff --git a/quic/core/crypto/quic_crypto_server_config.cc b/quic/core/crypto/quic_crypto_server_config.cc
index d6eebbb..e8ac18e 100644
--- a/quic/core/crypto/quic_crypto_server_config.cc
+++ b/quic/core/crypto/quic_crypto_server_config.cc
@@ -512,6 +512,7 @@
void QuicCryptoServerConfig::SetSourceAddressTokenKeys(
const std::vector<std::string>& keys) {
+ // TODO(b/208866709)
source_address_token_boxer_.SetKeys(keys);
}
@@ -1576,9 +1577,14 @@
std::unique_ptr<CryptoHandshakeMessage> msg =
CryptoFramer::ParseMessage(protobuf.config());
+ if (!msg) {
+ QUIC_LOG(WARNING) << "Failed to parse server config message";
+ return nullptr;
+ }
+
if (msg->tag() != kSCFG) {
QUIC_LOG(WARNING) << "Server config message has tag " << msg->tag()
- << " expected " << kSCFG;
+ << ", but expected " << kSCFG;
return nullptr;
}
