From 464147cadc694379b7717afb7b517fe05cdb323f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 18 Dec 2017 18:04:59 +0100 Subject: [PATCH 1/2] Fix SSLv3 MAC computation In a previous PR (Fix heap corruption in implementation of truncated HMAC extension #425) the place where MAC is computed was changed from the end of the SSL I/O buffer to a local buffer (then (part of) the content of the local buffer is either copied to the output buffer of compare to the input buffer). Unfortunately, this change was made only for TLS 1.0 and later, leaving SSL 3.0 in an inconsistent state due to ssl_mac() still writing to the old, hard-coded location, which, for MAC verification, resulted in later comparing the end of the input buffer (containing the computed MAC) to the local buffer (uninitialised), most likely resulting in MAC verification failure, hence no interop (even with ourselves). This commit completes the move to using a local buffer by using this strategy for SSL 3.0 too. Fortunately ssl_mac() was static so it's not a problem to change its signature. --- library/ssl_tls.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a8aa1c04f..b977cfbee 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1203,9 +1203,11 @@ int mbedtls_ssl_psk_derive_premaster( mbedtls_ssl_context *ssl, mbedtls_key_exch /* * SSLv3.0 MAC functions */ -static void ssl_mac( mbedtls_md_context_t *md_ctx, unsigned char *secret, - unsigned char *buf, size_t len, - unsigned char *ctr, int type ) +static void ssl_mac( mbedtls_md_context_t *md_ctx, + const unsigned char *secret, + const unsigned char *buf, size_t len, + const unsigned char *ctr, int type, + unsigned char out[20] ) { unsigned char header[11]; unsigned char padding[48]; @@ -1230,14 +1232,14 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, unsigned char *secret, mbedtls_md_update( md_ctx, padding, padlen ); mbedtls_md_update( md_ctx, header, 11 ); mbedtls_md_update( md_ctx, buf, len ); - mbedtls_md_finish( md_ctx, buf + len ); + mbedtls_md_finish( md_ctx, out ); memset( padding, 0x5C, padlen ); mbedtls_md_starts( md_ctx ); mbedtls_md_update( md_ctx, secret, md_size ); mbedtls_md_update( md_ctx, padding, padlen ); - mbedtls_md_update( md_ctx, buf + len, md_size ); - mbedtls_md_finish( md_ctx, buf + len ); + mbedtls_md_update( md_ctx, out, md_size ); + mbedtls_md_finish( md_ctx, out ); } #endif /* MBEDTLS_SSL_PROTO_SSL3 */ @@ -1282,10 +1284,15 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_PROTO_SSL3) if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) { + unsigned char mac[20]; /* SHA-1 at most */ + ssl_mac( &ssl->transform_out->md_ctx_enc, ssl->transform_out->mac_enc, ssl->out_msg, ssl->out_msglen, - ssl->out_ctr, ssl->out_msgtype ); + ssl->out_ctr, ssl->out_msgtype, + mac ); + + memcpy( ssl->out_msg + ssl->out_msglen, mac, ssl->transform_out->maclen ); } else #endif @@ -1932,7 +1939,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) ssl_mac( &ssl->transform_in->md_ctx_dec, ssl->transform_in->mac_dec, ssl->in_msg, ssl->in_msglen, - ssl->in_ctr, ssl->in_msgtype ); + ssl->in_ctr, ssl->in_msgtype, + mac_expect ); } else #endif /* MBEDTLS_SSL_PROTO_SSL3 */ From b053efb2954a954415369b42a9249282ec401876 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 19 Dec 2017 10:03:46 +0100 Subject: [PATCH 2/2] Fix magic constant in previous commit --- library/ssl_tls.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b977cfbee..d8df51353 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1203,11 +1203,12 @@ int mbedtls_ssl_psk_derive_premaster( mbedtls_ssl_context *ssl, mbedtls_key_exch /* * SSLv3.0 MAC functions */ +#define SSL_MAC_MAX_BYTES 20 /* MD-5 or SHA-1 */ static void ssl_mac( mbedtls_md_context_t *md_ctx, const unsigned char *secret, const unsigned char *buf, size_t len, const unsigned char *ctr, int type, - unsigned char out[20] ) + unsigned char out[SSL_MAC_MAX_BYTES] ) { unsigned char header[11]; unsigned char padding[48]; @@ -1284,7 +1285,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_PROTO_SSL3) if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) { - unsigned char mac[20]; /* SHA-1 at most */ + unsigned char mac[SSL_MAC_MAX_BYTES]; ssl_mac( &ssl->transform_out->md_ctx_enc, ssl->transform_out->mac_enc,