fix(tls): refuse a server certificate that does not verify - #21
Open
yspbwx2010 wants to merge 1 commit into
Open
yspbwx2010 wants to merge 1 commit into
yspbwx2010 wants to merge 1 commit into
Conversation
With verifySsl = true the TLS config used MBEDTLS_SSL_VERIFY_OPTIONAL, which lets the handshake finish whatever the certificate looks like, and nothing read mbedtls_ssl_get_verify_result afterwards. A self-signed certificate, one for another host and an expired one were all accepted. Use MBEDTLS_SSL_VERIFY_REQUIRED. A failed handshake now says why in the response's statusText: the text from mbedtls_x509_crt_verify_info for a verification failure, the mbedTLS error text for any other handshake error. A refused TCP connection still reads "Connection failed". When no CA bundle can be found and verifySsl is true, the connection now fails with a message that points at SSL_CERT_FILE, instead of going ahead without verification. Tests serve self-signed, expired and wrong-name certificates from the existing in-process server and trust them through SSL_CERT_FILE, so they need no network. On Windows, where there is no bundle file, load_ca_certs reads the roots in the system ROOT certificate store that are usable for TLS servers (serverAuth) when SSL_CERT_FILE is not set. That part is built only where platform::uses_winsock is true, and has not been run on Windows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #20
With
verifySsl = truethe handshake accepted any server certificate, becausesetup_tlsusedMBEDTLS_SSL_VERIFY_OPTIONALand nothing read the verification result afterwards. This switches toMBEDTLS_SSL_VERIFY_REQUIRED, so the chain, the validity dates and the hostname are all checked by mbedTLS during the handshake, and makes the failure say why.A failed handshake used to surface only as
Connection failed.TlsSocketnow keeps the reason (error()), andperform_exchangereturns it in place of the generic text when there is one: the text frommbedtls_x509_crt_verify_infofor a verification failure, the mbedTLS error text for any other handshake error, and a message for a CA bundle that cannot be parsed. A refused TCP connection still readsConnection failed. The comment intls.cppmsaying callers could inspect the result after the handshake is removed, since there was never a way to do that.One behaviour change: when
verifySslis true andload_ca_certsfinds no bundle (SSL_CERT_FILEunset or unreadable and none of the system locations present), the connection now fails instead of going ahead unverified. The message says to setSSL_CERT_FILEorverifySsl = false. I think failing is right, and curl does the same, but it is the part I would most like a second opinion on.It matters most on Windows, where there is no bundle file, so this also reads the system
ROOTcertificate store there:load_ca_certscallsCertOpenSystemStoreW/CertEnumCertificatesInStore(crypt32, linked with#pragma comment, assocket.cppmdoes for ws2_32) whenSSL_CERT_FILEis not set, and returns the certificates as PEM, so the rest is unchanged. Roots whose enhanced key usage does not include serverAuth are skipped, since Windows keeps roots it no longer trusts for TLS servers in that store behind such a restriction. Two limits: Windows downloads some roots on demand, so a root a browser would fetch may be missing from the store and the connection is refused until it is there orSSL_CERT_FILEis set; and a MinGW build has to add-lcrypt32itself (as it already adds-lws2_32), because clang ignores#pragma comment(lib, ...)there. It is guarded byplatform::uses_winsockas inplatform.cppm, so the Windows build that goes through the POSIX socket interface (openkal) does not read the store and still needsSSL_CERT_FILE. I could not build this part as a whole or run it: there is no Windows toolchain on my machine and CI here only covers Ubuntu. All I could do was compile the store-reading function on its own forx86_64-w64-windows-gnuagainst Wine's Windows headers, with a stand-in forstd::string, to check the API calls; the module itself has not been built for Windows and the function has not been run on Windows. Please treat that function as untested until someone with a Windows machine has tried it.verifySsl = falseis unchanged, and so is the CA loading order.I checked it against the four certificates from the issue (
openssl s_serveron 127.0.0.1, the issue's client,SSL_CERT_FILEpointing at the test CA):With
SSL_CERT_FILEunset, so only the system bundle, all four fail with a verification error. With/etc/ssland/etc/pkihidden and noSSL_CERT_FILE:and a port with nothing listening still gives
Connection failed.Tests are in
tests/test_tls_verify.cpp. They serve a self-signed, an expired and a wrong-name certificate from the existing in-process server intests/tls_test_server.hpp(it now takes an optional certificate and key, and carries two more test pairs) and trust them throughSSL_CERT_FILEpointing at a temporary file, so they use no network and not the machine's CA store. They cover a trusted certificate being accepted, an untrusted issuer, an expired certificate, a wrong hostname and an unparsable bundle being refused with the reason instatusText, andverifySsl = falsestill connecting. All six pass with the change; against the unpatched sources four of them fail. Not covered: the missing-bundle case, becauseload_ca_certsfalls back to the system locations and the test machine usually has one; I checked it by hiding them as above.examples/openkaltreats any status 0 as "no network" and exits 0, so a certificate failure there would read as a skipped run; I left it alone.test_poolandtest_framingpass unchanged on my machine. I ran them with a small stand-in for the gtest macros, becausecompat.gtestwas not available offline here, so a run in CI is still worth waiting for. I did not bump the version;CHANGELOG.mdhas an Unreleased entry and the README'sverifySslrow mentions the bundle and the Windows store.