Repository: randombit/botan
Pull request #3537 (2023-05-02 10:00:20)

Fixes: RFC-compliance of TLS (EtM extension + Supported Point Formats Extension)
Fix some RFC compliance issues raised in #2754. Follow up for #3520.

This fixes:

- TLS client: abort handshake when a server incorrectly sends EtM extension
- TLS: check that Supported Point Formats Extension contains uncompressed

Note that I don't see the fixes as security relevant. The RFC itself does not even mandate this checks. But I still think it is good that we enforce the other side to follow the RFC.

Some more details in the individual commit messages.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/59521453/badge)](https://coveralls.io/builds/59521453)

Coverage: 91.718% (-0.001%) from 91.719% when pulling **6ca33d8325a98894ee159a17e0d844269b071c9d on pl/tls_compliance** into **1881cd8ed31c4ba6c36cdb4dc93c9e89746ecf3d on master**.


Patch:
diff --git a/src/lib/tls/tls12/tls_client_impl_12.cpp b/src/lib/tls/tls12/tls_client_impl_12.cpp
index a1a21847e..87980fcf0 100644
--- a/src/lib/tls/tls12/tls_client_impl_12.cpp
+++ b/src/lib/tls/tls12/tls_client_impl_12.cpp
@@ -324,13 +324,24 @@ void Client_Impl_12::process_handshake_msg(const Handshake_State* active_state,
                              "Server replied with ciphersuite we didn't send");
          }
 
-      if(const auto suite = Ciphersuite::by_id(state.server_hello()->ciphersuite());
-         !suite || !suite->usable_in_version(state.server_hello()->legacy_version()))
+      const auto suite = Ciphersuite::by_id(state.server_hello()->ciphersuite());
+      if(!suite || !suite->usable_in_version(state.server_hello()->legacy_version()))
          {
          throw TLS_Exception(Alert::HandshakeFailure,
                              "Server replied using a ciphersuite not allowed in version it offered");
          }
 
+      // RFC 7366 3.:
+      //   If a server receives an encrypt-then-MAC request extension from a client
+      //   and then selects a stream or Authenticated Encryption with Associated
+      //   Data (AEAD) ciphersuite, it MUST NOT send an encrypt-then-MAC
+      //   response extension back to the client.
+      if(suite->aead_ciphersuite() && state.server_hello()->supports_encrypt_then_mac())
+         {
+         throw TLS_Exception(Alert::IllegalParameter,
+                             "Server replied using an AEAD ciphersuite and an encrypt-then-MAC response extension");
+         }
+
       if(Ciphersuite::is_scsv(state.server_hello()->ciphersuite()))
          {
          throw TLS_Exception(Alert::HandshakeFailure,
diff --git a/src/lib/tls/tls_ciphersuite.cpp b/src/lib/tls/tls_ciphersuite.cpp
index a2df7416b..8e91bc4e4 100644
--- a/src/lib/tls/tls_ciphersuite.cpp
+++ b/src/lib/tls/tls_ciphersuite.cpp
@@ -91,6 +91,11 @@ bool Ciphersuite::cbc_ciphersuite() const
    return (mac_algo() != "AEAD");
    }
 
+bool Ciphersuite::aead_ciphersuite() const
+   {
+   return (mac_algo() == "AEAD");
+   }
+
 bool Ciphersuite::signature_used() const
    {
    return auth_method() != Auth_Method::IMPLICIT;
diff --git a/src/lib/tls/tls_ciphersuite.h b/src/lib/tls/tls_ciphersuite.h
index 23ac87574..6b4902436 100644
--- a/src/lib/tls/tls_ciphersuite.h
+++ b/src/lib/tls/tls_ciphersuite.h
@@ -77,6 +77,11 @@ class BOTAN_PUBLIC_API(2,0) Ciphersuite final
        */
       bool cbc_ciphersuite() const;
 
+      /**
+       * @return true if this suite uses a AEAD cipher
+       */
+      bool aead_ciphersuite() const;
+
       bool signature_used() const;
 
       /**
diff --git a/src/lib/tls/tls_extensions.cpp b/src/lib/tls/tls_extensions.cpp
index d69e655a0..4151b2c0b 100644
--- a/src/lib/tls/tls_extensions.cpp
+++ b/src/lib/tls/tls_extensions.cpp
@@ -486,6 +486,7 @@ Supported_Point_Formats::Supported_Point_Formats(TLS_Data_Reader& reader,
    if(len + 1 != extension_size)
       throw Decoding_Error("Inconsistent length field in supported point formats list");
 
+   bool includes_uncompressed = false;
    for(size_t i = 0; i != len; ++i)
       {
       uint8_t format = reader.get_byte();
@@ -499,12 +500,29 @@ Supported_Point_Formats::Supported_Point_Formats(TLS_Data_Reader& reader,
       else if(static_cast<ECPointFormat>(format) == ANSIX962_COMPRESSED_PRIME)
          {
          m_prefers_compressed = true;
-         reader.discard_next(len-i-1);
-         return;
+         std::vector<uint8_t> remaining_formats = reader.get_fixed<uint8_t>(len-i-1);
+         includes_uncompressed = std::any_of(std::begin(remaining_formats),
+                                             std::end(remaining_formats), [](uint8_t remaining_format)
+            {
+            return static_cast<ECPointFormat>(remaining_format) == UNCOMPRESSED;
+            });
+         break;
          }
 
       // ignore ANSIX962_COMPRESSED_CHAR2, we don't support these curves
       }
+
+   // RFC 4492 5.1.:
+   //   If the Supported Point Formats Extension is indeed sent, it MUST contain the value 0 (uncompressed)
+   //   as one of the items in the list of point formats.
+   // Note:
+   //   RFC 8422 5.1.2. explicitly requires this check,
+   //   but only if the Supported Groups extension was sent.
+   if(!includes_uncompressed)
+      {
+      throw TLS_Exception(Alert::IllegalParameter,
+                          "Supported Point Formats Extension must contain the uncompressed point format");
+      }
    }
 
 namespace {


Test patch:
diff --git a/src/tests/test_tls.cpp b/src/tests/test_tls.cpp
index 3c2c7edfd..cce730ae5 100644
--- a/src/tests/test_tls.cpp
+++ b/src/tests/test_tls.cpp
@@ -404,10 +404,12 @@ class Test_TLS_Ciphersuites : public Test
 
                if(ciphersuite->cbc_ciphersuite() == false)
                   {
+                  result.test_eq("Expected AEAD ciphersuite", ciphersuite->aead_ciphersuite(), true);
                   result.test_eq("Expected MAC name for AEAD ciphersuites", ciphersuite->mac_algo(), "AEAD");
                   }
                else
                   {
+                  result.test_eq("Did not expect AEAD ciphersuite", ciphersuite->aead_ciphersuite(), false);
                   result.test_eq("MAC algo and PRF algo same for CBC suites", ciphersuite->prf_algo(), ciphersuite->mac_algo());
                   }