From f7a8814b723df9310ef826833631bb2bd131468f Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Tue, 2 Oct 2018 22:43:06 +0200
Subject: [PATCH 01/16] Minor readability improvement

Polish the beginning of mbedtls_rsa_rsaes_pkcs1_v15_decrypt a little,
to prepare for some behavior changes.
---
 library/rsa.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index 13beba4df..29d790157 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -964,18 +964,20 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
                                  int mode, size_t *olen,
                                  const unsigned char *input,
                                  unsigned char *output,
-                                 size_t output_max_len)
+                                 size_t output_max_len )
 {
     int ret;
-    size_t ilen, pad_count = 0, i;
-    unsigned char *p, bad, pad_done = 0;
+    size_t ilen = ctx->len;
+    size_t pad_count = 0;
+    size_t i;
+    unsigned bad = 0;
+    unsigned char pad_done = 0;
     unsigned char buf[MBEDTLS_MPI_MAX_SIZE];
+    unsigned char *p = buf;
 
     if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V15 )
         return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA );
 
-    ilen = ctx->len;
-
     if( ilen < 16 || ilen > sizeof( buf ) )
         return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA );
 
@@ -986,9 +988,6 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     if( ret != 0 )
         goto cleanup;
 
-    p = buf;
-    bad = 0;
-
     /*
      * Check and get padding len in "constant-time"
      */

From f50ee60ff3d42043d1ba9a4e6d6687cb91be315c Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Tue, 2 Oct 2018 22:44:41 +0200
Subject: [PATCH 02/16] Fix a timing-based Bleichenbacher attack on PKCS#1v1.5
 decryption

mbedtls_rsa_rsaes_pkcs1_v15_decrypt took care of calculating the
padding length without leaking the amount of padding or the validity
of the padding. However it then skipped the copying of the data if the
padding was invalid, which could allow an adversary to find out
whether the padding was valid through precise timing measurements,
especially if for a local attacker who could observe memory access via
cache timings.

Avoid this leak by always copying from the decryption buffer to the
output buffer, even when the padding is invalid. With invalid padding,
copy the same amount of data as what is expected on valid padding: the
minimum valid padding size if this fits in the output buffer,
otherwise the output buffer size. To avoid leaking payload data from
an unsuccessful decryption, zero the decryption buffer before copying
if the padding was invalid.
---
 library/rsa.c | 88 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 9 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index 29d790157..11e2b7385 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -955,6 +955,37 @@ cleanup:
 #endif /* MBEDTLS_PKCS1_V21 */
 
 #if defined(MBEDTLS_PKCS1_V15)
+/** Turn zero-or-nonzero into zero-or-all-bits-one, without branches.
+ *
+ * \param value     The value to analyze.
+ * \return          \c 0 if \p value is zero, otherwise \c 0xff.
+ */
+static unsigned unsigned_all_or_nothing( unsigned value )
+{
+    /* MSVC has a warning about unary minus on unsigned, but this is
+     * well-defined and precisely what we want to do here */
+#if defined(_MSC_VER)
+#pragma warning( push )
+#pragma warning( disable : 4146 )
+#endif
+    return( - ( ( value | - value ) >> ( sizeof( value ) * 8 - 1 ) ) );
+#if defined(_MSC_VER)
+#pragma warning( pop )
+#endif
+}
+
+/** Choose between two integer values, without branches.
+ *
+ * \param mask      Either \c 0 or \c ~0.
+ * \param if0       Value to use if \p mask = \c 0.
+ * \param if1       Value to use if \p mask = \c ~0.
+ * \return          \c if1 if \p value is zero, otherwise \c if0.
+ */
+static unsigned choose_int_from_mask( unsigned mask, unsigned if1, unsigned if0 )
+{
+    return( ( mask & if1 ) | (~mask & if0 ) );
+}
+
 /*
  * Implementation of the PKCS#1 v2.1 RSAES-PKCS1-V1_5-DECRYPT function
  */
@@ -972,6 +1003,10 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     size_t i;
     unsigned bad = 0;
     unsigned char pad_done = 0;
+    size_t plaintext_size = 0;
+    size_t plaintext_max_size = ( output_max_len > ilen - 11 ?
+                                  ilen - 11 :
+                                  output_max_len );
     unsigned char buf[MBEDTLS_MPI_MAX_SIZE];
     unsigned char *p = buf;
 
@@ -1025,23 +1060,58 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
         bad |= *p++; /* Must be zero */
     }
 
+    /* There must be at least 8 bytes of padding. */
     bad |= ( pad_count < 8 );
 
-    if( bad )
-    {
-        ret = MBEDTLS_ERR_RSA_INVALID_PADDING;
-        goto cleanup;
-    }
+    /* Set bad to zero if the padding is valid and
+     * all-bits-one otherwise. The whole calculation of bad
+     * is done in such a way to avoid branches. */
+    bad = unsigned_all_or_nothing( bad );
 
-    if( ilen - ( p - buf ) > output_max_len )
+    /* If the padding is valid, set plaintext_size to the number of
+     * remaining bytes after stripping the padding. If the padding
+     * is invalid, avoid leaking this fact through the size of the
+     * output: use the maximum message size that fits in the output
+     * buffer. Do it without branches to avoid leaking the padding
+     * validity through timing. RSA keys are small enough that all the
+     * size_t values involved fit in unsigned int. */
+    plaintext_size = choose_int_from_mask( bad,
+                                           (unsigned) plaintext_max_size,
+                                           (unsigned) ( ilen - ( p - buf ) ) );
+
+    /* Check if the decrypted plaintext fits in the output buffer.
+     * If the padding is bad, this will always be the case,
+     * thus we don't leak the padding validity by trying to produce
+     * a larger output than what the caller expects. */
+    if( plaintext_size > output_max_len )
     {
         ret = MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE;
         goto cleanup;
     }
 
-    *olen = ilen - (p - buf);
-    memcpy( output, p, *olen );
-    ret = 0;
+    /* Set ret to INVALID_PADDING if the padding is bad and to 0
+     * otherwise. At this point, the variable bad is zero if
+     * the padding is good and can be any nonzero value otherwise.
+     * Do this without branches to avoid timing attacks. */
+    ret = - ( bad & ( - MBEDTLS_ERR_RSA_INVALID_PADDING ) );
+
+    /* If the padding is bad, zero the data that we're about to copy
+     * to the output buffer. We need to copy the same amount of data
+     * from the same buffer whether the padding is good or not to
+     * avoid leaking the padding validity through overall timing or
+     * through memory or cache access patterns. */
+    for( i = 11; i < ilen; i++ )
+        buf[i] &= ~bad;
+
+    /* Copy the decrypted plaintext from the end of the buffer. */
+    memcpy( output, buf + ilen - plaintext_size, plaintext_size );
+
+    /* Report the amount of data we copied to the output buffer.
+     * When the padding is invalid, the value of *olen when this
+     * function returns is not specified. Making it equivalent to
+     * the good-padding case limits the risks of leaking the
+     * padding validity. */
+    *olen = plaintext_size;
 
 cleanup:
     mbedtls_zeroize( buf, sizeof( buf ) );

From b0034327cbbb0015c7ba4408705c4a886e810d85 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Wed, 3 Oct 2018 13:40:16 +0200
Subject: [PATCH 03/16] Add ChangeLog entry

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

diff --git a/ChangeLog b/ChangeLog
index 8c82d08d1..652f013e0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,12 @@ mbed TLS ChangeLog (Sorted per branch, date)
 
 = mbed TLS x.x.x branch released xxxx-xx-xx
 
+Security
+   * Fix a timing variation in RSA PKCS#1 v1.5 decryption that could
+     lead to a Bleichenbacher-style attack. In TLS, this affects
+     RSA-based ciphersuites without DHE or ECDHE. Reported by Yuval Yarom,
+     Eyal Ronen, Adi Shamir, David Wong and Daniel Genkin.
+
 Bugfix
     * Fix failure in hmac_drbg in the benchmark sample application, when
       MBEDTLS_THREADING_C is defined. Found by TrinityTonic, #1095

From 9f11f21a26bec6dfd6c60a2ef366adcac7e9e279 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 4 Oct 2018 18:32:29 +0200
Subject: [PATCH 04/16] Evolve choose_int_from_mask to if_int

Make the function more robust by taking an arbitrary zero/nonzero
argument instead of insisting on zero/all-bits-one. Update and fix its
documentation.
---
 library/rsa.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index 11e2b7385..ede1381a5 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -958,9 +958,9 @@ cleanup:
 /** Turn zero-or-nonzero into zero-or-all-bits-one, without branches.
  *
  * \param value     The value to analyze.
- * \return          \c 0 if \p value is zero, otherwise \c 0xff.
+ * \return          Zero if \p value is zero, otherwise all-bits-one.
  */
-static unsigned unsigned_all_or_nothing( unsigned value )
+static unsigned all_or_nothing_int( unsigned value )
 {
     /* MSVC has a warning about unary minus on unsigned, but this is
      * well-defined and precisely what we want to do here */
@@ -976,13 +976,17 @@ static unsigned unsigned_all_or_nothing( unsigned value )
 
 /** Choose between two integer values, without branches.
  *
- * \param mask      Either \c 0 or \c ~0.
- * \param if0       Value to use if \p mask = \c 0.
- * \param if1       Value to use if \p mask = \c ~0.
- * \return          \c if1 if \p value is zero, otherwise \c if0.
+ * This is equivalent to `cond ? if1 : if0`, but is likely to be compiled
+ * to code using bitwise operation rather than a branch.
+ *
+ * \param cond      Condition to test.
+ * \param if1       Value to use if \p cond is nonzero.
+ * \param if0       Value to use if \p cond is zero.
+ * \return          \c if1 if \p cond is nonzero, otherwise \c if0.
  */
-static unsigned choose_int_from_mask( unsigned mask, unsigned if1, unsigned if0 )
+static unsigned if_int( unsigned cond, unsigned if1, unsigned if0 )
 {
+    unsigned mask = all_or_nothing_int( cond );
     return( ( mask & if1 ) | (~mask & if0 ) );
 }
 
@@ -1066,7 +1070,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     /* Set bad to zero if the padding is valid and
      * all-bits-one otherwise. The whole calculation of bad
      * is done in such a way to avoid branches. */
-    bad = unsigned_all_or_nothing( bad );
+    bad = all_or_nothing_int( bad );
 
     /* If the padding is valid, set plaintext_size to the number of
      * remaining bytes after stripping the padding. If the padding
@@ -1075,9 +1079,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
      * buffer. Do it without branches to avoid leaking the padding
      * validity through timing. RSA keys are small enough that all the
      * size_t values involved fit in unsigned int. */
-    plaintext_size = choose_int_from_mask( bad,
-                                           (unsigned) plaintext_max_size,
-                                           (unsigned) ( ilen - ( p - buf ) ) );
+    plaintext_size = if_int( bad,
+                             (unsigned) plaintext_max_size,
+                             (unsigned) ( ilen - ( p - buf ) ) );
 
     /* Check if the decrypted plaintext fits in the output buffer.
      * If the padding is bad, this will always be the case,

From 2036508538d46a80576f05d7c241a21927622e74 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 4 Oct 2018 19:13:43 +0200
Subject: [PATCH 05/16] Bleichenbacher fix: don't leak the plaintext length
 (step 1)

mbedtls_rsa_rsaes_pkcs1_v15_decrypt takes care not to reveal whether
the padding is valid or not, even through timing or memory access
patterns. This is a defense against an attack published by
Bleichenbacher. The attacker can also obtain the same information by
observing the length of the plaintext. The current implementation
leaks the length of the plaintext through timing and memory access
patterns.

This commit is a first step towards fixing this leak. It reduces the
leak to a single memmove call inside the working buffer.
---
 library/rsa.c | 69 +++++++++++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index ede1381a5..ec7e1da9b 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1011,6 +1011,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     size_t plaintext_max_size = ( output_max_len > ilen - 11 ?
                                   ilen - 11 :
                                   output_max_len );
+    unsigned output_too_large;
     unsigned char buf[MBEDTLS_MPI_MAX_SIZE];
     unsigned char *p = buf;
 
@@ -1067,11 +1068,6 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     /* There must be at least 8 bytes of padding. */
     bad |= ( pad_count < 8 );
 
-    /* Set bad to zero if the padding is valid and
-     * all-bits-one otherwise. The whole calculation of bad
-     * is done in such a way to avoid branches. */
-    bad = all_or_nothing_int( bad );
-
     /* If the padding is valid, set plaintext_size to the number of
      * remaining bytes after stripping the padding. If the padding
      * is invalid, avoid leaking this fact through the size of the
@@ -1083,38 +1079,53 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
                              (unsigned) plaintext_max_size,
                              (unsigned) ( ilen - ( p - buf ) ) );
 
-    /* Check if the decrypted plaintext fits in the output buffer.
-     * If the padding is bad, this will always be the case,
-     * thus we don't leak the padding validity by trying to produce
-     * a larger output than what the caller expects. */
-    if( plaintext_size > output_max_len )
-    {
-        ret = MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE;
-        goto cleanup;
-    }
+    /* Set output_too_large to 0 if the plaintext fits in the output
+     * buffer and to 1 otherwise. This is the sign bit (1 for negative)
+     * of (output_max_len - plaintext_size). */
+    output_too_large = ( ( output_max_len - plaintext_size ) >>
+                         ( ( sizeof( output_max_len ) * 8 - 1 ) ) );
 
-    /* Set ret to INVALID_PADDING if the padding is bad and to 0
-     * otherwise. At this point, the variable bad is zero if
-     * the padding is good and can be any nonzero value otherwise.
-     * Do this without branches to avoid timing attacks. */
-    ret = - ( bad & ( - MBEDTLS_ERR_RSA_INVALID_PADDING ) );
+    /* Set ret without branches to avoid timing attacks. Return:
+     * - INVALID_PADDING if the padding is bad (bad != 0).
+     * - OUTPUT_TOO_LARGE if the padding is good but the decrypted
+     *   plaintext does not fit in the output buffer.
+     * - 0 if the padding is correct. */
+    ret = - if_int( bad, - MBEDTLS_ERR_RSA_INVALID_PADDING,
+            if_int( output_too_large, - MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE,
+                    0 ) );
 
-    /* If the padding is bad, zero the data that we're about to copy
-     * to the output buffer. We need to copy the same amount of data
+    /* If the padding is bad or the plaintext is too large, zero the
+     * data that we're about to copy to the output buffer.
+     * We need to copy the same amount of data
      * from the same buffer whether the padding is good or not to
      * avoid leaking the padding validity through overall timing or
      * through memory or cache access patterns. */
     for( i = 11; i < ilen; i++ )
-        buf[i] &= ~bad;
+        buf[i] &= ~( bad | output_too_large );
 
-    /* Copy the decrypted plaintext from the end of the buffer. */
-    memcpy( output, buf + ilen - plaintext_size, plaintext_size );
+    /* If the plaintext is too large, truncate it to the buffer size.
+     * Copy anyway to avoid revealing the length through timing, because
+     * revealing the length is as bad as revealing the padding validity
+     * for a Bleichenbacher attack. */
+    plaintext_size = if_int( output_too_large,
+                             (unsigned) plaintext_max_size,
+                             (unsigned) plaintext_size );
 
-    /* Report the amount of data we copied to the output buffer.
-     * When the padding is invalid, the value of *olen when this
-     * function returns is not specified. Making it equivalent to
-     * the good-padding case limits the risks of leaking the
-     * padding validity. */
+    /* Move the plaintext to the beginning of the working buffer so that
+     * its position no longer depends on the padding and we have enough
+     * room from the beginning of the plaintext to copy a number of bytes
+     * that does not depend on the padding.  */
+    /* FIXME: we need a constant-time, constant-trace memmove here. */
+    memmove( buf, p, plaintext_size );
+
+    /* Finally copy the decrypted plaintext plus trailing data
+     * into the output buffer. */
+    memcpy( output, buf, plaintext_max_size );
+
+    /* Report the amount of data we copied to the output buffer. In case
+     * of errors (bad padding or output too large), the value of *olen
+     * when this function returns is not specified. Making it equivalent
+     * to the good case limits the risks of leaking the padding validity. */
     *olen = plaintext_size;
 
 cleanup:

From 9fb28dd9e706154e70b0da0b789078c8ad2159a8 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 4 Oct 2018 21:18:30 +0200
Subject: [PATCH 06/16] Bleichenbacher fix: don't leak the plaintext length
 (step 2)

Replace memmove(to, to + offset, length) by a functionally equivalent
function that strives to make the same memory access patterns
regardless of the value of length. This fixes an information leak
through timing (especially timing of memory accesses via cache probes)
that leads to a Bleichenbacher-style attack on PKCS#1 v1.5 decryption
using the plaintext length as the observable.
---
 library/rsa.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index ec7e1da9b..becbd6fa4 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -974,6 +974,22 @@ static unsigned all_or_nothing_int( unsigned value )
 #endif
 }
 
+/** Check whether a size is out of bounds, without branches.
+ *
+ * This is equivalent to `size > max`, but is likely to be compiled to
+ * to code using bitwise operation rather than a branch.
+ *
+ * \param size      Size to check.
+ * \param max       Maximum desired value for \p size.
+ * \return          \c 0 if `size <= max`.
+ * \return          \c 1 if `size > max`.
+ */
+static unsigned size_greater_than( size_t size, size_t max )
+{
+    /* Return the sign bit (1 for negative) of (max - size). */
+    return( ( max - size ) >> ( sizeof( size_t ) * 8 - 1 ) );
+}
+
 /** Choose between two integer values, without branches.
  *
  * This is equivalent to `cond ? if1 : if0`, but is likely to be compiled
@@ -990,6 +1006,42 @@ static unsigned if_int( unsigned cond, unsigned if1, unsigned if0 )
     return( ( mask & if1 ) | (~mask & if0 ) );
 }
 
+/** Shift some data towards the left inside a buffer without leaking
+ * the length of the data through side channels.
+ *
+ * `mem_move_to_left(start, total, offset)` is functionally equivalent to
+ * ```
+ * memmove(start, start + offset, total - offset);
+ * memset(start + offset, 0, total - offset);
+ * ```
+ * but it strives to use a memory access pattern (and thus total timing)
+ * that does not depend on \p offset. This timing independence comes at
+ * the expense of performance.
+ *
+ * \param start     Pointer to the start of the buffer.
+ * \param total     Total size of the buffer.
+ * \param offset    Offset from which to copy \p total - \p offset bytes.
+ */
+static void mem_move_to_left( void *start,
+                              size_t total,
+                              size_t offset )
+{
+    volatile unsigned char *buf = start;
+    size_t i, n;
+    if( total == 0 )
+        return;
+    for( i = 0; i < total; i++ )
+    {
+        unsigned no_op = size_greater_than( total - offset, i );
+        /* The first `total - offset` passes are a no-op. The last
+         * `offset` passes shift the data one byte to the left and
+         * zero out the last byte. */
+        for( n = 0; n < total - 1; n++ )
+            buf[n] = if_int( no_op, buf[n], buf[n+1] );
+        buf[total-1] = if_int( no_op, buf[total-1], 0 );
+    }
+}
+
 /*
  * Implementation of the PKCS#1 v2.1 RSAES-PKCS1-V1_5-DECRYPT function
  */
@@ -1115,8 +1167,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
      * its position no longer depends on the padding and we have enough
      * room from the beginning of the plaintext to copy a number of bytes
      * that does not depend on the padding.  */
-    /* FIXME: we need a constant-time, constant-trace memmove here. */
-    memmove( buf, p, plaintext_size );
+    mem_move_to_left( buf, ilen, ilen - plaintext_size );
 
     /* Finally copy the decrypted plaintext plus trailing data
      * into the output buffer. */

From 08513ce341c65b812f3c5c16ff54f74997329f3e Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 4 Oct 2018 21:24:21 +0200
Subject: [PATCH 07/16] Use branch-free size comparison for the padding size

In mbedtls_rsa_rsaes_pkcs1_v15_decrypt, use size_greater_than (which
is based on bitwise operations) instead of the < operator to compare
sizes when the values being compared must not leak. Some compilers
compile < to a branch at least under some circumstances (observed with
gcc 5.4 for arm-gnueabi -O9 on a toy program).
---
 library/rsa.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index becbd6fa4..7588a7017 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1118,7 +1118,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     }
 
     /* There must be at least 8 bytes of padding. */
-    bad |= ( pad_count < 8 );
+    bad |= size_greater_than( 8, pad_count );
 
     /* If the padding is valid, set plaintext_size to the number of
      * remaining bytes after stripping the padding. If the padding
@@ -1132,10 +1132,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
                              (unsigned) ( ilen - ( p - buf ) ) );
 
     /* Set output_too_large to 0 if the plaintext fits in the output
-     * buffer and to 1 otherwise. This is the sign bit (1 for negative)
-     * of (output_max_len - plaintext_size). */
-    output_too_large = ( ( output_max_len - plaintext_size ) >>
-                         ( ( sizeof( output_max_len ) * 8 - 1 ) ) );
+     * buffer and to 1 otherwise. */
+    output_too_large = size_greater_than( plaintext_size,
+                                          plaintext_max_size );
 
     /* Set ret without branches to avoid timing attacks. Return:
      * - INVALID_PADDING if the padding is bad (bad != 0).

From f19aefb00b5bb1c73829d8cf33d8bebc08582389 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 4 Oct 2018 22:45:13 +0200
Subject: [PATCH 08/16] Minor optimization in the PKCS#1v1.5 unpadding step

Rather than doing the quadratic-time constant-memory-trace on the
whole working buffer, do it on the section of the buffer where the
data to copy has to lie, which can be significantly smaller if the
output buffer is significantly smaller than the working buffer, e.g.
for TLS RSA ciphersuites (48 bytes vs MBEDTLS_MPI_MAX_SIZE).
---
 library/rsa.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index 7588a7017..2ca27d991 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1162,15 +1162,19 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
                              (unsigned) plaintext_max_size,
                              (unsigned) plaintext_size );
 
-    /* Move the plaintext to the beginning of the working buffer so that
-     * its position no longer depends on the padding and we have enough
-     * room from the beginning of the plaintext to copy a number of bytes
-     * that does not depend on the padding.  */
-    mem_move_to_left( buf, ilen, ilen - plaintext_size );
+    /* Move the plaintext to the leftmost position where it can start in
+     * the working buffer, i.e. make it start plaintext_max_size from
+     * the end of the buffer. Do this with a memory access trace that
+     * does not depend on the plaintext size. After this move, the
+     * starting location of the plaintext is no longer sensitive
+     * information. */
+    p = buf + ilen - plaintext_max_size;
+    mem_move_to_left( p, plaintext_max_size,
+                      plaintext_max_size - plaintext_size );
 
-    /* Finally copy the decrypted plaintext plus trailing data
+    /* Finally copy the decrypted plaintext plus trailing zeros
      * into the output buffer. */
-    memcpy( output, buf, plaintext_max_size );
+    memcpy( output, p, plaintext_max_size );
 
     /* Report the amount of data we copied to the output buffer. In case
      * of errors (bad padding or output too large), the value of *olen

From c5552e8ea39c62daf9e482fc6807b6fbe3ea0845 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 5 Oct 2018 14:50:21 +0200
Subject: [PATCH 09/16] mbedtls_rsa_rsaes_pkcs1_v15_decrypt: remove the
 variable p

Get rid of the variable p. This makes it more apparent where the code
accesses the buffer at an offset whose value is sensitive.

No intended behavior change in this commit.
---
 library/rsa.c | 50 ++++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index 2ca27d991..facb11a52 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1055,17 +1055,26 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
 {
     int ret;
     size_t ilen = ctx->len;
-    size_t pad_count = 0;
     size_t i;
-    unsigned bad = 0;
-    unsigned char pad_done = 0;
-    size_t plaintext_size = 0;
     size_t plaintext_max_size = ( output_max_len > ilen - 11 ?
                                   ilen - 11 :
                                   output_max_len );
-    unsigned output_too_large;
     unsigned char buf[MBEDTLS_MPI_MAX_SIZE];
-    unsigned char *p = buf;
+    /* The following variables take sensitive values: their value must
+     * not leak into the observable behavior of the function other than
+     * the designated outputs (output, olen, return value). Otherwise
+     * this would open the execution of the function to
+     * side-channel-based variants of the Bleichenbacher padding oracle
+     * attack. Potential side channels include overall timing, memory
+     * access patterns (especially visible to an adversary who has access
+     * to a shared memory cache), and branches (especially visible to
+     * an adversary who has access to a shared code cache or to a shared
+     * branch predictor). */
+    size_t pad_count = 0;
+    unsigned bad = 0;
+    unsigned char pad_done = 0;
+    size_t plaintext_size = 0;
+    unsigned output_too_large;
 
     if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V15 )
         return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA );
@@ -1083,38 +1092,35 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     /*
      * Check and get padding len in "constant-time"
      */
-    bad |= *p++; /* First byte must be 0 */
+    bad |= buf[0]; /* First byte must be 0 */
 
     /* This test does not depend on secret data */
     if( mode == MBEDTLS_RSA_PRIVATE )
     {
-        bad |= *p++ ^ MBEDTLS_RSA_CRYPT;
+        bad |= buf[1] ^ MBEDTLS_RSA_CRYPT;
 
         /* Get padding len, but always read till end of buffer
          * (minus one, for the 00 byte) */
-        for( i = 0; i < ilen - 3; i++ )
+        for( i = 2; i < ilen - 1; i++ )
         {
-            pad_done  |= ((p[i] | (unsigned char)-p[i]) >> 7) ^ 1;
+            pad_done  |= ((buf[i] | (unsigned char)-buf[i]) >> 7) ^ 1;
             pad_count += ((pad_done | (unsigned char)-pad_done) >> 7) ^ 1;
         }
 
-        p += pad_count;
-        bad |= *p++; /* Must be zero */
+        bad |= buf[pad_count + 2];
     }
     else
     {
-        bad |= *p++ ^ MBEDTLS_RSA_SIGN;
+        bad |= buf[1] ^ MBEDTLS_RSA_SIGN;
 
         /* Get padding len, but always read till end of buffer
          * (minus one, for the 00 byte) */
-        for( i = 0; i < ilen - 3; i++ )
+        for( i = 2; i < ilen - 1; i++ )
         {
-            pad_done |= ( p[i] != 0xFF );
+            pad_done |= ( buf[i] != 0xFF );
             pad_count += ( pad_done == 0 );
         }
-
-        p += pad_count;
-        bad |= *p++; /* Must be zero */
+        bad |= buf[pad_count + 2];
     }
 
     /* There must be at least 8 bytes of padding. */
@@ -1129,7 +1135,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
      * size_t values involved fit in unsigned int. */
     plaintext_size = if_int( bad,
                              (unsigned) plaintext_max_size,
-                             (unsigned) ( ilen - ( p - buf ) ) );
+                             (unsigned) ( ilen - pad_count - 3 ) );
 
     /* Set output_too_large to 0 if the plaintext fits in the output
      * buffer and to 1 otherwise. */
@@ -1168,13 +1174,13 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
      * does not depend on the plaintext size. After this move, the
      * starting location of the plaintext is no longer sensitive
      * information. */
-    p = buf + ilen - plaintext_max_size;
-    mem_move_to_left( p, plaintext_max_size,
+    mem_move_to_left( buf + ilen - plaintext_max_size,
+                      plaintext_max_size,
                       plaintext_max_size - plaintext_size );
 
     /* Finally copy the decrypted plaintext plus trailing zeros
      * into the output buffer. */
-    memcpy( output, p, plaintext_max_size );
+    memcpy( output, buf + ilen - plaintext_max_size, plaintext_max_size );
 
     /* Report the amount of data we copied to the output buffer. In case
      * of errors (bad padding or output too large), the value of *olen

From fde301a409ef626571a956bfe1eda194dae0e91c Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 5 Oct 2018 15:06:12 +0200
Subject: [PATCH 10/16] Remove a remaining sensitive memory access in PKCS#1
 v1.5 decryption

---
 library/rsa.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index facb11a52..e968605d3 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1089,14 +1089,14 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
     if( ret != 0 )
         goto cleanup;
 
-    /*
-     * Check and get padding len in "constant-time"
-     */
-    bad |= buf[0]; /* First byte must be 0 */
+    /* Check and get padding length in constant time and constant
+     * memory trace. The first byte must be 0. */
+    bad |= buf[0];
 
-    /* This test does not depend on secret data */
     if( mode == MBEDTLS_RSA_PRIVATE )
     {
+        /* Decode EME-PKCS1-v1_5 padding: 0x00 || 0x02 || PS || 0x00
+         * where PS must be at least 8 nonzero bytes. */
         bad |= buf[1] ^ MBEDTLS_RSA_CRYPT;
 
         /* Get padding len, but always read till end of buffer
@@ -1106,23 +1106,26 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
             pad_done  |= ((buf[i] | (unsigned char)-buf[i]) >> 7) ^ 1;
             pad_count += ((pad_done | (unsigned char)-pad_done) >> 7) ^ 1;
         }
-
-        bad |= buf[pad_count + 2];
     }
     else
     {
+        /* Decode EMSA-PKCS1-v1_5 padding: 0x00 || 0x01 || PS || 0x00
+         * where PS must be at least 8 bytes with the value 0xFF. */
         bad |= buf[1] ^ MBEDTLS_RSA_SIGN;
 
         /* Get padding len, but always read till end of buffer
          * (minus one, for the 00 byte) */
         for( i = 2; i < ilen - 1; i++ )
         {
-            pad_done |= ( buf[i] != 0xFF );
-            pad_count += ( pad_done == 0 );
+            pad_done |= if_int( buf[i], 0, 1 );
+            pad_count += if_int( pad_done, 0, 1 );
+            bad |= if_int( pad_done, 0, buf[i] ^ 0xFF );
         }
-        bad |= buf[pad_count + 2];
     }
 
+    /* If pad_done is still zero, there's no data, only unfinished padding. */
+    bad |= if_int( pad_done, 0, 1 );
+
     /* There must be at least 8 bytes of padding. */
     bad |= size_greater_than( 8, pad_count );
 
@@ -1157,8 +1160,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
      * from the same buffer whether the padding is good or not to
      * avoid leaking the padding validity through overall timing or
      * through memory or cache access patterns. */
+    bad = all_or_nothing_int( bad | output_too_large );
     for( i = 11; i < ilen; i++ )
-        buf[i] &= ~( bad | output_too_large );
+        buf[i] &= ~bad;
 
     /* If the plaintext is too large, truncate it to the buffer size.
      * Copy anyway to avoid revealing the length through timing, because

From ed3bdd82f8349df9c5947775fc419005fd28fd0c Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 5 Oct 2018 15:42:52 +0200
Subject: [PATCH 11/16] Indicate the memory access variations in the changelog
 entry

---
 ChangeLog | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 652f013e0..98b95ca46 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,10 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date)
 = mbed TLS x.x.x branch released xxxx-xx-xx
 
 Security
-   * Fix a timing variation in RSA PKCS#1 v1.5 decryption that could
-     lead to a Bleichenbacher-style attack. In TLS, this affects
-     RSA-based ciphersuites without DHE or ECDHE. Reported by Yuval Yarom,
-     Eyal Ronen, Adi Shamir, David Wong and Daniel Genkin.
+   * Fix timing variations and memory access variations in RSA PKCS#1 v1.5
+     decryption that could lead to a Bleichenbacher-style padding oracle
+     attack. In TLS, this affects RSA-based ciphersuites without DHE or
+     ECDHE. Reported by Yuval Yarom, Eyal Ronen, Adi Shamir, David Wong and
+     Daniel Genkin.
 
 Bugfix
     * Fix failure in hmac_drbg in the benchmark sample application, when

From 2bd6518d5ed6af058cccc086cf4ead46142d39e7 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 5 Oct 2018 18:11:27 +0200
Subject: [PATCH 12/16] PKCS#1 v1.5 decoding: fix empty payload case

---
 library/rsa.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index e968605d3..8d55acd69 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1099,9 +1099,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
          * where PS must be at least 8 nonzero bytes. */
         bad |= buf[1] ^ MBEDTLS_RSA_CRYPT;
 
-        /* Get padding len, but always read till end of buffer
-         * (minus one, for the 00 byte) */
-        for( i = 2; i < ilen - 1; i++ )
+        /* Read the whole buffer. Set pad_done to nonzero if we find
+         * the 0x00 byte and remember the padding length in pad_count. */
+        for( i = 2; i < ilen; i++ )
         {
             pad_done  |= ((buf[i] | (unsigned char)-buf[i]) >> 7) ^ 1;
             pad_count += ((pad_done | (unsigned char)-pad_done) >> 7) ^ 1;
@@ -1113,9 +1113,10 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
          * where PS must be at least 8 bytes with the value 0xFF. */
         bad |= buf[1] ^ MBEDTLS_RSA_SIGN;
 
-        /* Get padding len, but always read till end of buffer
-         * (minus one, for the 00 byte) */
-        for( i = 2; i < ilen - 1; i++ )
+        /* Read the whole buffer. Set pad_done to nonzero if we find
+         * the 0x00 byte and remember the padding length in pad_count.
+         * If there's a non-0xff byte in the padding, the padding is bad. */
+        for( i = 2; i < ilen; i++ )
         {
             pad_done |= if_int( buf[i], 0, 1 );
             pad_count += if_int( pad_done, 0, 1 );

From 58e60097cd4f4a4ff655b005ab6a49587011fb54 Mon Sep 17 00:00:00 2001
From: Simon Butcher <simon.butcher@arm.com>
Date: Tue, 12 Apr 2016 11:31:00 +0100
Subject: [PATCH 13/16] Adds test_suite_pkcs1_v15 to tests/Makefile

---
 tests/Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile b/tests/Makefile
index 40e65fab0..9607ee5df 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -76,7 +76,7 @@ APPS =	test_suite_aes.ecb$(EXEXT)	test_suite_aes.cbc$(EXEXT)	\
 	test_suite_md$(EXEXT)		test_suite_mdx$(EXEXT)		\
 	test_suite_memory_buffer_alloc$(EXEXT)				\
 	test_suite_mpi$(EXEXT)						\
-	test_suite_pem$(EXEXT)						\
+	test_suite_pem$(EXEXT)			test_suite_pkcs1_v15$(EXEXT)	\
 	test_suite_pkcs1_v21$(EXEXT)	test_suite_pkcs5$(EXEXT)	\
 	test_suite_pkparse$(EXEXT)	test_suite_pkwrite$(EXEXT)	\
 	test_suite_pk$(EXEXT)						\
@@ -372,6 +372,10 @@ test_suite_pem$(EXEXT): test_suite_pem.c $(DEP)
 	echo "  CC    $<"
 	$(CC) $(LOCAL_CFLAGS) $(CFLAGS) $<	$(LOCAL_LDFLAGS) $(LDFLAGS) -o $@
 
+test_suite_pkcs1_v15$(EXEXT): test_suite_pkcs1_v15.c $(DEP)
+	echo "  CC    $<"
+	$(CC) $(LOCAL_CFLAGS) $(CFLAGS) $<	$(LOCAL_LDFLAGS) $(LDFLAGS) -o $@
+
 test_suite_pkcs1_v21$(EXEXT): test_suite_pkcs1_v21.c $(DEP)
 	echo "  CC    $<"
 	$(CC) $(LOCAL_CFLAGS) $(CFLAGS) $<	$(LOCAL_LDFLAGS) $(LDFLAGS) -o $@

From 3b3d5e24ea8101ed682bccaa24d936c4ea4e913f Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 5 Oct 2018 18:15:25 +0200
Subject: [PATCH 14/16] Add tests for PKCS#1 v1.5 decoding

Functional tests for various payload sizes and output buffer sizes.

When the padding is bad or the plaintext is too large for the output
buffer, verify that function writes some outputs. This doesn't
validate that the implementation is time-constant, but it at least
validates that it doesn't just return early without outputting anything.
---
 tests/suites/test_suite_pkcs1_v15.data     |  90 ++++++++++++
 tests/suites/test_suite_pkcs1_v15.function | 155 +++++++++++++++++++++
 2 files changed, 245 insertions(+)

diff --git a/tests/suites/test_suite_pkcs1_v15.data b/tests/suites/test_suite_pkcs1_v15.data
index 030940007..a4d6eb545 100644
--- a/tests/suites/test_suite_pkcs1_v15.data
+++ b/tests/suites/test_suite_pkcs1_v15.data
@@ -33,3 +33,93 @@ pkcs1_rsassa_v15_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0
 
 RSASSA-V15 Verification Test Vector Int
 pkcs1_rsassa_v15_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"2154f928615e5101fcdeb57bc08fc2f35c3d5996403861ae3efb1d0712f8bb05cc21f7f5f11f62e5b6ea9f0f2b62180e5cbe7ba535032d6ac8068fff7f362f73d2c3bf5eca6062a1723d7cfd5abb6dcf7e405f2dc560ffe6fc37d38bee4dc9e24fe2bece3e3b4a3f032701d3f0947b42930083dd4ad241b3309b514595482d42":0
+
+RSAES-V15 decoding: good, payload=max, tight output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505152535455565700":117:117:0
+
+RSAES-V15 decoding: good, payload=max, larger output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505152535455565700":117:128:0
+
+RSAES-V15 decoding: good, payload=max-1, tight output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000250515253545556575800":116:116:0
+
+RSAES-V15 decoding: good, payload=max-1, larger output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000250515253545556575800":116:117:0
+
+RSAES-V15 decoding: good, payload=1
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"00025050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505000":1:1:0
+
+RSAES-V15 decoding: good, empty payload
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505000":0:0:0
+
+RSAES-V15 decoding: payload=max, output too large
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505152535455565700":117:116:MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE
+
+RSAES-V15 decoding: payload=max-1, output too large
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000250515253545556575800":116:115:MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE
+
+RSAES-V15 decoding: bad first byte
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0102505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+RSAES-V15 decoding: bad second byte (0 instead of 2)
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0000505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+RSAES-V15 decoding: bad second byte (1 instead of 2)
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0001505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+RSAES-V15 decoding: padding too short (0)
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000200":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+RSAES-V15 decoding: padding too short (7)
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505050505050500000ffffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+RSAES-V15 decoding: unfinished padding
+pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: good, payload=max, tight output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffff00":117:117:0
+
+EMSA-V15 decoding: good, payload=max, larger output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffff00":117:128:0
+
+EMSA-V15 decoding: good, payload=max-1, tight output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffff00":116:116:0
+
+EMSA-V15 decoding: good, payload=max-1, larger output buffer
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffff00":116:117:0
+
+EMSA-V15 decoding: good, payload=1
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00":1:1:0
+
+EMSA-V15 decoding: good, empty payload
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00":0:0:0
+
+EMSA-V15 decoding: bad first byte
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0101ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: bad second byte (0 instead of 1)
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0000ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: bad second byte (2 instead of 1)
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0002ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: padding too short (0)
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"000100":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: padding too short (7)
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffff0000ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: invalid padding at first byte
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001fffffffffffffffe00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: invalid padding at last byte
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001feffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: unfinished padding
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: unfinished padding with invalid first byte
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001feffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
+
+EMSA-V15 decoding: unfinished padding with invalid last byte
+pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING
diff --git a/tests/suites/test_suite_pkcs1_v15.function b/tests/suites/test_suite_pkcs1_v15.function
index 09fe05bb3..7edeb3f59 100644
--- a/tests/suites/test_suite_pkcs1_v15.function
+++ b/tests/suites/test_suite_pkcs1_v15.function
@@ -108,6 +108,161 @@ exit:
 }
 /* END_CASE */
 
+/* BEGIN_CASE */
+void pkcs1_v15_decode( int mode,
+                       char *input_hex,
+                       int expected_plaintext_length_arg,
+                       int output_size_arg,
+                       int expected_result )
+{
+    size_t input_len;
+    size_t expected_plaintext_length = expected_plaintext_length_arg;
+    size_t output_size = output_size_arg;
+    rnd_pseudo_info rnd_info;
+    mbedtls_mpi P1, Q1, H, G;
+    mbedtls_rsa_context ctx;
+    static unsigned char N[128] = {
+        0xc4, 0x79, 0x4c, 0x6d, 0xb2, 0xe9, 0xdf, 0xc5,
+        0xe5, 0xd7, 0x55, 0x4b, 0xfb, 0x6c, 0x2e, 0xec,
+        0x84, 0xd0, 0x88, 0x12, 0xaf, 0xbf, 0xb4, 0xf5,
+        0x47, 0x3c, 0x7e, 0x92, 0x4c, 0x58, 0xc8, 0x73,
+        0xfe, 0x8f, 0x2b, 0x8f, 0x8e, 0xc8, 0x5c, 0xf5,
+        0x05, 0xeb, 0xfb, 0x0d, 0x7b, 0x2a, 0x93, 0xde,
+        0x15, 0x0d, 0xc8, 0x13, 0xcf, 0xd2, 0x6f, 0x0d,
+        0x9d, 0xad, 0x30, 0xe5, 0x70, 0x20, 0x92, 0x9e,
+        0xb3, 0x6b, 0xba, 0x5c, 0x50, 0x0f, 0xc3, 0xb2,
+        0x7e, 0x64, 0x07, 0x94, 0x7e, 0xc9, 0x4e, 0xc1,
+        0x65, 0x04, 0xaf, 0xb3, 0x9f, 0xde, 0xa8, 0x46,
+        0xfa, 0x6c, 0xf3, 0x03, 0xaf, 0x1c, 0x1b, 0xec,
+        0x75, 0x44, 0x66, 0x77, 0xc9, 0xde, 0x51, 0x33,
+        0x64, 0x27, 0xb0, 0xd4, 0x8d, 0x31, 0x6a, 0x11,
+        0x27, 0x3c, 0x99, 0xd4, 0x22, 0xc0, 0x9d, 0x12,
+        0x01, 0xc7, 0x4a, 0x73, 0xac, 0xbf, 0xc2, 0xbb
+    };
+    static unsigned char E[1] = { 0x03 };
+    static unsigned char P[64] = {
+        0xe5, 0x53, 0x1f, 0x88, 0x51, 0xee, 0x59, 0xf8,
+        0xc1, 0xe4, 0xcc, 0x5b, 0xb3, 0x75, 0x8d, 0xc8,
+        0xe8, 0x95, 0x2f, 0xd0, 0xef, 0x37, 0xb4, 0xcd,
+        0xd3, 0x9e, 0x48, 0x8b, 0x81, 0x58, 0x60, 0xb9,
+        0x27, 0x1d, 0xb6, 0x28, 0x92, 0x64, 0xa3, 0xa5,
+        0x64, 0xbd, 0xcc, 0x53, 0x68, 0xdd, 0x3e, 0x55,
+        0xea, 0x9d, 0x5e, 0xcd, 0x1f, 0x96, 0x87, 0xf1,
+        0x29, 0x75, 0x92, 0x70, 0x8f, 0x28, 0xfb, 0x2b
+    };
+    static unsigned char Q[64] = {
+        0xdb, 0x53, 0xef, 0x74, 0x61, 0xb4, 0x20, 0x3b,
+        0x3b, 0x87, 0x76, 0x75, 0x81, 0x56, 0x11, 0x03,
+        0x59, 0x31, 0xe3, 0x38, 0x4b, 0x8c, 0x7a, 0x9c,
+        0x05, 0xd6, 0x7f, 0x1e, 0x5e, 0x60, 0xf0, 0x4e,
+        0x0b, 0xdc, 0x34, 0x54, 0x1c, 0x2e, 0x90, 0x83,
+        0x14, 0xef, 0xc0, 0x96, 0x5c, 0x30, 0x10, 0xcc,
+        0xc1, 0xba, 0xa0, 0x54, 0x3f, 0x96, 0x24, 0xca,
+        0xa3, 0xfb, 0x55, 0xbc, 0x71, 0x29, 0x4e, 0xb1
+    };
+    unsigned char original[128];
+    unsigned char intermediate[128];
+    static unsigned char default_content[128] = {
+        /* A randomly generated pattern. */
+        0x4c, 0x27, 0x54, 0xa0, 0xce, 0x0d, 0x09, 0x4a,
+        0x1c, 0x38, 0x8e, 0x2d, 0xa3, 0xc4, 0xe0, 0x19,
+        0x4c, 0x99, 0xb2, 0xbf, 0xe6, 0x65, 0x7e, 0x58,
+        0xd7, 0xb6, 0x8a, 0x05, 0x2f, 0xa5, 0xec, 0xa4,
+        0x35, 0xad, 0x10, 0x36, 0xff, 0x0d, 0x08, 0x50,
+        0x74, 0x47, 0xc9, 0x9c, 0x4a, 0xe7, 0xfd, 0xfa,
+        0x83, 0x5f, 0x14, 0x5a, 0x1e, 0xe7, 0x35, 0x08,
+        0xad, 0xf7, 0x0d, 0x86, 0xdf, 0xb8, 0xd4, 0xcf,
+        0x32, 0xb9, 0x5c, 0xbe, 0xa3, 0xd2, 0x89, 0x70,
+        0x7b, 0xc6, 0x48, 0x7e, 0x58, 0x4d, 0xf3, 0xef,
+        0x34, 0xb7, 0x57, 0x54, 0x79, 0xc5, 0x8e, 0x0a,
+        0xa3, 0xbf, 0x6d, 0x42, 0x83, 0x25, 0x13, 0xa2,
+        0x95, 0xc0, 0x0d, 0x32, 0xec, 0x77, 0x91, 0x2b,
+        0x68, 0xb6, 0x8c, 0x79, 0x15, 0xfb, 0x94, 0xde,
+        0xb9, 0x2b, 0x94, 0xb3, 0x28, 0x23, 0x86, 0x3d,
+        0x37, 0x00, 0xe6, 0xf1, 0x1f, 0x4e, 0xd4, 0x42
+    };
+    unsigned char final[128];
+    size_t output_length = 0x7EA0;
+
+    memset( &rnd_info, 0, sizeof( rnd_pseudo_info ) );
+    mbedtls_mpi_init( &P1 ); mbedtls_mpi_init( &Q1 );
+    mbedtls_mpi_init( &H ); mbedtls_mpi_init( &G );
+    mbedtls_rsa_init( &ctx, MBEDTLS_RSA_PKCS_V15, 0 );
+
+    TEST_ASSERT( mbedtls_mpi_read_binary( &ctx.N, N, sizeof( N ) ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_read_binary( &ctx.E, E, sizeof( E ) ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_read_binary( &ctx.P, P, sizeof( P ) ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_read_binary( &ctx.Q, Q, sizeof( Q ) ) == 0 );
+
+    ctx.len = sizeof( N );
+    TEST_ASSERT( mbedtls_mpi_sub_int( &P1, &ctx.P, 1 ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_sub_int( &Q1, &ctx.Q, 1 ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_mul_mpi( &H, &P1, &Q1 ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_gcd( &G, &ctx.E, &H  ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_inv_mod( &ctx.D , &ctx.E, &H  ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_mod_mpi( &ctx.DP, &ctx.D, &P1 ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_mod_mpi( &ctx.DQ, &ctx.D, &Q1 ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_inv_mod( &ctx.QP, &ctx.Q, &ctx.P ) == 0 );
+    TEST_ASSERT( mbedtls_rsa_check_privkey( &ctx ) == 0 );
+
+    input_len = unhexify( original, input_hex );
+    memset( original + input_len, 'd', sizeof( original ) - input_len );
+    if( mode == MBEDTLS_RSA_PRIVATE )
+        TEST_ASSERT( mbedtls_rsa_public( &ctx, original, intermediate ) == 0 );
+    else
+        TEST_ASSERT( mbedtls_rsa_private( &ctx, &rnd_pseudo_rand, &rnd_info,
+                                          original, intermediate ) == 0 );
+
+    memcpy( final, default_content, sizeof( final ) );
+    TEST_ASSERT( mbedtls_rsa_pkcs1_decrypt( &ctx,
+                                            &rnd_pseudo_rand, &rnd_info,
+                                            mode,
+                                            &output_length,
+                                            intermediate,
+                                            final,
+                                            output_size ) == expected_result );
+    if( expected_result == 0 )
+    {
+        TEST_ASSERT( output_length == expected_plaintext_length );
+        TEST_ASSERT( memcmp( original + sizeof( N ) - output_length,
+                             final,
+                             output_length ) == 0 );
+    }
+    else if( expected_result == MBEDTLS_ERR_RSA_INVALID_PADDING ||
+             expected_result == MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE )
+    {
+        size_t max_payload_length =
+            output_size > sizeof( N ) - 11 ? sizeof( N ) - 11 : output_size;
+        size_t i;
+        size_t count = 0;
+
+#if !defined(MBEDTLS_RSA_ALT)
+        /* Check that the output in invalid cases is what the default
+         * implementation currently does. Alternative implementations
+         * may produce different output, so we only perform these precise
+         * checks when using the default implementation. */
+        TEST_ASSERT( output_length == max_payload_length );
+        for( i = 0; i < max_payload_length; i++ )
+            TEST_ASSERT( final[i] == 0 );
+#endif
+        /* Even in alternative implementations, the outputs must have
+         * changed, otherwise it indicates at least a timing vulnerability
+         * because no write to the outputs is performed in the bad case. */
+        TEST_ASSERT( output_length != 0x7EA0 );
+        for( i = 0; i < max_payload_length; i++ )
+            count += ( final[i] == default_content[i] );
+        /* If more than 16 bytes are unchanged in final, that's evidence
+         * that final wasn't overwritten. */
+        TEST_ASSERT( count < 16 );
+    }
+
+exit:
+    mbedtls_mpi_free( &P1 ); mbedtls_mpi_free( &Q1 );
+    mbedtls_mpi_free( &H ); mbedtls_mpi_free( &G );
+    mbedtls_rsa_free( &ctx );
+}
+/* END_CASE */
+
 /* BEGIN_CASE */
 void pkcs1_rsassa_v15_sign( int mod, int radix_P, char *input_P, int radix_Q,
                             char *input_Q, int radix_N, char *input_N,

From 996f30d38156aa4c432e49289b312d88e645bf78 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 12 Oct 2018 19:15:34 +0200
Subject: [PATCH 15/16] Fix likely-harmless undefined behavior surrounding
 volatile

The code was making two unsequenced reads from volatile locations.
This is undefined behavior. It was probably harmless because we didn't
care in what order the reads happened and the reads were from ordinary
memory, but UB is UB and IAR8 complained.
---
 library/rsa.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/library/rsa.c b/library/rsa.c
index 8d55acd69..a824c88bb 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1037,7 +1037,11 @@ static void mem_move_to_left( void *start,
          * `offset` passes shift the data one byte to the left and
          * zero out the last byte. */
         for( n = 0; n < total - 1; n++ )
-            buf[n] = if_int( no_op, buf[n], buf[n+1] );
+        {
+            unsigned char current = buf[n];
+            unsigned char next = buf[n+1];
+            buf[n] = if_int( no_op, current, next );
+        }
         buf[total-1] = if_int( no_op, buf[total-1], 0 );
     }
 }

From e3be8d672e654984ecc7f8be1a4fd783965b6cc5 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 12 Oct 2018 19:19:12 +0200
Subject: [PATCH 16/16] Fix undefined behavior in unsigned-to-signed conversion

The code assumed that `int x = - (unsigned) u` with 0 <= u < INT_MAX
sets `x` to the negative of u, but actually this calculates
(UINT_MAX - u) and then converts this value to int, which overflows.
Cast to int before applying the unary minus operator to guarantee the
desired behavior.
---
 library/rsa.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/library/rsa.c b/library/rsa.c
index a824c88bb..d640672a4 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1155,9 +1155,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx,
      * - OUTPUT_TOO_LARGE if the padding is good but the decrypted
      *   plaintext does not fit in the output buffer.
      * - 0 if the padding is correct. */
-    ret = - if_int( bad, - MBEDTLS_ERR_RSA_INVALID_PADDING,
-            if_int( output_too_large, - MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE,
-                    0 ) );
+    ret = - (int) if_int( bad, - MBEDTLS_ERR_RSA_INVALID_PADDING,
+                  if_int( output_too_large, - MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE,
+                          0 ) );
 
     /* If the padding is bad or the plaintext is too large, zero the
      * data that we're about to copy to the output buffer.