Merge #90 #91 #94

90: Fix ECDSA side channel r=jethrogb a=jack-fortanix

Backport of ARMmbed/mbed-crypto@247c4d3
which addresses the attack described in https://eprint.iacr.org/2020/055.pdf

91: Parse RSA CRT parameters from PKCS1 private keys r=jethrogb a=jack-fortanix

Currently they are ignored in the serialized key and then regenerated from P/Q/D. But that exposes key loading to a side channel attack on the modular inversion and GCD bignum functions when QP is computed. [And probably similar issues for DP/DQ during the division step but no attack there has been published yet.]

Also this reduces computational overhead of loading RSA private keys from memory which will be nice for us in roche.

Backport of ARMmbed/mbed-crypto#352

94: Fix docs on macro invocations r=jethrogb a=jethrogb



Co-authored-by: Jack Lloyd <[email protected]>
Co-authored-by: Jethro Beekman <[email protected]>

Author: 3 people

mbedtls-sys/vendor/library/ecdsa.c

@@ -358,6 +358,7 @@ static int ecdsa_sign_restartable( mbedtls_ecp_group *grp,
         MBEDTLS_MPI_CHK( mbedtls_mpi_add_mpi( &e, &e, s ) );
         MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &e, &e, &t ) );
         MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( pk, pk, &t ) );
+        MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( pk, pk, &grp->N ) ); // CVE-2019-18222
         MBEDTLS_MPI_CHK( mbedtls_mpi_inv_mod( s, pk, &grp->N ) );
         MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( s, s, &e ) );
         MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( s, s, &grp->N ) );

mbedtls-sys/vendor/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.
+    */
+
+    /* 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 )

mbedtls-sys/vendor/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 );

mbedtls/src/pk/mod.rs

@@ -315,8 +315,10 @@ impl Pk {
             .is_ok()
     }
 
-    /// Key length in bits
-    getter!(len() -> usize = fn pk_get_bitlen);
+    getter!(
+        /// Key length in bits
+        len() -> usize = fn pk_get_bitlen
+    );
     getter!(pk_type() -> Type = fn pk_get_type);
 
     pub fn curve(&self) -> Result<EcGroupId> {

mbedtls/src/ssl/config.rs

@@ -214,11 +214,15 @@ impl<'c> Config<'c> {
         };
     }
 
-    /// Client only: whether to remember and use session tickets
-    setter!(set_session_tickets(u: UseSessionTickets) = ssl_conf_session_tickets);
-
-    /// Client only: minimal FFDH group size
-    setter!(set_ffdh_min_bitlen(bitlen: c_uint) = ssl_conf_dhm_min_bitlen);
+    setter!(
+        /// Client only: whether to remember and use session tickets
+        set_session_tickets(u: UseSessionTickets) = ssl_conf_session_tickets
+    );
+
+    setter!(
+        /// Client only: minimal FFDH group size
+        set_ffdh_min_bitlen(bitlen: c_uint) = ssl_conf_dhm_min_bitlen
+    );
 
     // TODO: The lifetime restrictions on HandshakeContext here are too strict.
     // Once we need something else, we might fix it.

mbedtls/src/wrapper_macros.rs

@@ -227,7 +227,8 @@ macro_rules! define_struct {
 }
 
 macro_rules! setter {
-    { $rfn:ident($n:ident : $rty:ty) = $cfn:ident } => {
+    { $(#[$m:meta])* $rfn:ident($n:ident : $rty:ty) = $cfn:ident } => {
+        $(#[$m])*
         pub fn $rfn(&mut self, $n: $rty) {
             unsafe{::mbedtls_sys::$cfn(&mut self.inner,$n.into())}
         }
@@ -236,9 +237,10 @@ macro_rules! setter {
 
 // can't make this work without as as_XXX! macro, and there is no as_method!...
 macro_rules! setter_callback {
-    { $s:ident<$l:tt>::$rfn:ident($n:ident : $($rty:tt)+) = $cfn:ident } => {
+    { $(#[$m:meta])* $s:ident<$l:tt>::$rfn:ident($n:ident : $($rty:tt)+) = $cfn:ident } => {
         as_item!(
         impl<$l> $s<$l> {
+            $(#[$m])*
             pub fn $rfn<F: $($rty)+>(&mut self, $n: Option<&$l mut F>) {
                 unsafe{::mbedtls_sys::$cfn(
                     &mut self.inner,
@@ -252,12 +254,14 @@ macro_rules! setter_callback {
 }
 
 macro_rules! getter {
-    { $rfn:ident() -> $rty:ty = .$cfield:ident } => {
+    { $(#[$m:meta])* $rfn:ident() -> $rty:ty = .$cfield:ident } => {
+        $(#[$m])*
         pub fn $rfn(&self) -> $rty {
             self.inner.$cfield.into()
         }
     };
-    { $rfn:ident() -> $rty:ty = fn $cfn:ident } => {
+    { $(#[$m:meta])* $rfn:ident() -> $rty:ty = fn $cfn:ident } => {
+        $(#[$m])*
         pub fn $rfn(&self) -> $rty {
             unsafe{::mbedtls_sys::$cfn(&self.inner).into()}
         }