Add error checking to tls_session_secret_cb() calls

Failure of this undocumented callback was previously silently ignored.
Follow OpenSSL's behavior and throw an internal error (for lack of a
better choice) if the callback failed or if it set the master_key_length
to a negative number.

Unindent the success path and clean up some strange idioms.

ok jsing

diff --git a/lib/libssl/ssl_clnt.c b/lib/libssl/ssl_clnt.c
index f5b8802..6f93b55 100644 (file)
--- a/lib/libssl/ssl_clnt.c
+++ b/lib/libssl/ssl_clnt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_clnt.c,v 1.142 2022/06/06 13:18:34 tb Exp $ */
+/* $OpenBSD: ssl_clnt.c,v 1.143 2022/06/07 17:14:17 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -923,16 +923,26 @@ ssl3_get_server_hello(SSL *s)
         * Check if we want to resume the session based on external
         * pre-shared secret.
         */
-       if (s->internal->tls_session_secret_cb) {
+       if (s->internal->tls_session_secret_cb != NULL) {
                SSL_CIPHER *pref_cipher = NULL;
-               s->session->master_key_length = sizeof(s->session->master_key);
-               if (s->internal->tls_session_secret_cb(s, s->session->master_key,
-                   &s->session->master_key_length, NULL, &pref_cipher,
-                   s->internal->tls_session_secret_cb_arg)) {
-                       s->session->cipher = pref_cipher ? pref_cipher :
-                           ssl3_get_cipher_by_value(cipher_suite);
-                       s->s3->flags |= SSL3_FLAGS_CCS_OK;
+               int master_key_length = sizeof(s->session->master_key);
+
+               if (!s->internal->tls_session_secret_cb(s,
+                   s->session->master_key, &master_key_length, NULL,
+                   &pref_cipher, s->internal->tls_session_secret_cb_arg)) {
+                       SSLerror(s, ERR_R_INTERNAL_ERROR);
+                       goto err;
+               }
+               if (master_key_length <= 0) {
+                       SSLerror(s, ERR_R_INTERNAL_ERROR);
+                       goto err;
                }
+               s->session->master_key_length = master_key_length;
+
+               if ((s->session->cipher = pref_cipher) == NULL)
+                       s->session->cipher =
+                           ssl3_get_cipher_by_value(cipher_suite);
+               s->s3->flags |= SSL3_FLAGS_CCS_OK;
        }
 
        if (s->session->session_id_length != 0 &&

diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c
index 3593950..35f3d58 100644 (file)
--- a/lib/libssl/ssl_srvr.c
+++ b/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.141 2022/02/05 14:54:10 jsing Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.142 2022/06/07 17:14:17 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1055,34 +1055,41 @@ ssl3_get_client_hello(SSL *s)
                }
        }
 
-       if (!s->internal->hit && s->internal->tls_session_secret_cb) {
+       if (!s->internal->hit && s->internal->tls_session_secret_cb != NULL) {
                SSL_CIPHER *pref_cipher = NULL;
+               int master_key_length = sizeof(s->session->master_key);
 
-               s->session->master_key_length = sizeof(s->session->master_key);
-               if (s->internal->tls_session_secret_cb(s, s->session->master_key,
-                   &s->session->master_key_length, ciphers, &pref_cipher,
-                   s->internal->tls_session_secret_cb_arg)) {
-                       s->internal->hit = 1;
-                       s->session->ciphers = ciphers;
-                       s->session->verify_result = X509_V_OK;
+               if (!s->internal->tls_session_secret_cb(s,
+                   s->session->master_key, &master_key_length, ciphers,
+                   &pref_cipher, s->internal->tls_session_secret_cb_arg)) {
+                       SSLerror(s, ERR_R_INTERNAL_ERROR);
+                       goto err;
+               }
+               if (master_key_length <= 0) {
+                       SSLerror(s, ERR_R_INTERNAL_ERROR);
+                       goto err;
+               }
+               s->session->master_key_length = master_key_length;
 
-                       ciphers = NULL;
+               s->internal->hit = 1;
+               s->session->verify_result = X509_V_OK;
 
-                       /* check if some cipher was preferred by call back */
-                       pref_cipher = pref_cipher ? pref_cipher :
-                           ssl3_choose_cipher(s, s->session->ciphers,
-                           SSL_get_ciphers(s));
-                       if (pref_cipher == NULL) {
-                               al = SSL_AD_HANDSHAKE_FAILURE;
-                               SSLerror(s, SSL_R_NO_SHARED_CIPHER);
-                               goto fatal_err;
-                       }
-
-                       s->session->cipher = pref_cipher;
+               s->session->ciphers = ciphers;
+               ciphers = NULL;
 
-                       sk_SSL_CIPHER_free(s->cipher_list);
-                       s->cipher_list = sk_SSL_CIPHER_dup(s->session->ciphers);
+               /* Check if some cipher was preferred by the callback. */
+               if (pref_cipher == NULL)
+                       pref_cipher = ssl3_choose_cipher(s, s->session->ciphers,
+                           SSL_get_ciphers(s));
+               if (pref_cipher == NULL) {
+                       al = SSL_AD_HANDSHAKE_FAILURE;
+                       SSLerror(s, SSL_R_NO_SHARED_CIPHER);
+                       goto fatal_err;
                }
+               s->session->cipher = pref_cipher;
+
+               sk_SSL_CIPHER_free(s->cipher_list);
+               s->cipher_list = sk_SSL_CIPHER_dup(s->session->ciphers);
        }
 
        /*