diff --git a/ChangeLog.d/clean_pem_buffers.txt b/ChangeLog.d/clean_pem_buffers.txt new file mode 100644 index 000000000000..8ab6605430ff --- /dev/null +++ b/ChangeLog.d/clean_pem_buffers.txt @@ -0,0 +1,13 @@ +Bugfix + * As an optimisation to stop large buffers being written to the stack the + code was changed to write the raw der data to the same buffer that the + pem data would later be written to. Unfortunately even though the pem data + was always NULL terminated the der data would be left in the buffer after + the NULL termination. This is an issue as if you passed the same buffer + and length into one of the pem write functions and then into its + corresponding parse functions, then the parse function would fail, as its + test for whether the data was der or pem involved testing the last byte of + the buffer to see if it was zero. Tests have also been modified to try and + prevent this regression from happening again. + Thanks to Faless for reporting this in #3682 + diff --git a/library/pkwrite.c b/library/pkwrite.c index 0da3698189e6..2b0ab08d5f1c 100644 --- a/library/pkwrite.c +++ b/library/pkwrite.c @@ -575,6 +575,9 @@ int mbedtls_pk_write_pubkey_pem( mbedtls_pk_context *key, unsigned char *buf, si return( ret ); } + /* Clean any remaining der data from the buffer */ + memset( buf + olen, 0, size - olen ); + return( 0 ); } @@ -616,6 +619,9 @@ int mbedtls_pk_write_key_pem( mbedtls_pk_context *key, unsigned char *buf, size_ return( ret ); } + /* Clean any remaining der data from the buffer */ + memset( buf + olen, 0, size - olen ); + return( 0 ); } #endif /* MBEDTLS_PEM_WRITE_C */ diff --git a/library/x509write_crt.c b/library/x509write_crt.c index 32c6550968c4..b47ed89fcfbe 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -537,6 +537,9 @@ int mbedtls_x509write_crt_pem( mbedtls_x509write_cert *crt, return( ret ); } + /* Clean any remaining der data from the buffer */ + memset( buf + olen, 0, size - olen ); + return( 0 ); } #endif /* MBEDTLS_PEM_WRITE_C */ diff --git a/library/x509write_csr.c b/library/x509write_csr.c index c7c8032bec81..48004248a73d 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -334,6 +334,9 @@ int mbedtls_x509write_csr_pem( mbedtls_x509write_csr *ctx, unsigned char *buf, s return( ret ); } + /* Clean any remaining der data from the buffer */ + memset( buf + olen, 0, size - olen ); + return( 0 ); } #endif /* MBEDTLS_PEM_WRITE_C */ diff --git a/tests/suites/test_suite_pkwrite.function b/tests/suites/test_suite_pkwrite.function index 43c275ef255c..885228a9a376 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,22 @@ 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 < 5000; ++buf_index ) + { + TEST_ASSERT( buf[buf_index] == 0 ); + } + + TEST_ASSERT((5000 - pem_len) > 2); + 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 +59,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 +70,16 @@ 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 < 5000; ++buf_index ) + { + TEST_ASSERT( buf[buf_index] == 0 ); + } + + TEST_ASSERT((5000 - pem_len) > 2); + 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 9f2007d0bc32..bd8d7a87719f 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -224,7 +224,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 +293,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 < 4096; ++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 );