Skip to content

fix(tls): refuse a server certificate that does not verify - #21

Open
yspbwx2010 wants to merge 1 commit into
mcpplibs:masterfrom
yspbwx2010:fix/verify-certificates
Open

yspbwx2010 wants to merge 1 commit into
mcpplibs:masterfrom
yspbwx2010:fix/verify-certificates

Conversation

@yspbwx2010

Copy link
Copy Markdown
Contributor

Fixes #20

With verifySsl = true the handshake accepted any server certificate, because setup_tls used MBEDTLS_SSL_VERIFY_OPTIONAL and nothing read the verification result afterwards. This switches to MBEDTLS_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. TlsSocket now keeps the reason (error()), and perform_exchange returns it in place of the generic text when there is one: the text from mbedtls_x509_crt_verify_info for 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 reads Connection failed. The comment in tls.cppm saying callers could inspect the result after the handshake is removed, since there was never a way to do that.

One behaviour change: when verifySsl is true and load_ca_certs finds no bundle (SSL_CERT_FILE unset or unreadable and none of the system locations present), the connection now fails instead of going ahead unverified. The message says to set SSL_CERT_FILE or verifySsl = 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 ROOT certificate store there: load_ca_certs calls CertOpenSystemStoreW/CertEnumCertificatesInStore (crypt32, linked with #pragma comment, as socket.cppm does for ws2_32) when SSL_CERT_FILE is 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 or SSL_CERT_FILE is set; and a MinGW build has to add -lcrypt32 itself (as it already adds -lws2_32), because clang ignores #pragma comment(lib, ...) there. It is guarded by platform::uses_winsock as in platform.cppm, so the Windows build that goes through the POSIX socket interface (openkal) does not read the store and still needs SSL_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 for x86_64-w64-windows-gnu against Wine's Windows headers, with a stand-in for std::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 = false is unchanged, and so is the CA loading order.

I checked it against the four certificates from the issue (openssl s_server on 127.0.0.1, the issue's client, SSL_CERT_FILE pointing at the test CA):

[good] SUCCESS status=200 ok
[selfsigned] FAILED error="certificate verification failed: The certificate is not correctly signed by the trusted CA"
[wrongname] FAILED error="certificate verification failed: The certificate Common Name (CN) does not match with the expected CN"
[expired] FAILED error="certificate verification failed: The certificate validity has expired"

With SSL_CERT_FILE unset, so only the system bundle, all four fail with a verification error. With /etc/ssl and /etc/pki hidden and no SSL_CERT_FILE:

[good] FAILED error="no CA certificate bundle found; set SSL_CERT_FILE to a PEM file of trusted roots, or set verifySsl to false"

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 in tests/tls_test_server.hpp (it now takes an optional certificate and key, and carries two more test pairs) and trust them through SSL_CERT_FILE pointing 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 in statusText, and verifySsl = false still connecting. All six pass with the change; against the unpatched sources four of them fail. Not covered: the missing-bundle case, because load_ca_certs falls back to the system locations and the test machine usually has one; I checked it by hiding them as above.

examples/openkal treats 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_pool and test_framing pass unchanged on my machine. I ran them with a small stand-in for the gtest macros, because compat.gtest was not available offline here, so a run in CI is still worth waiting for. I did not bump the version; CHANGELOG.md has an Unreleased entry and the README's verifySsl row mentions the bundle and the Windows store.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

With verifySsl = true the TLS handshake accepts any certificate, because the verification result is never checked

1 participant