Skip to content

Commit

Permalink
Fix SSLv3 MAC computation
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mpg committed Dec 18, 2017
1 parent 6774fa6 commit 464147c
Showing 1 changed file with 16 additions and 8 deletions.
24 changes: 16 additions & 8 deletions library/ssl_tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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 */

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 */
Expand Down

0 comments on commit 464147c

Please sign in to comment.