From afc2717e846bc7824f1bdded2a5e8a264c22d7ea Mon Sep 17 00:00:00 2001
From: Doru Gucea <doru-cristian.gucea@nxp.com>
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 <doru-cristian.gucea@nxp.com>
---
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 d1b0716c968c..4419c2eed334 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 <stdlib.h>
+#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
- */
- 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 ) );
+ /* reserve space for the signature at the end of buf */
+ memmove( buf, c, len );
- if( len > (size_t)( c2 - buf ) )
- return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL );
+ /* copy the signature */
+ 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 ) );
+ /* 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 <simon.leet@microsoft.com>
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 <simon.leet@microsoft.com>
---
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 000000000000..abce20c4df74
--- /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 4419c2eed334..ac5eaaa4a75e 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 <simon.leet@microsoft.com>
Date: Sat, 18 Jul 2020 01:14:00 +0000
Subject: [PATCH 3/3] Classify #3464 ChangeLog entry as Bugfix
Signed-off-by: Simon Leet <simon.leet@microsoft.com>
---
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 abce20c4df74..6223698bbc8a 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.