From e79c93969353c3db310d70f60ab170c1f8765ead Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Fri, 31 May 2019 20:11:26 +0200 Subject: [PATCH 1/4] 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 10497e752..61d7ba44a 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 ) ); @@ -334,7 +344,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 d70ba0ed9..b65a11c6a 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -44,6 +44,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 ) ); @@ -159,7 +169,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 c775ee1cfeab07c4a5c31a0da6944846d0c87117 Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Fri, 31 May 2019 20:13:58 +0200 Subject: [PATCH 2/4] 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 b86e5807e..343a7367e 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -123,7 +123,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) && \ @@ -134,7 +134,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 4e36da32b33d63b36de85ec6071575f68ba3d100 Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Fri, 31 May 2019 20:16:50 +0200 Subject: [PATCH 3/4] 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 91950f940..136427503 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -416,6 +416,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, @@ -430,6 +434,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 906c42733..cba692beb 100644 --- a/include/mbedtls/rsa.h +++ b/include/mbedtls/rsa.h @@ -904,7 +904,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. @@ -951,7 +952,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. @@ -1012,7 +1014,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 11f38e2f6180aeb0a593579dd8efe31cecd085ac Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Tue, 4 Jun 2019 13:14:58 +0200 Subject: [PATCH 4/4] 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 | 17 +++++++++++++++++ 3 files changed, 25 insertions(+) create mode 100644 tests/data_files/secp521r1_prv.der diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile index d023c8d0c..bb816838c 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -745,6 +745,14 @@ server5.req.ku.sha1: server5.key $(MBEDTLS_CERT_REQ) output_file=$@ filename=$< key_usage=digital_signature,non_repudiation subject_name="C=NL,O=PolarSSL,CN=PolarSSL Server 1" md=SHA1 all_final += server5.req.ku.sha1 +### +### 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 936c665a1..5e7b248d2 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -590,6 +590,23 @@ 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 + 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