From 557b8d663aad4d0b3bc6b80f7718fa5cf7469adc Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Thu, 19 Nov 2020 09:46:56 +0000 Subject: [PATCH] Remove Extraneous bytes from buffer post pem write In order to remove large buffers from the stack, the der data is written into the same buffer that the pem is eventually written into, however although the pem data is zero terminated, there is now data left in the buffer after the zero termination, which can cause mbedtls_x509_crt_parse to fail to parse the same buffer if passed back in. Patches also applied to mbedtls_pk_write_pubkey_pem, and mbedtls_pk_write_key_pem, which use similar methods of writing der data to the same buffer, and tests modified to hopefully catch any future regression on this. Signed-off-by: Paul Elliott --- ChangeLog.d/clean_pem_buffers.txt | 6 ++++++ library/pem.c | 4 ++++ tests/suites/test_suite_pkwrite.function | 22 +++++++++++++++++++--- tests/suites/test_suite_x509write.function | 15 +++++++++++++-- 4 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 ChangeLog.d/clean_pem_buffers.txt diff --git a/ChangeLog.d/clean_pem_buffers.txt b/ChangeLog.d/clean_pem_buffers.txt new file mode 100644 index 000000000..818fad940 --- /dev/null +++ b/ChangeLog.d/clean_pem_buffers.txt @@ -0,0 +1,6 @@ +Bugfix + * In PEM writing functions, fill the trailing part of the buffer with null + bytes. This guarantees that the corresponding parsing function can read + the buffer back, which was the case for mbedtls_x509write_{crt,csr}_pem + until this property was inadvertently broken in Mbed TLS 2.19.0. + Fixes #3682. diff --git a/library/pem.c b/library/pem.c index 534d071b3..969d492e3 100644 --- a/library/pem.c +++ b/library/pem.c @@ -478,8 +478,12 @@ int mbedtls_pem_write_buffer( const char *header, const char *footer, *p++ = '\0'; *olen = p - buf; + /* Clean any remaining data previously written to the buffer */ + memset( buf + *olen, 0, buf_len - *olen ); + mbedtls_free( encode_buf ); return( 0 ); } #endif /* MBEDTLS_PEM_WRITE_C */ #endif /* MBEDTLS_PEM_PARSE_C || MBEDTLS_PEM_WRITE_C */ + diff --git a/tests/suites/test_suite_pkwrite.function b/tests/suites/test_suite_pkwrite.function index 43c275ef2..2bad4ed13 100644 --- a/tests/suites/test_suite_pkwrite.function +++ b/tests/suites/test_suite_pkwrite.function @@ -17,7 +17,7 @@ void pk_write_pubkey_check( char * key_file ) unsigned char check_buf[5000]; int ret; FILE *f; - size_t ilen; + size_t ilen, pem_len, buf_index; memset( buf, 0, sizeof( buf ) ); memset( check_buf, 0, sizeof( check_buf ) ); @@ -28,12 +28,20 @@ void pk_write_pubkey_check( char * key_file ) ret = mbedtls_pk_write_pubkey_pem( &key, buf, sizeof( buf )); TEST_ASSERT( ret == 0 ); + pem_len = strlen( (char *) buf ); + + // check that the rest of the buffer remains clear + for( buf_index = pem_len; buf_index < sizeof( buf ); ++buf_index ) + { + TEST_ASSERT( buf[buf_index] == 0 ); + } + f = fopen( key_file, "r" ); TEST_ASSERT( f != NULL ); ilen = fread( check_buf, 1, sizeof( check_buf ), f ); fclose( f ); - TEST_ASSERT( ilen == strlen( (char *) buf ) ); + TEST_ASSERT( ilen == pem_len ); TEST_ASSERT( memcmp( (char *) buf, (char *) check_buf, ilen ) == 0 ); exit: @@ -49,7 +57,7 @@ void pk_write_key_check( char * key_file ) unsigned char check_buf[5000]; int ret; FILE *f; - size_t ilen; + size_t ilen, pem_len, buf_index; memset( buf, 0, sizeof( buf ) ); memset( check_buf, 0, sizeof( check_buf ) ); @@ -60,6 +68,14 @@ void pk_write_key_check( char * key_file ) ret = mbedtls_pk_write_key_pem( &key, buf, sizeof( buf )); TEST_ASSERT( ret == 0 ); + pem_len = strlen( (char *) buf ); + + // check that the rest of the buffer remains clear + for( buf_index = pem_len; buf_index < sizeof( buf ); ++buf_index ) + { + TEST_ASSERT( buf[buf_index] == 0 ); + } + f = fopen( key_file, "r" ); TEST_ASSERT( f != NULL ); ilen = fread( check_buf, 1, sizeof( check_buf ), f ); diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index 9f2007d0b..3803377cc 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -104,7 +104,7 @@ void x509_csr_check( char * key_file, char * cert_req_check_file, int md_type, unsigned char buf[4096]; unsigned char check_buf[4000]; int ret; - size_t olen = 0, pem_len = 0; + size_t olen = 0, pem_len = 0, buf_index; int der_len = -1; FILE *f; const char *subject_name = "C=NL,O=PolarSSL,CN=PolarSSL Server 1"; @@ -130,6 +130,11 @@ void x509_csr_check( char * key_file, char * cert_req_check_file, int md_type, pem_len = strlen( (char *) buf ); + for( buf_index = pem_len; buf_index < sizeof( buf ); ++buf_index ) + { + TEST_ASSERT( buf[buf_index] == 0 ); + } + f = fopen( cert_req_check_file, "r" ); TEST_ASSERT( f != NULL ); olen = fread( check_buf, 1, sizeof( check_buf ), f ); @@ -224,7 +229,7 @@ void x509_crt_check( char *subject_key_file, char *subject_pwd, unsigned char check_buf[5000]; mbedtls_mpi serial; int ret; - size_t olen = 0, pem_len = 0; + size_t olen = 0, pem_len = 0, buf_index = 0; int der_len = -1; FILE *f; mbedtls_test_rnd_pseudo_info rnd_info; @@ -293,6 +298,12 @@ void x509_crt_check( char *subject_key_file, char *subject_pwd, pem_len = strlen( (char *) buf ); + // check that the rest of the buffer remains clear + for( buf_index = pem_len; buf_index < sizeof( buf ); ++buf_index ) + { + TEST_ASSERT( buf[buf_index] == 0 ); + } + f = fopen( cert_check_file, "r" ); TEST_ASSERT( f != NULL ); olen = fread( check_buf, 1, sizeof( check_buf ), f );