From 51a7ae1353f623d96bd2a7821dc12c1937baf89b Mon Sep 17 00:00:00 2001 From: Andres AG Date: Wed, 22 Feb 2017 16:23:26 +0000 Subject: [PATCH 1/2] Add missing ret code checks in PEM module Add missing return code checks in the functions pem_des_decrypt(), pem_3des_decrypt() and pem_aes_decrypt() so that the calling function mbedtls_pem_read_buffer() is notified of errors reported by the crypto primitives AES, DES and 3DES. --- ChangeLog | 10 ++++++++ library/pem.c | 63 +++++++++++++++++++++++++++++++++++---------------- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8797b1af6..daa9622f3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,15 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS x.x.x branch released xxxx-xx-xx + +Bugfix + * Fix unchecked return codes from AES, DES and 3DES functions in + pem_aes_decrypt(), pem_des_decrypt() and pem_des3_decrypt() respectively. + If a call to one of the functions of the cryptographic primitive modules + failed, the error may not be noticed by the function + mbedtls_pem_read_buffer() causing it to return invalid values. Found by + Guido Vranken. #756 + = mbed TLS 2.5.0 branch released 2017-05-17 Security diff --git a/library/pem.c b/library/pem.c index 8dd86a4ac..87401ba55 100644 --- a/library/pem.c +++ b/library/pem.c @@ -134,45 +134,55 @@ static void pem_pbkdf1( unsigned char *key, size_t keylen, /* * Decrypt with DES-CBC, using PBKDF1 for key derivation */ -static void pem_des_decrypt( unsigned char des_iv[8], - unsigned char *buf, size_t buflen, - const unsigned char *pwd, size_t pwdlen ) +static int pem_des_decrypt( unsigned char des_iv[8], + unsigned char *buf, size_t buflen, + const unsigned char *pwd, size_t pwdlen ) { mbedtls_des_context des_ctx; unsigned char des_key[8]; + int ret; mbedtls_des_init( &des_ctx ); pem_pbkdf1( des_key, 8, des_iv, pwd, pwdlen ); - mbedtls_des_setkey_dec( &des_ctx, des_key ); - mbedtls_des_crypt_cbc( &des_ctx, MBEDTLS_DES_DECRYPT, buflen, + if( ( ret = mbedtls_des_setkey_dec( &des_ctx, des_key ) ) != 0 ) + goto exit; + ret = mbedtls_des_crypt_cbc( &des_ctx, MBEDTLS_DES_DECRYPT, buflen, des_iv, buf, buf ); +exit: mbedtls_des_free( &des_ctx ); mbedtls_zeroize( des_key, 8 ); + + return( ret ); } /* * Decrypt with 3DES-CBC, using PBKDF1 for key derivation */ -static void pem_des3_decrypt( unsigned char des3_iv[8], - unsigned char *buf, size_t buflen, - const unsigned char *pwd, size_t pwdlen ) +static int pem_des3_decrypt( unsigned char des3_iv[8], + unsigned char *buf, size_t buflen, + const unsigned char *pwd, size_t pwdlen ) { mbedtls_des3_context des3_ctx; unsigned char des3_key[24]; + int ret; mbedtls_des3_init( &des3_ctx ); pem_pbkdf1( des3_key, 24, des3_iv, pwd, pwdlen ); - mbedtls_des3_set3key_dec( &des3_ctx, des3_key ); - mbedtls_des3_crypt_cbc( &des3_ctx, MBEDTLS_DES_DECRYPT, buflen, + if( ( ret = mbedtls_des3_set3key_dec( &des3_ctx, des3_key ) ) != 0 ) + goto exit; + ret = mbedtls_des3_crypt_cbc( &des3_ctx, MBEDTLS_DES_DECRYPT, buflen, des3_iv, buf, buf ); +exit: mbedtls_des3_free( &des3_ctx ); mbedtls_zeroize( des3_key, 24 ); + + return( ret ); } #endif /* MBEDTLS_DES_C */ @@ -180,23 +190,28 @@ static void pem_des3_decrypt( unsigned char des3_iv[8], /* * Decrypt with AES-XXX-CBC, using PBKDF1 for key derivation */ -static void pem_aes_decrypt( unsigned char aes_iv[16], unsigned int keylen, - unsigned char *buf, size_t buflen, - const unsigned char *pwd, size_t pwdlen ) +static int pem_aes_decrypt( unsigned char aes_iv[16], unsigned int keylen, + unsigned char *buf, size_t buflen, + const unsigned char *pwd, size_t pwdlen ) { mbedtls_aes_context aes_ctx; unsigned char aes_key[32]; + int ret; mbedtls_aes_init( &aes_ctx ); pem_pbkdf1( aes_key, keylen, aes_iv, pwd, pwdlen ); - mbedtls_aes_setkey_dec( &aes_ctx, aes_key, keylen * 8 ); - mbedtls_aes_crypt_cbc( &aes_ctx, MBEDTLS_AES_DECRYPT, buflen, + if( ( ret = mbedtls_aes_setkey_dec( &aes_ctx, aes_key, keylen * 8 ) ) != 0 ) + goto exit; + ret = mbedtls_aes_crypt_cbc( &aes_ctx, MBEDTLS_AES_DECRYPT, buflen, aes_iv, buf, buf ); +exit: mbedtls_aes_free( &aes_ctx ); mbedtls_zeroize( aes_key, keylen ); + + return( ret ); } #endif /* MBEDTLS_AES_C */ @@ -345,22 +360,30 @@ int mbedtls_pem_read_buffer( mbedtls_pem_context *ctx, const char *header, const return( MBEDTLS_ERR_PEM_PASSWORD_REQUIRED ); } + ret = 0; + #if defined(MBEDTLS_DES_C) if( enc_alg == MBEDTLS_CIPHER_DES_EDE3_CBC ) - pem_des3_decrypt( pem_iv, buf, len, pwd, pwdlen ); + ret = pem_des3_decrypt( pem_iv, buf, len, pwd, pwdlen ); else if( enc_alg == MBEDTLS_CIPHER_DES_CBC ) - pem_des_decrypt( pem_iv, buf, len, pwd, pwdlen ); + ret = pem_des_decrypt( pem_iv, buf, len, pwd, pwdlen ); #endif /* MBEDTLS_DES_C */ #if defined(MBEDTLS_AES_C) if( enc_alg == MBEDTLS_CIPHER_AES_128_CBC ) - pem_aes_decrypt( pem_iv, 16, buf, len, pwd, pwdlen ); + ret = pem_aes_decrypt( pem_iv, 16, buf, len, pwd, pwdlen ); else if( enc_alg == MBEDTLS_CIPHER_AES_192_CBC ) - pem_aes_decrypt( pem_iv, 24, buf, len, pwd, pwdlen ); + ret = pem_aes_decrypt( pem_iv, 24, buf, len, pwd, pwdlen ); else if( enc_alg == MBEDTLS_CIPHER_AES_256_CBC ) - pem_aes_decrypt( pem_iv, 32, buf, len, pwd, pwdlen ); + ret = pem_aes_decrypt( pem_iv, 32, buf, len, pwd, pwdlen ); #endif /* MBEDTLS_AES_C */ + if( ret != 0 ) + { + mbedtls_free( buf ); + return( ret ); + } + /* * The result will be ASN.1 starting with a SEQUENCE tag, with 1 to 3 * length bytes (allow 4 to be sure) in all known use cases. From a3b9adb6bd9e0d23b4e7797e5fdf2c310d9914e5 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Wed, 1 Mar 2017 11:53:29 +0000 Subject: [PATCH 2/2] Add negative testing for mbedtls_pem_read_buffer() --- tests/suites/test_suite_pem.data | 17 ++++++++++++++--- tests/suites/test_suite_pem.function | 13 +++++++++---- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/tests/suites/test_suite_pem.data b/tests/suites/test_suite_pem.data index 065e4a2b5..77546c586 100644 --- a/tests/suites/test_suite_pem.data +++ b/tests/suites/test_suite_pem.data @@ -17,11 +17,22 @@ PEM write (exactly two lines + 1) mbedtls_pem_write_buffer:"-----START TEST-----\n":"-----END TEST-----\n":"000102030405060708090A0B0C0D0E0F000102030405060708090A0B0C0D0E0F000102030405060708090A0B0C0D0E0F000102030405060708090A0B0C0D0E0F000102030405060708090A0B0C0D0E0F000102030405060708090A0B0C0D0E0F00":"-----START TEST-----\nAAECAwQFBgcICQoLDA0ODwABAgMEBQYHCAkKCwwNDg8AAQIDBAUGBwgJCgsMDQ4P\nAAECAwQFBgcICQoLDA0ODwABAgMEBQYHCAkKCwwNDg8AAQIDBAUGBwgJCgsMDQ4P\nAA==\n-----END TEST-----\n" PEM read (DES-EDE3-CBC + invalid iv) -mbedtls_pem_read_buffer:"^":"$":"^\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-EDE3-CBC,00$":MBEDTLS_ERR_PEM_INVALID_ENC_IV +mbedtls_pem_read_buffer:"^":"$":"^\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-EDE3-CBC,00$":"pwd":MBEDTLS_ERR_PEM_INVALID_ENC_IV PEM read (DES-CBC + invalid iv) -mbedtls_pem_read_buffer:"^":"$":"^\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-CBC,00$":MBEDTLS_ERR_PEM_INVALID_ENC_IV +mbedtls_pem_read_buffer:"^":"$":"^\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-CBC,00$":"pwd":MBEDTLS_ERR_PEM_INVALID_ENC_IV PEM read (unknown encryption algorithm) -mbedtls_pem_read_buffer:"^":"$":"^\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-,00$":MBEDTLS_ERR_PEM_UNKNOWN_ENC_ALG +mbedtls_pem_read_buffer:"^":"$":"^\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-,00$":"pwd":MBEDTLS_ERR_PEM_UNKNOWN_ENC_ALG +PEM read (malformed PEM DES-CBC) +depends_on:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC +mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-CBC,AA94892A169FA426\n\nMAAA\n-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_DES_INVALID_INPUT_LENGTH + +PEM read (malformed PEM DES-EDE3-CBC) +depends_on:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC +mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-EDE3-CBC,AA94892A169FA426\n\nMAAA\n-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_DES_INVALID_INPUT_LENGTH + +PEM read (malformed PEM AES-128-CBC) +depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC +mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-128-CBC,AA94892A169FA426AA94892A169FA426\n\nMAAA\n-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH diff --git a/tests/suites/test_suite_pem.function b/tests/suites/test_suite_pem.function index 5e022109c..c24595d47 100644 --- a/tests/suites/test_suite_pem.function +++ b/tests/suites/test_suite_pem.function @@ -1,6 +1,8 @@ /* BEGIN_HEADER */ #include "mbedtls/base64.h" #include "mbedtls/pem.h" +#include "mbedtls/des.h" +#include "mbedtls/aes.h" /* END_HEADER */ /* BEGIN_CASE depends_on:MBEDTLS_PEM_WRITE_C */ @@ -35,16 +37,19 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_AES_C:MBEDTLS_DES_C:MBEDTLS_MD5_C:MBEDTLS_CIPHER_MODE_CBC */ -void mbedtls_pem_read_buffer( char *header, char *footer, char *data, int ret ) +void mbedtls_pem_read_buffer( char *header, char *footer, char *data, + char *pwd, int res ) { mbedtls_pem_context ctx; + int ret; size_t use_len = 0; + size_t pwd_len = strlen( pwd ); mbedtls_pem_init( &ctx ); - TEST_ASSERT( mbedtls_pem_read_buffer( &ctx, header, footer, - (const unsigned char *)data, NULL, 0, - &use_len ) == ret ); + ret = mbedtls_pem_read_buffer( &ctx, header, footer, (unsigned char *)data, + (unsigned char *)pwd, pwd_len, &use_len ); + TEST_ASSERT( ret == res ); exit: mbedtls_pem_free( &ctx );