From: Viktor Dukhovni Date: Thu, 19 Dec 2024 17:25:15 +0000 (+1100) Subject: With SSL_VERIFY_PEER client RPK should abort on X509 error X-Git-Tag: openssl-3.3.3~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=87ebd203feffcf92ad5889df92f90bb0ee10a699;p=thirdparty%2Fopenssl.git With SSL_VERIFY_PEER client RPK should abort on X509 error While RPK performs X.509 checks correctly, at the SSL layer the SSL_VERIFY_PEER flag was not honoured and connections were allowed to complete even when the server was not verified. The client can of course determine this by calling SSL_get_verify_result(), but some may not know to do this. Added tests to make sure this does not regress. Fixes CVE-2024-12797 Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell Reviewed-by: Neil Horman (cherry picked from commit 6ae8e947d8e3f3f03eeb7d9ad993e341791900bc) --- diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index ccfea5d3a11..00bf4d5afc0 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1909,6 +1909,7 @@ static WORK_STATE tls_post_process_server_rpk(SSL_CONNECTION *sc, { size_t certidx; const SSL_CERT_LOOKUP *clu; + int v_ok; if (sc->session->peer_rpk == NULL) { SSLfatal(sc, SSL_AD_ILLEGAL_PARAMETER, @@ -1918,9 +1919,19 @@ static WORK_STATE tls_post_process_server_rpk(SSL_CONNECTION *sc, if (sc->rwstate == SSL_RETRY_VERIFY) sc->rwstate = SSL_NOTHING; - if (ssl_verify_rpk(sc, sc->session->peer_rpk) > 0 - && sc->rwstate == SSL_RETRY_VERIFY) + + ERR_set_mark(); + v_ok = ssl_verify_rpk(sc, sc->session->peer_rpk); + if (v_ok <= 0 && sc->verify_mode != SSL_VERIFY_NONE) { + ERR_clear_last_mark(); + SSLfatal(sc, ssl_x509err2alert(sc->verify_result), + SSL_R_CERTIFICATE_VERIFY_FAILED); + return WORK_ERROR; + } + ERR_pop_to_mark(); /* but we keep s->verify_result */ + if (v_ok > 0 && sc->rwstate == SSL_RETRY_VERIFY) { return WORK_MORE_A; + } if ((clu = ssl_cert_lookup_by_pkey(sc->session->peer_rpk, &certidx, SSL_CONNECTION_GET_CTX(sc))) == NULL) { diff --git a/test/rpktest.c b/test/rpktest.c index ac824798f1c..0be8461f77b 100644 --- a/test/rpktest.c +++ b/test/rpktest.c @@ -89,12 +89,14 @@ static int rpk_verify_server_cb(int ok, X509_STORE_CTX *ctx) * idx = 13 - resumption with client authentication * idx = 14 - resumption with client authentication, no ticket * idx = 15 - like 0, but use non-default libctx + * idx = 16 - like 7, but with SSL_VERIFY_PEER connection should fail + * idx = 17 - like 8, but with SSL_VERIFY_PEER connection should fail * - * 16 * 2 * 4 * 2 * 2 * 2 * 2 = 2048 tests + * 18 * 2 * 4 * 2 * 2 * 2 * 2 = 2048 tests */ static int test_rpk(int idx) { -# define RPK_TESTS 16 +# define RPK_TESTS 18 # define RPK_DIMS (2 * 4 * 2 * 2 * 2 * 2) SSL_CTX *cctx = NULL, *sctx = NULL; SSL *clientssl = NULL, *serverssl = NULL; @@ -114,6 +116,7 @@ static int test_rpk(int idx) int idx_cert, idx_prot; int client_auth = 0; int resumption = 0; + int want_error = SSL_ERROR_NONE; long server_verify_result = 0; long client_verify_result = 0; OSSL_LIB_CTX *test_libctx = NULL; @@ -188,7 +191,7 @@ static int test_rpk(int idx) #ifdef OPENSSL_NO_ECDSA /* Can't get other_key if it's ECDSA */ if (other_pkey == NULL && idx_cert == 0 - && (idx == 4 || idx == 6 || idx == 7)) { + && (idx == 4 || idx == 6 || idx == 7 || idx == 16)) { testresult = TEST_skip("EDCSA disabled"); goto end; } @@ -266,8 +269,10 @@ static int test_rpk(int idx) goto end; /* Only a private key */ if (idx == 1) { - if (idx_server_server_rpk == 0 || idx_client_server_rpk == 0) + if (idx_server_server_rpk == 0 || idx_client_server_rpk == 0) { expected = 0; + want_error = SSL_ERROR_SSL; + } } else { /* Add certificate */ if (!TEST_int_eq(SSL_use_certificate_file(serverssl, cert_file, SSL_FILETYPE_PEM), 1)) @@ -333,12 +338,14 @@ static int test_rpk(int idx) client_expected = -1; if (!TEST_true(SSL_add_expected_rpk(clientssl, other_pkey))) goto end; + SSL_set_verify(clientssl, SSL_VERIFY_NONE, rpk_verify_client_cb); client_verify_result = X509_V_ERR_DANE_NO_MATCH; break; case 8: if (idx_server_server_rpk == 1 && idx_client_server_rpk == 1) client_expected = -1; /* no peer keys */ + SSL_set_verify(clientssl, SSL_VERIFY_NONE, rpk_verify_client_cb); client_verify_result = X509_V_ERR_RPK_UNTRUSTED; break; case 9: @@ -370,9 +377,13 @@ static int test_rpk(int idx) if (!TEST_int_eq(SSL_use_PrivateKey_file(clientssl, privkey_file, SSL_FILETYPE_PEM), 1)) goto end; /* Since there's no cert, this is expected to fail without RPK support */ - if (!idx_server_client_rpk || !idx_client_client_rpk) + if (!idx_server_client_rpk || !idx_client_client_rpk) { expected = 0; - SSL_set_verify(serverssl, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, rpk_verify_server_cb); + want_error = SSL_ERROR_SSL; + SSL_set_verify(serverssl, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL); + } else { + SSL_set_verify(serverssl, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, rpk_verify_server_cb); + } client_auth = 1; break; case 11: @@ -449,12 +460,35 @@ static int test_rpk(int idx) if (!TEST_true(SSL_add_expected_rpk(clientssl, pkey))) goto end; break; + case 16: + if (idx_server_server_rpk == 1 && idx_client_server_rpk == 1) { + /* wrong expected server key */ + expected = 0; + want_error = SSL_ERROR_SSL; + SSL_set_verify(serverssl, SSL_VERIFY_PEER, NULL); + } + if (!TEST_true(SSL_add_expected_rpk(clientssl, other_pkey))) + goto end; + break; + case 17: + if (idx_server_server_rpk == 1 && idx_client_server_rpk == 1) { + /* no expected server keys */ + expected = 0; + want_error = SSL_ERROR_SSL; + SSL_set_verify(serverssl, SSL_VERIFY_PEER, NULL); + } + break; } - ret = create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE); + ret = create_ssl_connection(serverssl, clientssl, want_error); if (!TEST_int_eq(expected, ret)) goto end; + if (expected <= 0) { + testresult = 1; + goto end; + } + /* Make sure client gets RPK or certificate as configured */ if (expected == 1) { if (idx_server_server_rpk && idx_client_server_rpk) {