From b50c39ca4a87318f22999c9194b3fa63a371f111 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 | 13 ++++++++++++- programs/pkey/pk_sign.c | 12 +++++++++++- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/library/x509write_crt.c b/library/x509write_crt.c index 4cdb941a1..905de4cfa 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -49,6 +49,16 @@ static void mbedtls_zeroize( void *v, size_t n ) { volatile unsigned char *p = v; while( n-- ) *p++ = 0; } +/* + * 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 ) ); @@ -338,7 +348,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 d59354dc4..a73a1a021 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -43,11 +43,22 @@ #include "mbedtls/pem.h" #endif + /* Implementation that should never be optimized out by the compiler */ static void mbedtls_zeroize( void *v, size_t n ) { volatile unsigned char *p = v; while( n-- ) *p++ = 0; } +/* + * 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 ) ); @@ -163,7 +174,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 7ec46752a..0f08db972 100644 --- a/programs/pkey/pk_sign.c +++ b/programs/pkey/pk_sign.c @@ -59,6 +59,16 @@ int main( void ) #include #include +/* + * 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; @@ -68,7 +78,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 199707fcff2f54755767c4dff099b32b0be41234 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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index fa7110fe9..d2d250409 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -107,7 +107,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) && \ @@ -118,7 +118,8 @@ !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) ) ) #error "MBEDTLS_ECP_C defined, but not all prerequisites" #endif From eee98e9d82e92b47e267222571293dc556a30489 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 | 4 ++++ include/mbedtls/rsa.h | 3 +++ 2 files changed, 7 insertions(+) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index ee06b2fd2..8beb2af41 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -343,6 +343,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, diff --git a/include/mbedtls/rsa.h b/include/mbedtls/rsa.h index 5548f3c12..92efa474a 100644 --- a/include/mbedtls/rsa.h +++ b/include/mbedtls/rsa.h @@ -819,6 +819,7 @@ int mbedtls_rsa_rsaes_oaep_decrypt( mbedtls_rsa_context *ctx, * * \note The \p sig buffer must be as large as the size * of \p ctx->N. For example, 128 Bytes if RSA-1024 is used. + * A buffer length of #MBEDTLS_MPI_MAX_SIZE is always safe. * * \note For PKCS#1 v2.1 encoding, see comments on * mbedtls_rsa_rsassa_pss_sign() for details on @@ -862,6 +863,7 @@ int mbedtls_rsa_pkcs1_sign( mbedtls_rsa_context *ctx, * * \note The \p sig buffer must be as large as the size * of \p ctx->N. For example, 128 Bytes if RSA-1024 is used. + * A buffer length of #MBEDTLS_MPI_MAX_SIZE is always safe. */ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx, int (*f_rng)(void *, unsigned char *, size_t), @@ -902,6 +904,7 @@ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx, * * \note The \p sig buffer must be as large as the size * of \p ctx->N. For example, 128 Bytes if RSA-1024 is used. + * A buffer length of #MBEDTLS_MPI_MAX_SIZE is always safe. * * \note The \p hash_id in the RSA context is the one used for the * encoding. \p md_alg in the function call is the type of hash From 45d0ba15a08ad798f75514387c606849c10eb41f Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Tue, 4 Jun 2019 13:14:58 +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 | 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 af1898c3a..5fd4827e8 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -733,6 +733,14 @@ server1.req.cert_type_empty: server1.key $(MBEDTLS_CERT_REQ) output_file=$@ filename=$< subject_name="C=NL,O=PolarSSL,CN=PolarSSL Server 1" md=SHA1 force_ns_cert_type=1 all_final += server1.req.cert_type_empty +### +### A generic SECP521R1 private key +### + +secp521r1_prv.der: + $(OPENSSL) ecparam -genkey -name secp521r1 -noout -out secp521r1_prv.der +all_final += secp521r1_prv.der + ################################################################ ### Generate certificates for CRT write check tests ################################################################ 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 30e6d5f42..69c30c3ad 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -591,6 +591,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 From dff85e7e8a94e5204e7eed2a44cb13ec9184f007 Mon Sep 17 00:00:00 2001 From: k-stachowiak Date: Thu, 13 Jun 2019 11:54:49 +0200 Subject: [PATCH 5/5] Remove unnecessary empty line --- library/x509write_csr.c | 1 - 1 file changed, 1 deletion(-) diff --git a/library/x509write_csr.c b/library/x509write_csr.c index a73a1a021..d6e8c1306 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -43,7 +43,6 @@ #include "mbedtls/pem.h" #endif - /* Implementation that should never be optimized out by the compiler */ static void mbedtls_zeroize( void *v, size_t n ) { volatile unsigned char *p = v; while( n-- ) *p++ = 0;