Fix FFDHE selection in TLS to conform to RFC 7919 (#3743)

randombit · Oct 10, 2023 · e27bf3f · e27bf3f
2 parents 6f466a2 + a0b1eea
commit e27bf3f
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 5 deletions.
diff --git a/src/lib/tls/tls12/msg_server_kex.cpp b/src/lib/tls/tls12/msg_server_kex.cpp
@@ -50,9 +50,12 @@ Server_Key_Exchange::Server_Key_Exchange(Handshake_IO& io,
       m_shared_group = Group_Params::NONE;
 
       /*
-      If the client does not send any DH groups that we recognize in
-      the supported groups extension, but does offer DH ciphersuites,
-      we select a group arbitrarily
+      RFC 7919 requires that if the client sends any groups in the FFDHE
+      range, that we must select one of these. If this is not possible,
+      then we are required to reject the connection.
+
+      If the client did not send any DH groups, but did offer DH ciphersuites
+      and we selected one, then consult the policy for which DH group to pick.
       */
 
       if(dh_groups.empty()) {
@@ -65,7 +68,8 @@ Server_Key_Exchange::Server_Key_Exchange(Handshake_IO& io,
          throw TLS_Exception(Alert::HandshakeFailure, "Could not agree on a DH group with the client");
       }
 
-      BOTAN_ASSERT(m_shared_group.value().is_dh_named_group(), "DH ciphersuite is using a finite field group");
+      // The policy had better return a group we know about:
+      BOTAN_ASSERT(m_shared_group.value().is_dh_named_group(), "DH ciphersuite is using a known finite field group");
 
       // Note: TLS 1.2 allows defining and using arbitrary DH groups (additional
       //       to the named and standardized ones). This API doesn't allow the

diff --git a/src/lib/tls/tls_algos.h b/src/lib/tls/tls_algos.h
@@ -154,6 +154,11 @@ class BOTAN_PUBLIC_API(3, 2) Group_Params final {
                 m_code == Group_Params_Code::BRAINPOOL384R1 || m_code == Group_Params_Code::BRAINPOOL512R1;
       }
 
+      constexpr bool is_in_ffdhe_range() const {
+         // See RFC 7919
+         return wire_code() >= 256 && wire_code() < 512;
+      }
+
       constexpr bool is_dh_named_group() const {
          return m_code == Group_Params_Code::FFDHE_2048 || m_code == Group_Params_Code::FFDHE_3072 ||
                 m_code == Group_Params_Code::FFDHE_4096 || m_code == Group_Params_Code::FFDHE_6144 ||

diff --git a/src/lib/tls/tls_extensions.cpp b/src/lib/tls/tls_extensions.cpp
@@ -375,7 +375,7 @@ std::vector<Group_Params> Supported_Groups::ec_groups() const {
 std::vector<Group_Params> Supported_Groups::dh_groups() const {
    std::vector<Group_Params> dh;
    for(auto g : m_groups) {
-      if(g.is_dh_named_group()) {
+      if(g.is_in_ffdhe_range()) {
          dh.push_back(g);
       }
    }

diff --git a/src/lib/tls/tls_extensions.h b/src/lib/tls/tls_extensions.h
@@ -243,7 +243,11 @@ class BOTAN_UNSTABLE_API Supported_Groups final : public Extension {
       Extension_Code type() const override { return static_type(); }
 
       const std::vector<Group_Params>& groups() const;
+
+      // Returns the list of groups we recognize as ECDH curves
       std::vector<Group_Params> ec_groups() const;
+
+      // Returns the list of any groups in the FFDHE range
       std::vector<Group_Params> dh_groups() const;
 
       std::vector<uint8_t> serialize(Connection_Side whoami) const override;

diff --git a/src/lib/tls/tls_messages.h b/src/lib/tls/tls_messages.h
@@ -109,6 +109,7 @@ class BOTAN_UNSTABLE_API Client_Hello : public Handshake_Message {
 
       std::vector<Group_Params> supported_ecc_curves() const;
 
+      // This returns any groups in the FFDHE range
       std::vector<Group_Params> supported_dh_groups() const;
 
       std::vector<Protocol_Version> supported_versions() const;