From b10fd065be4378eab92923a05cc2670df0af5aca Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Wed, 29 Jan 2020 13:09:55 -0500 Subject: [PATCH 1/3] Parse RSA parameters DP, DQ and QP from PKCS1 private keys Otherwise these values are recomputed in mbedtls_rsa_deduce_crt, which currently suffers from side channel issues in the computation of QP (see https://eprint.iacr.org/2020/055). By loading the pre-computed values not only is the side channel avoided, but runtime overhead of loading RSA keys is reduced. Discussion in https://github.com/ARMmbed/mbed-crypto/issues/347 Backport of https://github.com/ARMmbed/mbed-crypto/pull/352 --- library/pkparse.c | 34 ++++++++++++++++++++++++++++++---- library/rsa.c | 11 ++++++++++- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/library/pkparse.c b/library/pkparse.c index ae210bca6..0ae24027f 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -768,14 +768,40 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, goto cleanup; p += len; - /* Complete the RSA private key */ - if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ) - goto cleanup; +#if !defined(MBEDTLS_RSA_NO_CRT) + /* + * The RSA CRT parameters DP, DQ and QP are nominally redundant, in + * that they can be easily recomputed from D, P and Q. However by + * parsing them from the PKCS1 structure it is possible to avoid + * recalculating them which both reduces the overhead of loading + * RSA private keys into memory and also avoids side channels which + * can arise when computing those values, since all of D, P, and Q + * are secret. See https://eprint.iacr.org/2020/055 for a + * description of one such attack. + */ - /* Check optional parameters */ + /* Import DP */ + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DP ) ) != 0) + goto cleanup; + + /* Import DQ */ + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DQ ) ) != 0) + goto cleanup; + + /* Import QP */ + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->QP ) ) != 0) + goto cleanup; + +#else + /* Verify existance of the CRT params */ if( ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 || ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 || ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 ) + goto cleanup; +#endif + + /* Complete the RSA private key */ + if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ) goto cleanup; if( p != end ) diff --git a/library/rsa.c b/library/rsa.c index af1a87859..09fd379fd 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -249,6 +249,9 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) { int ret = 0; int have_N, have_P, have_Q, have_D, have_E; +#if !defined(MBEDTLS_RSA_NO_CRT) + int have_DP, have_DQ, have_QP; +#endif int n_missing, pq_missing, d_missing, is_pub, is_priv; RSA_VALIDATE_RET( ctx != NULL ); @@ -259,6 +262,12 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) have_D = ( mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 ); have_E = ( mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0 ); +#if !defined(MBEDTLS_RSA_NO_CRT) + have_DP = ( mbedtls_mpi_cmp_int( &ctx->DP, 0 ) != 0 ); + have_DQ = ( mbedtls_mpi_cmp_int( &ctx->DQ, 0 ) != 0 ); + have_QP = ( mbedtls_mpi_cmp_int( &ctx->QP, 0 ) != 0 ); +#endif + /* * Check whether provided parameters are enough * to deduce all others. The following incomplete @@ -324,7 +333,7 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) */ #if !defined(MBEDTLS_RSA_NO_CRT) - if( is_priv ) + if( is_priv && ! ( have_DP && have_DQ && have_QP ) ) { ret = mbedtls_rsa_deduce_crt( &ctx->P, &ctx->Q, &ctx->D, &ctx->DP, &ctx->DQ, &ctx->QP ); From 216c44d6eb62a412a81abc99095f9723e257b7bb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 31 Jan 2020 12:05:53 +0100 Subject: [PATCH 2/3] Add changelog entry --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index f03b83d09..458c82a50 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,6 +14,11 @@ Security unless the RNG is broken, and could result in information disclosure or denial of service (application crash or extra resource consumption). Found by Auke Zeilstra and Peter Schwabe, using static analysis. + * To avoid a side channel vulnerability when parsing an RSA private key, + read all the CRT parameters from the DER structure rather than + reconstructing them. Found by Alejandro Cabrera Aldaya and Billy Bob + Brumley. Reported and fix contributed by Jack Lloyd. + ARMmbed/mbed-crypto#352 Bugfix From 4cc20f6f3dc35185e48f28f9f2cad5db857319ce Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 31 Jan 2020 12:20:10 +0100 Subject: [PATCH 3/3] Fix duplicated Bugfix section in the changelog --- ChangeLog | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 458c82a50..a8f9a79b2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,11 +2,6 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.16.X branch released XXXX-XX-XX -Bugfix - * Allow loading symlinked certificates. Fixes #3005. Reported and fixed - by Jonathan Bennett via #3008. - * Fix an unchecked call to mbedtls_md() in the x509write module. - Security * Fix potential memory overread when performing an ECDSA signature operation. The overread only happens with cryptographically low @@ -21,6 +16,9 @@ Security ARMmbed/mbed-crypto#352 Bugfix + * Allow loading symlinked certificates. Fixes #3005. Reported and fixed + by Jonathan Bennett via #3008. + * Fix an unchecked call to mbedtls_md() in the x509write module. = mbed TLS 2.16.4 branch released 2020-01-15