LibTLS: Simplify the way `verify_chain` is called

The `build_rsa_pre_master_secret` function originally called
`verify_chain_and_get_matching_certificate`, which verified the chain
and returned a certificate matching the specified hostname.

Since the first certificate in the chain should always be the one
matching with the hostname, we can simply use that one instead. This
means we can completely remove this method and just use `verify_chain`.

To make sure the hostname is still verified, `verify_chain` now also
checks that the first certificate in the chain matches the specified
hostname. If the hostname is empty, we currently fail the verification,
however this basically never happen, as the server name indication
extension is always used.

3 files changed, 47 insertions, 58 deletions

diff --git a/Userland/Libraries/LibTLS/HandshakeClient.cpp b/Userland/Libraries/LibTLS/HandshakeClient.cpp
index 52ec095677..cc4e0271b0 100644
--- a/Userland/Libraries/LibTLS/HandshakeClient.cpp
+++ b/Userland/Libraries/LibTLS/HandshakeClient.cpp
@@ -152,38 +152,6 @@ bool TLSv12::compute_master_secret_from_pre_master_secret(size_t length)
     return true;
 }
 
-static bool wildcard_matches(StringView host, StringView subject)
-{
-    if (host.matches(subject))
-        return true;
-
-    if (subject.starts_with("*."))
-        return wildcard_matches(host, subject.substring_view(2));
-
-    return false;
-}
-
-Optional<size_t> TLSv12::verify_chain_and_get_matching_certificate(StringView host) const
-{
-    if (m_context.certificates.is_empty() || !m_context.verify_chain())
-        return {};
-
-    if (host.is_empty())
-        return 0;
-
-    for (size_t i = 0; i < m_context.certificates.size(); ++i) {
-        auto& cert = m_context.certificates[i];
-        if (wildcard_matches(host, cert.subject.subject))
-            return i;
-        for (auto& san : cert.SAN) {
-            if (wildcard_matches(host, san))
-                return i;
-        }
-    }
-
-    return {};
-}
-
 void TLSv12::build_rsa_pre_master_secret(PacketBuilder& builder)
 {
     u8 random_bytes[48];
@@ -211,14 +179,7 @@ void TLSv12::build_rsa_pre_master_secret(PacketBuilder& builder)
     }
     m_context.premaster_key = premaster_key_result.release_value();
 
-    auto const& certificate_option = verify_chain_and_get_matching_certificate(m_context.extensions.SNI); // if the SNI is empty, we'll make a special case and match *a* leaf certificate.
-    if (!certificate_option.has_value()) {
-        dbgln("certificate verification failed :(");
-        alert(AlertLevel::Critical, AlertDescription::BadCertificate);
-        return;
-    }
-
-    auto& certificate = m_context.certificates[certificate_option.value()];
+    auto& certificate = m_context.certificates.first();
     if constexpr (TLS_DEBUG) {
         dbgln("PreMaster secret");
         print_buffer(m_context.premaster_key);
@@ -248,13 +209,6 @@ void TLSv12::build_rsa_pre_master_secret(PacketBuilder& builder)
 
 void TLSv12::build_dhe_rsa_pre_master_secret(PacketBuilder& builder)
 {
-    const auto& certificate_option = verify_chain_and_get_matching_certificate(m_context.extensions.SNI); // if the SNI is empty, we'll make a special case and match *a* leaf certificate.
-    if (!certificate_option.has_value()) {
-        dbgln("certificate verification failed :(");
-        alert(AlertLevel::Critical, AlertDescription::BadCertificate);
-        return;
-    }
-
     auto& dh = m_context.server_diffie_hellman_params;
     auto dh_p = Crypto::UnsignedBigInteger::import_data(dh.p.data(), dh.p.size());
     auto dh_g = Crypto::UnsignedBigInteger::import_data(dh.g.data(), dh.g.size());
@@ -302,13 +256,6 @@ void TLSv12::build_dhe_rsa_pre_master_secret(PacketBuilder& builder)
 
 void TLSv12::build_ecdhe_rsa_pre_master_secret(PacketBuilder& builder)
 {
-    const auto& certificate_option = verify_chain_and_get_matching_certificate(m_context.extensions.SNI); // if the SNI is empty, we'll make a special case and match *a* leaf certificate.
-    if (!certificate_option.has_value()) {
-        dbgln("certificate verification failed :(");
-        alert(AlertLevel::Critical, AlertDescription::BadCertificate);
-        return;
-    }
-
     // Create a random private key
     auto private_key_result = m_context.server_key_exchange_curve->generate_private_key();
     if (private_key_result.is_error()) {
@@ -414,6 +361,13 @@ ByteBuffer TLSv12::build_certificate()
 
 ByteBuffer TLSv12::build_client_key_exchange()
 {
+    bool chain_verified = m_context.verify_chain(m_context.extensions.SNI);
+    if (!chain_verified) {
+        dbgln("certificate verification failed :(");
+        alert(AlertLevel::Critical, AlertDescription::BadCertificate);
+        return {};
+    }
+
     PacketBuilder builder { MessageType::Handshake, m_context.options.version };
     builder.append((u8)HandshakeType::ClientKeyExchange);
 
diff --git a/Userland/Libraries/LibTLS/TLSv12.cpp b/Userland/Libraries/LibTLS/TLSv12.cpp
index 572c778e52..ab95df9e1c 100644
--- a/Userland/Libraries/LibTLS/TLSv12.cpp
+++ b/Userland/Libraries/LibTLS/TLSv12.cpp
@@ -191,7 +191,31 @@ void TLSv12::set_root_certificates(Vector<Certificate> certificates)
     dbgln_if(TLS_DEBUG, "{}: Set {} root certificates", this, m_context.root_certificates.size());
 }
 
-bool Context::verify_chain() const
+static bool wildcard_matches(StringView host, StringView subject)
+{
+    if (host.matches(subject))
+        return true;
+
+    if (subject.starts_with("*."))
+        return wildcard_matches(host, subject.substring_view(2));
+
+    return false;
+}
+
+static bool certificate_subject_matches_host(Certificate& cert, StringView host)
+{
+    if (wildcard_matches(host, cert.subject.subject))
+        return true;
+
+    for (auto& san : cert.SAN) {
+        if (wildcard_matches(host, san))
+            return true;
+    }
+
+    return false;
+}
+
+bool Context::verify_chain(StringView host) const
 {
     if (!options.validate_certificates)
         return true;
@@ -209,6 +233,19 @@ bool Context::verify_chain() const
         return false;
     }
 
+    if (!host.is_empty()) {
+        auto first_certificate = local_chain->first();
+        auto subject_matches = certificate_subject_matches_host(first_certificate, host);
+        if (!subject_matches) {
+            dbgln("verify_chain: First certificate does not match the hostname");
+            return false;
+        }
+    } else {
+        // FIXME: The host is taken from m_context.extensions.SNI, when is this empty?
+        dbgln("FIXME: verify_chain called without host");
+        return false;
+    }
+
     for (size_t cert_index = 0; cert_index < local_chain->size(); ++cert_index) {
         auto cert = local_chain->at(cert_index);
 
diff --git a/Userland/Libraries/LibTLS/TLSv12.h b/Userland/Libraries/LibTLS/TLSv12.h
index 20a8478388..8f6bef402b 100644
--- a/Userland/Libraries/LibTLS/TLSv12.h
+++ b/Userland/Libraries/LibTLS/TLSv12.h
@@ -261,7 +261,7 @@ struct Options {
 };
 
 struct Context {
-    bool verify_chain() const;
+    bool verify_chain(StringView host) const;
     bool verify_certificate_pair(Certificate& subject, Certificate& issuer) const;
 
     Options options;
@@ -561,8 +561,6 @@ private:
 
     bool compute_master_secret_from_pre_master_secret(size_t length);
 
-    Optional<size_t> verify_chain_and_get_matching_certificate(StringView host) const;
-
     void try_disambiguate_error() const;
 
     bool m_eof { false };