From c4638cc6401a88124f9f54d60439eda6d2368641 Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Fri, 31 May 2019 20:11:26 +0200 Subject: [PATCH 1/5] Change size of preallocated buffer for pk_sign() calls --- library/x509write_crt.c | 12 +++++++++++- library/x509write_csr.c | 12 +++++++++++- programs/pkey/pk_sign.c | 12 +++++++++++- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/library/x509write_crt.c b/library/x509write_crt.c index b6cb745a3..e237cb8ab 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -45,6 +45,16 @@ #include "mbedtls/pem.h" #endif /* MBEDTLS_PEM_WRITE_C */ +/* + * For the currently used signature algorithms the buffer to store any signature + * must be at least of size MAX(MBEDTLS_ECDSA_MAX_LEN, MBEDTLS_MPI_MAX_SIZE) + */ +#if MBEDTLS_ECDSA_MAX_LEN > MBEDTLS_MPI_MAX_SIZE +#define SIGNATURE_MAX_SIZE MBEDTLS_ECDSA_MAX_LEN +#else +#define SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE +#endif + void mbedtls_x509write_crt_init( mbedtls_x509write_cert *ctx ) { memset( ctx, 0, sizeof( mbedtls_x509write_cert ) ); @@ -317,7 +327,7 @@ int mbedtls_x509write_crt_der( mbedtls_x509write_cert *ctx, unsigned char *buf, size_t sig_oid_len = 0; unsigned char *c, *c2; unsigned char hash[64]; - unsigned char sig[MBEDTLS_MPI_MAX_SIZE]; + unsigned char sig[SIGNATURE_MAX_SIZE]; unsigned char tmp_buf[2048]; size_t sub_len = 0, pub_len = 0, sig_and_oid_len = 0, sig_len; size_t len = 0; diff --git a/library/x509write_csr.c b/library/x509write_csr.c index 8dc39e7a5..0d62d1d48 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -49,6 +49,16 @@ #include "mbedtls/pem.h" #endif +/* + * For the currently used signature algorithms the buffer to store any signature + * must be at least of size MAX(MBEDTLS_ECDSA_MAX_LEN, MBEDTLS_MPI_MAX_SIZE) + */ +#if MBEDTLS_ECDSA_MAX_LEN > MBEDTLS_MPI_MAX_SIZE +#define SIGNATURE_MAX_SIZE MBEDTLS_ECDSA_MAX_LEN +#else +#define SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE +#endif + void mbedtls_x509write_csr_init( mbedtls_x509write_csr *ctx ) { memset( ctx, 0, sizeof( mbedtls_x509write_csr ) ); @@ -138,7 +148,7 @@ int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, s size_t sig_oid_len = 0; unsigned char *c, *c2; unsigned char hash[64]; - unsigned char sig[MBEDTLS_MPI_MAX_SIZE]; + unsigned char sig[SIGNATURE_MAX_SIZE]; unsigned char tmp_buf[2048]; size_t pub_len = 0, sig_and_oid_len = 0, sig_len; size_t len = 0; diff --git a/programs/pkey/pk_sign.c b/programs/pkey/pk_sign.c index 7354082f1..4696c7c12 100644 --- a/programs/pkey/pk_sign.c +++ b/programs/pkey/pk_sign.c @@ -72,6 +72,16 @@ void mbedtls_param_failed( const char *failure_condition, } #endif +/* + * For the currently used signature algorithms the buffer to store any signature + * must be at least of size MAX(MBEDTLS_ECDSA_MAX_LEN, MBEDTLS_MPI_MAX_SIZE) + */ +#if MBEDTLS_ECDSA_MAX_LEN > MBEDTLS_MPI_MAX_SIZE +#define SIGNATURE_MAX_SIZE MBEDTLS_ECDSA_MAX_LEN +#else +#define SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE +#endif + int main( int argc, char *argv[] ) { FILE *f; @@ -81,7 +91,7 @@ int main( int argc, char *argv[] ) mbedtls_entropy_context entropy; mbedtls_ctr_drbg_context ctr_drbg; unsigned char hash[32]; - unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; + unsigned char buf[SIGNATURE_MAX_SIZE]; char filename[512]; const char *pers = "mbedtls_pk_sign"; size_t olen = 0; From 5dbe7caf2eeea340ad3a38b0280c772562d4edd4 Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Fri, 31 May 2019 20:13:58 +0200 Subject: [PATCH 2/5] Add missing MBEDTLS_ECP_C dependencies in check_config.h --- include/mbedtls/check_config.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 0fa74f061..f0b6c6064 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -134,7 +134,7 @@ #error "MBEDTLS_ECDSA_DETERMINISTIC defined, but not all prerequisites" #endif -#if defined(MBEDTLS_ECP_C) && ( !defined(MBEDTLS_BIGNUM_C) || ( \ +#if defined(MBEDTLS_ECP_C) && ( !defined(MBEDTLS_BIGNUM_C) || ( \ !defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) && \ !defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) && \ !defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) && \ @@ -145,7 +145,9 @@ !defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) && \ !defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) && \ !defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) && \ - !defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) ) ) + !defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) && \ + !defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) && \ + !defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) ) ) #error "MBEDTLS_ECP_C defined, but not all prerequisites" #endif From 49bd3e897e4ff7dfc1c09351213b3e22470abc7e Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Fri, 31 May 2019 20:16:50 +0200 Subject: [PATCH 3/5] Add documentation notes about the required size of the signature buffers --- include/mbedtls/pk.h | 8 ++++++++ include/mbedtls/rsa.h | 9 ++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index 24951a6e1..f89faec80 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -458,6 +458,10 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options, * * \note For RSA, md_alg may be MBEDTLS_MD_NONE if hash_len != 0. * For ECDSA, md_alg may never be MBEDTLS_MD_NONE. + * + * \note In order to ensure enough space for the signature, the + * \p sig buffer size must be of at least + * `max(MBEDTLS_ECDSA_MAX_LEN, MBEDTLS_MPI_MAX_SIZE)` bytes. */ int mbedtls_pk_sign( mbedtls_pk_context *ctx, mbedtls_md_type_t md_alg, const unsigned char *hash, size_t hash_len, @@ -472,6 +476,10 @@ int mbedtls_pk_sign( mbedtls_pk_context *ctx, mbedtls_md_type_t md_alg, * \c mbedtls_ecp_set_max_ops() to reduce blocking for ECC * operations. For RSA, same as \c mbedtls_pk_sign(). * + * \note In order to ensure enough space for the signature, the + * \p sig buffer size must be of at least + * `max(MBEDTLS_ECDSA_MAX_LEN, MBEDTLS_MPI_MAX_SIZE)` bytes. + * * \param ctx The PK context to use. It must have been set up * with a private key. * \param md_alg Hash algorithm used (see notes) diff --git a/include/mbedtls/rsa.h b/include/mbedtls/rsa.h index 489f2ed45..610e80486 100644 --- a/include/mbedtls/rsa.h +++ b/include/mbedtls/rsa.h @@ -907,7 +907,8 @@ int mbedtls_rsa_rsaes_oaep_decrypt( mbedtls_rsa_context *ctx, * the size of the hash corresponding to \p md_alg. * \param sig The buffer to hold the signature. This must be a writable * buffer of length \c ctx->len Bytes. For example, \c 256 Bytes - * for an 2048-bit RSA modulus. + * for an 2048-bit RSA modulus. A buffer length of + * #MBEDTLS_MPI_MAX_SIZE is always safe. * * \return \c 0 if the signing operation was successful. * \return An \c MBEDTLS_ERR_RSA_XXX error code on failure. @@ -954,7 +955,8 @@ int mbedtls_rsa_pkcs1_sign( mbedtls_rsa_context *ctx, * the size of the hash corresponding to \p md_alg. * \param sig The buffer to hold the signature. This must be a writable * buffer of length \c ctx->len Bytes. For example, \c 256 Bytes - * for an 2048-bit RSA modulus. + * for an 2048-bit RSA modulus. A buffer length of + * #MBEDTLS_MPI_MAX_SIZE is always safe. * * \return \c 0 if the signing operation was successful. * \return An \c MBEDTLS_ERR_RSA_XXX error code on failure. @@ -1015,7 +1017,8 @@ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx, * the size of the hash corresponding to \p md_alg. * \param sig The buffer to hold the signature. This must be a writable * buffer of length \c ctx->len Bytes. For example, \c 256 Bytes - * for an 2048-bit RSA modulus. + * for an 2048-bit RSA modulus. A buffer length of + * #MBEDTLS_MPI_MAX_SIZE is always safe. * * \return \c 0 if the signing operation was successful. * \return An \c MBEDTLS_ERR_RSA_XXX error code on failure. From c1955559ad7bab3be5666d4687269e7d2ddc1edd Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Mon, 10 Jun 2019 11:46:18 +0200 Subject: [PATCH 4/5] Add a test for signing content with a long ECDSA key Due to the way the current PK API works, it may have not been clear for the library clients, how big output buffers they should pass to the signing functions. Depending on the key type they depend on MPI or EC specific compile-time constants. Inside the library, there were places, where it was assumed that the MPI size will always be enough, even for ECDSA signatures. However, for very small sizes of the MBEDTLS_MPI_MAX_SIZE and sufficiently large key, the EC signature could exceed the MPI size and cause a stack overflow. This test establishes both conditions -- small MPI size and the use of a long ECDSA key -- and attempts to sign an arbitrary file. This can cause a stack overvlow if the signature buffers are not big enough, therefore the test is performed for an ASan build. --- tests/data_files/Makefile | 8 ++++++++ tests/data_files/secp521r1_prv.der | Bin 0 -> 223 bytes tests/scripts/all.sh | 18 ++++++++++++++++++ 3 files changed, 26 insertions(+) create mode 100644 tests/data_files/secp521r1_prv.der diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile index d1af18ce7..0aa78eb4c 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -786,6 +786,14 @@ ec_prv.pk8param.pem: ec_prv.pk8param.der $(OPENSSL) pkey -in $< -inform DER -out $@ all_final += ec_prv.pk8param.pem +### +### A generic SECP521R1 private key +### + +secp521r1_prv.der: + $(OPENSSL) ecparam -genkey -name secp521r1 -noout -out secp521r1_prv.der +all_final += secp521r1_prv.der + ################################################################ ### Generate CSRs for X.509 write test suite ################################################################ diff --git a/tests/data_files/secp521r1_prv.der b/tests/data_files/secp521r1_prv.der new file mode 100644 index 0000000000000000000000000000000000000000..4d342bdc25590e747f3dd45369c525a17eeafe4a GIT binary patch literal 223 zcmV<503iP`f!qQC0R%z;dbqNytV!Pn9Gx-kY_~1_fkN9pZuhX33j*q0c>w;eL4xg{ zKt>-fDT-%(a)8NM6PPC1#8 zkDC@p2S}y!0W4tS+`#2wI5*>!%9T%+)=Ms?OrfF%^Z0&EWhI$b@A@l|MQI|WVHYQ literal 0 HcmV?d00001 diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 369df1591..b222ff553 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -611,6 +611,24 @@ component_check_doxygen_warnings () { #### Build and test many configurations and targets ################################################################ +component_test_large_ecdsa_key_signature () { + + SMALL_MPI_MAX_SIZE=136 # Small enough to interfere with the EC signatures + + msg "build: cmake + MBEDTLS_MPI_MAX_SIZE=${SMALL_MPI_MAX_SIZE}, gcc, ASan" # ~ 1 min 50s + scripts/config.pl set MBEDTLS_MPI_MAX_SIZE $SMALL_MPI_MAX_SIZE + crypto/scripts/config.pl set MBEDTLS_MPI_MAX_SIZE $SMALL_MPI_MAX_SIZE + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + INEVITABLY_PRESENT_FILE=Makefile + SIGNATURE_FILE="${INEVITABLY_PRESENT_FILE}.sig" # Warning, this is rm -f'ed below + + msg "test: pk_sign secp521r1_prv.der for MBEDTLS_MPI_MAX_SIZE=${SMALL_MPI_MAX_SIZE} (ASan build)" # ~ 5s + if_build_succeeded programs/pkey/pk_sign tests/data_files/secp521r1_prv.der $INEVITABLY_PRESENT_FILE + rm -f $SIGNATURE_FILE +} + component_test_default_out_of_box () { msg "build: make, default config (out-of-box)" # ~1min make From 69969ef088359b225eb955a40a8b842679fcc6e9 Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Thu, 13 Jun 2019 11:53:17 +0200 Subject: [PATCH 5/5] Remove redundant config.pl call --- tests/scripts/all.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index b222ff553..ff93a6ce0 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -617,7 +617,6 @@ component_test_large_ecdsa_key_signature () { msg "build: cmake + MBEDTLS_MPI_MAX_SIZE=${SMALL_MPI_MAX_SIZE}, gcc, ASan" # ~ 1 min 50s scripts/config.pl set MBEDTLS_MPI_MAX_SIZE $SMALL_MPI_MAX_SIZE - crypto/scripts/config.pl set MBEDTLS_MPI_MAX_SIZE $SMALL_MPI_MAX_SIZE CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . make