TLS client: allow cert verify callback return -1 for SSL_ERROR_WANT_RETRY_VERIFY

The client-side cert verification callback function may not only return
as usual for success or 0 for failure, but also -1,
typically on failure verifying the server certificate.
This makes the handshake suspend and return control to the calling application
with SSL_ERROR_WANT_RETRY_VERIFY.
The app can for instance fetch further certificates or cert status information
needed for the verification.
Calling SSL_connect() again resumes the connection attempt
by retrying the server certificate verification step.
This process may even be repeated if need be.

The core implementation of the feature is in ssl/statem/statem_clnt.c,
splitting tls_process_server_certificate() into a preparation step
that just copies the certificates received from the server to s->session->peer_chain
(rather than having them in a local variable at first) and returns to the state machine,
and a post-processing step in tls_post_process_server_certificate() that can be repeated:
Try verifying the current contents of s->session->peer_chain basically as before,
but give the verification callback function the chance to pause connecting and
make the TLS state machine later call tls_post_process_server_certificate() again.
Otherwise processing continues as usual.

The documentation of the new feature is added to SSL_CTX_set_cert_verify_callback.pod
and SSL_want.pod.

This adds two tests:
* A generic test in test/helpers/handshake.c
  on the usability of the new server cert verification retry feature.
  It is triggered via test/ssl-tests/03-custom_verify.cnf.in (while the bulky auto-
  generated changes to test/ssl-tests/03-custom_verify.cnf can be basically ignored).
* A test in test/sslapitest.c that demonstrates the effectiveness of the approach
  for augmenting the cert chain provided by the server in between SSL_connect() calls.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/13906)

3 files changed, 45 insertions, 30 deletions

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index a8a1416073..c345a6b405 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3808,6 +3808,8 @@ int SSL_get_error(const SSL *s, int i)
     }
     if (SSL_want_x509_lookup(s))
         return SSL_ERROR_WANT_X509_LOOKUP;
+    if (SSL_want_retry_verify(s))
+        return SSL_ERROR_WANT_RETRY_VERIFY;
     if (SSL_want_async(s))
         return SSL_ERROR_WANT_ASYNC;
     if (SSL_want_async_job(s))
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 045db8265e..00594ec352 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1009,7 +1009,7 @@ size_t ossl_statem_client_max_message_size(SSL *s)
 }
 
 /*
- * Process a message that the client has been received from the server.
+ * Process a message that the client has received from the server.
  */
 MSG_PROCESS_RETURN ossl_statem_client_process_message(SSL *s, PACKET *pkt)
 {
@@ -1079,6 +1079,9 @@ WORK_STATE ossl_statem_client_post_process_message(SSL *s, WORK_STATE wst)
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return WORK_ERROR;
 
+    case TLS_ST_CR_CERT:
+        return tls_post_process_server_certificate(s, wst);
+
     case TLS_ST_CR_CERT_VRFY:
     case TLS_ST_CR_CERT_REQ:
         return tls_prepare_client_certificate(s, wst);
@@ -1762,20 +1765,16 @@ static MSG_PROCESS_RETURN tls_process_as_hello_retry_request(SSL *s,
     return MSG_PROCESS_ERROR;
 }
 
+/* prepare server cert verificaton by setting s->session->peer_chain from pkt */
 MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt)
 {
-    int i;
-    MSG_PROCESS_RETURN ret = MSG_PROCESS_ERROR;
     unsigned long cert_list_len, cert_len;
     X509 *x = NULL;
     const unsigned char *certstart, *certbytes;
-    STACK_OF(X509) *sk = NULL;
-    EVP_PKEY *pkey = NULL;
-    size_t chainidx, certidx;
+    size_t chainidx;
     unsigned int context = 0;
-    const SSL_CERT_LOOKUP *clu;
 
-    if ((sk = sk_X509_new_null()) == NULL) {
+    if ((s->session->peer_chain = sk_X509_new_null()) == NULL) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_MALLOC_FAILURE);
         goto err;
     }
@@ -1834,14 +1833,39 @@ MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt)
             OPENSSL_free(rawexts);
         }
 
-        if (!sk_X509_push(sk, x)) {
+        if (!sk_X509_push(s->session->peer_chain, x)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_MALLOC_FAILURE);
             goto err;
         }
         x = NULL;
     }
+    return MSG_PROCESS_CONTINUE_PROCESSING;
+
+ err:
+    X509_free(x);
+    sk_X509_pop_free(s->session->peer_chain, X509_free);
+    s->session->peer_chain = NULL;
+    return MSG_PROCESS_ERROR;
+}
+
+/*
+ * Verify the s->session->peer_chain and check server cert type.
+ * On success set s->session->peer and s->session->verify_result.
+ * Else the peer certificate verification callback may request retry.
+ */
+WORK_STATE tls_post_process_server_certificate(SSL *s, WORK_STATE wst)
+{
+    X509 *x;
+    EVP_PKEY *pkey = NULL;
+    const SSL_CERT_LOOKUP *clu;
+    size_t certidx;
+    int i;
 
-    i = ssl_verify_cert_chain(s, sk);
+    i = ssl_verify_cert_chain(s, s->session->peer_chain);
+    if (i == -1) {
+        s->rwstate = SSL_RETRY_VERIFY;
+        return WORK_MORE_A;
+    }
     /*
      * The documented interface is that SSL_VERIFY_PEER should be set in order
      * for client side verification of the server certificate to take place.
@@ -1859,35 +1883,31 @@ MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt)
     if (s->verify_mode != SSL_VERIFY_NONE && i <= 0) {
         SSLfatal(s, ssl_x509err2alert(s->verify_result),
                  SSL_R_CERTIFICATE_VERIFY_FAILED);
-        goto err;
+        return WORK_ERROR;
     }
     ERR_clear_error();          /* but we keep s->verify_result */
     if (i > 1) {
         SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, i);
-        goto err;
+        return WORK_ERROR;
     }
 
-    s->session->peer_chain = sk;
     /*
      * Inconsistency alert: cert_chain does include the peer's certificate,
      * which we don't include in statem_srvr.c
      */
-    x = sk_X509_value(sk, 0);
-    sk = NULL;
+    x = sk_X509_value(s->session->peer_chain, 0);
 
     pkey = X509_get0_pubkey(x);
 
     if (pkey == NULL || EVP_PKEY_missing_parameters(pkey)) {
-        x = NULL;
         SSLfatal(s, SSL_AD_INTERNAL_ERROR,
                  SSL_R_UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS);
-        goto err;
+        return WORK_ERROR;
     }
 
     if ((clu = ssl_cert_lookup_by_pkey(pkey, &certidx)) == NULL) {
-        x = NULL;
         SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_R_UNKNOWN_CERTIFICATE_TYPE);
-        goto err;
+        return WORK_ERROR;
     }
     /*
      * Check certificate type is consistent with ciphersuite. For TLS 1.3
@@ -1896,9 +1916,8 @@ MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt)
      */
     if (!SSL_IS_TLS13(s)) {
         if ((clu->amask & s->s3.tmp.new_cipher->algorithm_auth) == 0) {
-            x = NULL;
             SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_R_WRONG_CERTIFICATE_TYPE);
-            goto err;
+            return WORK_ERROR;
         }
     }
     s->session->peer_type = certidx;
@@ -1907,7 +1926,6 @@ MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt)
     X509_up_ref(x);
     s->session->peer = x;
     s->session->verify_result = s->verify_result;
-    x = NULL;
 
     /* Save the current hash state for when we receive the CertificateVerify */
     if (SSL_IS_TLS13(s)
@@ -1915,15 +1933,9 @@ MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt)
                                    sizeof(s->cert_verify_hash),
                                    &s->cert_verify_hash_len)) {
         /* SSLfatal() already called */;
-        goto err;
+        return WORK_ERROR;
     }
-
-    ret = MSG_PROCESS_CONTINUE_READING;
-
- err:
-    X509_free(x);
-    sk_X509_pop_free(sk, X509_free);
-    return ret;
+    return WORK_FINISHED_CONTINUE;
 }
 
 static int tls_process_ske_psk_preamble(SSL *s, PACKET *pkt)
diff --git a/ssl/statem/statem_local.h b/ssl/statem/statem_local.h
index 40c3724bed..bae55435bd 100644
--- a/ssl/statem/statem_local.h
+++ b/ssl/statem/statem_local.h
@@ -129,6 +129,7 @@ __owur int tls_construct_cert_status_body(SSL *s, WPACKET *pkt);
 __owur int tls_construct_cert_status(SSL *s, WPACKET *pkt);
 __owur MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt);
 __owur MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt);
+__owur WORK_STATE tls_post_process_server_certificate(SSL *s, WORK_STATE wst);
 __owur int ssl3_check_cert_and_algorithm(SSL *s);
 #ifndef OPENSSL_NO_NEXTPROTONEG
 __owur int tls_construct_next_proto(SSL *s, WPACKET *pkt);