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 <[email protected]>

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
+

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 */

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 */

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 */

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 );

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 );