From cd500f38322ba253342d98a52891162506bc1c32 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Oct 2018 22:43:06 +0200 Subject: [PATCH 01/15] 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 218504086..65f9815a6 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1373,18 +1373,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 ); @@ -1395,9 +1397,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 dabe87cd7104757c4ab8ef9382078e1c82b48e49 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Oct 2018 22:44:41 +0200 Subject: [PATCH 02/15] 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 65f9815a6..0052a2f50 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1364,6 +1364,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 */ @@ -1381,6 +1412,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; @@ -1434,23 +1469,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 ae97c25a9151f95c19c291a1a88de6a9d86aa820 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Oct 2018 13:40:16 +0200 Subject: [PATCH 03/15] Add ChangeLog entry --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0a9dc4f8d..6fe2069bf 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 b473916dcff02c39cd289549b8bc061aff6b50e7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 18:32:29 +0200 Subject: [PATCH 04/15] 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 0052a2f50..e942872c1 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1367,9 +1367,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 */ @@ -1385,13 +1385,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 ) ); } @@ -1475,7 +1479,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 @@ -1484,9 +1488,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 f9dd29e3a86319b46d4a689d6a7c995bb25644e0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 19:13:43 +0200 Subject: [PATCH 05/15] 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 e942872c1..3ca92a9c9 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1420,6 +1420,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; @@ -1476,11 +1477,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 @@ -1492,38 +1488,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 a04f8bbd0d81cd65978c508fed29e5736cb4da30 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 21:18:30 +0200 Subject: [PATCH 06/15] 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 3ca92a9c9..f70c6da80 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1383,6 +1383,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 @@ -1399,6 +1415,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 */ @@ -1524,8 +1576,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 cf1253e8f0b1ea07561dca57da0398c170c7f1eb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 21:24:21 +0200 Subject: [PATCH 07/15] 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 f70c6da80..31f53ad61 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1527,7 +1527,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 @@ -1541,10 +1541,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 087544bc9855cd43e56571d06453442255b5e68a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 22:45:13 +0200 Subject: [PATCH 08/15] 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 31f53ad61..b8d823d65 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1571,15 +1571,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 03fb3e36e44075b20898bd770599ee7fcf2b2eeb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 14:50:21 +0200 Subject: [PATCH 09/15] 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 b8d823d65..e185faee4 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1464,17 +1464,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 ); @@ -1492,38 +1501,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. */ @@ -1538,7 +1544,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. */ @@ -1577,13 +1583,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 0b330f764f8dcd1afe78b39dcd2db290b711f7d2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 15:06:12 +0200 Subject: [PATCH 10/15] 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 e185faee4..093793e49 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1498,14 +1498,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 @@ -1515,23 +1515,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 ); @@ -1566,8 +1569,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 5d2391e9aaffc6e585436e4f098d6ca2e988c2f6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 15:42:52 +0200 Subject: [PATCH 11/15] 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 6fe2069bf..3556522ea 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 23d7ceaca9e01da26eac50aee111ff0fe8190994 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 18:11:27 +0200 Subject: [PATCH 12/15] 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 093793e49..f49922421 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1508,9 +1508,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; @@ -1522,9 +1522,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 d3f978bd956688848a972dc1fafa31c7fcfb8c31 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 18:15:25 +0200 Subject: [PATCH 13/15] 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 | 148 +++++++++++++++++++++ 2 files changed, 238 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 7f8b1c82e..2b9cf297a 100644 --- a/tests/suites/test_suite_pkcs1_v15.function +++ b/tests/suites/test_suite_pkcs1_v15.function @@ -106,6 +106,154 @@ 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 Nmpi, Empi, Pmpi, Qmpi; + 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( &Nmpi ); mbedtls_mpi_init( &Empi ); + mbedtls_mpi_init( &Pmpi ); mbedtls_mpi_init( &Qmpi ); + mbedtls_rsa_init( &ctx, MBEDTLS_RSA_PKCS_V15, 0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary( &Nmpi, N, sizeof( N ) ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Empi, E, sizeof( E ) ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Pmpi, P, sizeof( P ) ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Qmpi, Q, sizeof( Q ) ) == 0 ); + + TEST_ASSERT( mbedtls_rsa_import( &ctx, &Nmpi, &Pmpi, &Qmpi, + NULL, &Empi ) == 0 ); + TEST_ASSERT( mbedtls_rsa_complete( &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( &Nmpi ); mbedtls_mpi_free( &Empi ); + mbedtls_mpi_free( &Pmpi ); mbedtls_mpi_free( &Qmpi ); + 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 66a28e991d365189af7efc5a34634462b1e532ab Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 12 Oct 2018 19:15:34 +0200 Subject: [PATCH 14/15] 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 f49922421..7bcc75133 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1446,7 +1446,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 84a21d5a54cb035094c24f02f648cfaddaf2d145 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 12 Oct 2018 19:19:12 +0200 Subject: [PATCH 15/15] 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 7bcc75133..4b3cc0213 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1564,9 +1564,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.