|
| 1 | +From 87ebd203feffcf92ad5889df92f90bb0ee10a699 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Viktor Dukhovni <viktor@openssl.org> |
| 3 | +Date: Fri, 20 Dec 2024 04:25:15 +1100 |
| 4 | +Subject: [PATCH] With SSL_VERIFY_PEER client RPK should abort on X509 error |
| 5 | + |
| 6 | +While RPK performs X.509 checks correctly, at the SSL layer the |
| 7 | +SSL_VERIFY_PEER flag was not honoured and connections were allowed to |
| 8 | +complete even when the server was not verified. The client can of |
| 9 | +course determine this by calling SSL_get_verify_result(), but some |
| 10 | +may not know to do this. |
| 11 | + |
| 12 | +Added tests to make sure this does not regress. |
| 13 | + |
| 14 | +Fixes CVE-2024-12797 |
| 15 | + |
| 16 | +Reviewed-by: Tomas Mraz <tomas@openssl.org> |
| 17 | +Reviewed-by: Matt Caswell <matt@openssl.org> |
| 18 | +Reviewed-by: Neil Horman <nhorman@openssl.org> |
| 19 | +(cherry picked from commit 6ae8e947d8e3f3f03eeb7d9ad993e341791900bc) |
| 20 | +--- |
| 21 | + openssl-src/openssl/ssl/statem/statem_clnt.c | 15 +++++++++++-- |
| 22 | + openssl-src/openssl/test/rpktest.c | 48 ++++++++++++++++++++++++++++++++++------ |
| 23 | + 2 files changed, 54 insertions(+), 9 deletions(-) |
| 24 | + |
| 25 | +diff --git a/vendor/openssl-src/openssl/ssl/statem/statem_clnt.c b/vendor/openssl-src/openssl/ssl/statem/statem_clnt.c |
| 26 | +index ccfea5d3a116f..00bf4d5afc0bb 100644 |
| 27 | +--- a/vendor/openssl-src/openssl/ssl/statem/statem_clnt.c |
| 28 | ++++ b/vendor/openssl-src/openssl/ssl/statem/statem_clnt.c |
| 29 | +@@ -1909,6 +1909,7 @@ static WORK_STATE tls_post_process_server_rpk(SSL_CONNECTION *sc, |
| 30 | + { |
| 31 | + size_t certidx; |
| 32 | + const SSL_CERT_LOOKUP *clu; |
| 33 | ++ int v_ok; |
| 34 | + |
| 35 | + if (sc->session->peer_rpk == NULL) { |
| 36 | + SSLfatal(sc, SSL_AD_ILLEGAL_PARAMETER, |
| 37 | +@@ -1918,9 +1919,19 @@ static WORK_STATE tls_post_process_server_rpk(SSL_CONNECTION *sc, |
| 38 | + |
| 39 | + if (sc->rwstate == SSL_RETRY_VERIFY) |
| 40 | + sc->rwstate = SSL_NOTHING; |
| 41 | +- if (ssl_verify_rpk(sc, sc->session->peer_rpk) > 0 |
| 42 | +- && sc->rwstate == SSL_RETRY_VERIFY) |
| 43 | ++ |
| 44 | ++ ERR_set_mark(); |
| 45 | ++ v_ok = ssl_verify_rpk(sc, sc->session->peer_rpk); |
| 46 | ++ if (v_ok <= 0 && sc->verify_mode != SSL_VERIFY_NONE) { |
| 47 | ++ ERR_clear_last_mark(); |
| 48 | ++ SSLfatal(sc, ssl_x509err2alert(sc->verify_result), |
| 49 | ++ SSL_R_CERTIFICATE_VERIFY_FAILED); |
| 50 | ++ return WORK_ERROR; |
| 51 | ++ } |
| 52 | ++ ERR_pop_to_mark(); /* but we keep s->verify_result */ |
| 53 | ++ if (v_ok > 0 && sc->rwstate == SSL_RETRY_VERIFY) { |
| 54 | + return WORK_MORE_A; |
| 55 | ++ } |
| 56 | + |
| 57 | + if ((clu = ssl_cert_lookup_by_pkey(sc->session->peer_rpk, &certidx, |
| 58 | + SSL_CONNECTION_GET_CTX(sc))) == NULL) { |
| 59 | +diff --git a/vendor/openssl-src/openssl/test/rpktest.c b/vendor/openssl-src/openssl/test/rpktest.c |
| 60 | +index ac824798f1c66..0be8461f77b53 100644 |
| 61 | +--- a/vendor/openssl-src/openssl/test/rpktest.c |
| 62 | ++++ b/vendor/openssl-src/openssl/test/rpktest.c |
| 63 | +@@ -89,12 +89,14 @@ static int rpk_verify_server_cb(int ok, X509_STORE_CTX *ctx) |
| 64 | + * idx = 13 - resumption with client authentication |
| 65 | + * idx = 14 - resumption with client authentication, no ticket |
| 66 | + * idx = 15 - like 0, but use non-default libctx |
| 67 | ++ * idx = 16 - like 7, but with SSL_VERIFY_PEER connection should fail |
| 68 | ++ * idx = 17 - like 8, but with SSL_VERIFY_PEER connection should fail |
| 69 | + * |
| 70 | +- * 16 * 2 * 4 * 2 * 2 * 2 * 2 = 2048 tests |
| 71 | ++ * 18 * 2 * 4 * 2 * 2 * 2 * 2 = 2048 tests |
| 72 | + */ |
| 73 | + static int test_rpk(int idx) |
| 74 | + { |
| 75 | +-# define RPK_TESTS 16 |
| 76 | ++# define RPK_TESTS 18 |
| 77 | + # define RPK_DIMS (2 * 4 * 2 * 2 * 2 * 2) |
| 78 | + SSL_CTX *cctx = NULL, *sctx = NULL; |
| 79 | + SSL *clientssl = NULL, *serverssl = NULL; |
| 80 | +@@ -114,6 +116,7 @@ static int test_rpk(int idx) |
| 81 | + int idx_cert, idx_prot; |
| 82 | + int client_auth = 0; |
| 83 | + int resumption = 0; |
| 84 | ++ int want_error = SSL_ERROR_NONE; |
| 85 | + long server_verify_result = 0; |
| 86 | + long client_verify_result = 0; |
| 87 | + OSSL_LIB_CTX *test_libctx = NULL; |
| 88 | +@@ -188,7 +191,7 @@ static int test_rpk(int idx) |
| 89 | + #ifdef OPENSSL_NO_ECDSA |
| 90 | + /* Can't get other_key if it's ECDSA */ |
| 91 | + if (other_pkey == NULL && idx_cert == 0 |
| 92 | +- && (idx == 4 || idx == 6 || idx == 7)) { |
| 93 | ++ && (idx == 4 || idx == 6 || idx == 7 || idx == 16)) { |
| 94 | + testresult = TEST_skip("EDCSA disabled"); |
| 95 | + goto end; |
| 96 | + } |
| 97 | +@@ -266,8 +269,10 @@ static int test_rpk(int idx) |
| 98 | + goto end; |
| 99 | + /* Only a private key */ |
| 100 | + if (idx == 1) { |
| 101 | +- if (idx_server_server_rpk == 0 || idx_client_server_rpk == 0) |
| 102 | ++ if (idx_server_server_rpk == 0 || idx_client_server_rpk == 0) { |
| 103 | + expected = 0; |
| 104 | ++ want_error = SSL_ERROR_SSL; |
| 105 | ++ } |
| 106 | + } else { |
| 107 | + /* Add certificate */ |
| 108 | + if (!TEST_int_eq(SSL_use_certificate_file(serverssl, cert_file, SSL_FILETYPE_PEM), 1)) |
| 109 | +@@ -333,12 +338,14 @@ static int test_rpk(int idx) |
| 110 | + client_expected = -1; |
| 111 | + if (!TEST_true(SSL_add_expected_rpk(clientssl, other_pkey))) |
| 112 | + goto end; |
| 113 | ++ SSL_set_verify(clientssl, SSL_VERIFY_NONE, rpk_verify_client_cb); |
| 114 | + client_verify_result = X509_V_ERR_DANE_NO_MATCH; |
| 115 | + break; |
| 116 | + case 8: |
| 117 | + if (idx_server_server_rpk == 1 && idx_client_server_rpk == 1) |
| 118 | + client_expected = -1; |
| 119 | + /* no peer keys */ |
| 120 | ++ SSL_set_verify(clientssl, SSL_VERIFY_NONE, rpk_verify_client_cb); |
| 121 | + client_verify_result = X509_V_ERR_RPK_UNTRUSTED; |
| 122 | + break; |
| 123 | + case 9: |
| 124 | +@@ -370,9 +377,13 @@ static int test_rpk(int idx) |
| 125 | + if (!TEST_int_eq(SSL_use_PrivateKey_file(clientssl, privkey_file, SSL_FILETYPE_PEM), 1)) |
| 126 | + goto end; |
| 127 | + /* Since there's no cert, this is expected to fail without RPK support */ |
| 128 | +- if (!idx_server_client_rpk || !idx_client_client_rpk) |
| 129 | ++ if (!idx_server_client_rpk || !idx_client_client_rpk) { |
| 130 | + expected = 0; |
| 131 | +- SSL_set_verify(serverssl, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, rpk_verify_server_cb); |
| 132 | ++ want_error = SSL_ERROR_SSL; |
| 133 | ++ SSL_set_verify(serverssl, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL); |
| 134 | ++ } else { |
| 135 | ++ SSL_set_verify(serverssl, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, rpk_verify_server_cb); |
| 136 | ++ } |
| 137 | + client_auth = 1; |
| 138 | + break; |
| 139 | + case 11: |
| 140 | +@@ -449,12 +460,35 @@ static int test_rpk(int idx) |
| 141 | + if (!TEST_true(SSL_add_expected_rpk(clientssl, pkey))) |
| 142 | + goto end; |
| 143 | + break; |
| 144 | ++ case 16: |
| 145 | ++ if (idx_server_server_rpk == 1 && idx_client_server_rpk == 1) { |
| 146 | ++ /* wrong expected server key */ |
| 147 | ++ expected = 0; |
| 148 | ++ want_error = SSL_ERROR_SSL; |
| 149 | ++ SSL_set_verify(serverssl, SSL_VERIFY_PEER, NULL); |
| 150 | ++ } |
| 151 | ++ if (!TEST_true(SSL_add_expected_rpk(clientssl, other_pkey))) |
| 152 | ++ goto end; |
| 153 | ++ break; |
| 154 | ++ case 17: |
| 155 | ++ if (idx_server_server_rpk == 1 && idx_client_server_rpk == 1) { |
| 156 | ++ /* no expected server keys */ |
| 157 | ++ expected = 0; |
| 158 | ++ want_error = SSL_ERROR_SSL; |
| 159 | ++ SSL_set_verify(serverssl, SSL_VERIFY_PEER, NULL); |
| 160 | ++ } |
| 161 | ++ break; |
| 162 | + } |
| 163 | + |
| 164 | +- ret = create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE); |
| 165 | ++ ret = create_ssl_connection(serverssl, clientssl, want_error); |
| 166 | + if (!TEST_int_eq(expected, ret)) |
| 167 | + goto end; |
| 168 | + |
| 169 | ++ if (expected <= 0) { |
| 170 | ++ testresult = 1; |
| 171 | ++ goto end; |
| 172 | ++ } |
| 173 | ++ |
| 174 | + /* Make sure client gets RPK or certificate as configured */ |
| 175 | + if (expected == 1) { |
| 176 | + if (idx_server_server_rpk && idx_client_server_rpk) { |
0 commit comments