From 5d5841f4506f524a41e8e77e08049b24bcac0686 Mon Sep 17 00:00:00 2001 From: Piotr Nowicki Date: Fri, 5 Jun 2020 16:33:24 +0200 Subject: [PATCH 1/2] Add mbedtls_platform_memmove() as a secured memcmp() Signed-off-by: Piotr Nowicki --- include/mbedtls/platform_util.h | 16 ++++++++++++++++ library/bignum.c | 6 +++++- library/nist_kw.c | 11 +++++++++-- library/pk.c | 10 ++++++++-- library/platform_util.c | 23 +++++++++++++++++++++++ 5 files changed, 61 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index fa9f3269c..d05c3c77a 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -198,6 +198,22 @@ void *mbedtls_platform_memset( void *ptr, int value, size_t num ); */ void *mbedtls_platform_memcpy( void *dst, const void *src, size_t num ); +/** + * \brief Secure memmove + * + * This is a constant-time version of memmove(). It is based on + * the double use of the mbedtls_platform_memcpy() function secured + * against side-channel attacks. + * + * \param dst Destination buffer where the data is being moved to. + * \param src Source buffer where the data is being moved from. + * \param num The length of the buffers in bytes. + * + * \return 0 if the operation was successful or -1 if memory allocation + * failed. + */ +int mbedtls_platform_memmove( void *dst, const void *src, size_t num ); + /** * \brief Secure memcmp * diff --git a/library/bignum.c b/library/bignum.c index 00df10d98..2cc8e224c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -558,7 +558,11 @@ static int mpi_write_hlp( mbedtls_mpi *X, int radix, length++; } while( mbedtls_mpi_cmp_int( X, 0 ) != 0 ); - memmove( *p, p_end, length ); + if( 0 != mbedtls_platform_memmove( *p, p_end, length ) ) + { + ret = MBEDTLS_ERR_MPI_ALLOC_FAILED; + goto cleanup; + } *p += length; cleanup: diff --git a/library/nist_kw.c b/library/nist_kw.c index 0ad270aff..5b5aa1caf 100644 --- a/library/nist_kw.c +++ b/library/nist_kw.c @@ -222,7 +222,10 @@ int mbedtls_nist_kw_wrap( mbedtls_nist_kw_context *ctx, } mbedtls_platform_memcpy( output, NIST_KW_ICV1, KW_SEMIBLOCK_LENGTH ); - memmove( output + KW_SEMIBLOCK_LENGTH, input, in_len ); + if( 0 != mbedtls_platform_memmove( output + KW_SEMIBLOCK_LENGTH, input, in_len ) ) + { + return MBEDTLS_ERR_CIPHER_ALLOC_FAILED; + } } else { @@ -343,7 +346,11 @@ static int unwrap( mbedtls_nist_kw_context *ctx, } mbedtls_platform_memcpy( A, input, KW_SEMIBLOCK_LENGTH ); - memmove( output, input + KW_SEMIBLOCK_LENGTH, ( semiblocks - 1 ) * KW_SEMIBLOCK_LENGTH ); + if( 0 != mbedtls_platform_memmove( output, input + KW_SEMIBLOCK_LENGTH, + ( semiblocks - 1 ) * KW_SEMIBLOCK_LENGTH ) ) + { + return( MBEDTLS_ERR_CIPHER_ALLOC_FAILED ); + } /* Calculate intermediate values */ for( t = s; t >= 1; t-- ) diff --git a/library/pk.c b/library/pk.c index 85575133d..f2df7db05 100644 --- a/library/pk.c +++ b/library/pk.c @@ -629,7 +629,10 @@ static int asn1_write_mpibuf( unsigned char **p, unsigned char *start, len = n_len; *p -= len; - memmove( *p, start, len ); + if( 0 != mbedtls_platform_memmove( *p, start, len ) ) + { + return( MBEDTLS_ERR_PK_ALLOC_FAILED ); + } /* ASN.1 DER encoding requires minimal length, so skip leading 0s. * Neither r nor s should be 0, but as a failsafe measure, still detect @@ -691,8 +694,11 @@ static int pk_ecdsa_sig_asn1_from_uecc( unsigned char *sig, size_t *sig_len, *--p = MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE; len += 2; + if( 0 != mbedtls_platform_memmove( sig, p, len ) ) + { + return( MBEDTLS_ERR_PK_ALLOC_FAILED ); + } ret = 0; - memmove( sig, p, len ); *sig_len = len; return( ret ); diff --git a/library/platform_util.c b/library/platform_util.c index 3869f30a5..694e23a6a 100644 --- a/library/platform_util.c +++ b/library/platform_util.c @@ -38,6 +38,12 @@ #include "mbedtls/platform.h" #include "mbedtls/threading.h" +#if !defined(MBEDTLS_PLATFORM_C) +#include +#define mbedtls_calloc calloc +#define mbedtls_free free +#endif + #if defined(MBEDTLS_ENTROPY_HARDWARE_ALT) #include "mbedtls/entropy_poll.h" #endif @@ -121,6 +127,23 @@ void *mbedtls_platform_memcpy( void *dst, const void *src, size_t num ) return( memcpy( (void *) dst, (void *) src, start_offset ) ); } +int mbedtls_platform_memmove( void *dst, const void *src, size_t num ) +{ + /* The buffers can have a common part, so we cannot do a copy from a random + * location. By using a temporary buffer we can do so, but the cost of it + * is using more memory and longer transfer time. */ + void *tmp = mbedtls_calloc( 1, num ); + if( tmp != NULL ) + { + mbedtls_platform_memcpy( tmp, src, num ); + mbedtls_platform_memcpy( dst, tmp, num ); + mbedtls_free( tmp ); + return 0; + } + + return -1; +} + int mbedtls_platform_memcmp( const void *buf1, const void *buf2, size_t num ) { volatile const unsigned char *A = (volatile const unsigned char *) buf1; From ce0aab44745e63f3500215ca4bc7e69f14bdacf7 Mon Sep 17 00:00:00 2001 From: Piotr Nowicki Date: Mon, 8 Jun 2020 14:08:49 +0200 Subject: [PATCH 2/2] Add new error code PLATFORM_ALLOC_FAILED for mbedtls_platform_memmove() Signed-off-by: Piotr Nowicki --- include/mbedtls/error.h | 2 +- include/mbedtls/nist_kw.h | 1 + include/mbedtls/platform.h | 1 + include/mbedtls/platform_util.h | 4 ++-- library/bignum.c | 6 +----- library/error.c | 2 ++ library/nist_kw.c | 12 +++++++----- library/pk.c | 12 +++++++----- library/platform_util.c | 2 +- 9 files changed, 23 insertions(+), 19 deletions(-) diff --git a/include/mbedtls/error.h b/include/mbedtls/error.h index a52f9f5db..9696f178b 100644 --- a/include/mbedtls/error.h +++ b/include/mbedtls/error.h @@ -86,7 +86,7 @@ * CHACHA20 3 0x0051-0x0055 * POLY1305 3 0x0057-0x005B * CHACHAPOLY 2 0x0054-0x0056 - * PLATFORM 3 0x0070-0x0072 0x0071-0x0071 + * PLATFORM 4 0x0070-0x0072 0x0071-0x0071 0x0076-0x0076 * * High-level module nr (3 bits - 0x0...-0x7...) * Name ID Nr of Errors diff --git a/include/mbedtls/nist_kw.h b/include/mbedtls/nist_kw.h index 3b67b59cd..c94c15a6f 100644 --- a/include/mbedtls/nist_kw.h +++ b/include/mbedtls/nist_kw.h @@ -133,6 +133,7 @@ void mbedtls_nist_kw_free( mbedtls_nist_kw_context *ctx ); * * \return \c 0 on success. * \return \c MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA for invalid input length. + * \return \c MBEDTLS_ERR_PLATFORM_ALLOC_FAILED in case of a memory allocation failure. * \return cipher-specific error code on failure of the underlying cipher. */ int mbedtls_nist_kw_wrap( mbedtls_nist_kw_context *ctx, mbedtls_nist_kw_mode_t mode, diff --git a/include/mbedtls/platform.h b/include/mbedtls/platform.h index ec1df15cf..8a88ce712 100644 --- a/include/mbedtls/platform.h +++ b/include/mbedtls/platform.h @@ -42,6 +42,7 @@ #define MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED -0x0070 /**< Hardware accelerator failed */ #define MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED -0x0072 /**< The requested feature is not supported by the platform */ #define MBEDTLS_ERR_PLATFORM_FAULT_DETECTED -0x0071 /**< A hardware fault was detected in a critical path. As a security precaution this should be treated as a potential physical attack */ +#define MBEDTLS_ERR_PLATFORM_ALLOC_FAILED -0x0076 /**< Memory allocation failed */ #if defined(MBEDTLS_PLATFORM_C) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index d05c3c77a..4e0f9897a 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -209,8 +209,8 @@ void *mbedtls_platform_memcpy( void *dst, const void *src, size_t num ); * \param src Source buffer where the data is being moved from. * \param num The length of the buffers in bytes. * - * \return 0 if the operation was successful or -1 if memory allocation - * failed. + * \return 0 if the operation was successful + * \return #MBEDTLS_ERR_PLATFORM_ALLOC_FAILED if a memory allocation failed */ int mbedtls_platform_memmove( void *dst, const void *src, size_t num ); diff --git a/library/bignum.c b/library/bignum.c index 2cc8e224c..f4ab0db63 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -558,11 +558,7 @@ static int mpi_write_hlp( mbedtls_mpi *X, int radix, length++; } while( mbedtls_mpi_cmp_int( X, 0 ) != 0 ); - if( 0 != mbedtls_platform_memmove( *p, p_end, length ) ) - { - ret = MBEDTLS_ERR_MPI_ALLOC_FAILED; - goto cleanup; - } + MBEDTLS_MPI_CHK( mbedtls_platform_memmove( *p, p_end, length ) ); *p += length; cleanup: diff --git a/library/error.c b/library/error.c index ecdec785b..893de7f5e 100644 --- a/library/error.c +++ b/library/error.c @@ -843,6 +843,8 @@ void mbedtls_strerror( int ret, char *buf, size_t buflen ) mbedtls_snprintf( buf, buflen, "PLATFORM - The requested feature is not supported by the platform" ); if( use_ret == -(MBEDTLS_ERR_PLATFORM_FAULT_DETECTED) ) mbedtls_snprintf( buf, buflen, "PLATFORM - A hardware fault was detected in a critical path. As a security precaution this should be treated as a potential physical attack" ); + if( use_ret == -(MBEDTLS_ERR_PLATFORM_ALLOC_FAILED) ) + mbedtls_snprintf( buf, buflen, "PLATFORM - Memory allocation failed" ); #endif /* MBEDTLS_PLATFORM_C */ #if defined(MBEDTLS_POLY1305_C) diff --git a/library/nist_kw.c b/library/nist_kw.c index 5b5aa1caf..2f7f082c1 100644 --- a/library/nist_kw.c +++ b/library/nist_kw.c @@ -222,9 +222,10 @@ int mbedtls_nist_kw_wrap( mbedtls_nist_kw_context *ctx, } mbedtls_platform_memcpy( output, NIST_KW_ICV1, KW_SEMIBLOCK_LENGTH ); - if( 0 != mbedtls_platform_memmove( output + KW_SEMIBLOCK_LENGTH, input, in_len ) ) + ret = mbedtls_platform_memmove( output + KW_SEMIBLOCK_LENGTH, input, in_len ); + if( ret != 0 ) { - return MBEDTLS_ERR_CIPHER_ALLOC_FAILED; + return ret; } } else @@ -346,10 +347,11 @@ static int unwrap( mbedtls_nist_kw_context *ctx, } mbedtls_platform_memcpy( A, input, KW_SEMIBLOCK_LENGTH ); - if( 0 != mbedtls_platform_memmove( output, input + KW_SEMIBLOCK_LENGTH, - ( semiblocks - 1 ) * KW_SEMIBLOCK_LENGTH ) ) + ret = mbedtls_platform_memmove( output, input + KW_SEMIBLOCK_LENGTH, + ( semiblocks - 1 ) * KW_SEMIBLOCK_LENGTH ); + if( ret != 0 ) { - return( MBEDTLS_ERR_CIPHER_ALLOC_FAILED ); + return ret; } /* Calculate intermediate values */ diff --git a/library/pk.c b/library/pk.c index f2df7db05..cf4cfbbaa 100644 --- a/library/pk.c +++ b/library/pk.c @@ -623,15 +623,17 @@ static int asn1_write_mpibuf( unsigned char **p, unsigned char *start, size_t n_len ) { size_t len = 0; + int ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; if( (size_t)( *p - start ) < n_len ) return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); len = n_len; *p -= len; - if( 0 != mbedtls_platform_memmove( *p, start, len ) ) + ret = mbedtls_platform_memmove( *p, start, len ); + if( ret != 0 ) { - return( MBEDTLS_ERR_PK_ALLOC_FAILED ); + return( ret ); } /* ASN.1 DER encoding requires minimal length, so skip leading 0s. @@ -694,11 +696,11 @@ static int pk_ecdsa_sig_asn1_from_uecc( unsigned char *sig, size_t *sig_len, *--p = MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE; len += 2; - if( 0 != mbedtls_platform_memmove( sig, p, len ) ) + ret = mbedtls_platform_memmove( sig, p, len ); + if( ret != 0 ) { - return( MBEDTLS_ERR_PK_ALLOC_FAILED ); + return( ret ); } - ret = 0; *sig_len = len; return( ret ); diff --git a/library/platform_util.c b/library/platform_util.c index 694e23a6a..17913b4d7 100644 --- a/library/platform_util.c +++ b/library/platform_util.c @@ -141,7 +141,7 @@ int mbedtls_platform_memmove( void *dst, const void *src, size_t num ) return 0; } - return -1; + return MBEDTLS_ERR_PLATFORM_ALLOC_FAILED; } int mbedtls_platform_memcmp( const void *buf1, const void *buf2, size_t num )