PSK related tweaks based on review feedback

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3670)

8 files changed, 57 insertions, 52 deletions

diff --git a/apps/s_client.c b/apps/s_client.c
index 60ce9c68af..393b311904 100644
--- a/apps/s_client.c
+++ b/apps/s_client.c
@@ -220,7 +220,6 @@ static int psk_use_session_cb(SSL *s, const EVP_MD *md,
     }
 
     cipher = SSL_SESSION_get0_cipher(usesess);
-
     if (cipher == NULL)
         goto err;
 
@@ -577,8 +576,7 @@ typedef enum OPTION_choice {
     OPT_DEBUG, OPT_TLSEXTDEBUG, OPT_STATUS, OPT_WDEBUG,
     OPT_MSG, OPT_MSGFILE, OPT_ENGINE, OPT_TRACE, OPT_SECURITY_DEBUG,
     OPT_SECURITY_DEBUG_VERBOSE, OPT_SHOWCERTS, OPT_NBIO_TEST, OPT_STATE,
-    OPT_PSK_IDENTITY, OPT_PSK,
-    OPT_PSK_SESS,
+    OPT_PSK_IDENTITY, OPT_PSK, OPT_PSK_SESS,
 #ifndef OPENSSL_NO_SRP
     OPT_SRPUSER, OPT_SRPPASS, OPT_SRP_STRENGTH, OPT_SRP_LATEUSER,
     OPT_SRP_MOREGROUPS,
diff --git a/doc/man3/SSL_CTX_set_psk_client_callback.pod b/doc/man3/SSL_CTX_set_psk_client_callback.pod
index 7e8fffef81..919b6af292 100644
--- a/doc/man3/SSL_CTX_set_psk_client_callback.pod
+++ b/doc/man3/SSL_CTX_set_psk_client_callback.pod
@@ -38,8 +38,8 @@ TLSv1.3 Pre-Shared Keys (PSKs) and PSKs for TLSv1.2 and below are not
 compatible.
 
 A client application wishing to use PSK ciphersuites for TLSv1.2 and below must
-provide a callback function which is called when the client is sending the
-ClientKeyExchange message to the server.
+provide a callback function. This function will be called when the client is
+sending the ClientKeyExchange message to the server.
 
 The purpose of the callback function is to select the PSK identity and
 the pre-shared key to use during the connection setup phase.
@@ -57,7 +57,7 @@ A client application wishing to use TLSv1.3 PSKs must set a different callback
 using either SSL_CTX_set_psk_use_session_callback() or
 SSL_set_psk_use_session_callback() as appropriate.
 
-The callback function is given a reference to the SSL connection in B<ssl>.
+The callback function is given a pointer to the SSL connection in B<ssl>.
 
 The first time the callback is called for a connection the B<md> parameter is
 NULL. In some circumstances the callback will be called a second time. In that
@@ -71,7 +71,7 @@ the PSK in B<*id>. The identifier length in bytes should be stored in B<*idlen>.
 The memory pointed to by B<*id> remains owned by the application and should
 be freed by it as required at any point after the handshake is complete.
 
-Additionally the callback should store a reference to an SSL_SESSION object in
+Additionally the callback should store a pointer to an SSL_SESSION object in
 B<*sess>. This is used as the basis for the PSK, and should, at a minimum, have
 the following fields set:
 
@@ -85,16 +85,16 @@ This can be set via a call to L<SSL_SESSION_set1_master_key(3)>.
 
 Only the handshake digest associated with the ciphersuite is relevant for the
 PSK (the server may go on to negotiate any ciphersuite which is compatible with
-the digest). The application can use any TLSv1.3 ciphersuite. Where B<md> is
-non-NULL the handshake digest for the ciphersuite should be the same.
+the digest). The application can use any TLSv1.3 ciphersuite. If B<md> is
+not NULL the handshake digest for the ciphersuite should be the same.
 The ciphersuite can be set via a call to <SSL_SESSION_set_cipher(3)>. The
 handshake digest of an SSL_CIPHER object can be checked using
 <SSL_CIPHER_get_handshake_digest(3)>.
 
 =item The protocol version
 
-This can be set via a call to L<SSL_SESSION_set_protocol_version> and should be
-TLS1_3_VERSION.
+This can be set via a call to L<SSL_SESSION_set_protocol_version(3)> and should
+be TLS1_3_VERSION.
 
 =back
 
@@ -118,7 +118,7 @@ has occurred so that L<SSL_session_reused(3)> will return true.
 
 =head1 RETURN VALUES
 
-Return values from the SSL_psk_client_cb_func callback are interpreted as
+Return values from the B<SSL_psk_client_cb_func> callback are interpreted as
 follows:
 
 On success (callback found a PSK identity and a pre-shared key to use)
diff --git a/doc/man3/SSL_CTX_use_psk_identity_hint.pod b/doc/man3/SSL_CTX_use_psk_identity_hint.pod
index 9dd14f8e54..4ded544db3 100644
--- a/doc/man3/SSL_CTX_use_psk_identity_hint.pod
+++ b/doc/man3/SSL_CTX_use_psk_identity_hint.pod
@@ -43,8 +43,8 @@ compatible.
 
 Identity hints are not relevant for TLSv1.3. A server application wishing to use
 PSK ciphersuites for TLSv1.2 and below may call SSL_CTX_use_psk_identity_hint()
-to set the given B<NULL>-terminated PSK identity hint B<hint> for SSL context
-object B<ctx>. SSL_use_psk_identity_hint() sets the given B<NULL>-terminated PSK
+to set the given B<NUL>-terminated PSK identity hint B<hint> for SSL context
+object B<ctx>. SSL_use_psk_identity_hint() sets the given B<NUL>-terminated PSK
 identity hint B<hint> for the SSL connection object B<ssl>. If B<hint> is
 B<NULL> the current hint from B<ctx> or B<ssl> is deleted.
 
@@ -57,7 +57,7 @@ client. The purpose of the callback function is to validate the
 received PSK identity and to fetch the pre-shared key used during the
 connection setup phase. The callback is set using the functions
 SSL_CTX_set_psk_server_callback() or SSL_set_psk_server_callback(). The callback
-function is given the connection in parameter B<ssl>, B<NULL>-terminated PSK
+function is given the connection in parameter B<ssl>, B<NUL>-terminated PSK
 identity sent by the client in parameter B<identity>, and a buffer B<psk> of
 length B<max_psk_len> bytes where the pre-shared key is to be stored.
 
@@ -65,7 +65,7 @@ A client application wishing to use TLSv1.3 PSKs must set a different callback
 using either SSL_CTX_set_psk_use_session_callback() or
 SSL_set_psk_use_session_callback() as appropriate.
 
-The callback function is given a reference to the SSL connection in B<ssl> and
+The callback function is given a pointer to the SSL connection in B<ssl> and
 an identity in B<identity> of length B<identity_len>. The callback function
 should identify an SSL_SESSION object that provides the PSK details and store it
 in B<*sess>. The SSL_SESSION object should, as a minimum, set the master key,
@@ -84,7 +84,7 @@ has occurred so that L<SSL_session_reused(3)> will return true.
 
 =head1 RETURN VALUES
 
-SSL_CTX_use_psk_identity_hint() and SSL_use_psk_identity_hint() return
+B<SSL_CTX_use_psk_identity_hint()> and B<SSL_use_psk_identity_hint()> return
 1 on success, 0 otherwise.
 
 Return values from the TLSv1.2 and below server callback are interpreted as
@@ -112,7 +112,7 @@ completely.
 
 =back
 
-The SSL_psk_find_session_cb_func callback should return 1 on success or 0 on
+The B<SSL_psk_find_session_cb_func> callback should return 1 on success or 0 on
 failure. In the event of failure the connection setup fails.
 
 =head1 COPYRIGHT
diff --git a/doc/man3/SSL_get_client_random.pod b/doc/man3/SSL_get_client_random.pod
index 83a1027bca..1e4c66672d 100644
--- a/doc/man3/SSL_get_client_random.pod
+++ b/doc/man3/SSL_get_client_random.pod
@@ -39,10 +39,10 @@ can be dangerous if misused; see NOTES below.
 SSL_SESSION_set1_master_key() sets the master key value associated with the
 SSL_SESSION B<sess>. For example, this could be used to set up a session based
 PSK (see L<SSL_CTX_set_psk_use_session_callback(3)>). The master key of length
-B<len> should be provided at B<in>. A copy of the supplied master key is taken
-by the function, so the caller is responsible for freeing and cleaning any
-memory associated with B<in>. The caller must ensure that the length of the ke
-is suitable for the ciphersuite associated with the SSL_SESSION.
+B<len> should be provided at B<in>. The supplied master key is copied by the
+function, so the caller is responsible for freeing and cleaning any memory
+associated with B<in>. The caller must ensure that the length of the key is
+suitable for the ciphersuite associated with the SSL_SESSION.
 
 =head1 NOTES
 
diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c
index 0afdfdaba1..64bb264b52 100644
--- a/ssl/ssl_ciph.c
+++ b/ssl/ssl_ciph.c
@@ -1933,9 +1933,8 @@ int SSL_CIPHER_get_auth_nid(const SSL_CIPHER *c)
 
 const EVP_MD *SSL_CIPHER_get_handshake_digest(const SSL_CIPHER *c)
 {
-    int idx = c->algorithm2;
+    int idx = c->algorithm2 & SSL_HANDSHAKE_MAC_MASK;
 
-    idx &= SSL_HANDSHAKE_MAC_MASK;
     if (idx < 0 || idx >= SSL_MD_NUM_IDX)
         return NULL;
     return ssl_digest_methods[idx];
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index f9c7b4451b..d8dd45eb5b 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3733,7 +3733,6 @@ int SSL_SESSION_set1_master_key(SSL_SESSION *sess, const unsigned char *in,
 
     memcpy(sess->master_key, in, len);
     sess->master_key_length = len;
-
     return 1;
 }
 
diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index d4af0329f3..846ee30091 100644
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -825,31 +825,35 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
     }
 
     if (s->session->ext.ticklen != 0) {
+        /* Get the digest associated with the ciphersuite in the session */
         if (s->session->cipher == NULL) {
             SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR);
             goto err;
         }
-
         mdres = ssl_md(s->session->cipher->algorithm2);
         if (mdres == NULL) {
-            /* Don't recognize this cipher so we can't use the session. Ignore it */
+            /*
+             * Don't recognize this cipher so we can't use the session.
+             * Ignore it
+             */
             goto dopsksess;
         }
 
         if (s->hello_retry_request && mdres != handmd) {
             /*
-             * Selected ciphersuite hash does not match the hash for the session so
-             * we can't use it.
+             * Selected ciphersuite hash does not match the hash for the session
+             * so we can't use it.
              */
             goto dopsksess;
         }
 
         /*
          * Technically the C standard just says time() returns a time_t and says
-         * nothing about the encoding of that type. In practice most implementations
-         * follow POSIX which holds it as an integral type in seconds since epoch.
-         * We've already made the assumption that we can do this in multiple places
-         * in the code, so portability shouldn't be an issue.
+         * nothing about the encoding of that type. In practice most
+         * implementations follow POSIX which holds it as an integral type in
+         * seconds since epoch. We've already made the assumption that we can do
+         * this in multiple places in the code, so portability shouldn't be an
+         * issue.
          */
         now = (uint32_t)time(NULL);
         agesec = now - (uint32_t)s->session->time;
@@ -867,15 +871,15 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
 
         if (agesec != 0 && agems / (uint32_t)1000 != agesec) {
             /*
-             * Overflow. Shouldn't happen unless this is a *really* old session. If
-             * so we just ignore it.
+             * Overflow. Shouldn't happen unless this is a *really* old session.
+             * If so we just ignore it.
              */
             goto dopsksess;
         }
 
         /*
-         * Obfuscate the age. Overflow here is fine, this addition is supposed to
-         * be mod 2^32.
+         * Obfuscate the age. Overflow here is fine, this addition is supposed
+         * to be mod 2^32.
          */
         agems += s->session->ext.tick_age_add;
 
@@ -956,15 +960,16 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
 
     msgstart = WPACKET_get_curr(pkt) - msglen;
 
-    if (dores && tls_psk_do_binder(s, mdres, msgstart, binderoffset, NULL,
-                                   resbinder, s->session, 1, 0) != 1) {
+    if (dores
+            && tls_psk_do_binder(s, mdres, msgstart, binderoffset, NULL,
+                                 resbinder, s->session, 1, 0) != 1) {
         SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
-    if (psksess != NULL && tls_psk_do_binder(s, mdpsk, msgstart,
-                                                binderoffset, NULL, pskbinder,
-                                                psksess, 1, 1) != 1) {
+    if (psksess != NULL
+            && tls_psk_do_binder(s, mdpsk, msgstart, binderoffset, NULL,
+                                 pskbinder, psksess, 1, 1) != 1) {
         SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_PSK, ERR_R_INTERNAL_ERROR);
         goto err;
     }
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 7a2f9a410e..215035ae24 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -1938,19 +1938,23 @@ static int find_session_cb_cnt = 0;
 static int use_session_cb(SSL *ssl, const EVP_MD *md, const unsigned char **id,
                           size_t *idlen, SSL_SESSION **sess)
 {
-    use_session_cb_cnt++;
-
-    /* The first call should always have a NULL md */
-    if (use_session_cb_cnt == 1 && md != NULL)
-        return 0;
+    switch (++use_session_cb_cnt) {
+    case 1:
+        /* The first call should always have a NULL md */
+        if (md != NULL)
+            return 0;
+        break;
 
-    /* The second call should always have an md */
-    if (use_session_cb_cnt == 2 && md == NULL)
-        return 0;
+    case 2:
+        /* The second call should always have an md */
+        if (md == NULL)
+            return 0;
+        break;
 
-    /* We should only be called a maximum of twice */
-    if (use_session_cb_cnt == 3)
+    default:
+        /* We should only be called a maximum of twice */
         return 0;
+    }
 
     if (psk != NULL)
         SSL_SESSION_up_ref(psk);