Skip to content

Commit

Permalink
Systematically avoid using IP addresses in TLS SNI
Browse files Browse the repository at this point in the history
Previously the tls_client cli util had a hack for this but any
other users who provided an IP address in TLS::Server_Information
would end up sending the raw IP in SNI.
  • Loading branch information
randombit committed May 15, 2024
1 parent 32af4f3 commit 7071e89
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 11 deletions.
13 changes: 5 additions & 8 deletions src/cli/tls_client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -73,6 +74,9 @@ class Callbacks : public Botan::TLS::Callbacks {
if(!status.empty() && status[0].contains(Botan::Certificate_Status_Code::OCSP_RESPONSE_GOOD)) {
output() << "Valid OCSP response for this server\n";
}
} else {
throw Botan::TLS::TLS_Exception(Botan::TLS::Alert::BadCertificate,
"Certificate validation failure: " + result.result_string());
}
}

Expand Down Expand Up @@ -231,13 +235,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");
Expand Down Expand Up @@ -267,7 +264,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);

Expand Down
33 changes: 30 additions & 3 deletions src/lib/tls/msg_client_hello.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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) :
Expand Down Expand Up @@ -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()));
}

Expand Down Expand Up @@ -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({}, {}));
Expand Down Expand Up @@ -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));
}

Expand Down

0 comments on commit 7071e89

Please sign in to comment.