From 80cc81103918e33b293b51c306b047b1e1911b72 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Wed, 22 Jan 2020 17:34:29 -0500 Subject: [PATCH 1/4] 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 --- library/pkparse.c | 27 +++++++++++++++++++++------ library/rsa.c | 8 ++++++-- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/library/pkparse.c b/library/pkparse.c index 596dae919..2311986f7 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -769,16 +769,31 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, goto cleanup; p += len; + /* Import DP */ + if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, + MBEDTLS_ASN1_INTEGER ) ) != 0 || + ( ret = mbedtls_mpi_read_binary( &rsa->DP, p, len ) ) != 0 ) + goto cleanup; + p += len; + + /* Import DQ */ + if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, + MBEDTLS_ASN1_INTEGER ) ) != 0 || + ( ret = mbedtls_mpi_read_binary( &rsa->DQ, p, len ) ) != 0 ) + goto cleanup; + p += len; + + /* Import QP */ + if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, + MBEDTLS_ASN1_INTEGER ) ) != 0 || + ( ret = mbedtls_mpi_read_binary( &rsa->QP, p, len ) ) != 0 ) + goto cleanup; + p += len; + /* Complete the RSA private key */ if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ) goto cleanup; - /* Check optional parameters */ - 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; - if( p != end ) { ret = MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + diff --git a/library/rsa.c b/library/rsa.c index 3c2f31438..7ea72cd84 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -249,7 +249,7 @@ static int rsa_check_context( mbedtls_rsa_context const *ctx, int is_priv, int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) { int ret = 0; - int have_N, have_P, have_Q, have_D, have_E; + int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP; int n_missing, pq_missing, d_missing, is_pub, is_priv; RSA_VALIDATE_RET( ctx != NULL ); @@ -259,6 +259,10 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) have_Q = ( mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 ); have_D = ( mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 ); have_E = ( mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0 ); + 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 ); + /* * Check whether provided parameters are enough @@ -325,7 +329,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 8c2631b6d36951c8f37d2318d28ceb6409bc4830 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Thu, 23 Jan 2020 17:23:52 -0500 Subject: [PATCH 2/4] Address review comments --- library/pkparse.c | 11 +++++++++++ library/rsa.c | 9 +++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/library/pkparse.c b/library/pkparse.c index 2311986f7..724197d79 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -769,6 +769,17 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, goto cleanup; p += len; + /* + * 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. + */ + /* Import DP */ if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, MBEDTLS_ASN1_INTEGER ) ) != 0 || diff --git a/library/rsa.c b/library/rsa.c index 7ea72cd84..dc34e38b4 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -249,7 +249,10 @@ static int rsa_check_context( mbedtls_rsa_context const *ctx, int is_priv, int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) { int ret = 0; - int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP; + 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,10 +262,12 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) have_Q = ( mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 ); 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 From 60239753d2c4d5b04984c971143037cc8ae03f97 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Mon, 27 Jan 2020 17:53:36 -0500 Subject: [PATCH 3/4] Avoid memory leak when RSA-CRT is not enabled in build --- library/pkparse.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/library/pkparse.c b/library/pkparse.c index 724197d79..ac631d93d 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -769,6 +769,7 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, goto cleanup; p += len; +#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 @@ -800,6 +801,13 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, ( ret = mbedtls_mpi_read_binary( &rsa->QP, p, len ) ) != 0 ) goto cleanup; p += len; +#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 ) From 2e9eef4f7b3939ac76b7c0790437b4a7bd6e1996 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Tue, 28 Jan 2020 14:43:52 -0500 Subject: [PATCH 4/4] Final review comments --- library/pkparse.c | 22 +++++++--------------- library/rsa.c | 2 +- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/library/pkparse.c b/library/pkparse.c index ac631d93d..7df30fea9 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -782,25 +782,17 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, */ /* Import DP */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_mpi_read_binary( &rsa->DP, p, len ) ) != 0 ) - goto cleanup; - p += len; + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DP ) ) != 0) + goto cleanup; /* Import DQ */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_mpi_read_binary( &rsa->DQ, p, len ) ) != 0 ) - goto cleanup; - p += len; + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DQ ) ) != 0) + goto cleanup; /* Import QP */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_mpi_read_binary( &rsa->QP, p, len ) ) != 0 ) - goto cleanup; - p += len; + 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 || diff --git a/library/rsa.c b/library/rsa.c index dc34e38b4..6c457468e 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -334,7 +334,7 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) */ #if !defined(MBEDTLS_RSA_NO_CRT) - if( is_priv && !(have_DP && have_DQ && have_QP)) + 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 );