Fix use of pem_read_buffer() in PK, DHM and X509

ChangeLog

@@ -55,6 +55,9 @@ API Changes
      available if POLARSSL_PEM_PARSE_C is defined (it never worked without).
    * Test certificates in certs.c are no longer guaranteed to be nul-terminated
      strings; use the new *_len variables instead of strlen().
+   * Functions mbedtls_x509_xxx_parse(), mbedtls_pk_parse_key(),
+     mbedtls_pk_parse_public_key() and mbedtls_dhm_parse_dhm() now expect the
+     length parameter to include the terminating null byte for PEM input.
    * Signature of mpi_mul_mpi() changed to make the last argument unsigned
    * Change SSL_DISABLE_RENEGOTIATION config.h flag to SSL_RENEGOTIATION
      (support for renegotiation now needs explicit enabling in config.h).

include/mbedtls/dhm.h

@@ -269,11 +269,12 @@ void mbedtls_dhm_free( mbedtls_dhm_context *ctx );
 #if defined(MBEDTLS_ASN1_PARSE_C)
 /** \ingroup x509_module */
 /**
- * \brief          Parse DHM parameters
+ * \brief          Parse DHM parameters in PEM or DER format
  *
  * \param dhm      DHM context to be initialized
  * \param dhmin    input buffer
  * \param dhminlen size of the buffer
+ *                 (including the terminating null byte for PEM data)
  *
  * \return         0 if successful, or a specific DHM or PEM error code
  */

include/mbedtls/pem.h

@@ -73,7 +73,7 @@ void mbedtls_pem_init( mbedtls_pem_context *ctx );
  * \param ctx       context to use
  * \param header    header string to seek and expect
  * \param footer    footer string to seek and expect
- * \param data      source data to look in
+ * \param data      source data to look in (must be nul-terminated)
  * \param pwd       password for decryption (can be NULL)
  * \param pwdlen    length of password
  * \param use_len   destination for total length used (set after header is

include/mbedtls/pk.h

@@ -427,11 +427,12 @@ mbedtls_pk_type_t mbedtls_pk_get_type( const mbedtls_pk_context *ctx );
 #if defined(MBEDTLS_PK_PARSE_C)
 /** \ingroup pk_module */
 /**
- * \brief           Parse a private key
+ * \brief           Parse a private key in PEM or DER format
  *
  * \param ctx       key to be initialized
  * \param key       input buffer
  * \param keylen    size of the buffer
+ *                  (including the terminating null byte for PEM data)
  * \param pwd       password for decryption (optional)
  * \param pwdlen    size of the password
  *
@@ -449,11 +450,12 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *ctx,
 
 /** \ingroup pk_module */
 /**
- * \brief           Parse a public key
+ * \brief           Parse a public key in PEM or DER format
  *
  * \param ctx       key to be initialized
  * \param key       input buffer
  * \param keylen    size of the buffer
+ *                  (including the terminating null byte for PEM data)
  *
  * \note            On entry, ctx must be empty, either freshly initialised
  *                  with mbedtls_pk_init() or reset with mbedtls_pk_free(). If you need a

include/mbedtls/x509_crl.h

@@ -101,7 +101,7 @@ mbedtls_x509_crl;
  *
  * \param chain    points to the start of the chain
  * \param buf      buffer holding the CRL data in DER format
- * \param buflen   size of the buffer
+ *                 (including the terminating null byte for PEM data)
  *
  * \return         0 if successful, or a specific X509 or PEM error code
  */
@@ -115,6 +115,7 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain,
  * \param chain    points to the start of the chain
  * \param buf      buffer holding the CRL data in PEM or DER format
  * \param buflen   size of the buffer
+ *                 (including the terminating null byte for PEM data)
  *
  * \return         0 if successful, or a specific X509 or PEM error code
  */

include/mbedtls/x509_crt.h

@@ -141,8 +141,9 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu
  *                 correctly, the first error is returned.
  *
  * \param chain    points to the start of the chain
- * \param buf      buffer holding the certificate data
+ * \param buf      buffer holding the certificate data in PEM or DER format
  * \param buflen   size of the buffer
+ *                 (including the terminating null byte for PEM data)
  *
  * \return         0 if all certificates parsed successfully, a positive number
  *                 if partly successful or a specific X509 or PEM error code

include/mbedtls/x509_csr.h

@@ -99,6 +99,7 @@ int mbedtls_x509_csr_parse_der( mbedtls_x509_csr *csr,
  * \param csr      CSR context to fill
  * \param buf      buffer holding the CRL data
  * \param buflen   size of the buffer
+ *                 (including the terminating null byte for PEM data)
  *
  * \return         0 if successful, or a specific X509 or PEM error code
  */

library/dhm.c

@@ -421,10 +421,14 @@ int mbedtls_dhm_parse_dhm( mbedtls_dhm_context *dhm, const unsigned char *dhmin,
 
     mbedtls_pem_init( &pem );
 
-    ret = mbedtls_pem_read_buffer( &pem,
-                           "-----BEGIN DH PARAMETERS-----",
-                           "-----END DH PARAMETERS-----",
-                           dhmin, NULL, 0, &dhminlen );
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( dhmin[dhminlen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
+        ret = mbedtls_pem_read_buffer( &pem,
+                               "-----BEGIN DH PARAMETERS-----",
+                               "-----END DH PARAMETERS-----",
+                               dhmin, NULL, 0, &dhminlen );
 
     if( ret == 0 )
     {
@@ -503,6 +507,10 @@ exit:
 #if defined(MBEDTLS_FS_IO)
 /*
  * Load all data from a file into a given buffer.
+ *
+ * The file is expected to contain either PEM or DER encoded data.
+ * A terminating null byte is always appended. It is included in the announced
+ * length only if the data looks like it is PEM encoded.
  */
 static int load_file( const char *path, unsigned char **buf, size_t *n )
 {
@@ -540,6 +548,9 @@ static int load_file( const char *path, unsigned char **buf, size_t *n )
 
     (*buf)[*n] = '\0';
 
+    if( strstr( (const char *) *buf, "-----BEGIN " ) != NULL )
+        ++*n;
+
     return( 0 );
 }
 
@@ -557,7 +568,7 @@ int mbedtls_dhm_parse_dhmfile( mbedtls_dhm_context *dhm, const char *path )
 
     ret = mbedtls_dhm_parse_dhm( dhm, buf, n );
 
-    mbedtls_zeroize( buf, n + 1 );
+    mbedtls_zeroize( buf, n );
     mbedtls_free( buf );
 
     return( ret );
@@ -584,7 +595,7 @@ int mbedtls_dhm_self_test( int verbose )
         mbedtls_printf( "  DHM parameter load: " );
 
     if( ( ret = mbedtls_dhm_parse_dhm( &dhm, (const unsigned char *) mbedtls_test_dhm_params,
-                               strlen( mbedtls_test_dhm_params ) ) ) != 0 )
+                               mbedtls_test_dhm_params_len ) ) != 0 )
     {
         if( verbose != 0 )
             mbedtls_printf( "failed\n" );

library/pkparse.c

@@ -69,6 +69,10 @@ static void mbedtls_zeroize( void *v, size_t n ) {
 
 /*
  * Load all data from a file into a given buffer.
+ *
+ * The file is expected to contain either PEM or DER encoded data.
+ * A terminating null byte is always appended. It is included in the announced
+ * length only if the data looks like it is PEM encoded.
  */
 int mbedtls_pk_load_file( const char *path, unsigned char **buf, size_t *n )
 {
@@ -106,6 +110,9 @@ int mbedtls_pk_load_file( const char *path, unsigned char **buf, size_t *n )
 
     (*buf)[*n] = '\0';
 
+    if( strstr( (const char *) *buf, "-----BEGIN " ) != NULL )
+        ++*n;
+
     return( 0 );
 }
 
@@ -128,7 +135,7 @@ int mbedtls_pk_parse_keyfile( mbedtls_pk_context *ctx,
         ret = mbedtls_pk_parse_key( ctx, buf, n,
                 (const unsigned char *) pwd, strlen( pwd ) );
 
-    mbedtls_zeroize( buf, n + 1 );
+    mbedtls_zeroize( buf, n );
     mbedtls_free( buf );
 
     return( ret );
@@ -148,7 +155,7 @@ int mbedtls_pk_parse_public_keyfile( mbedtls_pk_context *ctx, const char *path )
 
     ret = mbedtls_pk_parse_public_key( ctx, buf, n );
 
-    mbedtls_zeroize( buf, n + 1 );
+    mbedtls_zeroize( buf, n );
     mbedtls_free( buf );
 
     return( ret );
@@ -1064,10 +1071,15 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
     mbedtls_pem_init( &pem );
 
 #if defined(MBEDTLS_RSA_C)
-    ret = mbedtls_pem_read_buffer( &pem,
-                           "-----BEGIN RSA PRIVATE KEY-----",
-                           "-----END RSA PRIVATE KEY-----",
-                           key, pwd, pwdlen, &len );
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( key[keylen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
+        ret = mbedtls_pem_read_buffer( &pem,
+                               "-----BEGIN RSA PRIVATE KEY-----",
+                               "-----END RSA PRIVATE KEY-----",
+                               key, pwd, pwdlen, &len );
+
     if( ret == 0 )
     {
         if( ( pk_info = mbedtls_pk_info_from_type( MBEDTLS_PK_RSA ) ) == NULL )
@@ -1092,10 +1104,14 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
 #endif /* MBEDTLS_RSA_C */
 
 #if defined(MBEDTLS_ECP_C)
-    ret = mbedtls_pem_read_buffer( &pem,
-                           "-----BEGIN EC PRIVATE KEY-----",
-                           "-----END EC PRIVATE KEY-----",
-                           key, pwd, pwdlen, &len );
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( key[keylen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
+        ret = mbedtls_pem_read_buffer( &pem,
+                               "-----BEGIN EC PRIVATE KEY-----",
+                               "-----END EC PRIVATE KEY-----",
+                               key, pwd, pwdlen, &len );
     if( ret == 0 )
     {
         if( ( pk_info = mbedtls_pk_info_from_type( MBEDTLS_PK_ECKEY ) ) == NULL )
@@ -1119,10 +1135,14 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
         return( ret );
 #endif /* MBEDTLS_ECP_C */
 
-    ret = mbedtls_pem_read_buffer( &pem,
-                           "-----BEGIN PRIVATE KEY-----",
-                           "-----END PRIVATE KEY-----",
-                           key, NULL, 0, &len );
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( key[keylen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
+        ret = mbedtls_pem_read_buffer( &pem,
+                               "-----BEGIN PRIVATE KEY-----",
+                               "-----END PRIVATE KEY-----",
+                               key, NULL, 0, &len );
     if( ret == 0 )
     {
         if( ( ret = pk_parse_key_pkcs8_unencrypted_der( pk,
@@ -1138,10 +1158,14 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
         return( ret );
 
 #if defined(MBEDTLS_PKCS12_C) || defined(MBEDTLS_PKCS5_C)
-    ret = mbedtls_pem_read_buffer( &pem,
-                           "-----BEGIN ENCRYPTED PRIVATE KEY-----",
-                           "-----END ENCRYPTED PRIVATE KEY-----",
-                           key, NULL, 0, &len );
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( key[keylen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
+        ret = mbedtls_pem_read_buffer( &pem,
+                               "-----BEGIN ENCRYPTED PRIVATE KEY-----",
+                               "-----END ENCRYPTED PRIVATE KEY-----",
+                               key, NULL, 0, &len );
     if( ret == 0 )
     {
         if( ( ret = pk_parse_key_pkcs8_encrypted_der( pk,
@@ -1231,10 +1255,15 @@ int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx,
     mbedtls_pem_context pem;
 
     mbedtls_pem_init( &pem );
-    ret = mbedtls_pem_read_buffer( &pem,
-            "-----BEGIN PUBLIC KEY-----",
-            "-----END PUBLIC KEY-----",
-            key, NULL, 0, &len );
+
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( key[keylen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
+        ret = mbedtls_pem_read_buffer( &pem,
+                "-----BEGIN PUBLIC KEY-----",
+                "-----END PUBLIC KEY-----",
+                key, NULL, 0, &len );
 
     if( ret == 0 )
     {

library/x509.c

@@ -1008,7 +1008,7 @@ int mbedtls_x509_self_test( int verbose )
     mbedtls_x509_crt_init( &clicert );
 
     ret = mbedtls_x509_crt_parse( &clicert, (const unsigned char *) mbedtls_test_cli_crt,
-                          strlen( mbedtls_test_cli_crt ) );
+                           mbedtls_test_cli_crt_len );
     if( ret != 0 )
     {
         if( verbose != 0 )
@@ -1020,7 +1020,7 @@ int mbedtls_x509_self_test( int verbose )
     mbedtls_x509_crt_init( &cacert );
 
     ret = mbedtls_x509_crt_parse( &cacert, (const unsigned char *) mbedtls_test_ca_crt,
-                          strlen( mbedtls_test_ca_crt ) );
+                          mbedtls_test_ca_crt_len );
     if( ret != 0 )
     {
         if( verbose != 0 )

library/x509_crl.c

@@ -503,6 +503,11 @@ int mbedtls_x509_crl_parse( mbedtls_x509_crl *chain, const unsigned char *buf, s
     do
     {
         mbedtls_pem_init( &pem );
+
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( buf[buflen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
         ret = mbedtls_pem_read_buffer( &pem,
                                "-----BEGIN X509 CRL-----",
                                "-----END X509 CRL-----",
@@ -532,7 +537,9 @@ int mbedtls_x509_crl_parse( mbedtls_x509_crl *chain, const unsigned char *buf, s
             return( ret );
         }
     }
-    while( is_pem && buflen > 0 );
+    /* In the PEM case, buflen is 1 at the end, for the terminated NULL byte.
+     * And a valid CRL cannot be less than 1 byte anyway. */
+    while( is_pem && buflen > 1 );
 
     if( is_pem )
         return( 0 );
@@ -556,7 +563,7 @@ int mbedtls_x509_crl_parse_file( mbedtls_x509_crl *chain, const char *path )
 
     ret = mbedtls_x509_crl_parse( chain, buf, n );
 
-    mbedtls_zeroize( buf, n + 1 );
+    mbedtls_zeroize( buf, n );
     mbedtls_free( buf );
 
     return( ret );

library/x509_crt.c

@@ -852,8 +852,11 @@ int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, const unsigned char *buf, s
      * one or more PEM certificates.
      */
 #if defined(MBEDTLS_PEM_PARSE_C)
-    if( strstr( (const char *) buf, "-----BEGIN CERTIFICATE-----" ) != NULL )
+    if( buf[buflen - 1] == '\0' &&
+        strstr( (const char *) buf, "-----BEGIN CERTIFICATE-----" ) != NULL )
+    {
         buf_format = MBEDTLS_X509_FORMAT_PEM;
+    }
 #endif
 
     if( buf_format == MBEDTLS_X509_FORMAT_DER )
@@ -865,11 +868,13 @@ int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, const unsigned char *buf, s
         int ret;
         mbedtls_pem_context pem;
 
-        while( buflen > 0 )
+        /* 1 rather than 0 since the terminating NULL byte is counted in */
+        while( buflen > 1 )
         {
             size_t use_len;
             mbedtls_pem_init( &pem );
 
+            /* If we get there, we know the string is null-terminated */
             ret = mbedtls_pem_read_buffer( &pem,
                            "-----BEGIN CERTIFICATE-----",
                            "-----END CERTIFICATE-----",
@@ -953,7 +958,7 @@ int mbedtls_x509_crt_parse_file( mbedtls_x509_crt *chain, const char *path )
 
     ret = mbedtls_x509_crt_parse( chain, buf, n );
 
-    mbedtls_zeroize( buf, n + 1 );
+    mbedtls_zeroize( buf, n );
     mbedtls_free( buf );
 
     return( ret );

library/x509_csr.c

@@ -274,10 +274,15 @@ int mbedtls_x509_csr_parse( mbedtls_x509_csr *csr, const unsigned char *buf, siz
 
 #if defined(MBEDTLS_PEM_PARSE_C)
     mbedtls_pem_init( &pem );
-    ret = mbedtls_pem_read_buffer( &pem,
-                           "-----BEGIN CERTIFICATE REQUEST-----",
-                           "-----END CERTIFICATE REQUEST-----",
-                           buf, NULL, 0, &use_len );
+
+    /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */
+    if( buf[buflen - 1] != '\0' )
+        ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT;
+    else
+        ret = mbedtls_pem_read_buffer( &pem,
+                               "-----BEGIN CERTIFICATE REQUEST-----",
+                               "-----END CERTIFICATE REQUEST-----",
+                               buf, NULL, 0, &use_len );
 
     if( ret == 0 )
     {
@@ -315,7 +320,7 @@ int mbedtls_x509_csr_parse_file( mbedtls_x509_csr *csr, const char *path )
 
     ret = mbedtls_x509_csr_parse( csr, buf, n );
 
-    mbedtls_zeroize( buf, n + 1 );
+    mbedtls_zeroize( buf, n );
     mbedtls_free( buf );
 
     return( ret );