From 5ed5e90ec48e700a9bc17eb264e62c05e5ccbc15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 30 Apr 2019 11:41:40 +0200 Subject: [PATCH 01/21] Start splitting populate_transform() out of derive_keys() This is currently a dummy, just introducing the new name. --- library/ssl_tls.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 1ae28f760..5c4d30427 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -607,7 +607,13 @@ static void ssl_calc_finished_tls_sha384( mbedtls_ssl_context *, unsigned char * #endif #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ -int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) +/* + * This function will ultimetaly only be responsible for populating a + * transform structure from data passed as explicit parameters. + * + * For now however it's doing rather more in a rather less explicit way. + */ +static int ssl_populate_transform( mbedtls_ssl_context *ssl ) { int ret = 0; unsigned char tmp[64]; @@ -1142,6 +1148,11 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) return( 0 ); } +int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) +{ + return( ssl_populate_transform( ssl ) ); +} + #if defined(MBEDTLS_SSL_PROTO_SSL3) void ssl_calc_verify_ssl( mbedtls_ssl_context *ssl, unsigned char hash[36] ) { From 52aa520c9631ad223519e7fde5a1e5628b1a643a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 30 Apr 2019 11:54:22 +0200 Subject: [PATCH 02/21] Start extraction ssl_set_handshake_prfs() For now just moving code around, will improve signature in the next commit. --- library/ssl_tls.c | 110 ++++++++++++++++++++++++++-------------------- 1 file changed, 63 insertions(+), 47 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5c4d30427..031b5f58c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -657,53 +657,6 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } - /* - * Set appropriate PRF function and other SSL / TLS / TLS1.2 functions - */ -#if defined(MBEDTLS_SSL_PROTO_SSL3) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) - { - handshake->tls_prf = ssl3_prf; - handshake->calc_verify = ssl_calc_verify_ssl; - handshake->calc_finished = ssl_calc_finished_ssl; - } - else -#endif -#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) - if( ssl->minor_ver < MBEDTLS_SSL_MINOR_VERSION_3 ) - { - handshake->tls_prf = tls1_prf; - handshake->calc_verify = ssl_calc_verify_tls; - handshake->calc_finished = ssl_calc_finished_tls; - } - else -#endif -#if defined(MBEDTLS_SSL_PROTO_TLS1_2) -#if defined(MBEDTLS_SHA512_C) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 && - ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) - { - handshake->tls_prf = tls_prf_sha384; - handshake->calc_verify = ssl_calc_verify_tls_sha384; - handshake->calc_finished = ssl_calc_finished_tls_sha384; - } - else -#endif -#if defined(MBEDTLS_SHA256_C) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) - { - handshake->tls_prf = tls_prf_sha256; - handshake->calc_verify = ssl_calc_verify_tls_sha256; - handshake->calc_finished = ssl_calc_finished_tls_sha256; - } - else -#endif -#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - } - /* * SSLv3: * master = @@ -1148,8 +1101,71 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) return( 0 ); } +static int ssl_set_handshake_prfs( mbedtls_ssl_context *ssl ) +{ + mbedtls_ssl_handshake_params *handshake = ssl->handshake; + const mbedtls_ssl_ciphersuite_t *ciphersuite_info; + ciphersuite_info = handshake->ciphersuite_info; + + /* + * Set appropriate PRF function and other SSL / TLS / TLS1.2 functions + */ +#if defined(MBEDTLS_SSL_PROTO_SSL3) + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) + { + handshake->tls_prf = ssl3_prf; + handshake->calc_verify = ssl_calc_verify_ssl; + handshake->calc_finished = ssl_calc_finished_ssl; + } + else +#endif +#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) + if( ssl->minor_ver < MBEDTLS_SSL_MINOR_VERSION_3 ) + { + handshake->tls_prf = tls1_prf; + handshake->calc_verify = ssl_calc_verify_tls; + handshake->calc_finished = ssl_calc_finished_tls; + } + else +#endif +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) +#if defined(MBEDTLS_SHA512_C) + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 && + ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) + { + handshake->tls_prf = tls_prf_sha384; + handshake->calc_verify = ssl_calc_verify_tls_sha384; + handshake->calc_finished = ssl_calc_finished_tls_sha384; + } + else +#endif +#if defined(MBEDTLS_SHA256_C) + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) + { + handshake->tls_prf = tls_prf_sha256; + handshake->calc_verify = ssl_calc_verify_tls_sha256; + handshake->calc_finished = ssl_calc_finished_tls_sha256; + } + else +#endif +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + return( 0 ); +} + + int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) { + int ret; + + ret = ssl_set_handshake_prfs( ssl ); + if( ret != 0 ) + return( ret ); + return( ssl_populate_transform( ssl ) ); } From aa3c7011934f19f5dfdf9995f9a9dfa30d92decb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 30 Apr 2019 12:08:59 +0200 Subject: [PATCH 03/21] Fix signature of ssl_set_transform_prfs() --- library/ssl_tls.c | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 031b5f58c..47f375137 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1101,17 +1101,26 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) return( 0 ); } -static int ssl_set_handshake_prfs( mbedtls_ssl_context *ssl ) +/* + * Set appropriate PRF function and other SSL / TLS / TLS1.2 functions + * + * Inputs: + * - SSL/TLS minor version + * - hash associated with the ciphersuite (only used by TLS 1.2) + * + * Ouputs: + * - the tls_prf, calc_verify and calc_finished members of handshake structure + */ +static int ssl_set_handshake_prfs( mbedtls_ssl_handshake_params *handshake, + int minor_ver, + mbedtls_md_type_t hash ) { - mbedtls_ssl_handshake_params *handshake = ssl->handshake; - const mbedtls_ssl_ciphersuite_t *ciphersuite_info; - ciphersuite_info = handshake->ciphersuite_info; +#if !defined(MBEDTLS_SSL_PROTO_TLS1_2) || !defined(MBEDTLS_SHA512_C) + (void) hash; +#endif - /* - * Set appropriate PRF function and other SSL / TLS / TLS1.2 functions - */ #if defined(MBEDTLS_SSL_PROTO_SSL3) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) + if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) { handshake->tls_prf = ssl3_prf; handshake->calc_verify = ssl_calc_verify_ssl; @@ -1120,7 +1129,7 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_context *ssl ) else #endif #if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) - if( ssl->minor_ver < MBEDTLS_SSL_MINOR_VERSION_3 ) + if( minor_ver < MBEDTLS_SSL_MINOR_VERSION_3 ) { handshake->tls_prf = tls1_prf; handshake->calc_verify = ssl_calc_verify_tls; @@ -1130,8 +1139,8 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_context *ssl ) #endif #if defined(MBEDTLS_SSL_PROTO_TLS1_2) #if defined(MBEDTLS_SHA512_C) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 && - ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) + if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 && + hash == MBEDTLS_MD_SHA384 ) { handshake->tls_prf = tls_prf_sha384; handshake->calc_verify = ssl_calc_verify_tls_sha384; @@ -1140,7 +1149,7 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_context *ssl ) else #endif #if defined(MBEDTLS_SHA256_C) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) + if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) { handshake->tls_prf = tls_prf_sha256; handshake->calc_verify = ssl_calc_verify_tls_sha256; @@ -1150,7 +1159,6 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_context *ssl ) #endif #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } @@ -1161,10 +1169,17 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_context *ssl ) int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) { int ret; + const mbedtls_ssl_ciphersuite_t * const ciphersuite_info = + ssl->handshake->ciphersuite_info; - ret = ssl_set_handshake_prfs( ssl ); + ret = ssl_set_handshake_prfs( ssl->handshake, + ssl->minor_ver, + ciphersuite_info->mac ); if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "ssl_set_handshake_prfs", ret ); return( ret ); + } return( ssl_populate_transform( ssl ) ); } From 7edd5876ceae4dc095c96bee029d99c597cf1728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 3 May 2019 09:05:41 +0200 Subject: [PATCH 04/21] Start extracting ssl_compute_master() For now just moving code around, not changing indentation. Calling convention and signature are going to be adjusted in upcoming commits. --- library/ssl_tls.c | 163 ++++++++++++++++++++++++++-------------------- 1 file changed, 91 insertions(+), 72 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 47f375137..87e3c7b96 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -657,78 +657,6 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } - /* - * SSLv3: - * master = - * MD5( premaster + SHA1( 'A' + premaster + randbytes ) ) + - * MD5( premaster + SHA1( 'BB' + premaster + randbytes ) ) + - * MD5( premaster + SHA1( 'CCC' + premaster + randbytes ) ) - * - * TLSv1+: - * master = PRF( premaster, "master secret", randbytes )[0..47] - */ - if( handshake->resume == 0 ) - { - MBEDTLS_SSL_DEBUG_BUF( 3, "premaster secret", handshake->premaster, - handshake->pmslen ); - -#if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - if( ssl->handshake->extended_ms == MBEDTLS_SSL_EXTENDED_MS_ENABLED ) - { - unsigned char session_hash[48]; - size_t hash_len; - - MBEDTLS_SSL_DEBUG_MSG( 3, ( "using extended master secret" ) ); - - ssl->handshake->calc_verify( ssl, session_hash ); - -#if defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) - { -#if defined(MBEDTLS_SHA512_C) - if( ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) - { - hash_len = 48; - } - else -#endif - hash_len = 32; - } - else -#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ - hash_len = 36; - - MBEDTLS_SSL_DEBUG_BUF( 3, "session hash", session_hash, hash_len ); - - ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, - "extended master secret", - session_hash, hash_len, - session->master, 48 ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); - return( ret ); - } - - } - else -#endif - ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, - "master secret", - handshake->randbytes, 64, - session->master, 48 ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); - return( ret ); - } - - mbedtls_platform_zeroize( handshake->premaster, - sizeof(handshake->premaster) ); - } - else - MBEDTLS_SSL_DEBUG_MSG( 3, ( "no premaster (session resumed)" ) ); - /* * Swap the client and server random values. */ @@ -1165,6 +1093,90 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_handshake_params *handshake, return( 0 ); } +/* + * Compute master secret + */ +static int ssl_compute_master( mbedtls_ssl_context *ssl ) +{ + int ret; + mbedtls_ssl_handshake_params *handshake = ssl->handshake; + const mbedtls_ssl_ciphersuite_t *ciphersuite_info = handshake->ciphersuite_info; + mbedtls_ssl_session *session = ssl->session_negotiate; + + /* + * SSLv3: + * master = + * MD5( premaster + SHA1( 'A' + premaster + randbytes ) ) + + * MD5( premaster + SHA1( 'BB' + premaster + randbytes ) ) + + * MD5( premaster + SHA1( 'CCC' + premaster + randbytes ) ) + * + * TLSv1+: + * master = PRF( premaster, "master secret", randbytes )[0..47] + */ + if( handshake->resume == 0 ) + { + MBEDTLS_SSL_DEBUG_BUF( 3, "premaster secret", handshake->premaster, + handshake->pmslen ); + +#if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) + if( ssl->handshake->extended_ms == MBEDTLS_SSL_EXTENDED_MS_ENABLED ) + { + unsigned char session_hash[48]; + size_t hash_len; + + MBEDTLS_SSL_DEBUG_MSG( 3, ( "using extended master secret" ) ); + + ssl->handshake->calc_verify( ssl, session_hash ); + +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) + { +#if defined(MBEDTLS_SHA512_C) + if( ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) + { + hash_len = 48; + } + else +#endif + hash_len = 32; + } + else +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ + hash_len = 36; + + MBEDTLS_SSL_DEBUG_BUF( 3, "session hash", session_hash, hash_len ); + + ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, + "extended master secret", + session_hash, hash_len, + session->master, 48 ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); + return( ret ); + } + + } + else +#endif + ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, + "master secret", + handshake->randbytes, 64, + session->master, 48 ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); + return( ret ); + } + + mbedtls_platform_zeroize( handshake->premaster, + sizeof(handshake->premaster) ); + } + else + MBEDTLS_SSL_DEBUG_MSG( 3, ( "no premaster (session resumed)" ) ); + + return( 0 ); +} int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) { @@ -1181,6 +1193,13 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) return( ret ); } + ret = ssl_compute_master( ssl ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "ssl_compute_master", ret ); + return( ret ); + } + return( ssl_populate_transform( ssl ) ); } From dafe5227d4449bfd898c12b713ffb802686c65ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 3 May 2019 09:16:16 +0200 Subject: [PATCH 05/21] Reduce indentation in ssl_compute_master() Exit earlier when there's noting to do. For a small diff, review with 'git show -w'. --- library/ssl_tls.c | 91 +++++++++++++++++++++-------------------------- 1 file changed, 41 insertions(+), 50 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 87e3c7b96..bdfb34d64 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1094,7 +1094,7 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_handshake_params *handshake, } /* - * Compute master secret + * Compute master secret if needed */ static int ssl_compute_master( mbedtls_ssl_context *ssl ) { @@ -1103,65 +1103,46 @@ static int ssl_compute_master( mbedtls_ssl_context *ssl ) const mbedtls_ssl_ciphersuite_t *ciphersuite_info = handshake->ciphersuite_info; mbedtls_ssl_session *session = ssl->session_negotiate; - /* - * SSLv3: - * master = - * MD5( premaster + SHA1( 'A' + premaster + randbytes ) ) + - * MD5( premaster + SHA1( 'BB' + premaster + randbytes ) ) + - * MD5( premaster + SHA1( 'CCC' + premaster + randbytes ) ) - * - * TLSv1+: - * master = PRF( premaster, "master secret", randbytes )[0..47] - */ - if( handshake->resume == 0 ) + if( handshake->resume != 0 ) { - MBEDTLS_SSL_DEBUG_BUF( 3, "premaster secret", handshake->premaster, - handshake->pmslen ); + MBEDTLS_SSL_DEBUG_MSG( 3, ( "no premaster (session resumed)" ) ); + return( 0 ); + } + + MBEDTLS_SSL_DEBUG_BUF( 3, "premaster secret", handshake->premaster, + handshake->pmslen ); #if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - if( ssl->handshake->extended_ms == MBEDTLS_SSL_EXTENDED_MS_ENABLED ) - { - unsigned char session_hash[48]; - size_t hash_len; + if( ssl->handshake->extended_ms == MBEDTLS_SSL_EXTENDED_MS_ENABLED ) + { + unsigned char session_hash[48]; + size_t hash_len; - MBEDTLS_SSL_DEBUG_MSG( 3, ( "using extended master secret" ) ); + MBEDTLS_SSL_DEBUG_MSG( 3, ( "using extended master secret" ) ); - ssl->handshake->calc_verify( ssl, session_hash ); + ssl->handshake->calc_verify( ssl, session_hash ); #if defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) - { + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) + { #if defined(MBEDTLS_SHA512_C) - if( ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) - { - hash_len = 48; - } - else -#endif - hash_len = 32; + if( ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) + { + hash_len = 48; } else -#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ - hash_len = 36; - - MBEDTLS_SSL_DEBUG_BUF( 3, "session hash", session_hash, hash_len ); - - ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, - "extended master secret", - session_hash, hash_len, - session->master, 48 ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); - return( ret ); - } - +#endif + hash_len = 32; } else -#endif +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ + hash_len = 36; + + MBEDTLS_SSL_DEBUG_BUF( 3, "session hash", session_hash, hash_len ); + ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, - "master secret", - handshake->randbytes, 64, + "extended master secret", + session_hash, hash_len, session->master, 48 ); if( ret != 0 ) { @@ -1169,11 +1150,21 @@ static int ssl_compute_master( mbedtls_ssl_context *ssl ) return( ret ); } - mbedtls_platform_zeroize( handshake->premaster, - sizeof(handshake->premaster) ); } else - MBEDTLS_SSL_DEBUG_MSG( 3, ( "no premaster (session resumed)" ) ); +#endif + ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, + "master secret", + handshake->randbytes, 64, + session->master, 48 ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); + return( ret ); + } + + mbedtls_platform_zeroize( handshake->premaster, + sizeof(handshake->premaster) ); return( 0 ); } From c28c8895e5840980bae4f4530d678a3550175325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 3 May 2019 09:46:14 +0200 Subject: [PATCH 06/21] Improve signature of ssl_compute_master() Make it more explicit what's used. Unfortunately, we still need ssl as a parameter for debugging, and because calc_verify wants it as a parameter (for all TLS versions except SSL3 it would actually only need handshake, but SSL3 also accesses session_negotiate). It's also because of calc_verify that we can't make it const yet, but see next commit. --- library/ssl_tls.c | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bdfb34d64..c1a73f831 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1095,13 +1095,33 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_handshake_params *handshake, /* * Compute master secret if needed + * + * Parameters: + * [in/out] handshake + * [in] resume, premaster, extended_ms, calc_verify, tls_prf + * [out] premaster (cleared) + * [in] minor_ver (to compute hash_len) + * [in] hash_alg (to compute hash_len) + * [out] master + * [in] ssl: optionally used for debugging and calc_verify */ -static int ssl_compute_master( mbedtls_ssl_context *ssl ) +static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, + int minor_ver, + mbedtls_md_type_t hash_alg, + unsigned char *master, + mbedtls_ssl_context *ssl ) { int ret; - mbedtls_ssl_handshake_params *handshake = ssl->handshake; - const mbedtls_ssl_ciphersuite_t *ciphersuite_info = handshake->ciphersuite_info; - mbedtls_ssl_session *session = ssl->session_negotiate; + +#if !defined(MBEDTLS_DEBUG_C) && !defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) + (void) ssl; +#endif +#if !defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) || !defined(MBEDTLS_SSL_PROTO_TLS1_2) + (void) minor_ver; +#if defined(MBEDTLS_SHA512_C) + (void) hash_alg; +#endif +#endif if( handshake->resume != 0 ) { @@ -1113,20 +1133,20 @@ static int ssl_compute_master( mbedtls_ssl_context *ssl ) handshake->pmslen ); #if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - if( ssl->handshake->extended_ms == MBEDTLS_SSL_EXTENDED_MS_ENABLED ) + if( handshake->extended_ms == MBEDTLS_SSL_EXTENDED_MS_ENABLED ) { unsigned char session_hash[48]; size_t hash_len; MBEDTLS_SSL_DEBUG_MSG( 3, ( "using extended master secret" ) ); - ssl->handshake->calc_verify( ssl, session_hash ); + handshake->calc_verify( ssl, session_hash ); #if defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) + if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) { #if defined(MBEDTLS_SHA512_C) - if( ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) + if( hash_alg == MBEDTLS_MD_SHA384 ) { hash_len = 48; } @@ -1143,7 +1163,7 @@ static int ssl_compute_master( mbedtls_ssl_context *ssl ) ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, "extended master secret", session_hash, hash_len, - session->master, 48 ); + master, 48 ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); @@ -1156,7 +1176,7 @@ static int ssl_compute_master( mbedtls_ssl_context *ssl ) ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, "master secret", handshake->randbytes, 64, - session->master, 48 ); + master, 48 ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); @@ -1184,7 +1204,11 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) return( ret ); } - ret = ssl_compute_master( ssl ); + ret = ssl_compute_master( ssl->handshake, + ssl->minor_ver, + ciphersuite_info->mac, + ssl->session_negotiate->master, + ssl ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "ssl_compute_master", ret ); From ed3b7a949254ceb698f245950fc3936f6c48f0b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 3 May 2019 09:58:33 +0200 Subject: [PATCH 07/21] Constify ssl_context param of calc_verify() --- include/mbedtls/ssl_internal.h | 2 +- library/ssl_tls.c | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 17e5f6369..e37a09e52 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -390,7 +390,7 @@ struct mbedtls_ssl_handshake_params #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ void (*update_checksum)(mbedtls_ssl_context *, const unsigned char *, size_t); - void (*calc_verify)(mbedtls_ssl_context *, unsigned char *); + void (*calc_verify)(const mbedtls_ssl_context *, unsigned char *); void (*calc_finished)(mbedtls_ssl_context *, unsigned char *, int); int (*tls_prf)(const unsigned char *, size_t, const char *, const unsigned char *, size_t, diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c1a73f831..634659868 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -584,25 +584,25 @@ static void ssl_update_checksum_md5sha1( mbedtls_ssl_context *, const unsigned c #endif #if defined(MBEDTLS_SSL_PROTO_SSL3) -static void ssl_calc_verify_ssl( mbedtls_ssl_context *, unsigned char * ); +static void ssl_calc_verify_ssl( const mbedtls_ssl_context *, unsigned char * ); static void ssl_calc_finished_ssl( mbedtls_ssl_context *, unsigned char *, int ); #endif #if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) -static void ssl_calc_verify_tls( mbedtls_ssl_context *, unsigned char * ); +static void ssl_calc_verify_tls( const mbedtls_ssl_context *, unsigned char * ); static void ssl_calc_finished_tls( mbedtls_ssl_context *, unsigned char *, int ); #endif #if defined(MBEDTLS_SSL_PROTO_TLS1_2) #if defined(MBEDTLS_SHA256_C) static void ssl_update_checksum_sha256( mbedtls_ssl_context *, const unsigned char *, size_t ); -static void ssl_calc_verify_tls_sha256( mbedtls_ssl_context *,unsigned char * ); +static void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *,unsigned char * ); static void ssl_calc_finished_tls_sha256( mbedtls_ssl_context *,unsigned char *, int ); #endif #if defined(MBEDTLS_SHA512_C) static void ssl_update_checksum_sha384( mbedtls_ssl_context *, const unsigned char *, size_t ); -static void ssl_calc_verify_tls_sha384( mbedtls_ssl_context *, unsigned char * ); +static void ssl_calc_verify_tls_sha384( const mbedtls_ssl_context *, unsigned char * ); static void ssl_calc_finished_tls_sha384( mbedtls_ssl_context *, unsigned char *, int ); #endif #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ @@ -1109,7 +1109,7 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, int minor_ver, mbedtls_md_type_t hash_alg, unsigned char *master, - mbedtls_ssl_context *ssl ) + const mbedtls_ssl_context *ssl ) { int ret; @@ -1219,7 +1219,7 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) } #if defined(MBEDTLS_SSL_PROTO_SSL3) -void ssl_calc_verify_ssl( mbedtls_ssl_context *ssl, unsigned char hash[36] ) +void ssl_calc_verify_ssl( const mbedtls_ssl_context *ssl, unsigned char hash[36] ) { mbedtls_md5_context md5; mbedtls_sha1_context sha1; @@ -1268,7 +1268,7 @@ void ssl_calc_verify_ssl( mbedtls_ssl_context *ssl, unsigned char hash[36] ) #endif /* MBEDTLS_SSL_PROTO_SSL3 */ #if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) -void ssl_calc_verify_tls( mbedtls_ssl_context *ssl, unsigned char hash[36] ) +void ssl_calc_verify_tls( const mbedtls_ssl_context *ssl, unsigned char hash[36] ) { mbedtls_md5_context md5; mbedtls_sha1_context sha1; @@ -1296,7 +1296,7 @@ void ssl_calc_verify_tls( mbedtls_ssl_context *ssl, unsigned char hash[36] ) #if defined(MBEDTLS_SSL_PROTO_TLS1_2) #if defined(MBEDTLS_SHA256_C) -void ssl_calc_verify_tls_sha256( mbedtls_ssl_context *ssl, unsigned char hash[32] ) +void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *ssl, unsigned char hash[32] ) { mbedtls_sha256_context sha256; @@ -1317,7 +1317,7 @@ void ssl_calc_verify_tls_sha256( mbedtls_ssl_context *ssl, unsigned char hash[32 #endif /* MBEDTLS_SHA256_C */ #if defined(MBEDTLS_SHA512_C) -void ssl_calc_verify_tls_sha384( mbedtls_ssl_context *ssl, unsigned char hash[48] ) +void ssl_calc_verify_tls_sha384( const mbedtls_ssl_context *ssl, unsigned char hash[48] ) { mbedtls_sha512_context sha512; From a575975280bcacd212038298c3e6e21ea2546808 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 3 May 2019 11:43:28 +0200 Subject: [PATCH 08/21] Make calc_verify() return the length as well Simplifies ssl_compute_hash(), but unfortunately not so much the other uses. --- include/mbedtls/ssl_internal.h | 2 +- library/ssl_cli.c | 5 +-- library/ssl_srv.c | 5 ++- library/ssl_tls.c | 70 ++++++++++++++-------------------- 4 files changed, 36 insertions(+), 46 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index e37a09e52..63dad3d42 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -390,7 +390,7 @@ struct mbedtls_ssl_handshake_params #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ void (*update_checksum)(mbedtls_ssl_context *, const unsigned char *, size_t); - void (*calc_verify)(const mbedtls_ssl_context *, unsigned char *); + void (*calc_verify)(const mbedtls_ssl_context *, unsigned char *, size_t *); void (*calc_finished)(mbedtls_ssl_context *, unsigned char *, int); int (*tls_prf)(const unsigned char *, size_t, const char *, const unsigned char *, size_t, diff --git a/library/ssl_cli.c b/library/ssl_cli.c index ad7378fbc..d06984e3c 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3217,7 +3217,7 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) unsigned char hash[48]; unsigned char *hash_start = hash; mbedtls_md_type_t md_alg = MBEDTLS_MD_NONE; - unsigned int hashlen; + size_t hashlen; void *rs_ctx = NULL; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write certificate verify" ) ); @@ -3270,7 +3270,7 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) sign: #endif - ssl->handshake->calc_verify( ssl, hash ); + ssl->handshake->calc_verify( ssl, hash, &hashlen ); #if defined(MBEDTLS_SSL_PROTO_SSL3) || defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) @@ -3288,7 +3288,6 @@ sign: * sha_hash * SHA(handshake_messages); */ - hashlen = 36; md_alg = MBEDTLS_MD_NONE; /* diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 72d3c79bd..dbb4a1c21 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4163,7 +4163,10 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) } /* Calculate hash and verify signature */ - ssl->handshake->calc_verify( ssl, hash ); + { + size_t dummy_hlen; + ssl->handshake->calc_verify( ssl, hash, &dummy_hlen ); + } if( ( ret = mbedtls_pk_verify( &ssl->session_negotiate->peer_cert->pk, md_alg, hash_start, hashlen, diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 634659868..bf3896ab6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -584,25 +584,25 @@ static void ssl_update_checksum_md5sha1( mbedtls_ssl_context *, const unsigned c #endif #if defined(MBEDTLS_SSL_PROTO_SSL3) -static void ssl_calc_verify_ssl( const mbedtls_ssl_context *, unsigned char * ); +static void ssl_calc_verify_ssl( const mbedtls_ssl_context *, unsigned char *, size_t * ); static void ssl_calc_finished_ssl( mbedtls_ssl_context *, unsigned char *, int ); #endif #if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) -static void ssl_calc_verify_tls( const mbedtls_ssl_context *, unsigned char * ); +static void ssl_calc_verify_tls( const mbedtls_ssl_context *, unsigned char *, size_t * ); static void ssl_calc_finished_tls( mbedtls_ssl_context *, unsigned char *, int ); #endif #if defined(MBEDTLS_SSL_PROTO_TLS1_2) #if defined(MBEDTLS_SHA256_C) static void ssl_update_checksum_sha256( mbedtls_ssl_context *, const unsigned char *, size_t ); -static void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *,unsigned char * ); +static void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *,unsigned char *, size_t * ); static void ssl_calc_finished_tls_sha256( mbedtls_ssl_context *,unsigned char *, int ); #endif #if defined(MBEDTLS_SHA512_C) static void ssl_update_checksum_sha384( mbedtls_ssl_context *, const unsigned char *, size_t ); -static void ssl_calc_verify_tls_sha384( const mbedtls_ssl_context *, unsigned char * ); +static void ssl_calc_verify_tls_sha384( const mbedtls_ssl_context *, unsigned char *, size_t * ); static void ssl_calc_finished_tls_sha384( mbedtls_ssl_context *, unsigned char *, int ); #endif #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ @@ -1100,14 +1100,10 @@ static int ssl_set_handshake_prfs( mbedtls_ssl_handshake_params *handshake, * [in/out] handshake * [in] resume, premaster, extended_ms, calc_verify, tls_prf * [out] premaster (cleared) - * [in] minor_ver (to compute hash_len) - * [in] hash_alg (to compute hash_len) * [out] master * [in] ssl: optionally used for debugging and calc_verify */ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, - int minor_ver, - mbedtls_md_type_t hash_alg, unsigned char *master, const mbedtls_ssl_context *ssl ) { @@ -1115,12 +1111,6 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, #if !defined(MBEDTLS_DEBUG_C) && !defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) (void) ssl; -#endif -#if !defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) || !defined(MBEDTLS_SSL_PROTO_TLS1_2) - (void) minor_ver; -#if defined(MBEDTLS_SHA512_C) - (void) hash_alg; -#endif #endif if( handshake->resume != 0 ) @@ -1140,23 +1130,7 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, MBEDTLS_SSL_DEBUG_MSG( 3, ( "using extended master secret" ) ); - handshake->calc_verify( ssl, session_hash ); - -#if defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) - { -#if defined(MBEDTLS_SHA512_C) - if( hash_alg == MBEDTLS_MD_SHA384 ) - { - hash_len = 48; - } - else -#endif - hash_len = 32; - } - else -#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ - hash_len = 36; + handshake->calc_verify( ssl, session_hash, &hash_len ); MBEDTLS_SSL_DEBUG_BUF( 3, "session hash", session_hash, hash_len ); @@ -1205,8 +1179,6 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) } ret = ssl_compute_master( ssl->handshake, - ssl->minor_ver, - ciphersuite_info->mac, ssl->session_negotiate->master, ssl ); if( ret != 0 ) @@ -1219,7 +1191,9 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) } #if defined(MBEDTLS_SSL_PROTO_SSL3) -void ssl_calc_verify_ssl( const mbedtls_ssl_context *ssl, unsigned char hash[36] ) +void ssl_calc_verify_ssl( const mbedtls_ssl_context *ssl, + unsigned char hash[36], + size_t *hlen ) { mbedtls_md5_context md5; mbedtls_sha1_context sha1; @@ -1257,7 +1231,9 @@ void ssl_calc_verify_ssl( const mbedtls_ssl_context *ssl, unsigned char hash[36] mbedtls_sha1_update_ret( &sha1, hash + 16, 20 ); mbedtls_sha1_finish_ret( &sha1, hash + 16 ); - MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, 36 ); + *hlen = 36; + + MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, *hlen ); MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= calc verify" ) ); mbedtls_md5_free( &md5 ); @@ -1268,7 +1244,9 @@ void ssl_calc_verify_ssl( const mbedtls_ssl_context *ssl, unsigned char hash[36] #endif /* MBEDTLS_SSL_PROTO_SSL3 */ #if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) -void ssl_calc_verify_tls( const mbedtls_ssl_context *ssl, unsigned char hash[36] ) +void ssl_calc_verify_tls( const mbedtls_ssl_context *ssl, + unsigned char hash[36], + size_t *hlen ) { mbedtls_md5_context md5; mbedtls_sha1_context sha1; @@ -1284,7 +1262,9 @@ void ssl_calc_verify_tls( const mbedtls_ssl_context *ssl, unsigned char hash[36] mbedtls_md5_finish_ret( &md5, hash ); mbedtls_sha1_finish_ret( &sha1, hash + 16 ); - MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, 36 ); + *hlen = 36; + + MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, *hlen ); MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= calc verify" ) ); mbedtls_md5_free( &md5 ); @@ -1296,7 +1276,9 @@ void ssl_calc_verify_tls( const mbedtls_ssl_context *ssl, unsigned char hash[36] #if defined(MBEDTLS_SSL_PROTO_TLS1_2) #if defined(MBEDTLS_SHA256_C) -void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *ssl, unsigned char hash[32] ) +void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *ssl, + unsigned char hash[32], + size_t *hlen ) { mbedtls_sha256_context sha256; @@ -1307,7 +1289,9 @@ void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *ssl, unsigned char h mbedtls_sha256_clone( &sha256, &ssl->handshake->fin_sha256 ); mbedtls_sha256_finish_ret( &sha256, hash ); - MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, 32 ); + *hlen = 32; + + MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, *hlen ); MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= calc verify" ) ); mbedtls_sha256_free( &sha256 ); @@ -1317,7 +1301,9 @@ void ssl_calc_verify_tls_sha256( const mbedtls_ssl_context *ssl, unsigned char h #endif /* MBEDTLS_SHA256_C */ #if defined(MBEDTLS_SHA512_C) -void ssl_calc_verify_tls_sha384( const mbedtls_ssl_context *ssl, unsigned char hash[48] ) +void ssl_calc_verify_tls_sha384( const mbedtls_ssl_context *ssl, + unsigned char hash[48], + size_t *hlen ) { mbedtls_sha512_context sha512; @@ -1328,7 +1314,9 @@ void ssl_calc_verify_tls_sha384( const mbedtls_ssl_context *ssl, unsigned char h mbedtls_sha512_clone( &sha512, &ssl->handshake->fin_sha512 ); mbedtls_sha512_finish_ret( &sha512, hash ); - MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, 48 ); + *hlen = 48; + + MBEDTLS_SSL_DEBUG_BUF( 3, "calculated verify result", hash, *hlen ); MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= calc verify" ) ); mbedtls_sha512_free( &sha512 ); From bcf258e07731b452e58be99bfffb753d08a7e936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 3 May 2019 11:46:27 +0200 Subject: [PATCH 09/21] Remove duplicated branch in ssl_compute_master() --- library/ssl_tls.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bf3896ab6..b2f65944f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1138,19 +1138,15 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, "extended master secret", session_hash, hash_len, master, 48 ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); - return( ret ); - } - } else #endif - ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, - "master secret", - handshake->randbytes, 64, - master, 48 ); + { + ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, + "master secret", + handshake->randbytes, 64, + master, 48 ); + } if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); From 707728dfca9f841e39fe7acd98f464692f13e3e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 6 May 2019 12:05:58 +0200 Subject: [PATCH 10/21] Move handling of randbytes to derive_keys() --- library/ssl_tls.c | 47 ++++++++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b2f65944f..b3cef5d60 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -616,7 +616,6 @@ static void ssl_calc_finished_tls_sha384( mbedtls_ssl_context *, unsigned char * static int ssl_populate_transform( mbedtls_ssl_context *ssl ) { int ret = 0; - unsigned char tmp[64]; unsigned char keyblk[256]; unsigned char *key1; unsigned char *key2; @@ -633,8 +632,6 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) mbedtls_ssl_transform *transform = ssl->transform_negotiate; mbedtls_ssl_handshake_params *handshake = ssl->handshake; - MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> derive keys" ) ); - #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) transform->encrypt_then_mac = session->encrypt_then_mac; #endif @@ -657,14 +654,6 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } - /* - * Swap the client and server random values. - */ - memcpy( tmp, handshake->randbytes, 64 ); - memcpy( handshake->randbytes, tmp + 32, 32 ); - memcpy( handshake->randbytes + 32, tmp, 32 ); - mbedtls_platform_zeroize( tmp, sizeof( tmp ) ); - /* * SSLv3: * key block = @@ -691,9 +680,6 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_BUF( 4, "random bytes", handshake->randbytes, 64 ); MBEDTLS_SSL_DEBUG_BUF( 4, "key block", keyblk, 256 ); - mbedtls_platform_zeroize( handshake->randbytes, - sizeof( handshake->randbytes ) ); - /* * Determine the appropriate key, IV and MAC length. */ @@ -1024,8 +1010,6 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_ZLIB_SUPPORT */ - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= derive keys" ) ); - return( 0 ); } @@ -1165,6 +1149,9 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) const mbedtls_ssl_ciphersuite_t * const ciphersuite_info = ssl->handshake->ciphersuite_info; + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> derive keys" ) ); + + /* Set PRF, calc_verify and calc_finished function pointers */ ret = ssl_set_handshake_prfs( ssl->handshake, ssl->minor_ver, ciphersuite_info->mac ); @@ -1174,6 +1161,7 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) return( ret ); } + /* Compute master secret if needed */ ret = ssl_compute_master( ssl->handshake, ssl->session_negotiate->master, ssl ); @@ -1183,7 +1171,32 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) return( ret ); } - return( ssl_populate_transform( ssl ) ); + /* Swap the client and server random values: + * - MS derivation wanted client+server (RFC 5246 8.1) + * - key derivation wants server+client (RFC 5246 6.3) */ + { + unsigned char tmp[64]; + memcpy( tmp, ssl->handshake->randbytes, 64 ); + memcpy( ssl->handshake->randbytes, tmp + 32, 32 ); + memcpy( ssl->handshake->randbytes + 32, tmp, 32 ); + mbedtls_platform_zeroize( tmp, sizeof( tmp ) ); + } + + /* Populate transform structure */ + ret = ssl_populate_transform( ssl ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "ssl_populate_transform", ret ); + return( ret ); + } + + /* We no longer need Server/ClientHello.random values */ + mbedtls_platform_zeroize( ssl->handshake->randbytes, + sizeof( ssl->handshake->randbytes ) ); + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= derive keys" ) ); + + return( 0 ); } #if defined(MBEDTLS_SSL_PROTO_SSL3) From a1abb2609470c08d4a31ba35678c274ed49b0bd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 6 May 2019 12:44:24 +0200 Subject: [PATCH 11/21] Move compress_buf allocation to derive_keys --- library/ssl_tls.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b3cef5d60..6fa6ecf2f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -978,23 +978,10 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) mbedtls_platform_zeroize( keyblk, sizeof( keyblk ) ); + /* Initialize Zlib contexts */ #if defined(MBEDTLS_ZLIB_SUPPORT) - // Initialize compression - // if( session->compression == MBEDTLS_SSL_COMPRESS_DEFLATE ) { - if( ssl->compress_buf == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 3, ( "Allocating compression buffer" ) ); - ssl->compress_buf = mbedtls_calloc( 1, MBEDTLS_SSL_COMPRESS_BUFFER_LEN ); - if( ssl->compress_buf == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", - MBEDTLS_SSL_COMPRESS_BUFFER_LEN ) ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - } - } - MBEDTLS_SSL_DEBUG_MSG( 3, ( "Initializing zlib states" ) ); memset( &transform->ctx_deflate, 0, sizeof( transform->ctx_deflate ) ); @@ -1194,6 +1181,22 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) mbedtls_platform_zeroize( ssl->handshake->randbytes, sizeof( ssl->handshake->randbytes ) ); + /* Allocate compression buffer */ +#if defined(MBEDTLS_ZLIB_SUPPORT) + if( session->compression == MBEDTLS_SSL_COMPRESS_DEFLATE && + ssl->compress_buf == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Allocating compression buffer" ) ); + ssl->compress_buf = mbedtls_calloc( 1, MBEDTLS_SSL_COMPRESS_BUFFER_LEN ); + if( ssl->compress_buf == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", + MBEDTLS_SSL_COMPRESS_BUFFER_LEN ) ); + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + } +#endif + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= derive keys" ) ); return( 0 ); From 12a3f445b6f2a91ccde9df6b07260adefcda47a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 6 May 2019 12:55:40 +0200 Subject: [PATCH 12/21] Start refining parameters of populate_transform() Parameters 'handshake' and 'ssl' will be replaced with more fine-grained inputs in follow-up commits. --- library/ssl_tls.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6fa6ecf2f..2a3ed44bc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -608,12 +608,21 @@ static void ssl_calc_finished_tls_sha384( mbedtls_ssl_context *, unsigned char * #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ /* - * This function will ultimetaly only be responsible for populating a - * transform structure from data passed as explicit parameters. + * Populate a transform structure with session keys and all the other + * necessary information. * - * For now however it's doing rather more in a rather less explicit way. + * Parameters: + * - [in/out]: transform: structure to populate + * [in] must be just initialised with mbedtls_ssl_transform_init() + * [out] fully populate, ready for use by mbedtls_ssl_{en,de}crypt_buf() + * - [in] session: used members: encrypt_then_max, master, compression + * - [in] handshake: used members: prf, ciphersuite_info, randbytes + * - [in]: ssl: used members: minor_ver, conf->endpoint */ -static int ssl_populate_transform( mbedtls_ssl_context *ssl ) +static int ssl_populate_transform( mbedtls_ssl_transform *transform, + const mbedtls_ssl_session *session, + const mbedtls_ssl_handshake_params *handshake, + const mbedtls_ssl_context *ssl ) { int ret = 0; unsigned char keyblk[256]; @@ -628,10 +637,6 @@ static int ssl_populate_transform( mbedtls_ssl_context *ssl ) const mbedtls_cipher_info_t *cipher_info; const mbedtls_md_info_t *md_info; - mbedtls_ssl_session *session = ssl->session_negotiate; - mbedtls_ssl_transform *transform = ssl->transform_negotiate; - mbedtls_ssl_handshake_params *handshake = ssl->handshake; - #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) transform->encrypt_then_mac = session->encrypt_then_mac; #endif @@ -1170,7 +1175,10 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) } /* Populate transform structure */ - ret = ssl_populate_transform( ssl ); + ret = ssl_populate_transform( ssl->transform_negotiate, + ssl->session_negotiate, + ssl->handshake, + ssl ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "ssl_populate_transform", ret ); From 0bcfbc3e04eb4ae3fdd7ec8525e0c7ce8f449058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 6 May 2019 13:32:17 +0200 Subject: [PATCH 13/21] Remove "handshake" input from populate_transform() --- library/ssl_tls.c | 48 ++++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2a3ed44bc..7dc33c93b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -607,6 +607,11 @@ static void ssl_calc_finished_tls_sha384( mbedtls_ssl_context *, unsigned char * #endif #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ +/* Type for the TLS PRF */ +typedef int ssl_tls_prf_t(const unsigned char *, size_t, const char *, + const unsigned char *, size_t, + unsigned char *, size_t); + /* * Populate a transform structure with session keys and all the other * necessary information. @@ -615,13 +620,15 @@ static void ssl_calc_finished_tls_sha384( mbedtls_ssl_context *, unsigned char * * - [in/out]: transform: structure to populate * [in] must be just initialised with mbedtls_ssl_transform_init() * [out] fully populate, ready for use by mbedtls_ssl_{en,de}crypt_buf() - * - [in] session: used members: encrypt_then_max, master, compression - * - [in] handshake: used members: prf, ciphersuite_info, randbytes - * - [in]: ssl: used members: minor_ver, conf->endpoint + * - [in] session: used: ciphersuite, encrypt_then_mac, master, compression + * - [in] tls_prf: pointer to PRF to use for key derivation + * - [in] randbytes: buffer holding ServerHello.random + ClientHello.random + * - [in] ssl: used members: minor_ver, conf->endpoint */ static int ssl_populate_transform( mbedtls_ssl_transform *transform, const mbedtls_ssl_session *session, - const mbedtls_ssl_handshake_params *handshake, + ssl_tls_prf_t tls_prf, + const unsigned char randbytes[64], const mbedtls_ssl_context *ssl ) { int ret = 0; @@ -637,12 +644,23 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, const mbedtls_cipher_info_t *cipher_info; const mbedtls_md_info_t *md_info; + /* Copy info about negotiated version and extensions */ #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) transform->encrypt_then_mac = session->encrypt_then_mac; #endif transform->minor_ver = ssl->minor_ver; - ciphersuite_info = handshake->ciphersuite_info; + /* + * Get various info structures + */ + ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( session->ciphersuite ); + if( ciphersuite_info == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "ciphersuite info for %d not found", + session->ciphersuite ) ); + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + cipher_info = mbedtls_cipher_info_from_type( ciphersuite_info->cipher ); if( cipher_info == NULL ) { @@ -660,19 +678,10 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, } /* - * SSLv3: - * key block = - * MD5( master + SHA1( 'A' + master + randbytes ) ) + - * MD5( master + SHA1( 'BB' + master + randbytes ) ) + - * MD5( master + SHA1( 'CCC' + master + randbytes ) ) + - * MD5( master + SHA1( 'DDDD' + master + randbytes ) ) + - * ... - * - * TLSv1: - * key block = PRF( master, "key expansion", randbytes ) + * Compute key block using the PRF */ - ret = handshake->tls_prf( session->master, 48, "key expansion", - handshake->randbytes, 64, keyblk, 256 ); + ret = tls_prf( session->master, 48, "key expansion", + randbytes, 64, keyblk, 256 ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); @@ -682,7 +691,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, MBEDTLS_SSL_DEBUG_MSG( 3, ( "ciphersuite = %s", mbedtls_ssl_get_ciphersuite_name( session->ciphersuite ) ) ); MBEDTLS_SSL_DEBUG_BUF( 3, "master secret", session->master, 48 ); - MBEDTLS_SSL_DEBUG_BUF( 4, "random bytes", handshake->randbytes, 64 ); + MBEDTLS_SSL_DEBUG_BUF( 4, "random bytes", randbytes, 64 ); MBEDTLS_SSL_DEBUG_BUF( 4, "key block", keyblk, 256 ); /* @@ -1177,7 +1186,8 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) /* Populate transform structure */ ret = ssl_populate_transform( ssl->transform_negotiate, ssl->session_negotiate, - ssl->handshake, + ssl->handshake->tls_prf, + ssl->handshake->randbytes, ssl ); if( ret != 0 ) { From 1d10a98f56c7d5e1450d60588e3c6e6e6eb3d459 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 6 May 2019 13:48:22 +0200 Subject: [PATCH 14/21] Partially rm 'ssl' input from populate_transform() --- library/ssl_tls.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7dc33c93b..cb9267612 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -619,16 +619,23 @@ typedef int ssl_tls_prf_t(const unsigned char *, size_t, const char *, * Parameters: * - [in/out]: transform: structure to populate * [in] must be just initialised with mbedtls_ssl_transform_init() - * [out] fully populate, ready for use by mbedtls_ssl_{en,de}crypt_buf() + * [out] fully populated, ready for use by mbedtls_ssl_{en,de}crypt_buf() * - [in] session: used: ciphersuite, encrypt_then_mac, master, compression * - [in] tls_prf: pointer to PRF to use for key derivation * - [in] randbytes: buffer holding ServerHello.random + ClientHello.random - * - [in] ssl: used members: minor_ver, conf->endpoint + * - [in] minor_ver: SSL/TLS minor version + * - [in] endpoint: client or server + * - [in] ssl: optionally used for: + * - MBEDTLS_SSL_HW_RECORD_ACCEL: whole context + * - MBEDTLS_SSL_EXPORT_KEYS: ssl->conf->{f,p}_export_keys + * - MBEDTLS_DEBUG_C: ssl->conf->{f,p}_dbg */ static int ssl_populate_transform( mbedtls_ssl_transform *transform, const mbedtls_ssl_session *session, ssl_tls_prf_t tls_prf, const unsigned char randbytes[64], + int minor_ver, + unsigned endpoint, const mbedtls_ssl_context *ssl ) { int ret = 0; @@ -644,11 +651,17 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, const mbedtls_cipher_info_t *cipher_info; const mbedtls_md_info_t *md_info; +#if !defined(MBEDTLS_SSL_HW_RECORD_ACCEL) && \ + !defined(MBEDTLS_SSL_EXPORT_KEYS) && \ + !defined(MBEDTLS_DEBUG_C) + (void) ssl; +#endif + /* Copy info about negotiated version and extensions */ #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) transform->encrypt_then_mac = session->encrypt_then_mac; #endif - transform->minor_ver = ssl->minor_ver; + transform->minor_ver = minor_ver; /* * Get various info structures @@ -794,14 +807,14 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, } #if defined(MBEDTLS_SSL_PROTO_SSL3) || defined(MBEDTLS_SSL_PROTO_TLS1) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 || - ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_1 ) + if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 || + minor_ver == MBEDTLS_SSL_MINOR_VERSION_1 ) ; /* No need to adjust minlen */ else #endif #if defined(MBEDTLS_SSL_PROTO_TLS1_1) || defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_2 || - ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) + if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_2 || + minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) { transform->minlen += transform->ivlen; } @@ -830,7 +843,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, * Finally setup the cipher contexts, IVs and MAC secrets. */ #if defined(MBEDTLS_SSL_CLI_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) + if( endpoint == MBEDTLS_SSL_IS_CLIENT ) { key1 = keyblk + mac_key_len * 2; key2 = keyblk + mac_key_len * 2 + keylen; @@ -850,7 +863,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, else #endif /* MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) + if( endpoint == MBEDTLS_SSL_IS_SERVER ) { key1 = keyblk + mac_key_len * 2 + keylen; key2 = keyblk + mac_key_len * 2; @@ -876,7 +889,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, #if defined(MBEDTLS_SSL_SOME_MODES_USE_MAC) #if defined(MBEDTLS_SSL_PROTO_SSL3) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) + if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) { if( mac_key_len > sizeof( transform->mac_enc ) ) { @@ -891,7 +904,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, #endif /* MBEDTLS_SSL_PROTO_SSL3 */ #if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 ) + if( minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 ) { /* For HMAC-based ciphersuites, initialize the HMAC transforms. For AEAD-based ciphersuites, there is nothing to do here. */ @@ -1188,6 +1201,8 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) ssl->session_negotiate, ssl->handshake->tls_prf, ssl->handshake->randbytes, + ssl->minor_ver, + ssl->conf->endpoint, ssl ); if( ret != 0 ) { From 86e48c213c89df42040aa8e2e7d9a951b5e52a79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 7 May 2019 10:17:56 +0200 Subject: [PATCH 15/21] Enforce promise to not use whole ssl context Configs with no DEBUG_C are use for example in test-ref-configs.pl, which also runs parts of compat.sh or ssl-opt.sh on them, so the added 'ssl = NULL' statements will be exercised in those tests at least. --- library/ssl_tls.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index cb9267612..0f416b1fe 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -654,6 +654,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, #if !defined(MBEDTLS_SSL_HW_RECORD_ACCEL) && \ !defined(MBEDTLS_SSL_EXPORT_KEYS) && \ !defined(MBEDTLS_DEBUG_C) + ssl = NULL; /* make sure we don't use it except for those cases */ (void) ssl; #endif @@ -1108,6 +1109,7 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, int ret; #if !defined(MBEDTLS_DEBUG_C) && !defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) + ssl = NULL; /* make sure we don't use it except for debug and EMS */ (void) ssl; #endif @@ -1589,6 +1591,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, /* The SSL context is only used for debugging purposes! */ #if !defined(MBEDTLS_DEBUG_C) + ssl = NULL; /* make sure we don't use it except for debug */ ((void) ssl); #endif @@ -1982,6 +1985,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, unsigned char add_data[13]; #if !defined(MBEDTLS_DEBUG_C) + ssl = NULL; /* make sure we don't use it except for debug */ ((void) ssl); #endif From cf31216ace928e5a2332eb8a6f1bf482f6da7099 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 10 May 2019 10:25:00 +0200 Subject: [PATCH 16/21] Fix typo in comment --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0f416b1fe..27df022f6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1035,7 +1035,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, * - SSL/TLS minor version * - hash associated with the ciphersuite (only used by TLS 1.2) * - * Ouputs: + * Outputs: * - the tls_prf, calc_verify and calc_finished members of handshake structure */ static int ssl_set_handshake_prfs( mbedtls_ssl_handshake_params *handshake, From 84ef8bde6847f19d0fbff5413a8b3abfd1210464 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 10 May 2019 10:50:04 +0200 Subject: [PATCH 17/21] Remove 'session' input from populate_tranform() When using this function to deserialize, it's not a problem to have a session structure as input as we'll have one around anyway (most probably freshly deserialised). However for tests it's convenient to be able to build a transform without having a session structure around. Also, removing this structure from parameters makes the function signature more uniform, the only exception left being the ssl param at the end that's hard to avoid for now. --- library/ssl_tls.c | 51 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 27df022f6..4329c08ed 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -620,7 +620,11 @@ typedef int ssl_tls_prf_t(const unsigned char *, size_t, const char *, * - [in/out]: transform: structure to populate * [in] must be just initialised with mbedtls_ssl_transform_init() * [out] fully populated, ready for use by mbedtls_ssl_{en,de}crypt_buf() - * - [in] session: used: ciphersuite, encrypt_then_mac, master, compression + * - [in] ciphersuite + * - [in] master + * - [in] encrypt_then_mac + * - [in] trunc_hmac + * - [in] compression * - [in] tls_prf: pointer to PRF to use for key derivation * - [in] randbytes: buffer holding ServerHello.random + ClientHello.random * - [in] minor_ver: SSL/TLS minor version @@ -631,7 +635,17 @@ typedef int ssl_tls_prf_t(const unsigned char *, size_t, const char *, * - MBEDTLS_DEBUG_C: ssl->conf->{f,p}_dbg */ static int ssl_populate_transform( mbedtls_ssl_transform *transform, - const mbedtls_ssl_session *session, + int ciphersuite, + const unsigned char master[48], +#if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) + int encrypt_then_mac, +#endif +#if defined(MBEDTLS_SSL_TRUNCATED_HMAC) + int trunc_hmac, +#endif +#if defined(MBEDTLS_ZLIB_SUPPORT) + int compression, +#endif ssl_tls_prf_t tls_prf, const unsigned char randbytes[64], int minor_ver, @@ -660,18 +674,18 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, /* Copy info about negotiated version and extensions */ #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) - transform->encrypt_then_mac = session->encrypt_then_mac; + transform->encrypt_then_mac = encrypt_then_mac; #endif transform->minor_ver = minor_ver; /* * Get various info structures */ - ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( session->ciphersuite ); + ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( ciphersuite ); if( ciphersuite_info == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "ciphersuite info for %d not found", - session->ciphersuite ) ); + ciphersuite ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } @@ -694,8 +708,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, /* * Compute key block using the PRF */ - ret = tls_prf( session->master, 48, "key expansion", - randbytes, 64, keyblk, 256 ); + ret = tls_prf( master, 48, "key expansion", randbytes, 64, keyblk, 256 ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "prf", ret ); @@ -703,8 +716,8 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, } MBEDTLS_SSL_DEBUG_MSG( 3, ( "ciphersuite = %s", - mbedtls_ssl_get_ciphersuite_name( session->ciphersuite ) ) ); - MBEDTLS_SSL_DEBUG_BUF( 3, "master secret", session->master, 48 ); + mbedtls_ssl_get_ciphersuite_name( ciphersuite ) ) ); + MBEDTLS_SSL_DEBUG_BUF( 3, "master secret", master, 48 ); MBEDTLS_SSL_DEBUG_BUF( 4, "random bytes", randbytes, 64 ); MBEDTLS_SSL_DEBUG_BUF( 4, "key block", keyblk, 256 ); @@ -766,7 +779,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, * (rfc 6066 page 13 or rfc 2104 section 4), * so we only need to adjust the length here. */ - if( session->trunc_hmac == MBEDTLS_SSL_TRUNC_HMAC_ENABLED ) + if( trunc_hmac == MBEDTLS_SSL_TRUNC_HMAC_ENABLED ) { transform->maclen = MBEDTLS_SSL_TRUNCATED_HMAC_LEN; @@ -794,7 +807,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, * 2. IV except for SSL3 and TLS 1.0 */ #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) - if( session->encrypt_then_mac == MBEDTLS_SSL_ETM_ENABLED ) + if( encrypt_then_mac == MBEDTLS_SSL_ETM_ENABLED ) { transform->minlen = transform->maclen + cipher_info->block_size; @@ -949,7 +962,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, if( ssl->conf->f_export_keys != NULL ) { ssl->conf->f_export_keys( ssl->conf->p_export_keys, - session->master, keyblk, + master, keyblk, mac_key_len, keylen, iv_copy_len ); } @@ -1008,7 +1021,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, /* Initialize Zlib contexts */ #if defined(MBEDTLS_ZLIB_SUPPORT) - if( session->compression == MBEDTLS_SSL_COMPRESS_DEFLATE ) + if( compression == MBEDTLS_SSL_COMPRESS_DEFLATE ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "Initializing zlib states" ) ); @@ -1200,7 +1213,17 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) /* Populate transform structure */ ret = ssl_populate_transform( ssl->transform_negotiate, - ssl->session_negotiate, + ssl->session_negotiate->ciphersuite, + ssl->session_negotiate->master, +#if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) + ssl->session_negotiate->encrypt_then_mac, +#endif +#if defined(MBEDTLS_SSL_TRUNCATED_HMAC) + ssl->session_negotiate->trunc_hmac, +#endif +#if defined(MBEDTLS_ZLIB_SUPPORT) + ssl->session_negotiate->compression, +#endif ssl->handshake->tls_prf, ssl->handshake->randbytes, ssl->minor_ver, From 5478e1e5ede50c9c881dcdbafa51dbbc20c58d20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 20 May 2019 10:07:29 +0200 Subject: [PATCH 18/21] Remove redundant debug message. Two consecutive messages (ie no branch between them) at the same level are not needed, so only keep the one that has the most information. --- library/ssl_tls.c | 2 -- tests/ssl-opt.sh | 20 ++++++++++---------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4329c08ed..cb4705998 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1141,8 +1141,6 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, unsigned char session_hash[48]; size_t hash_len; - MBEDTLS_SSL_DEBUG_MSG( 3, ( "using extended master secret" ) ); - handshake->calc_verify( ssl, session_hash, &hash_len ); MBEDTLS_SSL_DEBUG_BUF( 3, "session hash", session_hash, hash_len ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 9a9629417..92eb62cb9 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1210,8 +1210,8 @@ run_test "Extended Master Secret: default" \ -s "found extended master secret extension" \ -s "server hello, adding extended master secret extension" \ -c "found extended_master_secret extension" \ - -c "using extended master secret" \ - -s "using extended master secret" + -c "session hash" \ + -s "session hash" run_test "Extended Master Secret: client enabled, server disabled" \ "$P_SRV debug_level=3 extended_ms=0" \ @@ -1221,8 +1221,8 @@ run_test "Extended Master Secret: client enabled, server disabled" \ -s "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "using extended master secret" \ - -S "using extended master secret" + -C "session hash" \ + -S "session hash" run_test "Extended Master Secret: client disabled, server enabled" \ "$P_SRV debug_level=3 extended_ms=1" \ @@ -1232,8 +1232,8 @@ run_test "Extended Master Secret: client disabled, server enabled" \ -S "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "using extended master secret" \ - -S "using extended master secret" + -C "session hash" \ + -S "session hash" requires_config_enabled MBEDTLS_SSL_PROTO_SSL3 run_test "Extended Master Secret: client SSLv3, server enabled" \ @@ -1244,8 +1244,8 @@ run_test "Extended Master Secret: client SSLv3, server enabled" \ -S "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "using extended master secret" \ - -S "using extended master secret" + -C "session hash" \ + -S "session hash" requires_config_enabled MBEDTLS_SSL_PROTO_SSL3 run_test "Extended Master Secret: client enabled, server SSLv3" \ @@ -1256,8 +1256,8 @@ run_test "Extended Master Secret: client enabled, server SSLv3" \ -S "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "using extended master secret" \ - -S "using extended master secret" + -C "session hash" \ + -S "session hash" # Tests for FALLBACK_SCSV From 42c814fdc12fe5f36cbddf6b51790f8dbd6c5772 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 20 May 2019 10:10:17 +0200 Subject: [PATCH 19/21] Clarify comment about TLS versions The previous comment used "TLS" as a shortcut for "TLS 1.0/1.1" which was confusing. This partially reflected the names of the calc_verify/finished that go ssl, tls (for 1.0/1.1) tls_shaxxx (for 1.2), but still it's clearer to be explicit in the comment - and perhaps in the long term the function names could be clarified instead. --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index cb4705998..8ad77b8c3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1042,7 +1042,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, } /* - * Set appropriate PRF function and other SSL / TLS / TLS1.2 functions + * Set appropriate PRF function and other SSL / TLS 1.0/1.1 / TLS1.2 functions * * Inputs: * - SSL/TLS minor version From 762d011ece6ddef36fa080e2d6576e0002a8c0f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 20 May 2019 10:27:20 +0200 Subject: [PATCH 20/21] Fix alignment issues --- library/ssl_tls.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8ad77b8c3..b4e796091 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -716,7 +716,7 @@ static int ssl_populate_transform( mbedtls_ssl_transform *transform, } MBEDTLS_SSL_DEBUG_MSG( 3, ( "ciphersuite = %s", - mbedtls_ssl_get_ciphersuite_name( ciphersuite ) ) ); + mbedtls_ssl_get_ciphersuite_name( ciphersuite ) ) ); MBEDTLS_SSL_DEBUG_BUF( 3, "master secret", master, 48 ); MBEDTLS_SSL_DEBUG_BUF( 4, "random bytes", randbytes, 64 ); MBEDTLS_SSL_DEBUG_BUF( 4, "key block", keyblk, 256 ); @@ -1247,7 +1247,7 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ) if( ssl->compress_buf == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", - MBEDTLS_SSL_COMPRESS_BUFFER_LEN ) ); + MBEDTLS_SSL_COMPRESS_BUFFER_LEN ) ); return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); } } From 9c5bcc9220cf5cd2972c6afd532f3778c9e9d3f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 20 May 2019 12:09:50 +0200 Subject: [PATCH 21/21] Use more specific name in debug message for testing While 'session hash' is currently unique, so suitable to prove that the intended code path has been taken, it's a generic enough phrase that in the future we might add other debug messages containing it in completely unrelated code paths. In order to future-proof the accuracy of the test, let's use a more specific string. --- library/ssl_tls.c | 3 ++- tests/ssl-opt.sh | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b4e796091..f990e8cec 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1143,7 +1143,8 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake, handshake->calc_verify( ssl, session_hash, &hash_len ); - MBEDTLS_SSL_DEBUG_BUF( 3, "session hash", session_hash, hash_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "session hash for extended master secret", + session_hash, hash_len ); ret = handshake->tls_prf( handshake->premaster, handshake->pmslen, "extended master secret", diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 92eb62cb9..977903e10 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1210,8 +1210,8 @@ run_test "Extended Master Secret: default" \ -s "found extended master secret extension" \ -s "server hello, adding extended master secret extension" \ -c "found extended_master_secret extension" \ - -c "session hash" \ - -s "session hash" + -c "session hash for extended master secret" \ + -s "session hash for extended master secret" run_test "Extended Master Secret: client enabled, server disabled" \ "$P_SRV debug_level=3 extended_ms=0" \ @@ -1221,8 +1221,8 @@ run_test "Extended Master Secret: client enabled, server disabled" \ -s "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "session hash" \ - -S "session hash" + -C "session hash for extended master secret" \ + -S "session hash for extended master secret" run_test "Extended Master Secret: client disabled, server enabled" \ "$P_SRV debug_level=3 extended_ms=1" \ @@ -1232,8 +1232,8 @@ run_test "Extended Master Secret: client disabled, server enabled" \ -S "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "session hash" \ - -S "session hash" + -C "session hash for extended master secret" \ + -S "session hash for extended master secret" requires_config_enabled MBEDTLS_SSL_PROTO_SSL3 run_test "Extended Master Secret: client SSLv3, server enabled" \ @@ -1244,8 +1244,8 @@ run_test "Extended Master Secret: client SSLv3, server enabled" \ -S "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "session hash" \ - -S "session hash" + -C "session hash for extended master secret" \ + -S "session hash for extended master secret" requires_config_enabled MBEDTLS_SSL_PROTO_SSL3 run_test "Extended Master Secret: client enabled, server SSLv3" \ @@ -1256,8 +1256,8 @@ run_test "Extended Master Secret: client enabled, server SSLv3" \ -S "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ - -C "session hash" \ - -S "session hash" + -C "session hash for extended master secret" \ + -S "session hash for extended master secret" # Tests for FALLBACK_SCSV