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 <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(cherry picked from commit 87ebd20)

Author: Viktor Dukhovni

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) {

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) {