Replace handling of negative verification result with SSL_set_retry_verify() Provide a different mechanism to indicate that the application wants to retry the verification. The negative result of the callback function now indicates an error again. Instead the SSL_set_retry_verify() can be called from the callback to indicate that the handshake should be suspended. Fixes #17568 Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from https://github.com/openssl/openssl/pull/17825)
diff --git a/CHANGES.md b/CHANGES.md index c7e3391..eb61749 100644 --- a/CHANGES.md +++ b/CHANGES.md
@@ -123,6 +123,12 @@ *Paul Dale* + * The negative return value handling of the certificate verification callback + was reverted. The replacement is to set the verification retry state with + the SSL_set_retry_verify() function. + + *Tomáš Mráz* + ### Changes between 3.0.0 and 3.0.1 [14 dec 2021] * Fixed invalid handling of X509_verify_cert() internal errors in libssl
diff --git a/doc/build.info b/doc/build.info index 97e6bd3..4e1db23 100644 --- a/doc/build.info +++ b/doc/build.info
@@ -2547,6 +2547,10 @@ GENERATE[html/man3/SSL_set_fd.html]=man3/SSL_set_fd.pod DEPEND[man/man3/SSL_set_fd.3]=man3/SSL_set_fd.pod GENERATE[man/man3/SSL_set_fd.3]=man3/SSL_set_fd.pod +DEPEND[html/man3/SSL_set_retry_verify.html]=man3/SSL_set_retry_verify.pod +GENERATE[html/man3/SSL_set_retry_verify.html]=man3/SSL_set_retry_verify.pod +DEPEND[man/man3/SSL_set_retry_verify.3]=man3/SSL_set_retry_verify.pod +GENERATE[man/man3/SSL_set_retry_verify.3]=man3/SSL_set_retry_verify.pod DEPEND[html/man3/SSL_set_session.html]=man3/SSL_set_session.pod GENERATE[html/man3/SSL_set_session.html]=man3/SSL_set_session.pod DEPEND[man/man3/SSL_set_session.3]=man3/SSL_set_session.pod @@ -3356,6 +3360,7 @@ html/man3/SSL_set_bio.html \ html/man3/SSL_set_connect_state.html \ html/man3/SSL_set_fd.html \ +html/man3/SSL_set_retry_verify.html \ html/man3/SSL_set_session.html \ html/man3/SSL_set_shutdown.html \ html/man3/SSL_set_verify_result.html \ @@ -3948,6 +3953,7 @@ man/man3/SSL_set_bio.3 \ man/man3/SSL_set_connect_state.3 \ man/man3/SSL_set_fd.3 \ +man/man3/SSL_set_retry_verify.3 \ man/man3/SSL_set_session.3 \ man/man3/SSL_set_shutdown.3 \ man/man3/SSL_set_verify_result.3 \
diff --git a/doc/man3/SSL_CTX_set_cert_verify_callback.pod b/doc/man3/SSL_CTX_set_cert_verify_callback.pod index fdeeaee..ecd312c 100644 --- a/doc/man3/SSL_CTX_set_cert_verify_callback.pod +++ b/doc/man3/SSL_CTX_set_cert_verify_callback.pod
@@ -36,16 +36,18 @@ In client mode, the behaviour is as follows. All values, including 0, are ignored if the verification mode is B<SSL_VERIFY_NONE>. -Otherwise, when the return value is 0, the handshake will fail. +Otherwise, when the return value is less than or equal to 0, the handshake will +fail. -In client mode I<callback> may also return -1, -typically on failure verifying the server certificate. -This makes the handshake suspend and return control to the calling application -with B<SSL_ERROR_WANT_RETRY_VERIFY>. -The app can for instance fetch further certificates or cert status information -needed for the verification. -Calling L<SSL_connect(3)> again resumes the connection attempt -by retrying the server certificate verification step. +In client mode I<callback> may also call the L<SSL_set_retry_verify(3)> +function on the B<SSL> object set in the I<x509_store_ctx> ex data (see +L<SSL_get_ex_data_X509_STORE_CTX_idx(3)>) and return 1. This would be +typically done in case the certificate verification was not yet able +to succeed. This makes the handshake suspend and return control to the +calling application with B<SSL_ERROR_WANT_RETRY_VERIFY>. The app can for +instance fetch further certificates or cert status information needed for +the verification. Calling L<SSL_connect(3)> again resumes the connection +attempt by retrying the server certificate verification step. This process may even be repeated if need be. In any case a viable verification result value must be reflected @@ -89,6 +91,7 @@ L<ssl(7)>, L<SSL_CTX_set_verify(3)>, L<X509_STORE_CTX_set_error(3)>, L<SSL_get_verify_result(3)>, +L<SSL_set_retry_verify(3)>, L<SSL_CTX_load_verify_locations(3)> =head1 COPYRIGHT
diff --git a/doc/man3/SSL_CTX_set_verify.pod b/doc/man3/SSL_CTX_set_verify.pod index e3271af..c085097 100644 --- a/doc/man3/SSL_CTX_set_verify.pod +++ b/doc/man3/SSL_CTX_set_verify.pod
@@ -44,6 +44,21 @@ B<SSL_get_ex_data_X509_STORE_CTX_idx> can be called to get the data index of the current SSL object that is doing the verification. +In client mode B<verify_callback> may also call the L<SSL_set_retry_verify(3)> +function on the B<SSL> object set in the I<x509_store_ctx> ex data (see +L<SSL_get_ex_data_X509_STORE_CTX_idx(3)>) and return 1. +This would be typically done in case the certificate verification was not yet +able to succeed. +This makes the handshake suspend and return control to the calling application +with B<SSL_ERROR_WANT_RETRY_VERIFY>. +The application can for instance fetch further certificates or cert status +information needed for the verification. +Note that the handshake may still be aborted if a subsequent invocation of the +callback (e.g. at a lower depth, or for a separate error condition) returns 0. +Calling L<SSL_connect(3)> again resumes the connection attempt by retrying the +server certificate verification step. +This process may even be repeated if need be. + SSL_CTX_set_verify_depth() sets the maximum B<depth> for the certificate chain verification that shall be allowed for B<ctx>.
diff --git a/doc/man3/SSL_set_retry_verify.pod b/doc/man3/SSL_set_retry_verify.pod new file mode 100644 index 0000000..4eb7f44 --- /dev/null +++ b/doc/man3/SSL_set_retry_verify.pod
@@ -0,0 +1,70 @@ +=pod + +=head1 NAME + +SSL_set_retry_verify - indicate that certificate verification should be retried + +=head1 SYNOPSIS + + #include <openssl/ssl.h> + + int SSL_set_retry_verify(SSL *ssl); + +=head1 DESCRIPTION + +SSL_set_retry_verify() should be called from the certificate verification +callback on a client when the application wants to indicate that the handshake +should be suspended and the control should be returned to the application. +L<SSL_want_retry_verify(3)> will return 1 as a consequence until the handshake +is resumed again by the application, retrying the verification step. + +Please refer to L<SSL_CTX_set_cert_verify_callback(3)> for further details. + +=head1 NOTES + +The effect of calling SSL_set_retry_verify() outside of the certificate +verification callback on the client side is undefined. + +=head1 RETURN VALUES + +SSL_set_retry verify() returns 1 on success, 0 otherwise. + +=head1 EXAMPLES + +The following code snippet shows how to obtain the B<SSL> object associated +with the B<X509_STORE_CTX> to call the SSL_set_retry_verify() function: + + int idx = SSL_get_ex_data_X509_STORE_CTX_idx(); + SSL *ssl; + + /* this should not happen but check anyway */ + if (idx < 0 + || (ssl = X509_STORE_CTX_get_ex_data(ctx, idx)) == NULL) + return 0; + + if (/* we need to retry verification callback */) + return SSL_set_retry_verify(ssl); + + /* do normal processing of the verification callback */ + +=head1 SEE ALSO + +L<ssl(7)>, L<SSL_connect(3)>, L<SSL_CTX_set_cert_verify_callback(3)>, +L<SSL_want_retry_verify(3)> + +=head1 HISTORY + +SSL_set_retry_verify() was added in OpenSSL 3.0.2 to replace backwards +incompatible handling of a negative return value from the verification +callback. + +=head1 COPYRIGHT + +Copyright 2022 The OpenSSL Project Authors. All Rights Reserved. + +Licensed under the Apache License 2.0 (the "License"). You may not use +this file except in compliance with the License. You can obtain a copy +in the file LICENSE in the source distribution or at +L<https://www.openssl.org/source/license.html>. + +=cut
diff --git a/doc/man3/SSL_want.pod b/doc/man3/SSL_want.pod index 831094a..2292013 100644 --- a/doc/man3/SSL_want.pod +++ b/doc/man3/SSL_want.pod
@@ -70,8 +70,8 @@ =item SSL_RETRY_VERIFY -The operation did not complete because an application callback set by -SSL_CTX_set_cert_verify_callback() has asked to be called again. +The operation did not complete because a certificate verification callback +has asked to be called again via L<SSL_set_retry_verify(3)>. A call to L<SSL_get_error(3)> should return B<SSL_ERROR_WANT_RETRY_VERIFY>. =item SSL_ASYNC_PAUSED
diff --git a/doc/man3/X509_verify_cert.pod b/doc/man3/X509_verify_cert.pod index a14a0b2..6172ea5 100644 --- a/doc/man3/X509_verify_cert.pod +++ b/doc/man3/X509_verify_cert.pod
@@ -52,9 +52,7 @@ incorrectly, such as with no certificate set in I<ctx>, or when it is called twice in succession without reinitialising I<ctx> for the second call. A negative return value can also happen due to internal resource problems -or because an internal inconsistency has been detected -or if a retry operation is requested during internal lookups -(which never happens with standard lookup methods). +or because an internal inconsistency has been detected. Applications must interpret any return value <= 0 as an error. The X509_STORE_CTX_verify() behaves like X509_verify_cert() except that its
diff --git a/include/openssl/ssl.h.in b/include/openssl/ssl.h.in index 47f2779..b496dbc 100644 --- a/include/openssl/ssl.h.in +++ b/include/openssl/ssl.h.in
@@ -1309,6 +1309,7 @@ # define SSL_CTRL_GET_TMP_KEY 133 # define SSL_CTRL_GET_NEGOTIATED_GROUP 134 # define SSL_CTRL_GET_IANA_GROUPS 135 +# define SSL_CTRL_SET_RETRY_VERIFY 136 # define SSL_CERT_SET_FIRST 1 # define SSL_CERT_SET_NEXT 2 # define SSL_CERT_SET_SERVER 3 @@ -2135,6 +2136,8 @@ SSL_CTX_ctrl(ctx,SSL_CTRL_SET_MAX_PIPELINES,m,NULL) # define SSL_set_max_pipelines(ssl,m) \ SSL_ctrl(ssl,SSL_CTRL_SET_MAX_PIPELINES,m,NULL) +# define SSL_set_retry_verify(ssl) \ + (SSL_ctrl(ssl,SSL_CTRL_SET_RETRY_VERIFY,0,NULL) > 0) void SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len); void SSL_set_default_read_buffer_len(SSL *s, size_t len);
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 9138cd6..ab56e66 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c
@@ -2393,6 +2393,9 @@ return 1; case SSL_CTRL_GET_RI_SUPPORT: return s->s3.send_connection_binding; + case SSL_CTRL_SET_RETRY_VERIFY: + s->rwstate = SSL_RETRY_VERIFY; + return 1; case SSL_CTRL_CERT_FLAGS: return (s->cert->cert_flags |= larg); case SSL_CTRL_CLEAR_CERT_FLAGS:
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 2b0bfc7..1c48894 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c
@@ -1859,9 +1859,10 @@ size_t certidx; int i; + if (s->rwstate == SSL_RETRY_VERIFY) + s->rwstate = SSL_NOTHING; i = ssl_verify_cert_chain(s, s->session->peer_chain); - if (i == -1) { - s->rwstate = SSL_RETRY_VERIFY; + if (i > 0 && s->rwstate == SSL_RETRY_VERIFY) { return WORK_MORE_A; } /* @@ -1878,7 +1879,7 @@ * (less clean) historic behaviour of performing validation if any flag is * set. The *documented* interface remains the same. */ - if (s->verify_mode != SSL_VERIFY_NONE && i == 0) { + if (s->verify_mode != SSL_VERIFY_NONE && i <= 0) { SSLfatal(s, ssl_x509err2alert(s->verify_result), SSL_R_CERTIFICATE_VERIFY_FAILED); return WORK_ERROR;
diff --git a/test/helpers/handshake.c b/test/helpers/handshake.c index cee11bb..0e50301 100644 --- a/test/helpers/handshake.c +++ b/test/helpers/handshake.c
@@ -305,10 +305,18 @@ static int n_retries = 0; static int verify_retry_cb(X509_STORE_CTX *ctx, void *arg) { + int idx = SSL_get_ex_data_X509_STORE_CTX_idx(); + SSL *ssl; + + /* this should not happen but check anyway */ + if (idx < 0 + || (ssl = X509_STORE_CTX_get_ex_data(ctx, idx)) == NULL) + return 0; + if (--n_retries < 0) return 1; - X509_STORE_CTX_set_error(ctx, X509_V_ERR_APPLICATION_VERIFICATION); - return -1; + + return SSL_set_retry_verify(ssl); } static int verify_accept_cb(X509_STORE_CTX *ctx, void *arg) {
diff --git a/test/sslapitest.c b/test/sslapitest.c index 0c6a5e1..437a9c8 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c
@@ -557,10 +557,19 @@ static int verify_retry_cb(X509_STORE_CTX *ctx, void *arg) { int res = X509_verify_cert(ctx); + int idx = SSL_get_ex_data_X509_STORE_CTX_idx(); + SSL *ssl; + + /* this should not happen but check anyway */ + if (idx < 0 + || (ssl = X509_STORE_CTX_get_ex_data(ctx, idx)) == NULL) + return 0; if (res == 0 && X509_STORE_CTX_get_error(ctx) == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY) - return -1; /* indicate SSL_ERROR_WANT_RETRY_VERIFY */ + /* indicate SSL_ERROR_WANT_RETRY_VERIFY */ + return SSL_set_retry_verify(ssl); + return res; }
diff --git a/util/other.syms b/util/other.syms index 8276681..3eaafee 100644 --- a/util/other.syms +++ b/util/other.syms
@@ -677,3 +677,4 @@ EVP_PKEY_size define EVP_PKEY_id define EVP_PKEY_base_id define +SSL_set_retry_verify define