From 7d5995f08c30c9276f93726f82318584cb5e08a8 Mon Sep 17 00:00:00 2001 From: Jelle Raaijmakers Date: Sat, 10 Apr 2021 00:32:59 +0200 Subject: [PATCH] LibTLS: Support empty SNI data in ServerHello According to RFC6066, empty extension_data for an SNI extension is absolutely one of the possibilities - so let's support this instead of spamming the debug log. --- Userland/Libraries/LibTLS/ClientHandshake.cpp | 129 +++++++++--------- Userland/Libraries/LibTLS/TLSv12.h | 4 + 2 files changed, 72 insertions(+), 61 deletions(-) diff --git a/Userland/Libraries/LibTLS/ClientHandshake.cpp b/Userland/Libraries/LibTLS/ClientHandshake.cpp index 1e693fff19..999035a0c4 100644 --- a/Userland/Libraries/LibTLS/ClientHandshake.cpp +++ b/Userland/Libraries/LibTLS/ClientHandshake.cpp @@ -117,27 +117,24 @@ ssize_t TLSv12::handle_hello(ReadonlyBytes buffer, WritePacketStage& write_packe // The handshake hash function is _always_ SHA256 m_context.handshake_hash.initialize(Crypto::Hash::HashKind::SHA256); - if (buffer.size() - res < 1) { - dbgln("not enough data for compression spec"); + // Compression method + if (buffer.size() - res < 1) return (i8)Error::NeedMoreData; - } u8 compression = buffer[res++]; - if (compression != 0) { - dbgln("Server told us to compress, we will not!"); + if (compression != 0) return (i8)Error::CompressionNotSupported; + + if (m_context.connection_status != ConnectionStatus::Renegotiating) + m_context.connection_status = ConnectionStatus::Negotiating; + if (m_context.is_server) { + dbgln("unsupported: server mode"); + write_packets = WritePacketStage::ServerHandshake; } - if (res > 0) { - if (m_context.connection_status != ConnectionStatus::Renegotiating) - m_context.connection_status = ConnectionStatus::Negotiating; - if (m_context.is_server) { - dbgln("unsupported: server mode"); - write_packets = WritePacketStage::ServerHandshake; - } - } - - if (res > 2) { - res += 2; + // Presence of extensions is determined by availability of bytes after compression_method + if (buffer.size() - res >= 2) { + auto extensions_bytes_total = AK::convert_between_host_and_network_endian(*(const u16*)buffer.offset_pointer(res += 2)); + dbgln_if(TLS_DEBUG, "Extensions bytes total: {}", extensions_bytes_total); } while ((ssize_t)buffer.size() - res >= 4) { @@ -146,60 +143,70 @@ ssize_t TLSv12::handle_hello(ReadonlyBytes buffer, WritePacketStage& write_packe u16 extension_length = AK::convert_between_host_and_network_endian(*(const u16*)buffer.offset_pointer(res)); res += 2; - dbgln_if(TLS_DEBUG, "extension {} with length {}", (u16)extension_type, extension_length); + dbgln_if(TLS_DEBUG, "Extension {} with length {}", (u16)extension_type, extension_length); - if (extension_length) { - if (buffer.size() - res < extension_length) { - dbgln("not enough data for extension"); - return (i8)Error::NeedMoreData; - } + if (buffer.size() - res < extension_length) + return (i8)Error::NeedMoreData; - dbgln("Encountered extension {} with length {}", (u16)extension_type, extension_length); - // SNI - if (extension_type == HandshakeExtension::ServerName) { - u16 sni_host_length = AK::convert_between_host_and_network_endian(*(const u16*)buffer.offset_pointer(res + 3)); - if (buffer.size() - res - 5 < sni_host_length) { - dbgln("Not enough data for sni {} < {}", (buffer.size() - res - 5), sni_host_length); + if (extension_type == HandshakeExtension::ServerName) { + // RFC6066 section 3: SNI extension_data can be empty in the server hello + if (extension_length > 0) { + // ServerNameList total size + if (buffer.size() - res < 2) return (i8)Error::NeedMoreData; - } + auto sni_name_list_bytes = AK::convert_between_host_and_network_endian(*(const u16*)buffer.offset_pointer(res += 2)); + dbgln_if(TLS_DEBUG, "SNI: expecting ServerNameList of {} bytes", sni_name_list_bytes); - if (sni_host_length) { - m_context.extensions.SNI = String { (const char*)buffer.offset_pointer(res + 5), sni_host_length }; - dbgln("server name indicator: {}", m_context.extensions.SNI); - } - } else if (extension_type == HandshakeExtension::ApplicationLayerProtocolNegotiation && m_context.alpn.size()) { - if (buffer.size() - res > 2) { - auto alpn_length = AK::convert_between_host_and_network_endian(*(const u16*)buffer.offset_pointer(res)); - if (alpn_length && alpn_length <= extension_length - 2) { - const u8* alpn = buffer.offset_pointer(res + 2); - size_t alpn_position = 0; - while (alpn_position < alpn_length) { - u8 alpn_size = alpn[alpn_position++]; - if (alpn_size + alpn_position >= extension_length) - break; - String alpn_str { (const char*)alpn + alpn_position, alpn_length }; - if (alpn_size && m_context.alpn.contains_slow(alpn_str)) { - m_context.negotiated_alpn = alpn_str; - dbgln("negotiated alpn: {}", alpn_str); - break; - } - alpn_position += alpn_length; - if (!m_context.is_server) // server hello must contain one ALPN - break; + // Exactly one ServerName should be present + if (buffer.size() - res < 3) + return (i8)Error::NeedMoreData; + auto sni_name_type = (NameType)(*(const u8*)buffer.offset_pointer(res++)); + auto sni_name_length = AK::convert_between_host_and_network_endian(*(const u16*)buffer.offset_pointer(res += 2)); + + if (sni_name_type != NameType::HostName) + return (i8)Error::NotUnderstood; + + if (sizeof(sni_name_type) + sizeof(sni_name_length) + sni_name_length != sni_name_list_bytes) + return (i8)Error::BrokenPacket; + + // Read out the host_name + if (buffer.size() - res < sni_name_length) + return (i8)Error::NeedMoreData; + m_context.extensions.SNI = String { (const char*)buffer.offset_pointer(res), sni_name_length }; + res += sni_name_length; + dbgln("SNI host_name: {}", m_context.extensions.SNI); + } + } else if (extension_type == HandshakeExtension::ApplicationLayerProtocolNegotiation && m_context.alpn.size()) { + if (buffer.size() - res > 2) { + auto alpn_length = AK::convert_between_host_and_network_endian(*(const u16*)buffer.offset_pointer(res)); + if (alpn_length && alpn_length <= extension_length - 2) { + const u8* alpn = buffer.offset_pointer(res + 2); + size_t alpn_position = 0; + while (alpn_position < alpn_length) { + u8 alpn_size = alpn[alpn_position++]; + if (alpn_size + alpn_position >= extension_length) + break; + String alpn_str { (const char*)alpn + alpn_position, alpn_length }; + if (alpn_size && m_context.alpn.contains_slow(alpn_str)) { + m_context.negotiated_alpn = alpn_str; + dbgln("negotiated alpn: {}", alpn_str); + break; } + alpn_position += alpn_length; + if (!m_context.is_server) // server hello must contain one ALPN + break; } } - } else if (extension_type == HandshakeExtension::SignatureAlgorithms) { - dbgln("supported signatures: "); - print_buffer(buffer.slice(res, extension_length)); - // FIXME: what are we supposed to do here? - } else { - dbgln("Encountered unknown extension {} with length {}", (u16)extension_type, extension_length); } res += extension_length; + } else if (extension_type == HandshakeExtension::SignatureAlgorithms) { + dbgln("supported signatures: "); + print_buffer(buffer.slice(res, extension_length)); + res += extension_length; + // FIXME: what are we supposed to do here? } else { - // Zero-length extensions. dbgln("Encountered unknown extension {} with length {}", (u16)extension_type, extension_length); + res += extension_length; } } @@ -372,9 +379,8 @@ ssize_t TLSv12::handle_payload(ReadonlyBytes vbuffer) if (m_context.is_server) { dbgln("unsupported: server mode"); VERIFY_NOT_REACHED(); - } else { - payload_res = handle_hello(buffer.slice(1, payload_size), write_packets); } + payload_res = handle_hello(buffer.slice(1, payload_size), write_packets); break; case HelloVerifyRequest: dbgln("unsupported: DTLS"); @@ -585,6 +591,7 @@ ssize_t TLSv12::handle_payload(ReadonlyBytes vbuffer) } case Error::NeedMoreData: // Ignore this, as it's not an "error" + dbgln_if(TLS_DEBUG, "More data needed"); break; default: dbgln("Unknown TLS::Error with value {}", payload_res); diff --git a/Userland/Libraries/LibTLS/TLSv12.h b/Userland/Libraries/LibTLS/TLSv12.h index 235d2bb98b..41e832d2e8 100644 --- a/Userland/Libraries/LibTLS/TLSv12.h +++ b/Userland/Libraries/LibTLS/TLSv12.h @@ -175,6 +175,10 @@ enum class HandshakeExtension : u16 { SignatureAlgorithms = 0x0d, }; +enum class NameType : u8 { + HostName = 0x00, +}; + enum class WritePacketStage { Initial = 0, ClientHandshake = 1,