From: tb <tb@openbsd.org>
Date: Tue, 7 Jun 2022 17:14:17 +0000 (+0000)
Subject: Add error checking to tls_session_secret_cb() calls
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=573b2ffa241b22968e4f09c84acd0cd50905d4c0;p=openbsd

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 f5b8802a69e..6f93b55ddce 100644
--- 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 359395051a8..35f3d585ace 100644
--- 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);
 	}
 
 	/*