Merge GH #630 TLS server checks client signature_algorithms

Only partially resolves GH #619 see both issues for discussion.

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);