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 <paul.elliott@arm.com>
This commit is contained in:
Paul Elliott 2020-11-19 09:46:56 +00:00
parent 3f35b87db9
commit 319b5939dd
4 changed files with 42 additions and 5 deletions

View file

@ -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.

View file

@ -508,8 +508,12 @@ int mbedtls_pem_write_buffer( const char *header, const char *footer,
*p++ = '\0'; *p++ = '\0';
*olen = p - buf; *olen = p - buf;
/* Clean any remaining data previously written to the buffer */
memset( buf + *olen, 0, buf_len - *olen );
mbedtls_free( encode_buf ); mbedtls_free( encode_buf );
return( 0 ); return( 0 );
} }
#endif /* MBEDTLS_PEM_WRITE_C */ #endif /* MBEDTLS_PEM_WRITE_C */
#endif /* MBEDTLS_PEM_PARSE_C || MBEDTLS_PEM_WRITE_C */ #endif /* MBEDTLS_PEM_PARSE_C || MBEDTLS_PEM_WRITE_C */

View file

@ -17,7 +17,7 @@ void pk_write_pubkey_check( char * key_file )
unsigned char check_buf[5000]; unsigned char check_buf[5000];
int ret; int ret;
FILE *f; FILE *f;
size_t ilen; size_t ilen, pem_len, buf_index;
memset( buf, 0, sizeof( buf ) ); memset( buf, 0, sizeof( buf ) );
memset( check_buf, 0, sizeof( check_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 )); ret = mbedtls_pk_write_pubkey_pem( &key, buf, sizeof( buf ));
TEST_ASSERT( ret == 0 ); 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" ); f = fopen( key_file, "r" );
TEST_ASSERT( f != NULL ); TEST_ASSERT( f != NULL );
ilen = fread( check_buf, 1, sizeof( check_buf ), f ); ilen = fread( check_buf, 1, sizeof( check_buf ), f );
fclose( f ); fclose( f );
TEST_ASSERT( ilen == strlen( (char *) buf ) ); TEST_ASSERT( ilen == pem_len );
TEST_ASSERT( memcmp( (char *) buf, (char *) check_buf, ilen ) == 0 ); TEST_ASSERT( memcmp( (char *) buf, (char *) check_buf, ilen ) == 0 );
exit: exit:
@ -49,7 +57,7 @@ void pk_write_key_check( char * key_file )
unsigned char check_buf[5000]; unsigned char check_buf[5000];
int ret; int ret;
FILE *f; FILE *f;
size_t ilen; size_t ilen, pem_len, buf_index;
memset( buf, 0, sizeof( buf ) ); memset( buf, 0, sizeof( buf ) );
memset( check_buf, 0, sizeof( check_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 )); ret = mbedtls_pk_write_key_pem( &key, buf, sizeof( buf ));
TEST_ASSERT( ret == 0 ); 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" ); f = fopen( key_file, "r" );
TEST_ASSERT( f != NULL ); TEST_ASSERT( f != NULL );
ilen = fread( check_buf, 1, sizeof( check_buf ), f ); ilen = fread( check_buf, 1, sizeof( check_buf ), f );

View file

@ -45,7 +45,7 @@ void x509_csr_check( char * key_file, char * cert_req_check_file, int md_type,
unsigned char buf[4096]; unsigned char buf[4096];
unsigned char check_buf[4000]; unsigned char check_buf[4000];
int ret; int ret;
size_t olen = 0, pem_len = 0; size_t olen = 0, pem_len = 0, buf_index;
int der_len = -1; int der_len = -1;
FILE *f; FILE *f;
const char *subject_name = "C=NL,O=PolarSSL,CN=PolarSSL Server 1"; const char *subject_name = "C=NL,O=PolarSSL,CN=PolarSSL Server 1";
@ -71,6 +71,11 @@ void x509_csr_check( char * key_file, char * cert_req_check_file, int md_type,
pem_len = strlen( (char *) buf ); 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" ); f = fopen( cert_req_check_file, "r" );
TEST_ASSERT( f != NULL ); TEST_ASSERT( f != NULL );
olen = fread( check_buf, 1, sizeof( check_buf ), f ); olen = fread( check_buf, 1, sizeof( check_buf ), f );
@ -113,7 +118,7 @@ void x509_crt_check( char *subject_key_file, char *subject_pwd,
unsigned char check_buf[5000]; unsigned char check_buf[5000];
mbedtls_mpi serial; mbedtls_mpi serial;
int ret; int ret;
size_t olen = 0, pem_len = 0; size_t olen = 0, pem_len = 0, buf_index = 0;
int der_len = -1; int der_len = -1;
FILE *f; FILE *f;
rnd_pseudo_info rnd_info; rnd_pseudo_info rnd_info;
@ -182,6 +187,12 @@ void x509_crt_check( char *subject_key_file, char *subject_pwd,
pem_len = strlen( (char *) buf ); 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" ); f = fopen( cert_check_file, "r" );
TEST_ASSERT( f != NULL ); TEST_ASSERT( f != NULL );
olen = fread( check_buf, 1, sizeof( check_buf ), f ); olen = fread( check_buf, 1, sizeof( check_buf ), f );