TLS Server should respect client signature_algorithms. Stricter TLS hello decoding.

If the client sent a signature_algorithms extension, we should negotiate a ciphersuite in
the shared union of the ciphersuite list and the extension, instead of ignoring it.
Found by Juraj Somorovsky GH #619

The TLS v1.2 spec says that clients should only send the signature_algorithms
extension in a hello for that version. Enforce that when decoding client hellos
to prevent this extension from confusing a v1.0 negotiation.

TLS v1.2 spec says ANON signature type is prohibited in the signature_algorithms extension
in the client hello. Prohibit it.

Reorder the TLS extensions in the client hello so there is no chance an empty extension is
the last extension in the list. Some implementations apparently reject such hellos, even
(perhaps especially) when they do not recognize the extension, this bug was mentioned on
the ietf-tls mailing list a while back.

5 files changed, 89 insertions, 31 deletions

diff --git a/src/lib/tls/msg_client_hello.cpp b/src/lib/tls/msg_client_hello.cpp
index 69f9a5e11..56e226a40 100644
--- a/src/lib/tls/msg_client_hello.cpp
+++ b/src/lib/tls/msg_client_hello.cpp
@@ -80,25 +80,27 @@ Client_Hello::Client_Hello(Handshake_IO& io,
                                     client_settings.srp_identifier() != "")),
    m_comp_methods(policy.compression())
    {
+   BOTAN_ASSERT(policy.acceptable_protocol_version(client_settings.protocol_version()),
+                "Our policy accepts the version we are offering");
+
+   /*
+   * Place all empty extensions in front to avoid a bug in some sytems
+   * which reject hellos when the last extension in the list is empty.
+   */
    m_extensions.add(new Extended_Master_Secret);
+   m_extensions.add(new Session_Ticket());
+   if(policy.negotiate_encrypt_then_mac())
+      m_extensions.add(new Encrypt_then_MAC);
+
    m_extensions.add(new Renegotiation_Extension(reneg_info));
    m_extensions.add(new Server_Name_Indicator(client_settings.hostname()));
-   m_extensions.add(new Session_Ticket());
-   m_extensions.add(new Supported_Elliptic_Curves(policy.allowed_ecc_curves()));
 
-   if(m_version.supports_negotiable_signature_algorithms())
-      m_extensions.add(new Signature_Algorithms(policy.allowed_signature_hashes(),
-                                                policy.allowed_signature_methods()));
+   if(reneg_info.empty() && !next_protocols.empty())
+      m_extensions.add(new Application_Layer_Protocol_Notification(next_protocols));
 
    if(m_version.is_datagram_protocol())
      m_extensions.add(new SRTP_Protection_Profiles(policy.srtp_profiles()));
 
-   if(reneg_info.empty() && !next_protocols.empty())
-      m_extensions.add(new Application_Layer_Protocol_Notification(next_protocols));
-   
-   if(policy.negotiate_encrypt_then_mac())
-      m_extensions.add(new Encrypt_then_MAC);
-
 #if defined(BOTAN_HAS_SRP6)
    m_extensions.add(new SRP_Identifier(client_settings.srp_identifier()));
 #else
@@ -108,8 +110,11 @@ Client_Hello::Client_Hello(Handshake_IO& io,
       }
 #endif
 
-   BOTAN_ASSERT(policy.acceptable_protocol_version(client_settings.protocol_version()),
-                "Our policy accepts the version we are offering");
+   m_extensions.add(new Supported_Elliptic_Curves(policy.allowed_ecc_curves()));
+
+   if(m_version.supports_negotiable_signature_algorithms())
+      m_extensions.add(new Signature_Algorithms(policy.allowed_signature_hashes(),
+                                                policy.allowed_signature_methods()));
 
    if(policy.send_fallback_scsv(client_settings.protocol_version()))
       m_suites.push_back(TLS_FALLBACK_SCSV);
@@ -256,6 +261,15 @@ Client_Hello::Client_Hello(const std::vector<byte>& buf)
          m_extensions.add(new Renegotiation_Extension());
          }
       }
+
+   // Parsing complete, now any additional decoding checks
+
+   if(m_version.supports_negotiable_signature_algorithms() == false)
+      {
+      if(m_extensions.has<Signature_Algorithms>())
+         throw TLS_Exception(Alert::HANDSHAKE_FAILURE,
+                             "Client sent signature_algorithms extension in version that doesn't support it");
+      }
    }
 
 bool Client_Hello::sent_fallback_scsv() const
diff --git a/src/lib/tls/tls_extensions.cpp b/src/lib/tls/tls_extensions.cpp
index 3dceb505a..e38e4ccdc 100644
--- a/src/lib/tls/tls_extensions.cpp
+++ b/src/lib/tls/tls_extensions.cpp
@@ -463,16 +463,27 @@ Signature_Algorithms::Signature_Algorithms(TLS_Data_Reader& reader,
 
    while(len)
       {
-      const std::string hash_code = hash_algo_name(reader.get_byte());
-      const std::string sig_code = sig_algo_name(reader.get_byte());
-
+      const byte hash_code = reader.get_byte();
+      const byte sig_code = reader.get_byte();
       len -= 2;
 
+      if(sig_code == 0)
+         {
+         /*
+         RFC 5247 7.4.1.4.1 explicitly prohibits anonymous (0) signature code in
+         the client hello. ("It MUST NOT appear in this extension.")
+         */
+         throw TLS_Exception(Alert::DECODE_ERROR, "Client sent ANON signature");
+         }
+
+      const std::string hash_name = hash_algo_name(hash_code);
+      const std::string sig_name = sig_algo_name(sig_code);
+
       // If not something we know, ignore it completely
-      if(hash_code.empty() || sig_code.empty())
+      if(hash_name.empty() || sig_name.empty())
          continue;
 
-      m_supported_algos.push_back(std::make_pair(hash_code, sig_code));
+      m_supported_algos.push_back(std::make_pair(hash_name, sig_name));
       }
    }
 
diff --git a/src/lib/tls/tls_extensions.h b/src/lib/tls/tls_extensions.h
index dc69eec36..4bd564a85 100644
--- a/src/lib/tls/tls_extensions.h
+++ b/src/lib/tls/tls_extensions.h
@@ -274,8 +274,9 @@ class Signature_Algorithms final : public Extension
       static std::string sig_algo_name(byte code);
       static byte sig_algo_code(const std::string& name);
 
-      std::vector<std::pair<std::string, std::string> >
-         supported_signature_algorthms() const
+      // [(hash,sig),(hash,sig),...]
+      const std::vector<std::pair<std::string, std::string>>&
+      supported_signature_algorthms() const
          {
          return m_supported_algos;
          }
@@ -287,13 +288,13 @@ class Signature_Algorithms final : public Extension
       Signature_Algorithms(const std::vector<std::string>& hashes,
                            const std::vector<std::string>& sig_algos);
 
-      explicit Signature_Algorithms(const std::vector<std::pair<std::string, std::string> >& algos) :
+      explicit Signature_Algorithms(const std::vector<std::pair<std::string, std::string>>& algos) :
          m_supported_algos(algos) {}
 
       Signature_Algorithms(TLS_Data_Reader& reader,
                            u16bit extension_size);
    private:
-      std::vector<std::pair<std::string, std::string> > m_supported_algos;
+      std::vector<std::pair<std::string, std::string>> m_supported_algos;
    };
 
 /**
diff --git a/src/lib/tls/tls_messages.h b/src/lib/tls/tls_messages.h
index 8ccb2fbff..cf35053f2 100644
--- a/src/lib/tls/tls_messages.h
+++ b/src/lib/tls/tls_messages.h
@@ -19,6 +19,7 @@
 #include <botan/x509cert.h>
 #include <vector>
 #include <string>
+#include <set>
 
 namespace Botan {
 
@@ -105,6 +106,14 @@ class Client_Hello final : public Handshake_Message
          return std::vector<std::pair<std::string, std::string>>();
          }
 
+      std::set<std::string> supported_sig_algos() const
+         {
+         std::set<std::string> sig;
+         for(auto&& hash_and_sig : supported_algos())
+            sig.insert(hash_and_sig.second);
+         return sig;
+         }
+
       std::vector<std::string> supported_ecc_curves() const
          {
          if(Supported_Elliptic_Curves* ecc = m_extensions.get<Supported_Elliptic_Curves>())
@@ -167,6 +176,11 @@ class Client_Hello final : public Handshake_Message
          return m_extensions.has<Encrypt_then_MAC>();
          }
 
+      bool sent_signature_algorithms() const
+         {
+         return m_extensions.has<Signature_Algorithms>();
+         }
+
       std::vector<std::string> next_protocols() const
          {
          if(auto alpn = m_extensions.get<Application_Layer_Protocol_Notification>())
diff --git a/src/lib/tls/tls_server.cpp b/src/lib/tls/tls_server.cpp
index 40aa18d27..1676ef659 100644
--- a/src/lib/tls/tls_server.cpp
+++ b/src/lib/tls/tls_server.cpp
@@ -154,11 +154,11 @@ u16bit choose_ciphersuite(
    Protocol_Version version,
    Credentials_Manager& creds,
    const std::map<std::string, std::vector<X509_Certificate> >& cert_chains,
-   const Client_Hello* client_hello)
+   const Client_Hello& client_hello)
    {
    const bool our_choice = policy.server_uses_own_ciphersuite_preferences();
-   const bool have_srp = creds.attempt_srp("tls-server", client_hello->sni_hostname());
-   const std::vector<u16bit> client_suites = client_hello->ciphersuites();
+   const bool have_srp = creds.attempt_srp("tls-server", client_hello.sni_hostname());
+   const std::vector<u16bit> client_suites = client_hello.ciphersuites();
    const std::vector<u16bit> server_suites = policy.ciphersuite_list(version, have_srp);
 
    if(server_suites.empty())
@@ -166,7 +166,11 @@ u16bit choose_ciphersuite(
                           "Policy forbids us from negotiating any ciphersuite");
 
    const bool have_shared_ecc_curve =
-      (policy.choose_curve(client_hello->supported_ecc_curves()) != "");
+      (policy.choose_curve(client_hello.supported_ecc_curves()) != "");
+
+   /*
+   Walk down one list in preference order
+   */
 
    std::vector<u16bit> pref_list = server_suites;
    std::vector<u16bit> other_list = client_suites;
@@ -174,19 +178,33 @@ u16bit choose_ciphersuite(
    if(!our_choice)
       std::swap(pref_list, other_list);
 
+   const std::set<std::string> client_sig_algos = client_hello.supported_sig_algos();
+
    for(auto suite_id : pref_list)
       {
       if(!value_exists(other_list, suite_id))
          continue;
 
-      Ciphersuite suite = Ciphersuite::by_id(suite_id);
+      const Ciphersuite suite = Ciphersuite::by_id(suite_id);
 
-      if(!have_shared_ecc_curve && suite.ecc_ciphersuite())
+      if(suite.valid() == false)
          continue;
 
-      if(suite.sig_algo() != "" && cert_chains.count(suite.sig_algo()) == 0)
+      if(suite.ecc_ciphersuite() && have_shared_ecc_curve == false)
          continue;
 
+      // For non-anon ciphersuites
+      if(suite.sig_algo() != "")
+         {
+         // Do we have any certificates for this sig?
+         if(cert_chains.count(suite.sig_algo()) == 0)
+            continue;
+
+         // Client reques
+         if(!client_sig_algos.empty() && client_sig_algos.count(suite.sig_algo()) == 0)
+            continue;
+         }
+
 #if defined(BOTAN_HAS_SRP6)
       /*
       The client may offer SRP cipher suites in the hello message but
@@ -196,7 +214,7 @@ u16bit choose_ciphersuite(
       client hello message.
        - RFC 5054 section 2.5.1.2
       */
-      if(suite.kex_algo() == "SRP_SHA" && client_hello->srp_identifier() == "")
+      if(suite.kex_algo() == "SRP_SHA" && client_hello.srp_identifier() == "")
          throw TLS_Exception(Alert::UNKNOWN_PSK_IDENTITY,
                              "Client wanted SRP but did not send username");
 #endif
@@ -747,7 +765,7 @@ void Server::session_create(Server_Handshake_State& pending_state,
                                               pending_state.version(),
                                               m_creds,
                                               cert_chains,
-                                              pending_state.client_hello()),
+                                              *pending_state.client_hello()),
                            choose_compression(policy(),
                                               pending_state.client_hello()->compression_methods()),
                            have_session_ticket_key);