From 43b37cbc92f35bae312f08a0ae285c582412c46d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 12 May 2015 11:20:10 +0200 Subject: [PATCH] Fix use of pem_read_buffer() in PK, DHM and X509 --- ChangeLog | 3 ++ include/mbedtls/dhm.h | 3 +- include/mbedtls/pem.h | 2 +- include/mbedtls/pk.h | 6 ++-- include/mbedtls/x509_crl.h | 3 +- include/mbedtls/x509_crt.h | 3 +- include/mbedtls/x509_csr.h | 1 + library/dhm.c | 23 ++++++++---- library/pkparse.c | 73 ++++++++++++++++++++++++++------------ library/x509.c | 4 +-- library/x509_crl.c | 11 ++++-- library/x509_crt.c | 11 ++++-- library/x509_csr.c | 15 +++++--- 13 files changed, 112 insertions(+), 46 deletions(-) diff --git a/ChangeLog b/ChangeLog index 361eea39b..58c925f72 100644 --- a/ChangeLog +++ b/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). diff --git a/include/mbedtls/dhm.h b/include/mbedtls/dhm.h index 4d1661929..caa630d6d 100644 --- a/include/mbedtls/dhm.h +++ b/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 */ diff --git a/include/mbedtls/pem.h b/include/mbedtls/pem.h index ef945292a..f826abdcb 100644 --- a/include/mbedtls/pem.h +++ b/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 diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index 44efef081..3ed8b5e95 100644 --- a/include/mbedtls/pk.h +++ b/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 diff --git a/include/mbedtls/x509_crl.h b/include/mbedtls/x509_crl.h index 395c1b37e..7b40739eb 100644 --- a/include/mbedtls/x509_crl.h +++ b/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 */ diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index e184deeff..aced7eb0f 100644 --- a/include/mbedtls/x509_crt.h +++ b/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 diff --git a/include/mbedtls/x509_csr.h b/include/mbedtls/x509_csr.h index c46bc613d..0082ada2e 100644 --- a/include/mbedtls/x509_csr.h +++ b/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 */ diff --git a/library/dhm.c b/library/dhm.c index c5e41a36f..92fd6110f 100644 --- a/library/dhm.c +++ b/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" ); diff --git a/library/pkparse.c b/library/pkparse.c index f5a985322..edf6e315b 100644 --- a/library/pkparse.c +++ b/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 ) { diff --git a/library/x509.c b/library/x509.c index 55daf74d4..08a1ec371 100644 --- a/library/x509.c +++ b/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 ) diff --git a/library/x509_crl.c b/library/x509_crl.c index a915abacc..fc4b2dfdf 100644 --- a/library/x509_crl.c +++ b/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 ); diff --git a/library/x509_crt.c b/library/x509_crt.c index 4ebae7796..059b60f47 100644 --- a/library/x509_crt.c +++ b/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 ); diff --git a/library/x509_csr.c b/library/x509_csr.c index f6afa1ef6..5ec1b8694 100644 --- a/library/x509_csr.c +++ b/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 );