From: Matt Caswell Date: Wed, 21 Jun 2017 11:17:30 +0000 (+0100) Subject: PSK related tweaks based on review feedback X-Git-Tag: OpenSSL_1_1_1-pre1~1229 X-Git-Url: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff_plain;h=72257204bd2a88773461150765dfd0e0a428ee86 PSK related tweaks based on review feedback Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3670) --- 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. +The callback function is given a pointer to the SSL connection in B. The first time the callback is called for a connection the B 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. 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 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 is +not NULL the handshake digest for the ciphersuite should be the same. The ciphersuite can be set via a call to . The handshake digest of an SSL_CIPHER object can be checked using . =item The protocol version -This can be set via a call to L and should be -TLS1_3_VERSION. +This can be set via a call to L and should +be TLS1_3_VERSION. =back @@ -118,7 +118,7 @@ has occurred so that L will return true. =head1 RETURN VALUES -Return values from the SSL_psk_client_cb_func callback are interpreted as +Return values from the B 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-terminated PSK identity hint B for SSL context -object B. SSL_use_psk_identity_hint() sets the given B-terminated PSK +to set the given B-terminated PSK identity hint B for SSL context +object B. SSL_use_psk_identity_hint() sets the given B-terminated PSK identity hint B for the SSL connection object B. If B is B the current hint from B or B 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, B-terminated PSK +function is given the connection in parameter B, B-terminated PSK identity sent by the client in parameter B, and a buffer B of length B 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 and +The callback function is given a pointer to the SSL connection in B and an identity in B of length B. 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 will return true. =head1 RETURN VALUES -SSL_CTX_use_psk_identity_hint() and SSL_use_psk_identity_hint() return +B and B 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 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. For example, this could be used to set up a session based PSK (see L). The master key of length -B should be provided at B. 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. The caller must ensure that the length of the ke -is suitable for the ciphersuite associated with the SSL_SESSION. +B should be provided at B. The supplied master key is copied by the +function, so the caller is responsible for freeing and cleaning any memory +associated with B. 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);