From afc2717e846bc7824f1bdded2a5e8a264c22d7ea Mon Sep 17 00:00:00 2001 From: Doru Gucea Date: Fri, 14 Dec 2018 21:08:35 +0200 Subject: [PATCH 1/3] Avoid stack-allocation of large memory buffers Using a stack-buffer with a size > 2K could easily produce a stack overflow for an embedded device which has a limited stack size. This commit dynamically allocates the large CSR buffer. This commit avoids using a temporary buffer for storing the OIDs. A single buffer is used: a) OIDs are written backwards starting with the end of the buffer; b) OIDs are memmove'd to the beginning of the buffer; c) signature over this OIDs is computed and written backwards from the end of the buffer; d) the two memory regions are compacted. Signed-off-by: Doru Gucea --- library/x509write_csr.c | 114 ++++++++++++++++++++++++++-------------- 1 file changed, 74 insertions(+), 40 deletions(-) diff --git a/library/x509write_csr.c b/library/x509write_csr.c index d1b0716c9..4419c2eed 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -81,6 +81,14 @@ #define SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE #endif +#if defined(MBEDTLS_PLATFORM_C) +#include "mbedtls/platform.h" +#else +#include +#define mbedtls_calloc calloc +#define mbedtls_free free +#endif + void mbedtls_x509write_csr_init( mbedtls_x509write_csr *ctx ) { memset( ctx, 0, sizeof( mbedtls_x509write_csr ) ); @@ -187,68 +195,74 @@ int mbedtls_x509write_csr_set_ns_cert_type( mbedtls_x509write_csr *ctx, return( 0 ); } -int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, size_t size, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +static int x509write_csr_der_internal( mbedtls_x509write_csr *ctx, + unsigned char *buf, + size_t size, unsigned char *sig, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) { int ret; const char *sig_oid; size_t sig_oid_len = 0; unsigned char *c, *c2; unsigned char hash[64]; - 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; mbedtls_pk_type_t pk_alg; /* - * Prepare data to be signed in tmp_buf + * Writing strategy: + * 1. start writing from the back of buf + * 2. sign the written data and place the signature at the start of buf + * 3. compact memory locations by moving the signature towards right */ - c = tmp_buf + sizeof( tmp_buf ); + c = buf + size; - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_x509_write_extensions( &c, tmp_buf, ctx->extensions ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_x509_write_extensions( &c, buf, + ctx->extensions ) ); if( len ) { - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, tmp_buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, tmp_buf, MBEDTLS_ASN1_CONSTRUCTED | - MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, tmp_buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, tmp_buf, MBEDTLS_ASN1_CONSTRUCTED | - MBEDTLS_ASN1_SET ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SET ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_oid( &c, tmp_buf, MBEDTLS_OID_PKCS9_CSR_EXT_REQ, - MBEDTLS_OID_SIZE( MBEDTLS_OID_PKCS9_CSR_EXT_REQ ) ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_oid( &c, buf, + MBEDTLS_OID_PKCS9_CSR_EXT_REQ, + MBEDTLS_OID_SIZE( MBEDTLS_OID_PKCS9_CSR_EXT_REQ ) ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, tmp_buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, tmp_buf, MBEDTLS_ASN1_CONSTRUCTED | - MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); } - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, tmp_buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, tmp_buf, MBEDTLS_ASN1_CONSTRUCTED | - MBEDTLS_ASN1_CONTEXT_SPECIFIC ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_CONTEXT_SPECIFIC ) ); MBEDTLS_ASN1_CHK_ADD( pub_len, mbedtls_pk_write_pubkey_der( ctx->key, - tmp_buf, c - tmp_buf ) ); + buf, c - buf ) ); c -= pub_len; len += pub_len; /* * Subject ::= Name */ - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_x509_write_names( &c, tmp_buf, ctx->subject ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_x509_write_names( &c, buf, + ctx->subject ) ); /* * Version ::= INTEGER { v1(0), v2(1), v3(2) } */ - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_int( &c, tmp_buf, 0 ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_int( &c, buf, 0 ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, tmp_buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, tmp_buf, MBEDTLS_ASN1_CONSTRUCTED | - MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); /* * Prepare signature @@ -271,32 +285,52 @@ int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, s return( MBEDTLS_ERR_X509_INVALID_ALG ); if( ( ret = mbedtls_oid_get_oid_by_sig_alg( pk_alg, ctx->md_alg, - &sig_oid, &sig_oid_len ) ) != 0 ) + &sig_oid, &sig_oid_len ) ) != 0 ) { return( ret ); } - /* - * Write data to output buffer - */ + /* reserve space for the signature at the end of buf */ + memmove( buf, c, len ); + + /* copy the signature */ c2 = buf + size; - MBEDTLS_ASN1_CHK_ADD( sig_and_oid_len, mbedtls_x509_write_sig( &c2, buf, - sig_oid, sig_oid_len, sig, sig_len ) ); - - if( len > (size_t)( c2 - buf ) ) - return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + MBEDTLS_ASN1_CHK_ADD( sig_and_oid_len, mbedtls_x509_write_sig( &c2, + buf + len, sig_oid, sig_oid_len, sig, sig_len ) ); + /* compact oids and signature memory locations */ c2 -= len; - memcpy( c2, c, len ); + memmove( c2, buf, len ); len += sig_and_oid_len; MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c2, buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c2, buf, MBEDTLS_ASN1_CONSTRUCTED | - MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c2, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); + memset( buf, 0, c2 - buf); return( (int) len ); } +int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, + size_t size, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + int ret; + unsigned char *sig; + + if( ( sig = mbedtls_calloc( 1, MBEDTLS_MPI_MAX_SIZE ) ) == NULL ) + { + return( MBEDTLS_ERR_ASN1_ALLOC_FAILED ); + } + + ret = x509write_csr_der_internal( ctx, buf, size, sig, f_rng, p_rng ); + + mbedtls_free( sig ); + + return( ret ); +} + #define PEM_BEGIN_CSR "-----BEGIN CERTIFICATE REQUEST-----\n" #define PEM_END_CSR "-----END CERTIFICATE REQUEST-----\n" From 1535a431490a61420cd5cede08ad4b36dbdfed67 Mon Sep 17 00:00:00 2001 From: Simon Leet Date: Fri, 26 Jun 2020 21:23:32 +0000 Subject: [PATCH 2/3] Revise comments for x509write_csr_der_internal Address remaining PR comments for #2118 - Add ChangeLog.d/x509write_csr_heap_alloc.txt. - Fix parameter alignment per Gille's recommendation. - Update comments to more explicitly describe the manipulation of buf. - Replace use of `MBEDTLS_MPI_MAX_SIZE` as `sig` buffer size for call to `x509write_csr_der_internal()` with more intuitive `MBEDTLS_PK_SIGNATURE_MAX_SIZE`. - Update `mbedtls_x509write_csr_der()` to return `MBEDTLS_ERR_X509_ALLOC_FAILED` on mbedtls_calloc error. Signed-off-by: Simon Leet --- ChangeLog.d/x509write_csr_heap_alloc.txt | 4 ++ library/x509write_csr.c | 84 +++++++++++++++--------- 2 files changed, 58 insertions(+), 30 deletions(-) create mode 100644 ChangeLog.d/x509write_csr_heap_alloc.txt diff --git a/ChangeLog.d/x509write_csr_heap_alloc.txt b/ChangeLog.d/x509write_csr_heap_alloc.txt new file mode 100644 index 000000000..abce20c4d --- /dev/null +++ b/ChangeLog.d/x509write_csr_heap_alloc.txt @@ -0,0 +1,4 @@ +Changes + * Reduce the stack consumption of mbedtls_x509write_csr_der() which + previously could lead to stack overflow on constrained devices. + Contributed by Doru Gucea and Simon Leet in #3464. diff --git a/library/x509write_csr.c b/library/x509write_csr.c index 4419c2eed..ac5eaaa4a 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -197,7 +197,8 @@ int mbedtls_x509write_csr_set_ns_cert_type( mbedtls_x509write_csr *ctx, static int x509write_csr_der_internal( mbedtls_x509write_csr *ctx, unsigned char *buf, - size_t size, unsigned char *sig, + size_t size, + unsigned char *sig, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { @@ -210,12 +211,7 @@ static int x509write_csr_der_internal( mbedtls_x509write_csr *ctx, size_t len = 0; mbedtls_pk_type_t pk_alg; - /* - * Writing strategy: - * 1. start writing from the back of buf - * 2. sign the written data and place the signature at the start of buf - * 3. compact memory locations by moving the signature towards right - */ + /* Write the CSR backwards starting from the end of buf */ c = buf + size; MBEDTLS_ASN1_CHK_ADD( len, mbedtls_x509_write_extensions( &c, buf, @@ -224,25 +220,34 @@ static int x509write_csr_der_internal( mbedtls_x509write_csr *ctx, if( len ) { MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, - MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, + mbedtls_asn1_write_tag( + &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, - MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SET ) ); + MBEDTLS_ASN1_CHK_ADD( len, + mbedtls_asn1_write_tag( + &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SET ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_oid( &c, buf, - MBEDTLS_OID_PKCS9_CSR_EXT_REQ, - MBEDTLS_OID_SIZE( MBEDTLS_OID_PKCS9_CSR_EXT_REQ ) ) ); + MBEDTLS_ASN1_CHK_ADD( len, + mbedtls_asn1_write_oid( + &c, buf, MBEDTLS_OID_PKCS9_CSR_EXT_REQ, + MBEDTLS_OID_SIZE( MBEDTLS_OID_PKCS9_CSR_EXT_REQ ) ) ); MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, - MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, + mbedtls_asn1_write_tag( + &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); } MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, - MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_CONTEXT_SPECIFIC ) ); + MBEDTLS_ASN1_CHK_ADD( len, + mbedtls_asn1_write_tag( + &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_CONTEXT_SPECIFIC ) ); MBEDTLS_ASN1_CHK_ADD( pub_len, mbedtls_pk_write_pubkey_der( ctx->key, buf, c - buf ) ); @@ -261,11 +266,14 @@ static int x509write_csr_der_internal( mbedtls_x509write_csr *ctx, MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_int( &c, buf, 0 ) ); MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c, buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c, buf, - MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, + mbedtls_asn1_write_tag( + &c, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); /* - * Prepare signature + * Sign the written CSR data into the sig buffer + * Note: hash errors can happen only after an internal error */ ret = mbedtls_md( mbedtls_md_info_from_type( ctx->md_alg ), c, len, hash ); if( ret != 0 ) @@ -290,22 +298,38 @@ static int x509write_csr_der_internal( mbedtls_x509write_csr *ctx, return( ret ); } - /* reserve space for the signature at the end of buf */ + /* + * Move the written CSR data to the start of buf to create space for + * writing the signature into buf. + */ memmove( buf, c, len ); - /* copy the signature */ + /* + * Write sig and its OID into buf backwards from the end of buf. + * Note: mbedtls_x509_write_sig will check for c2 - ( buf + len ) < sig_len + * and return MBEDTLS_ERR_ASN1_BUF_TOO_SMALL if needed. + */ c2 = buf + size; - MBEDTLS_ASN1_CHK_ADD( sig_and_oid_len, mbedtls_x509_write_sig( &c2, - buf + len, sig_oid, sig_oid_len, sig, sig_len ) ); + MBEDTLS_ASN1_CHK_ADD( sig_and_oid_len, + mbedtls_x509_write_sig( &c2, buf + len, sig_oid, sig_oid_len, + sig, sig_len ) ); - /* compact oids and signature memory locations */ + /* + * Compact the space between the CSR data and signature by moving the + * CSR data to the start of the signature. + */ c2 -= len; memmove( c2, buf, len ); + /* ASN encode the total size and tag the CSR data with it. */ len += sig_and_oid_len; MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_len( &c2, buf, len ) ); - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_asn1_write_tag( &c2, buf, - MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); + MBEDTLS_ASN1_CHK_ADD( len, + mbedtls_asn1_write_tag( + &c2, buf, + MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ); + + /* Zero the unused bytes at the start of buf */ memset( buf, 0, c2 - buf); return( (int) len ); @@ -319,9 +343,9 @@ int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, int ret; unsigned char *sig; - if( ( sig = mbedtls_calloc( 1, MBEDTLS_MPI_MAX_SIZE ) ) == NULL ) + if( ( sig = mbedtls_calloc( 1, SIGNATURE_MAX_SIZE ) ) == NULL ) { - return( MBEDTLS_ERR_ASN1_ALLOC_FAILED ); + return( MBEDTLS_ERR_X509_ALLOC_FAILED ); } ret = x509write_csr_der_internal( ctx, buf, size, sig, f_rng, p_rng ); From a242f50acd0cb7fcb6dd8b90063f9e70733be483 Mon Sep 17 00:00:00 2001 From: Simon Leet Date: Sat, 18 Jul 2020 01:14:00 +0000 Subject: [PATCH 3/3] Classify #3464 ChangeLog entry as Bugfix Signed-off-by: Simon Leet --- ChangeLog.d/x509write_csr_heap_alloc.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/x509write_csr_heap_alloc.txt b/ChangeLog.d/x509write_csr_heap_alloc.txt index abce20c4d..6223698bb 100644 --- a/ChangeLog.d/x509write_csr_heap_alloc.txt +++ b/ChangeLog.d/x509write_csr_heap_alloc.txt @@ -1,4 +1,4 @@ -Changes +Bugfix * Reduce the stack consumption of mbedtls_x509write_csr_der() which previously could lead to stack overflow on constrained devices. Contributed by Doru Gucea and Simon Leet in #3464.