Merge pull request #4059 from randombit/jack/tls-sni-fixes

Fix various SNI related issues

src/cli/tls_client.cpp

@@ -16,6 +16,7 @@
    #include <botan/ocsp.h>
    #include <botan/tls_callbacks.h>
    #include <botan/tls_client.h>
+   #include <botan/tls_exceptn.h>
    #include <botan/tls_policy.h>
    #include <botan/tls_session_manager_memory.h>
    #include <botan/x509path.h>
@@ -63,16 +64,25 @@ class Callbacks : public Botan::TLS::Callbacks {
 
          auto ocsp_timeout = std::chrono::milliseconds(1000);
 
+         const std::string checked_name = flag_set("skip-hostname-check") ? "" : std::string(hostname);
+
          Botan::Path_Validation_Result result = Botan::x509_path_validate(
-            cert_chain, restrictions, trusted_roots, hostname, usage, tls_current_timestamp(), ocsp_timeout, ocsp);
+            cert_chain, restrictions, trusted_roots, checked_name, usage, tls_current_timestamp(), ocsp_timeout, ocsp);
 
-         output() << "Certificate validation status: " << result.result_string() << "\n";
          if(result.successful_validation()) {
+            output() << "Certificate validation status: " << result.result_string() << "\n";
             auto status = result.all_statuses();
 
             if(!status.empty() && status[0].contains(Botan::Certificate_Status_Code::OCSP_RESPONSE_GOOD)) {
                output() << "Valid OCSP response for this server\n";
             }
+         } else {
+            if(flag_set("ignore-cert-error")) {
+               output() << "Certificate validation status: " << result.result_string() << "\n";
+            } else {
+               throw Botan::TLS::TLS_Exception(Botan::TLS::Alert::BadCertificate,
+                                               "Certificate validation failure: " + result.result_string());
+            }
          }
       }
 
@@ -160,6 +170,7 @@ class TLS_Client final : public Command {
             Command(
                "tls_client host --port=443 --print-certs --policy=default "
                "--skip-system-cert-store --trusted-cas= --trusted-pubkey-sha256= "
+               "--skip-hostname-check --ignore-cert-error "
                "--tls-version=default --session-db= --session-db-pass= "
                "--next-protocols= --type=tcp --client-cert= --client-cert-key= "
                "--psk= --psk-identity= --psk-prf=SHA-256 --debug") {
@@ -231,13 +242,6 @@ class TLS_Client final : public Command {
             }
          }
 
-         struct sockaddr_storage addrbuf;
-         std::string hostname;
-         if(!host.empty() && inet_pton(AF_INET, host.c_str(), &addrbuf) != 1 &&
-            inet_pton(AF_INET6, host.c_str(), &addrbuf) != 1) {
-            hostname = host;
-         }
-
          m_sockfd = connect_to_host(host, port, use_tcp);
 
          const auto client_crt_path = get_arg_maybe("client-cert");
@@ -267,7 +271,7 @@ class TLS_Client final : public Command {
                                    creds,
                                    policy,
                                    rng_as_shared(),
-                                   Botan::TLS::Server_Information(hostname, port),
+                                   Botan::TLS::Server_Information(host, port),
                                    version,
                                    protocols_to_offer);
 

src/lib/tls/msg_client_hello.cpp

@@ -18,6 +18,7 @@
 #include <botan/tls_exceptn.h>
 #include <botan/tls_version.h>
 
+#include <botan/internal/parsing.h>
 #include <botan/internal/stl_util.h>
 #include <botan/internal/tls_handshake_hash.h>
 #include <botan/internal/tls_handshake_io.h>
@@ -448,6 +449,28 @@ Client_Hello_12::Client_Hello_12(std::unique_ptr<Client_Hello_Internal> data) :
    }
 }
 
+namespace {
+
+// Avoid sending an IPv4/IPv6 address in SNI as this is prohibitied
+bool hostname_acceptable_for_sni(std::string_view hostname) {
+   if(hostname.empty()) {
+      return false;
+   }
+
+   if(string_to_ipv4(hostname).has_value()) {
+      return false;
+   }
+
+   // IPv6? Anyway ':' is not valid in DNS
+   if(hostname.find(':') != std::string_view::npos) {
+      return false;
+   }
+
+   return true;
+}
+
+}  // namespace
+
 // Note: This delegates to the Client_Hello_12 constructor to take advantage
 //       of the sanity checks there.
 Client_Hello_12::Client_Hello_12(const std::vector<uint8_t>& buf) :
@@ -491,7 +514,7 @@ Client_Hello_12::Client_Hello_12(Handshake_IO& io,
 
    m_data->extensions().add(new Supported_Versions(m_data->legacy_version(), policy));
 
-   if(!client_settings.hostname().empty()) {
+   if(hostname_acceptable_for_sni(client_settings.hostname())) {
       m_data->extensions().add(new Server_Name_Indicator(client_settings.hostname()));
    }
 
@@ -572,7 +595,11 @@ Client_Hello_12::Client_Hello_12(Handshake_IO& io,
 
    m_data->extensions().add(new Renegotiation_Extension(reneg_info));
 
-   m_data->extensions().add(new Server_Name_Indicator(session.session.server_info().hostname()));
+   const std::string hostname = session.session.server_info().hostname();
+
+   if(hostname_acceptable_for_sni(hostname)) {
+      m_data->extensions().add(new Server_Name_Indicator(hostname));
+   }
 
    if(policy.support_cert_status_message()) {
       m_data->extensions().add(new Certificate_Status_Request({}, {}));
@@ -758,7 +785,7 @@ Client_Hello_13::Client_Hello_13(const Policy& policy,
       m_data->m_session_id = Session_ID(make_hello_random(rng, cb, policy));
    }
 
-   if(!hostname.empty()) {
+   if(hostname_acceptable_for_sni(hostname)) {
       m_data->extensions().add(new Server_Name_Indicator(hostname));
    }
 

src/lib/tls/tls_callbacks.cpp

@@ -99,7 +99,7 @@ void TLS::Callbacks::tls_verify_cert_chain(const std::vector<X509_Certificate>&
    Path_Validation_Result result = x509_path_validate(cert_chain,
                                                       restrictions,
                                                       trusted_roots,
-                                                      (usage == Usage_Type::TLS_SERVER_AUTH ? hostname : ""),
+                                                      hostname,
                                                       usage,
                                                       tls_current_timestamp(),
                                                       tls_verify_cert_chain_ocsp_timeout(),

src/lib/x509/x509cert.cpp

@@ -603,16 +603,21 @@ bool X509_Certificate::matches_dns_name(std::string_view name) const {
       return false;
    }
 
-   auto issued_names = subject_info("DNS");
+   if(auto req_ipv4 = string_to_ipv4(name)) {
+      const auto& ipv4_names = subject_alt_name().ipv4_address();
+      return ipv4_names.contains(req_ipv4.value());
+   } else {
+      auto issued_names = subject_info("DNS");
 
-   // Fall back to CN only if no DNS names are set (RFC 6125 sec 6.4.4)
-   if(issued_names.empty()) {
-      issued_names = subject_info("Name");
-   }
+      // Fall back to CN only if no DNS names are set (RFC 6125 sec 6.4.4)
+      if(issued_names.empty()) {
+         issued_names = subject_info("Name");
+      }
 
-   for(const auto& issued_name : issued_names) {
-      if(host_wildcard_match(issued_name, name)) {
-         return true;
+      for(const auto& issued_name : issued_names) {
+         if(host_wildcard_match(issued_name, name)) {
+            return true;
+         }
       }
    }
 

src/lib/x509/x509cert.h

@@ -355,6 +355,9 @@ class BOTAN_PUBLIC_API(2, 0) X509_Certificate : public X509_Object {
       * Check if a certain DNS name matches up with the information in
       * the cert
       * @param name DNS name to match
+      *
+      * Note: this will also accept a dotted quad input, in which case
+      * the SAN for IPv4 addresses will be checked.
       */
       bool matches_dns_name(std::string_view name) const;