Merge pull request #4384 from Rohde-Schwarz/refactor/x25519_x448_zero…

…_rejection

Refactor: Centralize X25519/X448 all-zero result rejection

src/lib/pubkey/curve448/x448/x448.cpp

@@ -116,6 +116,20 @@ class X448_KA_Operation final : public PK_Ops::Key_Agreement_with_KDF {
  auto shared_secret = encode_point(x448(k, u));
  CT::unpoison(shared_secret);
 
+ // RFC 7748 Section 6.2
+ // As with X25519, both sides MAY check, without leaking extra
+ // information about the value of K, whether the resulting shared K
+ // is the all-zero value and abort if so.
+ //
+ // TODO: once the generic Key Agreement operation creation is equipped
+ // with a more flexible parameterization, this check could be
+ // made optional.
+ // For instance: `sk->agree().with_optional_sanity_checks(true)`.
+ // See also: https://github.com/randombit/botan/pull/4318
+ if(CT::all_zeros(shared_secret.data(), shared_secret.size()).as_bool()) {
+ throw Invalid_Argument("X448 public point appears to be of low order");
+ }
+
  return shared_secret;
  }
 

src/lib/pubkey/x25519/x25519.cpp

@@ -10,6 +10,7 @@
 #include <botan/ber_dec.h>
 #include <botan/der_enc.h>
 #include <botan/rng.h>
+#include <botan/internal/ct_utils.h>
 #include <botan/internal/fmt.h>
 #include <botan/internal/pk_ops_impl.h>
 
@@ -120,7 +121,24 @@ class X25519_KA_Operation final : public PK_Ops::Key_Agreement_with_KDF {
 
  size_t agreed_value_size() const override { return 32; }
 
- secure_vector<uint8_t> raw_agree(const uint8_t w[], size_t w_len) override { return m_key.agree(w, w_len); }
+ secure_vector<uint8_t> raw_agree(const uint8_t w[], size_t w_len) override {
+ auto shared_key = m_key.agree(w, w_len);
+
+ // RFC 7748 Section 6.1
+ // Both [parties] MAY check, without leaking extra information about
+ // the value of K, whether K is the all-zero value and abort if so.
+ //
+ // TODO: once the generic Key Agreement operation creation is equipped
+ // with a more flexible parameterization, this check could be
+ // made optional.
+ // For instance: `sk->agree().with_optional_sanity_checks(true)`.
+ // See also: https://github.com/randombit/botan/pull/4318
+ if(CT::all_zeros(shared_key.data(), shared_key.size()).as_bool()) {
+ throw Invalid_Argument("X25519 public point appears to be of low order");
+ }
+
+ return shared_key;
+ }
 
  private:
  const X25519_PrivateKey& m_key;

src/lib/tls/tls12/msg_client_kex.cpp

@@ -118,15 +118,6 @@ Client_Key_Exchange::Client_Key_Exchange(Handshake_IO& io,
  auto shared_secret =
  state.callbacks().tls_ephemeral_key_agreement(curve_id, *private_key, peer_public_value, rng, policy);
 
- // RFC 8422 - 5.11.
- // With X25519 and X448, a receiving party MUST check whether the
- // computed premaster secret is the all-zero value and abort the
- // handshake if so, as described in Section 6 of [RFC7748].
- if((curve_id == Group_Params::X25519 || curve_id == Group_Params::X448) &&
- CT::all_zeros(shared_secret.data(), shared_secret.size()).as_bool()) {
- throw TLS_Exception(Alert::DecryptError, "Bad X25519 or X448 key exchange");
- }
-
  if(kex_algo == Kex_Algo::ECDH) {
  m_pre_master = std::move(shared_secret);
  } else {
@@ -275,19 +266,6 @@ Client_Key_Exchange::Client_Key_Exchange(const std::vector<uint8_t>& contents,
  shared_secret = CT::strip_leading_zeros(shared_secret);
  }
 
- if(kex_algo == Kex_Algo::ECDH || kex_algo == Kex_Algo::ECDHE_PSK) {
- // RFC 8422 - 5.11.
- // With X25519 and X448, a receiving party MUST check whether the
- // computed premaster secret is the all-zero value and abort the
- // handshake if so, as described in Section 6 of [RFC7748].
- BOTAN_ASSERT_NOMSG(state.server_kex()->params().size() >= 3);
- Group_Params group = static_cast<Group_Params>(state.server_kex()->params().at(2));
- if((group == Group_Params::X25519 || group == Group_Params::X448) &&
- CT::all_zeros(shared_secret.data(), shared_secret.size()).as_bool()) {
- throw TLS_Exception(Alert::DecryptError, "Bad X25519 or X448 key exchange");
- }
- }
-
  if(kex_algo == Kex_Algo::ECDHE_PSK) {
  append_tls_length_value(m_pre_master, shared_secret, 2);
  append_tls_length_value(m_pre_master, psk.bits_of(), 2);

src/lib/tls/tls13/tls_extensions_key_share.cpp

@@ -119,22 +119,10 @@ class Key_Share_Entry {
  const Policy& policy,
  Callbacks& cb,
  RandomNumberGenerator& rng) {
+ auto scope = scoped_cleanup([&] { m_private_key.reset(); });
  BOTAN_ASSERT_NOMSG(m_group == received.m_group);
  BOTAN_STATE_CHECK(m_private_key != nullptr);
-
- auto shared_secret = cb.tls_kem_decapsulate(m_group, *m_private_key, received.m_key_exchange, rng, policy);
- m_private_key.reset();
-
- // RFC 8422 - 5.11.
- // With X25519 and X448, a receiving party MUST check whether the
- // computed premaster secret is the all-zero value and abort the
- // handshake if so, as described in Section 6 of [RFC7748].
- if((m_group == Named_Group::X25519 || m_group == Named_Group::X448) &&
- CT::all_zeros(shared_secret.data(), shared_secret.size()).as_bool()) {
- throw TLS_Exception(Alert::DecryptError, "Bad X25519 or X448 key exchange");
- }
-
- return shared_secret;
+ return cb.tls_kem_decapsulate(m_group, *m_private_key, received.m_key_exchange, rng, policy);
  }
 
  private:

src/lib/tls/tls_callbacks.cpp

@@ -345,6 +345,13 @@ secure_vector<uint8_t> TLS::Callbacks::tls_ephemeral_key_agreement(
  X25519_PublicKey peer_key(public_value);
  policy.check_peer_key_acceptable(peer_key);
 
+ // RFC 8422 - 5.11.
+ // With X25519 and X448, a receiving party MUST check whether the
+ // computed premaster secret is the all-zero value and abort the
+ // handshake if so, as described in Section 6 of [RFC7748].
+ //
+ // This is done within the key agreement operation and throws
+ // an Invalid_Argument exception if the shared secret is all-zero.
  return agree(private_key, peer_key);
  }
 #endif
@@ -358,6 +365,13 @@ secure_vector<uint8_t> TLS::Callbacks::tls_ephemeral_key_agreement(
  X448_PublicKey peer_key(public_value);
  policy.check_peer_key_acceptable(peer_key);
 
+ // RFC 8422 - 5.11.
+ // With X25519 and X448, a receiving party MUST check whether the
+ // computed premaster secret is the all-zero value and abort the
+ // handshake if so, as described in Section 6 of [RFC7748].
+ //
+ // This is done within the key agreement operation and throws
+ // an Invalid_Argument exception if the shared secret is all-zero.
  return agree(private_key, peer_key);
  }
 #endif

src/tests/test_pubkey.cpp

@@ -510,11 +510,15 @@ Test::Result PK_Key_Agreement_Test::run_one_test(const std::string& header, cons
  try {
  kas = std::make_unique<Botan::PK_Key_Agreement>(*privkey, this->rng(), kdf, provider);
 
- auto derived_key = kas->derive_key(key_len, pubkey).bits_of();
- result.test_eq(provider, "agreement", derived_key, shared);
+ if(agreement_should_fail(header, vars)) {
+ result.test_throws("key agreement fails", [&] { kas->derive_key(key_len, pubkey); });
+ } else {
+ auto derived_key = kas->derive_key(key_len, pubkey).bits_of();
+ result.test_eq(provider, "agreement", derived_key, shared);
 
- if(key_len == 0 && kdf == "Raw") {
- result.test_eq("Expected size", derived_key.size(), kas->agreed_value_size());
+ if(key_len == 0 && kdf == "Raw") {
+ result.test_eq("Expected size", derived_key.size(), kas->agreed_value_size());
+ }
  }
  } catch(Botan::Lookup_Error&) {
  //result.test_note("Skipping key agreement with with " + provider);

src/tests/test_pubkey.h

@@ -156,6 +156,11 @@ class PK_Key_Agreement_Test : public PK_Test {
  const std::string& optional_keys = "") :
  PK_Test(algo, test_src, required_keys, optional_keys) {}
 
+ virtual bool agreement_should_fail(const std::string& header, const VarMap& vars) const {
+ BOTAN_UNUSED(header, vars);
+ return false;
+ }
+
  virtual std::unique_ptr<Botan::Private_Key> load_our_key(const std::string& header, const VarMap& vars) = 0;
 
  virtual std::vector<uint8_t> load_their_key(const std::string& header, const VarMap& vars) = 0;

src/tests/test_x25519.cpp

@@ -22,6 +22,16 @@ class X25519_Agreement_Tests final : public PK_Key_Agreement_Test {
  public:
  X25519_Agreement_Tests() : PK_Key_Agreement_Test("X25519", "pubkey/x25519.vec", "Secret,CounterKey,K") {}
 
+ bool agreement_should_fail(const std::string& /*unused*/, const VarMap& vars) const override {
+ for(const auto byte : vars.get_req_bin("K")) {
+ if(byte != 0) {
+ return false;
+ }
+ }
+
+ return true;
+ }
+
  std::string default_kdf(const VarMap& /*unused*/) const override { return "Raw"; }
 
  std::unique_ptr<Botan::Private_Key> load_our_key(const std::string& /*header*/, const VarMap& vars) override {

src/tests/test_x448.cpp

@@ -34,6 +34,16 @@ class X448_Agreement_Tests final : public PK_Key_Agreement_Test {
  public:
  X448_Agreement_Tests() : PK_Key_Agreement_Test("X448", "pubkey/x448.vec", "Secret,CounterKey,K") {}
 
+ bool agreement_should_fail(const std::string& /*unused*/, const VarMap& vars) const override {
+ for(const auto byte : vars.get_req_bin("K")) {
+ if(byte != 0) {
+ return false;
+ }
+ }
+
+ return true;
+ }
+
  std::string default_kdf(const VarMap& /*unused*/) const override { return "Raw"; }
 
  std::unique_ptr<Botan::Private_Key> load_our_key(const std::string& /*header*/, const VarMap& vars) override {