From 2cc69fffcf431085f18f4e59c3c5188297f97b87 Mon Sep 17 00:00:00 2001 From: Nicholas Wilson Date: Wed, 13 Apr 2016 11:44:29 +0100 Subject: [PATCH 1/5] Shut up a clang-analyzer warning The function appears to be safe, since grow() is called with sensible arguments in previous functions. Ideally Clang would be clever enough to realise this. Even if N has size MBEDTLS_MPI_MAX_LIMBS, which will cause the grow to fail, the affected lines in montmul won't be reached. Having this sanity check can hardly hurt though. --- library/bignum.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 7841bea433..81af57d5a7 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1542,12 +1542,15 @@ static void mpi_montg_init( mbedtls_mpi_uint *mm, const mbedtls_mpi *N ) /* * Montgomery multiplication: A = A * B * R^-1 mod N (HAC 14.36) */ -static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi *N, mbedtls_mpi_uint mm, +static int mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi *N, mbedtls_mpi_uint mm, const mbedtls_mpi *T ) { size_t i, n, m; mbedtls_mpi_uint u0, u1, *d; + if( T->n < N->n + 1 || T->p == NULL ) + return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + memset( T->p, 0, T->n * ciL ); d = T->p; @@ -1575,12 +1578,14 @@ static void mpi_montmul( mbedtls_mpi *A, const mbedtls_mpi *B, const mbedtls_mpi else /* prevent timing attacks */ mpi_sub_hlp( n, A->p, T->p ); + + return( 0 ); } /* * Montgomery reduction: A = A * R^-1 mod N */ -static void mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, mbedtls_mpi_uint mm, const mbedtls_mpi *T ) +static int mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, mbedtls_mpi_uint mm, const mbedtls_mpi *T ) { mbedtls_mpi_uint z = 1; mbedtls_mpi U; @@ -1588,7 +1593,7 @@ static void mpi_montred( mbedtls_mpi *A, const mbedtls_mpi *N, mbedtls_mpi_uint U.n = U.s = (int) z; U.p = &z; - mpi_montmul( A, &U, N, mm, T ); + return( mpi_montmul( A, &U, N, mm, T ) ); } /* @@ -1665,13 +1670,13 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi else MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &W[1], A ) ); - mpi_montmul( &W[1], &RR, N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( &W[1], &RR, N, mm, &T ) ); /* * X = R^2 * R^-1 mod N = R mod N */ MBEDTLS_MPI_CHK( mbedtls_mpi_copy( X, &RR ) ); - mpi_montred( X, N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montred( X, N, mm, &T ) ); if( wsize > 1 ) { @@ -1684,7 +1689,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &W[j], &W[1] ) ); for( i = 0; i < wsize - 1; i++ ) - mpi_montmul( &W[j], &W[j], N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( &W[j], &W[j], N, mm, &T ) ); /* * W[i] = W[i - 1] * W[1] @@ -1694,7 +1699,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi MBEDTLS_MPI_CHK( mbedtls_mpi_grow( &W[i], N->n + 1 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &W[i], &W[i - 1] ) ); - mpi_montmul( &W[i], &W[1], N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( &W[i], &W[1], N, mm, &T ) ); } } @@ -1731,7 +1736,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi /* * out of window, square X */ - mpi_montmul( X, X, N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( X, X, N, mm, &T ) ); continue; } @@ -1749,12 +1754,12 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi * X = X^wsize R^-1 mod N */ for( i = 0; i < wsize; i++ ) - mpi_montmul( X, X, N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( X, X, N, mm, &T ) ); /* * X = X * W[wbits] R^-1 mod N */ - mpi_montmul( X, &W[wbits], N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( X, &W[wbits], N, mm, &T ) ); state--; nbits = 0; @@ -1767,18 +1772,18 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi */ for( i = 0; i < nbits; i++ ) { - mpi_montmul( X, X, N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( X, X, N, mm, &T ) ); wbits <<= 1; if( ( wbits & ( one << wsize ) ) != 0 ) - mpi_montmul( X, &W[1], N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montmul( X, &W[1], N, mm, &T ) ); } /* * X = A^E * R * R^-1 mod N = A^E mod N */ - mpi_montred( X, N, mm, &T ); + MBEDTLS_MPI_CHK( mpi_montred( X, N, mm, &T ) ); if( neg ) { From 409401c044da02c84cb3ec5f4edb01d79a1f16a7 Mon Sep 17 00:00:00 2001 From: Nicholas Wilson Date: Wed, 13 Apr 2016 11:48:25 +0100 Subject: [PATCH 2/5] Shut up a few clang-analyze warnings about use of uninitialized variables The functions are all safe, Clang just isn't clever enough to realise it. --- library/pkcs12.c | 2 +- library/rsa.c | 19 +++++++++++++++++-- programs/hash/generic_sum.c | 2 +- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/library/pkcs12.c b/library/pkcs12.c index 7023b9dbc8..c603a13577 100644 --- a/library/pkcs12.c +++ b/library/pkcs12.c @@ -93,7 +93,7 @@ static int pkcs12_pbe_derive_key_iv( mbedtls_asn1_buf *pbe_params, mbedtls_md_ty unsigned char *key, size_t keylen, unsigned char *iv, size_t ivlen ) { - int ret, iterations; + int ret, iterations = 0; mbedtls_asn1_buf salt; size_t i; unsigned char unipwd[PKCS12_MAX_PWDLEN * 2 + 2]; diff --git a/library/rsa.c b/library/rsa.c index fba68ddfc7..60559e2ac7 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -797,7 +797,12 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, int ret; size_t ilen, pad_count = 0, i; unsigned char *p, bad, pad_done = 0; +#ifdef __clang_analyzer__ + /* Shut up Clang, mbedtls_rsa_public/private writes to this */ + unsigned char buf[MBEDTLS_MPI_MAX_SIZE] = { }; +#else unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; +#endif if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V15 ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); @@ -1175,13 +1180,18 @@ int mbedtls_rsa_rsassa_pss_verify_ext( mbedtls_rsa_context *ctx, int ret; size_t siglen; unsigned char *p; - unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; unsigned char result[MBEDTLS_MD_MAX_SIZE]; unsigned char zeros[8]; unsigned int hlen; size_t slen, msb; const mbedtls_md_info_t *md_info; mbedtls_md_context_t md_ctx; +#ifdef __clang_analyzer__ + /* Shut up Clang, mbedtls_rsa_public/private writes to this */ + unsigned char buf[MBEDTLS_MPI_MAX_SIZE] = { }; +#else + unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; +#endif if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V21 ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); @@ -1320,10 +1330,15 @@ int mbedtls_rsa_rsassa_pkcs1_v15_verify( mbedtls_rsa_context *ctx, int ret; size_t len, siglen, asn1_len; unsigned char *p, *end; - unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; mbedtls_md_type_t msg_md_alg; const mbedtls_md_info_t *md_info; mbedtls_asn1_buf oid; +#ifdef __clang_analyzer__ + /* Shut up Clang, mbedtls_rsa_public/private writes to this */ + unsigned char buf[MBEDTLS_MPI_MAX_SIZE] = { }; +#else + unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; +#endif if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V15 ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); diff --git a/programs/hash/generic_sum.c b/programs/hash/generic_sum.c index f071d311e3..7805a79bcf 100644 --- a/programs/hash/generic_sum.c +++ b/programs/hash/generic_sum.c @@ -83,7 +83,7 @@ static int generic_check( const mbedtls_md_info_t *md_info, char *filename ) int nb_err1, nb_err2; int nb_tot1, nb_tot2; unsigned char sum[MBEDTLS_MD_MAX_SIZE]; - char buf[MBEDTLS_MD_MAX_SIZE * 2 + 1], line[1024]; + char buf[MBEDTLS_MD_MAX_SIZE * 2 + 1] = { }, line[1024]; char diff; if( ( f = fopen( filename, "rb" ) ) == NULL ) From daf534dcf921895604c2a73a767d7ce9a6de1906 Mon Sep 17 00:00:00 2001 From: Nicholas Wilson Date: Wed, 13 Apr 2016 11:50:33 +0100 Subject: [PATCH 3/5] Remove a dead store to silence clang-analyze --- library/ssl_cli.c | 1 - 1 file changed, 1 deletion(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 52ddf9a92f..d1ef7dd353 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -265,7 +265,6 @@ static void ssl_write_supported_elliptic_curves_ext( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_ECP_C) for( grp_id = ssl->conf->curve_list; *grp_id != MBEDTLS_ECP_DP_NONE; grp_id++ ) { - info = mbedtls_ecp_curve_info_from_grp_id( *grp_id ); #else for( info = mbedtls_ecp_curve_list(); info->grp_id != MBEDTLS_ECP_DP_NONE; info++ ) { From 5d5e421d089bf02488f5a0f06ae3a727ff9aadd7 Mon Sep 17 00:00:00 2001 From: Nicholas Wilson Date: Wed, 13 Apr 2016 11:51:05 +0100 Subject: [PATCH 4/5] Refactor slightly to silence a clang-analyze warning Since the buffer is used in a few places, it seems Clang isn't clever enough to realise that the first byte is never touched. So, even though the function has a correct null check for ssl->handshake, Clang complains. Pulling the handshake type out into its own variable is enough for Clang's analysis to kick in though. --- library/ssl_tls.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 1c44b7ddb9..2e41bcad2c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2708,7 +2708,7 @@ void mbedtls_ssl_send_flight_completed( mbedtls_ssl_context *ssl ) */ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) { - int ret, done = 0; + int ret, done = 0, out_msg_type; size_t len = ssl->out_msglen; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write record" ) ); @@ -2724,7 +2724,9 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) #endif if( ssl->out_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) { - if( ssl->out_msg[0] != MBEDTLS_SSL_HS_HELLO_REQUEST && + out_msg_type = ssl->out_msg[0]; + + if( out_msg_type != MBEDTLS_SSL_HS_HELLO_REQUEST && ssl->handshake == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -2751,7 +2753,7 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) len += 8; /* Write message_seq and update it, except for HelloRequest */ - if( ssl->out_msg[0] != MBEDTLS_SSL_HS_HELLO_REQUEST ) + if( out_msg_type != MBEDTLS_SSL_HS_HELLO_REQUEST ) { ssl->out_msg[4] = ( ssl->handshake->out_msg_seq >> 8 ) & 0xFF; ssl->out_msg[5] = ( ssl->handshake->out_msg_seq ) & 0xFF; @@ -2769,7 +2771,7 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_PROTO_DTLS */ - if( ssl->out_msg[0] != MBEDTLS_SSL_HS_HELLO_REQUEST ) + if( out_msg_type != MBEDTLS_SSL_HS_HELLO_REQUEST ) ssl->handshake->update_checksum( ssl, ssl->out_msg, len ); } From 42d47f0fb5414bc5cb07e0353303a6d81297dda3 Mon Sep 17 00:00:00 2001 From: Nicholas Wilson Date: Wed, 13 Apr 2016 11:53:27 +0100 Subject: [PATCH 5/5] Silence a clang-analyze warning The check is already effectively performed later in the function, but implicitly, so Clang's analysis fail to notice the functions are in fact safe. Pulling the check up to the top helps Clang to verify the behaviour. --- library/x509_csr.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/x509_csr.c b/library/x509_csr.c index f8c45f8d28..603d06b64f 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -104,7 +104,7 @@ int mbedtls_x509_csr_parse_der( mbedtls_x509_csr *csr, /* * Check for valid input */ - if( csr == NULL || buf == NULL ) + if( csr == NULL || buf == NULL || buflen == 0 ) return( MBEDTLS_ERR_X509_BAD_INPUT_DATA ); mbedtls_x509_csr_init( csr ); @@ -274,14 +274,14 @@ int mbedtls_x509_csr_parse( mbedtls_x509_csr *csr, const unsigned char *buf, siz /* * Check for valid input */ - if( csr == NULL || buf == NULL ) + if( csr == NULL || buf == NULL || buflen == 0 ) return( MBEDTLS_ERR_X509_BAD_INPUT_DATA ); #if defined(MBEDTLS_PEM_PARSE_C) mbedtls_pem_init( &pem ); /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ - if( buflen == 0 || buf[buflen - 1] != '\0' ) + if( buf[buflen - 1] != '\0' ) ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT; else ret = mbedtls_pem_read_buffer( &pem,