From 100e147c71e0db398085ada3d8b8e074a79a7ad2 Mon Sep 17 00:00:00 2001
From: Jack Lloyd <jack.lloyd@fortanix.com>
Date: Wed, 29 Jan 2020 13:13:04 -0500
Subject: [PATCH 1/2] 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     |  8 +++++++-
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/library/pkparse.c b/library/pkparse.c
index ec9b55f8c5a0..3bad0ce6dd75 100644
--- a/library/pkparse.c
+++ b/library/pkparse.c
@@ -754,14 +754,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.
+    */
+
+    /* 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;
 
-    /* Check optional parameters */
+#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 4b3cc0213d3a..98c529f00509 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -251,6 +251,12 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx )
     const int have_D = ( mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 );
     const int have_E = ( mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0 );
 
+#if !defined(MBEDTLS_RSA_NO_CRT)
+    const int have_DP = ( mbedtls_mpi_cmp_int( &ctx->DP, 0 ) != 0 );
+    const int have_DQ = ( mbedtls_mpi_cmp_int( &ctx->DQ, 0 ) != 0 );
+    const int have_QP = ( mbedtls_mpi_cmp_int( &ctx->QP, 0 ) != 0 );
+#endif
+
     /*
      * Check whether provided parameters are enough
      * to deduce all others. The following incomplete
@@ -316,7 +322,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 f664c4d87827f0951e4ed6da536d9999b7f6b8c8 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 31 Jan 2020 12:05:53 +0100
Subject: [PATCH 2/2] Add changelog entry

---
 ChangeLog | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 2a993b99b654..0dac49721c9a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,13 @@ mbed TLS ChangeLog (Sorted per branch, date)
 
 = mbed TLS 2.7.X branch released XXXX-XX-XX
 
+Security
+   * 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
    * Allow loading symlinked certificates. Fixes #3005. Reported and fixed
      by Jonathan Bennett <JBennett@incomsystems.biz> via #3008.