From e1cb35b719859bd5f14a507c3fbbff2d4284a278 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 6 Sep 2023 15:48:08 +0100 Subject: [PATCH 1/4] Add new mbedtls_pkcs12_pbe_ext function to replace old function Add new mbedtls_pkcs12_pbe_ext function to replace old mbedtls_pkcs12_pbe function that have security issues. Signed-off-by: Waleed Elmelegy --- include/mbedtls/pkcs12.h | 55 ++++++++++++++++++- library/pkcs12.c | 61 +++++++++++++++++---- tests/suites/test_suite_pkcs12.data | 22 +++++--- tests/suites/test_suite_pkcs12.function | 71 ++++++++++++++++++------- 4 files changed, 171 insertions(+), 38 deletions(-) diff --git a/include/mbedtls/pkcs12.h b/include/mbedtls/pkcs12.h index 8d003b5ea..363f812b9 100644 --- a/include/mbedtls/pkcs12.h +++ b/include/mbedtls/pkcs12.h @@ -79,7 +79,7 @@ extern "C" { * \param pwd Latin1-encoded password used. This may only be \c NULL when * \p pwdlen is 0. No null terminator should be used. * \param pwdlen length of the password (may be 0) - * \param input the input data + * \param data the input data * \param len data length * \param output Output buffer. * On success, it contains the encrypted or decrypted data, @@ -96,9 +96,60 @@ extern "C" { int mbedtls_pkcs12_pbe(mbedtls_asn1_buf *pbe_params, int mode, mbedtls_cipher_type_t cipher_type, mbedtls_md_type_t md_type, const unsigned char *pwd, size_t pwdlen, - const unsigned char *input, size_t len, + const unsigned char *data, size_t len, unsigned char *output); +#if defined(MBEDTLS_CIPHER_PADDING_PKCS7) + +/** + * \brief PKCS12 Password Based function (encryption / decryption) + * for cipher-based and mbedtls_md-based PBE's + * + * + * \warning When decrypting: + * - This function validates the CBC padding and returns + * #MBEDTLS_ERR_PKCS12_PASSWORD_MISMATCH if the padding is + * invalid. Note that this can help active adversaries + * attempting to brute-forcing the password. Note also that + * there is no guarantee that an invalid password will be + * detected (the chances of a valid padding with a random + * password are about 1/255). + * + * \param pbe_params an ASN1 buffer containing the pkcs-12 PbeParams structure + * \param mode either #MBEDTLS_PKCS12_PBE_ENCRYPT or + * #MBEDTLS_PKCS12_PBE_DECRYPT + * \param cipher_type the cipher used + * \param md_type the mbedtls_md used + * \param pwd Latin1-encoded password used. This may only be \c NULL when + * \p pwdlen is 0. No null terminator should be used. + * \param pwdlen length of the password (may be 0) + * \param data the input data + * \param len data length + * \param output Output buffer. + * On success, it contains the encrypted or decrypted data, + * possibly followed by the CBC padding. + * On failure, the content is indeterminate. + * For decryption, there must be enough room for \p len + * bytes. + * For encryption, there must be enough room for + * \p len + 1 bytes, rounded up to the block size of + * the block cipher identified by \p pbe_params. + * \param output_size size of output buffer. + * This must be big enough to accommodate for output plus + * padding data. + * \param output_len On success, length of actual data written to the output buffer. + * + * \return 0 if successful, or a MBEDTLS_ERR_XXX code + */ +int mbedtls_pkcs12_pbe_ext(mbedtls_asn1_buf *pbe_params, int mode, + mbedtls_cipher_type_t cipher_type, mbedtls_md_type_t md_type, + const unsigned char *pwd, size_t pwdlen, + const unsigned char *data, size_t len, + unsigned char *output, size_t output_size, + size_t *output_len); + +#endif /* MBEDTLS_CIPHER_PADDING_PKCS7 */ + #endif /* MBEDTLS_ASN1_PARSE_C */ /** diff --git a/library/pkcs12.c b/library/pkcs12.c index 6d523054d..819581584 100644 --- a/library/pkcs12.c +++ b/library/pkcs12.c @@ -47,7 +47,7 @@ static int pkcs12_parse_pbe_params(mbedtls_asn1_buf *params, mbedtls_asn1_buf *salt, int *iterations) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - unsigned char **p = ¶ms->p; + unsigned char *p = params->p; const unsigned char *end = params->p + params->len; /* @@ -62,18 +62,18 @@ static int pkcs12_parse_pbe_params(mbedtls_asn1_buf *params, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); } - if ((ret = mbedtls_asn1_get_tag(p, end, &salt->len, MBEDTLS_ASN1_OCTET_STRING)) != 0) { + if ((ret = mbedtls_asn1_get_tag(&p, end, &salt->len, MBEDTLS_ASN1_OCTET_STRING)) != 0) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS12_PBE_INVALID_FORMAT, ret); } - salt->p = *p; - *p += salt->len; + salt->p = p; + p += salt->len; - if ((ret = mbedtls_asn1_get_int(p, end, iterations)) != 0) { + if ((ret = mbedtls_asn1_get_int(&p, end, iterations)) != 0) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS12_PBE_INVALID_FORMAT, ret); } - if (*p != end) { + if (p != end) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS12_PBE_INVALID_FORMAT, MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); } @@ -129,18 +129,46 @@ static int pkcs12_pbe_derive_key_iv(mbedtls_asn1_buf *pbe_params, mbedtls_md_typ #undef PKCS12_MAX_PWDLEN +#if !defined(MBEDTLS_CIPHER_PADDING_PKCS7) +int mbedtls_pkcs12_pbe_ext(mbedtls_asn1_buf *pbe_params, int mode, + mbedtls_cipher_type_t cipher_type, mbedtls_md_type_t md_type, + const unsigned char *pwd, size_t pwdlen, + const unsigned char *data, size_t len, + unsigned char *output, size_t output_size, + size_t *output_len); +#endif + int mbedtls_pkcs12_pbe(mbedtls_asn1_buf *pbe_params, int mode, mbedtls_cipher_type_t cipher_type, mbedtls_md_type_t md_type, const unsigned char *pwd, size_t pwdlen, const unsigned char *data, size_t len, unsigned char *output) +{ + size_t output_len = 0; + + /* We assume caller of the function is providing a big enough output buffer + * so we pass output_size as SIZE_MAX to pass checks, However, no guarantees + * for the output size actually being correct. + */ + return mbedtls_pkcs12_pbe_ext(pbe_params, mode, cipher_type, md_type, + pwd, pwdlen, data, len, output, SIZE_MAX, + &output_len); +} + +int mbedtls_pkcs12_pbe_ext(mbedtls_asn1_buf *pbe_params, int mode, + mbedtls_cipher_type_t cipher_type, mbedtls_md_type_t md_type, + const unsigned char *pwd, size_t pwdlen, + const unsigned char *data, size_t len, + unsigned char *output, size_t output_size, + size_t *output_len) { int ret, keylen = 0; unsigned char key[32]; unsigned char iv[16]; const mbedtls_cipher_info_t *cipher_info; mbedtls_cipher_context_t cipher_ctx; - size_t olen = 0; + size_t finish_olen = 0; + unsigned int padlen = 0; if (pwd == NULL && pwdlen != 0) { return MBEDTLS_ERR_PKCS12_BAD_INPUT_DATA; @@ -153,6 +181,19 @@ int mbedtls_pkcs12_pbe(mbedtls_asn1_buf *pbe_params, int mode, keylen = (int) mbedtls_cipher_info_get_key_bitlen(cipher_info) / 8; + if (mode == MBEDTLS_PKCS12_PBE_DECRYPT) { + if (output_size < len) { + return MBEDTLS_ERR_ASN1_BUF_TOO_SMALL; + } + } + + if (mode == MBEDTLS_PKCS12_PBE_ENCRYPT) { + padlen = cipher_info->block_size - (len % cipher_info->block_size); + if (output_size < (len + padlen)) { + return MBEDTLS_ERR_ASN1_BUF_TOO_SMALL; + } + } + if ((ret = pkcs12_pbe_derive_key_iv(pbe_params, md_type, pwd, pwdlen, key, keylen, iv, mbedtls_cipher_info_get_iv_size(cipher_info))) != 0) { @@ -201,14 +242,16 @@ int mbedtls_pkcs12_pbe(mbedtls_asn1_buf *pbe_params, int mode, } if ((ret = mbedtls_cipher_update(&cipher_ctx, data, len, - output, &olen)) != 0) { + output, output_len)) != 0) { goto exit; } - if ((ret = mbedtls_cipher_finish(&cipher_ctx, output + olen, &olen)) != 0) { + if ((ret = mbedtls_cipher_finish(&cipher_ctx, output + (*output_len), &finish_olen)) != 0) { ret = MBEDTLS_ERR_PKCS12_PASSWORD_MISMATCH; } + *output_len += finish_olen; + exit: mbedtls_platform_zeroize(key, sizeof(key)); mbedtls_platform_zeroize(iv, sizeof(iv)); diff --git a/tests/suites/test_suite_pkcs12.data b/tests/suites/test_suite_pkcs12.data index 074c6c8cf..c4e4d773a 100644 --- a/tests/suites/test_suite_pkcs12.data +++ b/tests/suites/test_suite_pkcs12.data @@ -36,28 +36,36 @@ pkcs12_derive_key:MBEDTLS_MD_MD5:48:"0123456789abcdef":USE_GIVEN_INPUT:"01234567 PBE Encrypt, pad = 7 (OK) depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 -pkcs12_pbe_encrypt:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"300E0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"AAAAAAAAAAAAAAAAAA":0:"5F2C15056A36F3A78856E9E662DD27CB" +pkcs12_pbe_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"AAAAAAAAAAAAAAAAAA":16:0:"5F2C15056A36F3A78856E9E662DD27CB" PBE Encrypt, pad = 8 (OK) depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 -pkcs12_pbe_encrypt:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"300E0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"AAAAAAAAAAAAAAAA":0:"5F2C15056A36F3A70F70A3D4EC4004A8" +pkcs12_pbe_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"AAAAAAAAAAAAAAAA":16:0:"5F2C15056A36F3A70F70A3D4EC4004A8" + +PBE Encrypt, pad = 8 (Invalid output size) +depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 +pkcs12_pbe_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"AAAAAAAAAAAAAAAA":15:MBEDTLS_ERR_ASN1_BUF_TOO_SMALL:"5F2C15056A36F3A70F70A3D4EC4004A8" PBE Encrypt, pad = 8 (PKCS7 padding disabled) depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:!MBEDTLS_CIPHER_PADDING_PKCS7 -pkcs12_pbe_encrypt:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"300E0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"AAAAAAAAAAAAAAAA":MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE:"" +pkcs12_pbe_encrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"AAAAAAAAAAAAAAAA":0:MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE:"" PBE Decrypt, pad = 7 (OK) depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 -pkcs12_pbe_decrypt:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"300E0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A78856E9E662DD27CB":0:"AAAAAAAAAAAAAAAAAA" +pkcs12_pbe_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A78856E9E662DD27CB":16:0:"AAAAAAAAAAAAAAAAAA" + +PBE Decrypt, pad = 8 (Invalid output size) +depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 +pkcs12_pbe_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A70F70A3D4EC4004A8":15:MBEDTLS_ERR_ASN1_BUF_TOO_SMALL:"AAAAAAAAAAAAAAAA" PBE Decrypt, pad = 8 (OK) depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 -pkcs12_pbe_decrypt:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"300E0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A70F70A3D4EC4004A8":0:"AAAAAAAAAAAAAAAA" +pkcs12_pbe_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A70F70A3D4EC4004A8":16:0:"AAAAAAAAAAAAAAAA" PBE Decrypt, (Invalid padding & PKCS7 padding disabled) depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:!MBEDTLS_CIPHER_PADDING_PKCS7 -pkcs12_pbe_decrypt:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"300E0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A79F2B90F1428110E2":0:"AAAAAAAAAAAAAAAAAA07070707070708" +pkcs12_pbe_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A79F2B90F1428110E2":16:0:"AAAAAAAAAAAAAAAAAA07070707070708" PBE Decrypt, (Invalid padding & PKCS7 padding enabled) depends_on:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 -pkcs12_pbe_decrypt:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"300E0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A79F2B90F1428110E2":MBEDTLS_ERR_PKCS12_PASSWORD_MISMATCH:"AAAAAAAAAAAAAAAAAA07070707070708" +pkcs12_pbe_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:MBEDTLS_CIPHER_DES_EDE3_CBC:MBEDTLS_MD_SHA1:"0409CCCCCCCCCCCCCCCCCC02010A":"BBBBBBBBBBBBBBBBBB":"5F2C15056A36F3A79F2B90F1428110E2":16:MBEDTLS_ERR_PKCS12_PASSWORD_MISMATCH:"AAAAAAAAAAAAAAAAAA07070707070708" diff --git a/tests/suites/test_suite_pkcs12.function b/tests/suites/test_suite_pkcs12.function index b55709815..92b4d5dc2 100644 --- a/tests/suites/test_suite_pkcs12.function +++ b/tests/suites/test_suite_pkcs12.function @@ -70,33 +70,47 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_ASN1_PARSE_C */ -void pkcs12_pbe_encrypt(int cipher, int md, data_t *params_hex, data_t *pw, - data_t *data, int ref_ret, data_t *ref_out) +void pkcs12_pbe_encrypt(int params_tag, int cipher, int md, data_t *params_hex, data_t *pw, + data_t *data, int outsize, int ref_ret, data_t *ref_out) { int my_ret; mbedtls_asn1_buf pbe_params; unsigned char *my_out = NULL; mbedtls_cipher_type_t cipher_alg = (mbedtls_cipher_type_t) cipher; mbedtls_md_type_t md_alg = (mbedtls_md_type_t) md; - size_t block_size; +#if defined(MBEDTLS_CIPHER_PADDING_PKCS7) + size_t my_out_len = 0; +#endif MD_PSA_INIT(); - block_size = mbedtls_cipher_info_get_block_size(mbedtls_cipher_info_from_type(cipher_alg)); - TEST_CALLOC(my_out, ((data->len/block_size) + 1) * block_size); + TEST_CALLOC(my_out, outsize); - pbe_params.tag = params_hex->x[0]; - pbe_params.len = params_hex->x[1]; - pbe_params.p = params_hex->x + 2; + pbe_params.tag = params_tag; + pbe_params.len = params_hex->len; + pbe_params.p = params_hex->x; - my_ret = mbedtls_pkcs12_pbe(&pbe_params, MBEDTLS_PKCS12_PBE_ENCRYPT, cipher_alg, - md_alg, pw->x, pw->len, data->x, data->len, my_out); - TEST_EQUAL(my_ret, ref_ret); + if (ref_ret != MBEDTLS_ERR_ASN1_BUF_TOO_SMALL) { + my_ret = mbedtls_pkcs12_pbe(&pbe_params, MBEDTLS_PKCS12_PBE_ENCRYPT, cipher_alg, + md_alg, pw->x, pw->len, data->x, data->len, my_out); + TEST_EQUAL(my_ret, ref_ret); + } if (ref_ret == 0) { ASSERT_COMPARE(my_out, ref_out->len, ref_out->x, ref_out->len); } +#if defined(MBEDTLS_CIPHER_PADDING_PKCS7) + my_ret = mbedtls_pkcs12_pbe_ext(&pbe_params, MBEDTLS_PKCS12_PBE_ENCRYPT, cipher_alg, + md_alg, pw->x, pw->len, data->x, data->len, my_out, + outsize, &my_out_len); + TEST_EQUAL(my_ret, ref_ret); + if (ref_ret == 0) { + ASSERT_COMPARE(my_out, my_out_len, + ref_out->x, ref_out->len); + } +#endif + exit: mbedtls_free(my_out); MD_PSA_DONE(); @@ -104,31 +118,48 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_ASN1_PARSE_C */ -void pkcs12_pbe_decrypt(int cipher, int md, data_t *params_hex, data_t *pw, - data_t *data, int ref_ret, data_t *ref_out) +void pkcs12_pbe_decrypt(int params_tag, int cipher, int md, data_t *params_hex, data_t *pw, + data_t *data, int outsize, int ref_ret, data_t *ref_out) { int my_ret; mbedtls_asn1_buf pbe_params; unsigned char *my_out = NULL; mbedtls_cipher_type_t cipher_alg = (mbedtls_cipher_type_t) cipher; mbedtls_md_type_t md_alg = (mbedtls_md_type_t) md; +#if defined(MBEDTLS_CIPHER_PADDING_PKCS7) + size_t my_out_len = 0; +#endif MD_PSA_INIT(); - TEST_CALLOC(my_out, data->len); + TEST_CALLOC(my_out, outsize); - pbe_params.tag = params_hex->x[0]; - pbe_params.len = params_hex->x[1]; - pbe_params.p = params_hex->x + 2; + pbe_params.tag = params_tag; + pbe_params.len = params_hex->len; + pbe_params.p = params_hex->x; + + if (ref_ret != MBEDTLS_ERR_ASN1_BUF_TOO_SMALL) { + my_ret = mbedtls_pkcs12_pbe(&pbe_params, MBEDTLS_PKCS12_PBE_DECRYPT, cipher_alg, + md_alg, pw->x, pw->len, data->x, data->len, my_out); + TEST_EQUAL(my_ret, ref_ret); + } - my_ret = mbedtls_pkcs12_pbe(&pbe_params, MBEDTLS_PKCS12_PBE_DECRYPT, cipher_alg, - md_alg, pw->x, pw->len, data->x, data->len, my_out); - TEST_EQUAL(my_ret, ref_ret); if (ref_ret == 0) { ASSERT_COMPARE(my_out, ref_out->len, ref_out->x, ref_out->len); } +#if defined(MBEDTLS_CIPHER_PADDING_PKCS7) + my_ret = mbedtls_pkcs12_pbe_ext(&pbe_params, MBEDTLS_PKCS12_PBE_DECRYPT, cipher_alg, + md_alg, pw->x, pw->len, data->x, data->len, my_out, + outsize, &my_out_len); + TEST_EQUAL(my_ret, ref_ret); + if (ref_ret == 0) { + ASSERT_COMPARE(my_out, my_out_len, + ref_out->x, ref_out->len); + } +#endif + exit: mbedtls_free(my_out); MD_PSA_DONE(); From 57d09b72ef59d352a54a40e4b32dc359a873a96a Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 12 Sep 2023 14:05:10 +0100 Subject: [PATCH 2/4] Return back to modifying input parameters in pkcs12_parse_pbe_params Return back to modifying input parameters in pkcs12_parse_pbe_params to avoid change in behaviour. Signed-off-by: Waleed Elmelegy --- library/pkcs12.c | 12 ++++++------ tests/suites/test_suite_pkcs12.function | 10 ++++++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/library/pkcs12.c b/library/pkcs12.c index 819581584..ad0f9e6b5 100644 --- a/library/pkcs12.c +++ b/library/pkcs12.c @@ -47,7 +47,7 @@ static int pkcs12_parse_pbe_params(mbedtls_asn1_buf *params, mbedtls_asn1_buf *salt, int *iterations) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - unsigned char *p = params->p; + unsigned char **p = ¶ms->p; const unsigned char *end = params->p + params->len; /* @@ -62,18 +62,18 @@ static int pkcs12_parse_pbe_params(mbedtls_asn1_buf *params, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); } - if ((ret = mbedtls_asn1_get_tag(&p, end, &salt->len, MBEDTLS_ASN1_OCTET_STRING)) != 0) { + if ((ret = mbedtls_asn1_get_tag(p, end, &salt->len, MBEDTLS_ASN1_OCTET_STRING)) != 0) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS12_PBE_INVALID_FORMAT, ret); } - salt->p = p; - p += salt->len; + salt->p = *p; + *p += salt->len; - if ((ret = mbedtls_asn1_get_int(&p, end, iterations)) != 0) { + if ((ret = mbedtls_asn1_get_int(p, end, iterations)) != 0) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS12_PBE_INVALID_FORMAT, ret); } - if (p != end) { + if (*p != end) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS12_PBE_INVALID_FORMAT, MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); } diff --git a/tests/suites/test_suite_pkcs12.function b/tests/suites/test_suite_pkcs12.function index 92b4d5dc2..3e8ff5b31 100644 --- a/tests/suites/test_suite_pkcs12.function +++ b/tests/suites/test_suite_pkcs12.function @@ -101,6 +101,11 @@ void pkcs12_pbe_encrypt(int params_tag, int cipher, int md, data_t *params_hex, } #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) + + pbe_params.tag = params_tag; + pbe_params.len = params_hex->len; + pbe_params.p = params_hex->x; + my_ret = mbedtls_pkcs12_pbe_ext(&pbe_params, MBEDTLS_PKCS12_PBE_ENCRYPT, cipher_alg, md_alg, pw->x, pw->len, data->x, data->len, my_out, outsize, &my_out_len); @@ -150,6 +155,11 @@ void pkcs12_pbe_decrypt(int params_tag, int cipher, int md, data_t *params_hex, } #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) + + pbe_params.tag = params_tag; + pbe_params.len = params_hex->len; + pbe_params.p = params_hex->x; + my_ret = mbedtls_pkcs12_pbe_ext(&pbe_params, MBEDTLS_PKCS12_PBE_DECRYPT, cipher_alg, md_alg, pw->x, pw->len, data->x, data->len, my_out, outsize, &my_out_len); From 0684965f5aad346ee50ec28bd48f7e2bdd7a1896 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 13 Sep 2023 13:35:16 +0100 Subject: [PATCH 3/4] Modify changelog entry to add pkcs12 pbe functions Signed-off-by: Waleed Elmelegy --- ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt b/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt index a1fded3aa..e8509c662 100644 --- a/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt +++ b/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt @@ -1,6 +1,7 @@ Security - * Developers using mbedtls_pkcs5_pbes2() should review the size of the output - buffer passed to this function, and note that the output after decryption - may include CBC padding. Consider moving to the new function - mbedtls_pkcs5_pbes2_ext() which checks for overflow of the output buffer - and reports the actual length of the output. + * Developers using mbedtls_pkcs5_pbes2() or mbedtls_pkcs12_pbe() should review + the size of the output buffer passed to this function, and note that the + output after decryption may include CBC padding. Consider moving to the + new functions mbedtls_pkcs5_pbes2_ext() or mbedtls_pkcs12_pbe_ext() which + checks for overflow of the output buffer and reports the actual length + of the output. From 50888643f41b0544a2d42887d1eb842fbf205127 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 14 Sep 2023 18:27:17 +0100 Subject: [PATCH 4/4] Reduce line size in new pkcs function changelog Signed-off-by: Waleed Elmelegy --- ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt b/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt index e8509c662..f2e7a4a2c 100644 --- a/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt +++ b/ChangeLog.d/add-new-pkcs5-pbe2-ext-fun.txt @@ -1,7 +1,7 @@ Security - * Developers using mbedtls_pkcs5_pbes2() or mbedtls_pkcs12_pbe() should review - the size of the output buffer passed to this function, and note that the - output after decryption may include CBC padding. Consider moving to the - new functions mbedtls_pkcs5_pbes2_ext() or mbedtls_pkcs12_pbe_ext() which - checks for overflow of the output buffer and reports the actual length - of the output. + * Developers using mbedtls_pkcs5_pbes2() or mbedtls_pkcs12_pbe() should + review the size of the output buffer passed to this function, and note + that the output after decryption may include CBC padding. Consider moving + to the new functions mbedtls_pkcs5_pbes2_ext() or mbedtls_pkcs12_pbe_ext() + which checks for overflow of the output buffer and reports the actual + length of the output.