From afc256831cf3e0c52f6e6da64bd6277d8a14d816 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Sun, 13 Sep 2026 03:17:13 +0200 Subject: [PATCH] Fix TLS client certificate verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⓘ Prevent certificate-chain errors from being overwritten while preserving password-login fallback for invalid optional certificates. Reset verification state for every TLS handshake and add regression coverage. --- CHANGELOG.md | 1 + miniserv-lib.pl | 32 ++++++++++---- t/miniserv.t | 112 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd418726c..134bd7174 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ * Fix module name validation to prevent loading files outside installed modules * Fix ACL operations bypassing configured allowed paths in File Manager * Fix arbitrary file reads in Software Packages module +* Fix TLS client certificate verification #### 2.660 (August 20, 2026) * Add support for creating `vfsv1` Linux quota files for limits above 4 TiB, while preserving existing quota file formats diff --git a/miniserv-lib.pl b/miniserv-lib.pl index 42335b6f7..8bed5940a 100644 --- a/miniserv-lib.pl +++ b/miniserv-lib.pl @@ -2510,11 +2510,19 @@ foreach $d (@_) { sub verify_client { -local $cert = Net::SSLeay::X509_STORE_CTX_get_current_cert($_[1]); -if ($cert) { - local $errnum = Net::SSLeay::X509_STORE_CTX_get_error($_[1]); - $verified_client = 1 if (!$errnum); +local ($preverify_ok, $ctx) = @_; +local $cert = Net::SSLeay::X509_STORE_CTX_get_current_cert($ctx); +local $errnum = Net::SSLeay::X509_STORE_CTX_get_error($ctx); +if (!$preverify_ok || $errnum) { + # Once any certificate fails, later successful checks must not + # mark the whole chain as verified. + $verified_client = 0; } +elsif ($cert && !defined($verified_client)) { + $verified_client = 1; + } +# Continue the handshake so an invalid optional client certificate can fall +# back to another login method instead of preventing access altogether. return 1; } @@ -3625,11 +3633,9 @@ if ($@) { if ($client_certs) { Net::SSLeay::CTX_load_verify_locations( $ssl_ctx, $config{'ca'}, ""); - eval { - Net::SSLeay::set_verify( - $ssl_ctx, &Net::SSLeay::VERIFY_PEER, \&verify_client); - }; - if ($@) { + # Register verification on the context so every SSL connection created + # from it invokes the client-certificate callback. + if (defined(&Net::SSLeay::CTX_set_verify)) { Net::SSLeay::CTX_set_verify( $ssl_ctx, &Net::SSLeay::VERIFY_PEER, \&verify_client); } @@ -3710,6 +3716,13 @@ if (!$sn) { local (undef, $myip, undef) = &get_address_ip($sn, $ipv6); local $ssl_ctx = $ssl_contexts{$myip} || $ssl_contexts{"*"}; local $ssl_con = Net::SSLeay::new($ssl_ctx->{'ctx'}); +if ($client_certs && !defined(&Net::SSLeay::CTX_set_verify)) { + # Older Net::SSLeay versions only provide the connection-level API. + # Register it on the SSL object rather than passing an incompatible + # context. + Net::SSLeay::set_verify( + $ssl_con, &Net::SSLeay::VERIFY_PEER, \&verify_client); + } if ($config{'ssl_cipher_list'}) { # Force use of ciphers eval "Net::SSLeay::set_cipher_list( @@ -3722,6 +3735,7 @@ if ($config{'ssl_cipher_list'}) { # Accept the SSL connection Net::SSLeay::set_fd($ssl_con, fileno($sock)); +$verified_client = undef; alarm(10); $SIG{'ALRM'} = sub { die "timeout" }; my $ok = Net::SSLeay::accept($ssl_con); diff --git a/t/miniserv.t b/t/miniserv.t index 12ccd2760..2dddb9fe0 100644 --- a/t/miniserv.t +++ b/t/miniserv.t @@ -16,6 +16,7 @@ use warnings; use Test::More; use File::Basename qw(dirname); use File::Spec; +use File::Temp qw(tempfile); my $lib = File::Spec->rel2abs( File::Spec->catfile(dirname(__FILE__), '..', 'miniserv-lib.pl')); @@ -786,6 +787,117 @@ subtest 'password_crypt' => sub { 'DES stored hash + correct password verifies'); }; +# verify_client — preserve optional-certificate fallback without allowing a +# successful certificate check to hide an error elsewhere in the chain. +subtest 'verify_client' => sub { + no warnings qw(redefine once); + my $has_cert = 1; + my $error = 0; + local *Net::SSLeay::X509_STORE_CTX_get_current_cert = sub { + return $has_cert; + }; + local *Net::SSLeay::X509_STORE_CTX_get_error = sub { + return $error; + }; + + # A fully valid chain is eligible for certificate authentication. + { + local $miniserv::verified_client; + is(miniserv::verify_client(1, 'ctx'), 1, + 'valid issuer check continues the handshake'); + is(miniserv::verify_client(1, 'ctx'), 1, + 'valid leaf check continues the handshake'); + is($miniserv::verified_client, 1, + 'all-valid chain is marked verified'); + } + + # OpenSSL checks from the issuer toward the leaf, so a bad leaf must clear + # an earlier success while still allowing password authentication fallback. + { + local $miniserv::verified_client; + $error = 0; + miniserv::verify_client(1, 'ctx'); + $error = 10; + is(miniserv::verify_client(0, 'ctx'), 1, + 'invalid leaf still continues the handshake'); + is($miniserv::verified_client, 0, + 'invalid leaf clears successful issuer state'); + } + + # Do not let a later successful callback erase an earlier chain error. + { + local $miniserv::verified_client; + $error = 20; + miniserv::verify_client(0, 'ctx'); + $error = 0; + miniserv::verify_client(1, 'ctx'); + is($miniserv::verified_client, 0, + 'chain remains invalid after a later successful check'); + } + + # Treat pre-verification failure as authoritative even if a binding reports + # no numeric error. + { + local $miniserv::verified_client; + $error = 0; + miniserv::verify_client(0, 'ctx'); + is($miniserv::verified_client, 0, + 'pre-verification failure rejects the chain'); + } + + # A successful callback without a current certificate cannot establish + # client identity, while an error without one still rejects the chain. + { + local $miniserv::verified_client; + $has_cert = 0; + $error = 0; + miniserv::verify_client(1, 'ctx'); + is($miniserv::verified_client, undef, + 'callback without a certificate does not verify a client'); + $error = 20; + miniserv::verify_client(0, 'ctx'); + is($miniserv::verified_client, 0, + 'error without a current certificate rejects the chain'); + } +}; + +# create_ssl_context — client-certificate verification belongs to the SSL +# context, not to an individual SSL connection object. +subtest 'create_ssl_context client verification' => sub { + no warnings qw(redefine once); + my ($keyfh, $keyfile) = tempfile(); + my ($certfh, $certfile) = tempfile(); + close($keyfh); + close($certfh); + my @verify_args; + my $legacy_calls = 0; + + local $miniserv::client_certs = 1; + local %miniserv::config = ( + 'ca' => $certfile, + 'dhparams_file' => "$certfile.missing", + ); + local *Net::SSLeay::new_x_ctx = sub { return 'ctx'; }; + local *Net::SSLeay::VERIFY_PEER = sub { return 1; }; + local *Net::SSLeay::FILETYPE_PEM = sub { return 1; }; + local *Net::SSLeay::CTX_load_verify_locations = sub { return 1; }; + local *Net::SSLeay::CTX_set_verify = sub { @verify_args = @_; }; + local *Net::SSLeay::set_verify = sub { $legacy_calls++; }; + local *Net::SSLeay::CTX_use_PrivateKey_file = sub { return 1; }; + local *Net::SSLeay::CTX_use_certificate_file = sub { return 1; }; + local *Net::SSLeay::CTX_set_options = sub { return 1; }; + local *miniserv::cert_names = sub { return { 'alt' => [] }; }; + + my $result = miniserv::create_ssl_context($keyfile, $certfile, 'none'); + is($result->{'ctx'}, 'ctx', 'SSL context is created'); + is($verify_args[0], 'ctx', 'verification is registered on the context'); + is($verify_args[1], 1, + 'peer verification is enabled'); + is($verify_args[2], \&miniserv::verify_client, + 'client-certificate callback is registered'); + is($legacy_calls, 0, 'connection-level verification API is not used'); +}; + # hash_session_id — three independent code paths, picked by which crypto # globals are set. Each branch gets its own subtest so the cache and globals # can be reset cleanly via `local`.