From 9d5785be8ff5b681d7b0a0688c7fb91ce5634e9f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:06:25 +0000 Subject: [PATCH 01/10] Clarify use of blinding in RSA private key operations --- include/mbedtls/rsa.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/rsa.h b/include/mbedtls/rsa.h index 6b2e54b49..8e34e6269 100644 --- a/include/mbedtls/rsa.h +++ b/include/mbedtls/rsa.h @@ -220,7 +220,7 @@ int mbedtls_rsa_public( mbedtls_rsa_context *ctx, * \brief Do an RSA private key operation * * \param ctx RSA context - * \param f_rng RNG function (Needed for blinding) + * \param f_rng RNG function (used for blinding) * \param p_rng RNG parameter * \param input input buffer * \param output output buffer @@ -229,6 +229,18 @@ int mbedtls_rsa_public( mbedtls_rsa_context *ctx, * * \note The input and output buffers must be large * enough (eg. 128 bytes if RSA-1024 is used). + * + * \note Blinding is used if and only if a PRNG is provided. + * + * \note If blinding is used, both the base of exponentation + * and the exponent are blinded, providing protection + * against some side-channel attacks. + * + * \warning It is deprecated and a security risk to not provide + * a PRNG here and thereby prevent the use of blinding. + * Future versions of the library may enforce the presence + * of a PRNG. + * */ int mbedtls_rsa_private( mbedtls_rsa_context *ctx, int (*f_rng)(void *, unsigned char *, size_t), From 41f5a0fe97ce46c9c596f76216b56ee91c0494aa Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:06:51 +0000 Subject: [PATCH 02/10] Ensure that RSA_NO_CRT gets disabled by `config.pl full` --- scripts/config.pl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/config.pl b/scripts/config.pl index d8d6a20ed..7108f506a 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -31,7 +31,7 @@ EOU # for our eyes only: # $0 [-f ] full|realfull -# Things that shouldn't be enabled with "full". +# The following options are disabled instead of enabled with "full". # Notes: # - MBEDTLS_X509_ALLOW_EXTENSIONS_NON_V3 and # MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION could be enabled if the @@ -47,6 +47,7 @@ MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES MBEDTLS_NO_PLATFORM_ENTROPY MBEDTLS_REMOVE_ARC4_CIPHERSUITES MBEDTLS_SSL_HW_RECORD_ACCEL +MBEDTLS_RSA_NO_CRT MBEDTLS_X509_ALLOW_EXTENSIONS_NON_V3 MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION MBEDTLS_ZLIB_SUPPORT From 929359284347a3b006cd16d9df216abbebeec849 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:07:09 +0000 Subject: [PATCH 03/10] Add test case for RSA_NO_CRT to all.sh --- tests/scripts/all.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 6defcdc2a..cacb7caf0 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -279,6 +279,23 @@ OPENSSL_CMD="$OPENSSL_LEGACY" tests/compat.sh -m 'ssl3' msg "build: SSLv3 - ssl-opt.sh (ASan build)" # ~ 6 min tests/ssl-opt.sh +msg "build: Default + RSA_NO_CRT (ASan build)" # ~ 6 min +cleanup +cp "$CONFIG_H" "$CONFIG_BAK" +scripts/config.pl set MBEDTLS_RSA_NO_CRT +CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . +make + +msg "test: RSA_NO_CRT - main suites (inc. selftests) (ASan build)" # ~ 50s +make test + +msg "test: RSA_NO_CRT - RSA-related part of ssl-opt.sh (ASan build)" # ~ 5s +tests/ssl-opt.sh -f RSA + +msg "test: RSA_NO_CRT - RSA-related part of compat.sh (ASan build)" # ~ 3 min +tests/compat.sh -t RSA + + msg "build: cmake, full config, clang" # ~ 50s cleanup cp "$CONFIG_H" "$CONFIG_BAK" From a82f89181c90cbe8507b4d9d949c86ccf712b96d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:08:27 +0000 Subject: [PATCH 04/10] Verify result of RSA private key operation --- library/bignum.c | 2 +- library/rsa.c | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 52edd3def..4f94e207c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1609,7 +1609,7 @@ int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, const mbedtls_mpi mbedtls_mpi RR, T, W[ 2 << MBEDTLS_MPI_WINDOW_SIZE ], Apos; int neg; - if( mbedtls_mpi_cmp_int( N, 0 ) < 0 || ( N->p[0] & 1 ) == 0 ) + if( mbedtls_mpi_cmp_int( N, 0 ) <= 0 || ( N->p[0] & 1 ) == 0 ) return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); if( mbedtls_mpi_cmp_int( E, 0 ) < 0 ) diff --git a/library/rsa.c b/library/rsa.c index 40ea642dc..70d95fa72 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -403,10 +403,17 @@ int mbedtls_rsa_private( mbedtls_rsa_context *ctx, mbedtls_mpi *DQ = &ctx->DQ; #endif + /* Temporaries holding the initial input and the double + * checked result; should be the same in the end. */ + mbedtls_mpi I, C; + /* Make sure we have private key info, prevent possible misuse */ if( ctx->P.p == NULL || ctx->Q.p == NULL || ctx->D.p == NULL ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); + mbedtls_mpi_init( &I ); + mbedtls_mpi_init( &C ); + mbedtls_mpi_init( &T ); mbedtls_mpi_init( &T1 ); mbedtls_mpi_init( &T2 ); mbedtls_mpi_init( &P1 ); mbedtls_mpi_init( &Q1 ); mbedtls_mpi_init( &R ); @@ -434,6 +441,8 @@ int mbedtls_rsa_private( mbedtls_rsa_context *ctx, goto cleanup; } + MBEDTLS_MPI_CHK( mbedtls_mpi_copy( &I, &T ) ); + if( f_rng != NULL ) { /* @@ -522,6 +531,15 @@ int mbedtls_rsa_private( mbedtls_rsa_context *ctx, MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &T, &T, &ctx->N ) ); } + /* Verify the result to prevent glitching attacks. */ + MBEDTLS_MPI_CHK( mbedtls_mpi_exp_mod( &C, &T, &ctx->E, + &ctx->N, &ctx->RN ) ); + if( mbedtls_mpi_cmp_mpi( &C, &I ) != 0 ) + { + ret = MBEDTLS_ERR_RSA_VERIFY_FAILED; + goto cleanup; + } + olen = ctx->len; MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( &T, output, olen ) ); @@ -544,6 +562,9 @@ cleanup: #endif } + mbedtls_mpi_free( &C ); + mbedtls_mpi_free( &I ); + if( ret != 0 ) return( MBEDTLS_ERR_RSA_PRIVATE_FAILED + ret ); From de0b70c3660169719c2fa761fd942328e32f175e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:08:53 +0000 Subject: [PATCH 05/10] Check precisely for the needed RSA context fields in rsa_private --- library/rsa.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 70d95fa72..ae4e61657 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -408,8 +408,33 @@ int mbedtls_rsa_private( mbedtls_rsa_context *ctx, mbedtls_mpi I, C; /* Make sure we have private key info, prevent possible misuse */ - if( ctx->P.p == NULL || ctx->Q.p == NULL || ctx->D.p == NULL ) +#if defined(MBEDTLS_RSA_NO_CRT) + if( mbedtls_mpi_cmp_int( &ctx->N, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->D, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->E, 0 ) == 0 || + ( f_rng != NULL && mbedtls_mpi_cmp_int( &ctx->P, 0 ) == 0 ) || + ( f_rng != NULL && mbedtls_mpi_cmp_int( &ctx->Q, 0 ) == 0 ) ) + { return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); + } +#else /* ! MBEDTLS_RSA_NO_CRT */ + if( mbedtls_mpi_cmp_int( &ctx->N, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->E, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->P, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->Q, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->DP, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->DQ, 0 ) == 0 || + mbedtls_mpi_cmp_int( &ctx->QP, 0 ) == 0 ) + { + return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); + } +#endif /* ! MBEDTLS_RSA_NO_CRT */ + + +#if defined(MBEDTLS_THREADING_C) + if( ( ret = mbedtls_mutex_lock( &ctx->mutex ) ) != 0 ) + return( ret ); +#endif mbedtls_mpi_init( &I ); mbedtls_mpi_init( &C ); @@ -428,12 +453,6 @@ int mbedtls_rsa_private( mbedtls_rsa_context *ctx, #endif } - -#if defined(MBEDTLS_THREADING_C) - if( ( ret = mbedtls_mutex_lock( &ctx->mutex ) ) != 0 ) - return( ret ); -#endif - MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( &T, input, ctx->len ) ); if( mbedtls_mpi_cmp_mpi( &T, &ctx->N ) >= 0 ) { @@ -726,7 +745,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_encrypt( mbedtls_rsa_context *ctx, return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); olen = ctx->len; - + // first comparison checks for overflow if( ilen + 11 < ilen || olen < ilen + 11 ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); From 21f83753f5bb98086098b9d03b88abc3aac86369 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:09:33 +0000 Subject: [PATCH 06/10] Remove signature verification from mbedtls_rsa_rsassa_pkcs1_v15_sign This is no longer necessary as we're now always verifying the result of rsa_private. --- library/rsa.c | 42 +----------------------------------------- 1 file changed, 1 insertion(+), 41 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index ae4e61657..0d698f750 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1207,11 +1207,6 @@ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx, size_t nb_pad, olen, oid_size = 0; unsigned char *p = sig; const char *oid = NULL; - unsigned char *sig_try = NULL, *verif = NULL; - size_t i; - unsigned char diff; - volatile unsigned char diff_no_optimize; - int ret; if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V15 ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); @@ -1277,42 +1272,7 @@ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx, if( mode == MBEDTLS_RSA_PUBLIC ) return( mbedtls_rsa_public( ctx, sig, sig ) ); - /* - * In order to prevent Lenstra's attack, make the signature in a - * temporary buffer and check it before returning it. - */ - sig_try = mbedtls_calloc( 1, ctx->len ); - if( sig_try == NULL ) - return( MBEDTLS_ERR_MPI_ALLOC_FAILED ); - - verif = mbedtls_calloc( 1, ctx->len ); - if( verif == NULL ) - { - mbedtls_free( sig_try ); - return( MBEDTLS_ERR_MPI_ALLOC_FAILED ); - } - - MBEDTLS_MPI_CHK( mbedtls_rsa_private( ctx, f_rng, p_rng, sig, sig_try ) ); - MBEDTLS_MPI_CHK( mbedtls_rsa_public( ctx, sig_try, verif ) ); - - /* Compare in constant time just in case */ - for( diff = 0, i = 0; i < ctx->len; i++ ) - diff |= verif[i] ^ sig[i]; - diff_no_optimize = diff; - - if( diff_no_optimize != 0 ) - { - ret = MBEDTLS_ERR_RSA_PRIVATE_FAILED; - goto cleanup; - } - - memcpy( sig, sig_try, ctx->len ); - -cleanup: - mbedtls_free( sig_try ); - mbedtls_free( verif ); - - return( ret ); + return( mbedtls_rsa_private( ctx, f_rng, p_rng, sig, sig ) ); } #endif /* MBEDTLS_PKCS1_V15 */ From e2ccaddf0a252ea8ef4a0214702b78daf0871f27 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:10:23 +0000 Subject: [PATCH 07/10] Ensure RSA test suite calls rsa_private with PRNG --- tests/suites/test_suite_pk.function | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 5fa8a693a..77b453cdd 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -43,16 +43,19 @@ int mbedtls_rsa_decrypt_func( void *ctx, int mode, size_t *olen, const unsigned char *input, unsigned char *output, size_t output_max_len ) { - return( mbedtls_rsa_pkcs1_decrypt( (mbedtls_rsa_context *) ctx, NULL, NULL, mode, olen, - input, output, output_max_len ) ); + return( mbedtls_rsa_pkcs1_decrypt( (mbedtls_rsa_context *) ctx, + rnd_std_rand, NULL, mode, olen, + input, output, output_max_len ) ); } int mbedtls_rsa_sign_func( void *ctx, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng, int mode, mbedtls_md_type_t md_alg, unsigned int hashlen, const unsigned char *hash, unsigned char *sig ) { - return( mbedtls_rsa_pkcs1_sign( (mbedtls_rsa_context *) ctx, f_rng, p_rng, mode, - md_alg, hashlen, hash, sig ) ); + ((void) f_rng); + ((void) p_rng); + return( mbedtls_rsa_pkcs1_sign( (mbedtls_rsa_context *) ctx, rnd_std_rand, NULL, mode, + md_alg, hashlen, hash, sig ) ); } size_t mbedtls_rsa_key_len_func( void *ctx ) { From d43764f9d3620b44b5d977ec9c229e0bf41051c5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Nov 2017 15:10:38 +0000 Subject: [PATCH 08/10] Adapt ChangeLog --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 8f7843dc6..6e96fcd43 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,10 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.1.x released xxxx-xx-xx +Security + * Verify results of RSA private key operations to defend + against Bellcore glitch attack. + Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7. From b81fcd00e64b7c6e534fe56eb23a1c58a1cbd408 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 3 May 2017 15:09:31 +0100 Subject: [PATCH 09/10] Correct memory leak in RSA self test The RSA self test didn't free the RSA context on failure. --- library/rsa.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 0d698f750..9324f45eb 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1790,7 +1790,8 @@ int mbedtls_rsa_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( "failed\n" ); - return( 1 ); + ret = 1; + goto cleanup; } if( verbose != 0 ) @@ -1804,7 +1805,8 @@ int mbedtls_rsa_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( "failed\n" ); - return( 1 ); + ret = 1; + goto cleanup; } if( verbose != 0 ) @@ -1817,7 +1819,8 @@ int mbedtls_rsa_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( "failed\n" ); - return( 1 ); + ret = 1; + goto cleanup; } if( memcmp( rsa_decrypted, rsa_plaintext, len ) != 0 ) @@ -1825,7 +1828,8 @@ int mbedtls_rsa_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( "failed\n" ); - return( 1 ); + ret = 1; + goto cleanup; } if( verbose != 0 ) @@ -1843,7 +1847,8 @@ int mbedtls_rsa_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( "failed\n" ); - return( 1 ); + ret = 1; + goto cleanup; } if( verbose != 0 ) @@ -1855,7 +1860,8 @@ int mbedtls_rsa_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( "failed\n" ); - return( 1 ); + ret = 1; + goto cleanup; } if( verbose != 0 ) From 41b6189ef71615cfe9e7c3085dba2b3b7e51d8c1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 13 Mar 2018 10:31:56 +0000 Subject: [PATCH 10/10] Adapt ChangeLog Add note about fix of memory leak in RSA self test. --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index 6e96fcd43..027c560f2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,7 @@ Security Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7. + * Fix memory leak in RSA self test. = mbed TLS 2.1.9 branch released 2017-08-10