Fully disabled TLS 1.2 - #11324
Open
Frauschi wants to merge 2 commits into
Open
Conversation
test_tls13_downgrade_sentinel() builds a TLS 1.2 server, and a TLS 1.1 one when old TLS is enabled, to make the peer produce the downgrade sentinel a TLS 1.3 client has to reject. Its guard did not mention WOLFSSL_NO_TLS12, so a build without TLS 1.2 failed to compile the unit tests at wolfTLSv1_2_server_method(). There is nothing to downgrade to in such a build, so require TLS 1.2 for the whole test. It still runs everywhere it did before.
The option set ENABLED_TLSV12 to no, which the summary printed and a couple of derived settings read, but nothing defined WOLFSSL_NO_TLS12, so every line of the TLS 1.2 implementation was still compiled and still reachable. The one place configure.ac adds that define sits in the FIPS lean-aesgcm bundle and is guarded by "TLS 1.2 is still on and the user did not ask for it", which is false exactly when --disable-tlsv12 was passed. Only the CMake build honoured the switch. Define it once the bundles have had their say, so --enable-tinytls13 and the other bundles that turn the version off get it as well. The case guard keeps the FIPS bundles from adding it twice.
|
Can one of the admins verify this patch? |
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
2 participants
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.
--disable-tlsv12never compiled TLS 1.2 out. The option setENABLED_TLSV12=no, which the configure summary printed and a couple of derived settings read, but nothing definedWOLFSSL_NO_TLS12, so the whole TLS 1.2 implementation was still built and a peer could still negotiate it. The single placeconfigure.acadds that define sits in the FIPS lean-aesgcm bundle, behind a condition that is false exactly when the user asked for the option, so no autotools configuration ever reached it. Only the CMakeWOLFSSL_TLSV12=nopath honoured the switch.This makes the option do what it says, and fixes what that exposed.
Behavioral change
--disable-tlsv12now definesWOLFSSL_NO_TLS12. A build that passes it and still expects to negotiate TLS 1.2 has to stop passing it. The define is added after the bundles have had their say, so--enable-tinytls13and the other bundles that turn the version off pick it up too.Because the pre-TLS-1.3 handshake is now actually compiled out, configure rejects the four combinations that depend on it rather than producing a build that cannot work:
--enable-oldtls, since TLS 1.0 and 1.1 use the TLS 1.2 handshake--enable-mcast, which rides on DTLS 1.2 and its NULL cipher suite--enable-dtls13, for the same reason on the datagram sideCMake rejects the same four for
-DWOLFSSL_TLSV12=no. It previously accepted-DWOLFSSL_OLD_TLS=yesalongside it and definedNO_OLD_TLSanyway, so the reported option and the build disagreed.The sniffer, the examples and the test suite now pick their code paths by version where they used to assume TLS 1.2 was present. Four
no-tlsv12-*entries were added to.github/configs/os-check-linux.jsoncovering the option on its own axes: OCSP stapling plus the OpenSSL compatibility layer, status_request_v2 without status_request, DTLS 1.3 with connection ID and session tickets, and the sniffer.Three bugs this uncovered
These are independent of the TLS 1.2 work and are kept as their own commits, so they can be split out if preferred. All three reproduce on master.
PK callbacks could not complete a TLS 1.3 handshake.
myEccKeyGen()generated the key share straight into the library'secc_key, whilemyEccSharedSecret()is handed only the peer's key on the client side, so the client had no private key left to reach:wolfSSL_connect error -170, ECC input argument wrong type. The example callbacks now keep the private half application-side for TLS and DTLS 1.3, which is whatTEST_PK_PRIVKEYalready modelled and what a real PK-callback application has to do anyway.scripts/pkcallbacks.testpassed no-v, so it always negotiated TLS 1.2 and never covered this; it now runs the build's default plus every version the build reports.Chain OCSP stapling failed the handshake when a responder was unreachable. The chain loops suppress the responder errors that mean "no status to staple", but
OCSP_INVALID_STATUSwas missing from that set, and it is whatCheckOcspRequest()returns when it cannot reach the responder at all. A TLS 1.3 server built withWOLFSSL_TLS_OCSP_MULTItherefore dropped every handshake whenever the responder for any intermediate was down, having already obtained the leaf status the peer asked about. The leaf path inCreateOcspResponse()deliberately still fails hard, andOCSP_CERT_REVOKEDstays fatal everywhere.dtls_benchdid not build without one side.examples/benchmark/dtls_bench.ccalledwolfSSL_accept()andwolfSSL_connect()unconditionally, so aNO_WOLFSSL_SERVERbuild failed to link. That is the existingdtls13-client-minimalos-check entry, which passesCPPFLAGS=-DNO_WOLFSSL_SERVERand gets the benchmark built becauseos-check.ymldoes not pass--disable-examples.Testing
Built and
make checked clean (0 failures) on macOS/arm64 with clang, each with-pedantic -Wdeclaration-after-statement -Wnull-dereference -Wno-overlength-strings -Werror:--disable-tlsv12 --enable-ocspstapling --enable-opensslextra--enable-tls13 --disable-tlsv12 --enable-ocspstapling2(-Wall -Wextra -Werror)--disable-tlsv12 --enable-dtls --enable-dtls13 --enable-dtlscid --enable-session-ticket--enable-sniffer --disable-tlsv12snifftestreportstls_v13withouttls_v12and decodes the ecc, dh and hrr pcaps--enable-tinytls13--enable-all --disable-mcast --disable-oldtls --disable-tlsv12--enable-all --disable-mcast --disable-oldtls--enable-all --enable-pkcallbacks, with and without-DTEST_PK_PRIVKEYRejection paths checked by hand: configure errors on
--disable-tlsv12with each of--enable-oldtls,--disable-tls13,--enable-mcastand--enable-dtlswithout dtls13; CMakeFATAL_ERRORs onWOLFSSL_OLD_TLS=yes,WOLFSSL_TLS13=no,WOLFSSL_MCAST=yesandWOLFSSL_DTLS=yeswithoutWOLFSSL_DTLS13, and accepts the valid combinations.For the PK-callback fix specifically: the new
-v 4leg ofscripts/pkcallbacks.testfails against pre-fix binaries and passes after, and the HelloRetryRequest re-keygen path (server -Y) and a two-curve client both complete. For the OCSP fix, a revoked server certificate is still rejected with-360 OCSP Cert revoked, so the suppression does not mask revocation.