From c9f4040f7f3356293e90c58d11f6567def641e08 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 8 Aug 2023 15:28:15 +0100 Subject: [PATCH 1/9] Switch pkparse to use new mbedtls_pkcs5_pbes2_ext function Switch pkparse to use new mbedtls_pkcs5_pbes2_ext function and deprecate mbedtls_pkcs5_pbes2 function. Signed-off-by: Waleed Elmelegy --- include/mbedtls/build_info.h | 8 ++++++++ include/mbedtls/pkcs5.h | 15 +++++++++++---- library/pkcs5.c | 2 ++ library/pkparse.c | 17 ++++++++++++----- tests/suites/test_suite_pkcs5.function | 5 +++++ 5 files changed, 38 insertions(+), 9 deletions(-) diff --git a/include/mbedtls/build_info.h b/include/mbedtls/build_info.h index c0424da82f..1786ddaeac 100644 --- a/include/mbedtls/build_info.h +++ b/include/mbedtls/build_info.h @@ -229,6 +229,14 @@ #define MBEDTLS_PK_HAVE_ECC_KEYS #endif /* MBEDTLS_PK_USE_PSA_EC_DATA || MBEDTLS_ECP_C */ +/* Historically pkparse did not check the CBC padding when decrypting + * a key. This was a bug, which is now fixed. As a consequence, pkparse + * now needs PKCS7 padding support, but existing configurations might not + * enable it, so we enable it here. */ +#if defined(MBEDTLS_PK_PARSE_C) && defined(MBEDTLS_PKCS5_C) && defined(MBEDTLS_CIPHER_MODE_CBC) +#define MBEDTLS_CIPHER_PADDING_PKCS7 +#endif + /* The following blocks make it easier to disable all of TLS, * or of TLS 1.2 or 1.3 or DTLS, without having to manually disable all * key exchanges, options and extensions related to them. */ diff --git a/include/mbedtls/pkcs5.h b/include/mbedtls/pkcs5.h index be54b25f0e..8b086aa2e2 100644 --- a/include/mbedtls/pkcs5.h +++ b/include/mbedtls/pkcs5.h @@ -25,6 +25,7 @@ #define MBEDTLS_PKCS5_H #include "mbedtls/build_info.h" +#include "mbedtls/platform_util.h" #include "mbedtls/asn1.h" #include "mbedtls/md.h" @@ -50,12 +51,17 @@ extern "C" { #if defined(MBEDTLS_ASN1_PARSE_C) +#if !defined(MBEDTLS_DEPRECATED_REMOVED) /** * \brief PKCS#5 PBES2 function * * \note When encrypting, #MBEDTLS_CIPHER_PADDING_PKCS7 must * be enabled at compile time. * + * \deprecated This function is deprecated and will be removed in a + * future version of the library. + * Please use mbedtls_pkcs5_pbes2_ext() instead. + * * \warning When decrypting: * - if #MBEDTLS_CIPHER_PADDING_PKCS7 is enabled at compile * time, this function validates the CBC padding and returns @@ -86,10 +92,11 @@ extern "C" { * * \returns 0 on success, or a MBEDTLS_ERR_XXX code if verification fails. */ -int mbedtls_pkcs5_pbes2(const mbedtls_asn1_buf *pbe_params, int mode, - const unsigned char *pwd, size_t pwdlen, - const unsigned char *data, size_t datalen, - unsigned char *output); +int MBEDTLS_DEPRECATED mbedtls_pkcs5_pbes2(const mbedtls_asn1_buf *pbe_params, int mode, + const unsigned char *pwd, size_t pwdlen, + const unsigned char *data, size_t datalen, + unsigned char *output); +#endif /* MBEDTLS_DEPRECATED_REMOVED */ #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) diff --git a/library/pkcs5.c b/library/pkcs5.c index 7a209ddd7b..2756d058e0 100644 --- a/library/pkcs5.c +++ b/library/pkcs5.c @@ -119,6 +119,7 @@ int mbedtls_pkcs5_pbes2_ext(const mbedtls_asn1_buf *pbe_params, int mode, size_t *output_len); #endif +#if !defined(MBEDTLS_DEPRECATED_REMOVED) int mbedtls_pkcs5_pbes2(const mbedtls_asn1_buf *pbe_params, int mode, const unsigned char *pwd, size_t pwdlen, const unsigned char *data, size_t datalen, @@ -133,6 +134,7 @@ int mbedtls_pkcs5_pbes2(const mbedtls_asn1_buf *pbe_params, int mode, return mbedtls_pkcs5_pbes2_ext(pbe_params, mode, pwd, pwdlen, data, datalen, output, SIZE_MAX, &output_len); } +#endif int mbedtls_pkcs5_pbes2_ext(const mbedtls_asn1_buf *pbe_params, int mode, const unsigned char *pwd, size_t pwdlen, diff --git a/library/pkparse.c b/library/pkparse.c index fe01a1149b..188cc286ec 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -1417,6 +1417,13 @@ static int pk_parse_key_pkcs8_unencrypted_der( #endif /* MBEDTLS_PK_HAVE_ECC_KEYS */ return MBEDTLS_ERR_PK_UNKNOWN_PK_ALG; +#if !defined(MBEDTLS_PKCS12_C) + end = p + len; + if (end != (key + keylen)) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PK_KEY_INVALID_FORMAT, + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); + } +#endif return 0; } @@ -1445,6 +1452,7 @@ static int pk_parse_key_pkcs8_encrypted_der( mbedtls_cipher_type_t cipher_alg; mbedtls_md_type_t md_alg; #endif + size_t outlen = 0; p = key; end = p + keylen; @@ -1499,14 +1507,14 @@ static int pk_parse_key_pkcs8_encrypted_der( return ret; } - + outlen = len; decrypted = 1; } else #endif /* MBEDTLS_PKCS12_C */ #if defined(MBEDTLS_PKCS5_C) if (MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS5_PBES2, &pbe_alg_oid) == 0) { - if ((ret = mbedtls_pkcs5_pbes2(&pbe_params, MBEDTLS_PKCS5_DECRYPT, pwd, pwdlen, - p, len, buf)) != 0) { + if ((ret = mbedtls_pkcs5_pbes2_ext(&pbe_params, MBEDTLS_PKCS5_DECRYPT, pwd, pwdlen, + p, len, buf, len, &outlen)) != 0) { if (ret == MBEDTLS_ERR_PKCS5_PASSWORD_MISMATCH) { return MBEDTLS_ERR_PK_PASSWORD_MISMATCH; } @@ -1524,8 +1532,7 @@ static int pk_parse_key_pkcs8_encrypted_der( if (decrypted == 0) { return MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE; } - - return pk_parse_key_pkcs8_unencrypted_der(pk, buf, len, f_rng, p_rng); + return pk_parse_key_pkcs8_unencrypted_der(pk, buf, outlen, f_rng, p_rng); } #endif /* MBEDTLS_PKCS12_C || MBEDTLS_PKCS5_C */ diff --git a/tests/suites/test_suite_pkcs5.function b/tests/suites/test_suite_pkcs5.function index fd4fde1170..2b0b0c1e00 100644 --- a/tests/suites/test_suite_pkcs5.function +++ b/tests/suites/test_suite_pkcs5.function @@ -46,6 +46,7 @@ void pbes2_encrypt(int params_tag, data_t *params_hex, data_t *pw, ASSERT_ALLOC(my_out, outsize); +#if defined(MBEDTLS_TEST_DEPRECATED) if (ref_ret != MBEDTLS_ERR_ASN1_BUF_TOO_SMALL) { my_ret = mbedtls_pkcs5_pbes2(¶ms, MBEDTLS_PKCS5_ENCRYPT, pw->x, pw->len, data->x, data->len, my_out); @@ -55,6 +56,7 @@ void pbes2_encrypt(int params_tag, data_t *params_hex, data_t *pw, ASSERT_COMPARE(my_out, ref_out->len, ref_out->x, ref_out->len); } +#endif #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) my_ret = mbedtls_pkcs5_pbes2_ext(¶ms, MBEDTLS_PKCS5_ENCRYPT, @@ -93,6 +95,7 @@ void pbes2_decrypt(int params_tag, data_t *params_hex, data_t *pw, ASSERT_ALLOC(my_out, outsize); +#if defined(MBEDTLS_TEST_DEPRECATED) if (ref_ret != MBEDTLS_ERR_ASN1_BUF_TOO_SMALL) { my_ret = mbedtls_pkcs5_pbes2(¶ms, MBEDTLS_PKCS5_DECRYPT, pw->x, pw->len, data->x, data->len, my_out); @@ -102,6 +105,8 @@ void pbes2_decrypt(int params_tag, data_t *params_hex, data_t *pw, ASSERT_COMPARE(my_out, ref_out->len, ref_out->x, ref_out->len); } +#endif + #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) my_ret = mbedtls_pkcs5_pbes2_ext(¶ms, MBEDTLS_PKCS5_DECRYPT, pw->x, pw->len, data->x, data->len, my_out, From d527896b7ef1164d94104a4dd4ad59828c1639bb Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 12 Sep 2023 14:42:49 +0100 Subject: [PATCH 2/9] Switch pkparse to use new mbedtls_pkcs12_pbe_ext function Switch pkparse to use new mbedtls_pkcs12_pbe_ext function and deprecate mbedtls_pkcs12_pbe function. Signed-off-by: Waleed Elmelegy --- include/mbedtls/pkcs12.h | 8 +++++++- library/pkcs12.c | 2 ++ library/pkparse.c | 9 ++++----- tests/suites/test_suite_pkcs12.function | 4 ++++ 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/pkcs12.h b/include/mbedtls/pkcs12.h index 363f812b9a..18a53cf457 100644 --- a/include/mbedtls/pkcs12.h +++ b/include/mbedtls/pkcs12.h @@ -52,6 +52,7 @@ extern "C" { #if defined(MBEDTLS_ASN1_PARSE_C) +#if !defined(MBEDTLS_DEPRECATED_REMOVED) /** * \brief PKCS12 Password Based function (encryption / decryption) * for cipher-based and mbedtls_md-based PBE's @@ -59,6 +60,10 @@ extern "C" { * \note When encrypting, #MBEDTLS_CIPHER_PADDING_PKCS7 must * be enabled at compile time. * + * \deprecated This function is deprecated and will be removed in a + * future version of the library. + * Please use mbedtls_pkcs12_pbe_ext() instead. + * * \warning When decrypting: * - if #MBEDTLS_CIPHER_PADDING_PKCS7 is enabled at compile * time, this function validates the CBC padding and returns @@ -93,11 +98,12 @@ extern "C" { * * \return 0 if successful, or a MBEDTLS_ERR_XXX code */ -int mbedtls_pkcs12_pbe(mbedtls_asn1_buf *pbe_params, int mode, +int MBEDTLS_DEPRECATED 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); +#endif /* MBEDTLS_DEPRECATED_REMOVED */ #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) diff --git a/library/pkcs12.c b/library/pkcs12.c index ad0f9e6b59..dd3a24037a 100644 --- a/library/pkcs12.c +++ b/library/pkcs12.c @@ -138,6 +138,7 @@ int mbedtls_pkcs12_pbe_ext(mbedtls_asn1_buf *pbe_params, int mode, size_t *output_len); #endif +#if !defined(MBEDTLS_DEPRECATED_REMOVED) 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, @@ -154,6 +155,7 @@ int mbedtls_pkcs12_pbe(mbedtls_asn1_buf *pbe_params, int mode, pwd, pwdlen, data, len, output, SIZE_MAX, &output_len); } +#endif int mbedtls_pkcs12_pbe_ext(mbedtls_asn1_buf *pbe_params, int mode, mbedtls_cipher_type_t cipher_type, mbedtls_md_type_t md_type, diff --git a/library/pkparse.c b/library/pkparse.c index 188cc286ec..9a8c7eedbb 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -1417,13 +1417,12 @@ static int pk_parse_key_pkcs8_unencrypted_der( #endif /* MBEDTLS_PK_HAVE_ECC_KEYS */ return MBEDTLS_ERR_PK_UNKNOWN_PK_ALG; -#if !defined(MBEDTLS_PKCS12_C) end = p + len; if (end != (key + keylen)) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PK_KEY_INVALID_FORMAT, MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); } -#endif + return 0; } @@ -1498,16 +1497,16 @@ static int pk_parse_key_pkcs8_encrypted_der( */ #if defined(MBEDTLS_PKCS12_C) if (mbedtls_oid_get_pkcs12_pbe_alg(&pbe_alg_oid, &md_alg, &cipher_alg) == 0) { - if ((ret = mbedtls_pkcs12_pbe(&pbe_params, MBEDTLS_PKCS12_PBE_DECRYPT, + if ((ret = mbedtls_pkcs12_pbe_ext(&pbe_params, MBEDTLS_PKCS12_PBE_DECRYPT, cipher_alg, md_alg, - pwd, pwdlen, p, len, buf)) != 0) { + pwd, pwdlen, p, len, buf, len, &outlen)) != 0) { if (ret == MBEDTLS_ERR_PKCS12_PASSWORD_MISMATCH) { return MBEDTLS_ERR_PK_PASSWORD_MISMATCH; } return ret; } - outlen = len; + decrypted = 1; } else #endif /* MBEDTLS_PKCS12_C */ diff --git a/tests/suites/test_suite_pkcs12.function b/tests/suites/test_suite_pkcs12.function index 3e8ff5b319..1d0c287fd8 100644 --- a/tests/suites/test_suite_pkcs12.function +++ b/tests/suites/test_suite_pkcs12.function @@ -90,6 +90,7 @@ void pkcs12_pbe_encrypt(int params_tag, int cipher, int md, data_t *params_hex, pbe_params.len = params_hex->len; pbe_params.p = params_hex->x; +#if defined(MBEDTLS_TEST_DEPRECATED) 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); @@ -99,6 +100,7 @@ void pkcs12_pbe_encrypt(int params_tag, int cipher, int md, data_t *params_hex, ASSERT_COMPARE(my_out, ref_out->len, ref_out->x, ref_out->len); } +#endif #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) @@ -143,6 +145,7 @@ void pkcs12_pbe_decrypt(int params_tag, int cipher, int md, data_t *params_hex, pbe_params.len = params_hex->len; pbe_params.p = params_hex->x; +#if defined(MBEDTLS_TEST_DEPRECATED) 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); @@ -153,6 +156,7 @@ void pkcs12_pbe_decrypt(int params_tag, int cipher, int md, data_t *params_hex, ASSERT_COMPARE(my_out, ref_out->len, ref_out->x, ref_out->len); } +#endif #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) From 5e48cad7f00f6fe6a2a9f390e82491b4bae5b4c1 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 12 Sep 2023 14:52:48 +0100 Subject: [PATCH 3/9] Fix codestyle issues in pkcs12.h & pkparse.c Signed-off-by: Waleed Elmelegy --- include/mbedtls/pkcs12.h | 9 +++++---- library/pkparse.c | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/pkcs12.h b/include/mbedtls/pkcs12.h index 18a53cf457..ba1a2edf03 100644 --- a/include/mbedtls/pkcs12.h +++ b/include/mbedtls/pkcs12.h @@ -99,10 +99,11 @@ extern "C" { * \return 0 if successful, or a MBEDTLS_ERR_XXX code */ int MBEDTLS_DEPRECATED 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); + 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); #endif /* MBEDTLS_DEPRECATED_REMOVED */ #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) diff --git a/library/pkparse.c b/library/pkparse.c index 9a8c7eedbb..67bb72032f 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -1498,8 +1498,8 @@ static int pk_parse_key_pkcs8_encrypted_der( #if defined(MBEDTLS_PKCS12_C) if (mbedtls_oid_get_pkcs12_pbe_alg(&pbe_alg_oid, &md_alg, &cipher_alg) == 0) { if ((ret = mbedtls_pkcs12_pbe_ext(&pbe_params, MBEDTLS_PKCS12_PBE_DECRYPT, - cipher_alg, md_alg, - pwd, pwdlen, p, len, buf, len, &outlen)) != 0) { + cipher_alg, md_alg, + pwd, pwdlen, p, len, buf, len, &outlen)) != 0) { if (ret == MBEDTLS_ERR_PKCS12_PASSWORD_MISMATCH) { return MBEDTLS_ERR_PK_PASSWORD_MISMATCH; } From 1db5cdaf574b37fa7279595bd409ae61764cf3a5 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 20 Sep 2023 18:00:48 +0100 Subject: [PATCH 4/9] Add tests to test pkcs8 parsing of encrypted keys Signed-off-by: Waleed Elmelegy --- library/pk_internal.h | 9 +++++++++ library/pkparse.c | 10 +++++----- tests/suites/test_suite_pkparse.data | 8 ++++++++ tests/suites/test_suite_pkparse.function | 19 +++++++++++++++++++ 4 files changed, 41 insertions(+), 5 deletions(-) diff --git a/library/pk_internal.h b/library/pk_internal.h index 416ef234f6..9892de61b8 100644 --- a/library/pk_internal.h +++ b/library/pk_internal.h @@ -117,5 +117,14 @@ static inline mbedtls_ecp_group_id mbedtls_pk_get_group_id(const mbedtls_pk_cont #endif /* MBEDTLS_ECP_DP_CURVE25519_ENABLED || MBEDTLS_ECP_DP_CURVE448_ENABLED */ #endif /* MBEDTLS_PK_HAVE_ECC_KEYS */ +#if defined(MBEDTLS_TEST_HOOKS) + +MBEDTLS_STATIC_TESTABLE int mbedtls_pk_parse_key_pkcs8_encrypted_der( + mbedtls_pk_context *pk, + unsigned char *key, size_t keylen, + const unsigned char *pwd, size_t pwdlen, + int (*f_rng)(void *, unsigned char *, size_t), void *p_rng); + +#endif #endif /* MBEDTLS_PK_INTERNAL_H */ diff --git a/library/pkparse.c b/library/pkparse.c index 67bb72032f..31e3eb9325 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -1436,7 +1436,7 @@ static int pk_parse_key_pkcs8_unencrypted_der( * */ #if defined(MBEDTLS_PKCS12_C) || defined(MBEDTLS_PKCS5_C) -static int pk_parse_key_pkcs8_encrypted_der( +MBEDTLS_STATIC_TESTABLE int mbedtls_pk_parse_key_pkcs8_encrypted_der( mbedtls_pk_context *pk, unsigned char *key, size_t keylen, const unsigned char *pwd, size_t pwdlen, @@ -1650,8 +1650,8 @@ int mbedtls_pk_parse_key(mbedtls_pk_context *pk, key, NULL, 0, &len); } if (ret == 0) { - if ((ret = pk_parse_key_pkcs8_encrypted_der(pk, pem.buf, pem.buflen, - pwd, pwdlen, f_rng, p_rng)) != 0) { + if ((ret = mbedtls_pk_parse_key_pkcs8_encrypted_der(pk, pem.buf, pem.buflen, + pwd, pwdlen, f_rng, p_rng)) != 0) { mbedtls_pk_free(pk); } @@ -1683,8 +1683,8 @@ int mbedtls_pk_parse_key(mbedtls_pk_context *pk, memcpy(key_copy, key, keylen); - ret = pk_parse_key_pkcs8_encrypted_der(pk, key_copy, keylen, - pwd, pwdlen, f_rng, p_rng); + ret = mbedtls_pk_parse_key_pkcs8_encrypted_der(pk, key_copy, keylen, + pwd, pwdlen, f_rng, p_rng); mbedtls_zeroize_and_free(key_copy, keylen); } diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data index 8e272bd107..40449e0968 100644 --- a/tests/suites/test_suite_pkparse.data +++ b/tests/suites/test_suite_pkparse.data @@ -1219,6 +1219,14 @@ Key ASN1 (OneAsymmetricKey X25519, unsupported version 2 with public key and uns depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED pk_parse_key:"3072020101300506032b656e04220420b06d829655543a51cba36e53522bc0acfd60af59466555fb3e1e796872ab1a59a01f301d060a2a864886f70d01090914310f0c0d437572646c65204368616972738121009bc3b0e93d8233fe6a8ba6138948cc12a91362d5c2ed81584db05ab5419c9d11":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +Key ASN1 (Encrypted key PKCS5, trailing garbage data) +depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED +pk_parse_key_encrypted:"307C304006092A864886F70D01050D3033301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC3949100438AD100BAC552FD0AE70BECAFA60F5E519B6180C77E8DB0B9ECC6F23FEDD30AB9BDCA2AF9F97BC470FC3A82DCA2364E22642DE0AF9275A82CB":"AAAAAAAAAAAAAAAAAA":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH + +Key ASN1 (Encrypted key PKCS12, trailing garbage data) +depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED +pk_parse_key_encrypted:"3058301C060A2A864886F70D010C0103300E0409CCCCCCCCCCCCCCCCCC02010A04380A8CAF39C4FA001884D0583B323C5E70942444FBE1F650B92F8ADF4AD7BD5049B4748F53A2531139EBF253FE01E8FC925C82C759C944B4D0":"AAAAAAAAAAAAAAAAAA":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH + # From RFC8410 Appendix A but made into version 0 OneAsymmetricKey X25519, doesn't match masking requirements #1 depends_on:MBEDTLS_ECP_DP_CURVE25519_ENABLED diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function index 0d9a0c8fc8..257e115554 100644 --- a/tests/suites/test_suite_pkparse.function +++ b/tests/suites/test_suite_pkparse.function @@ -150,6 +150,25 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +void pk_parse_key_encrypted(data_t *buf, data_t *pass, int result) +{ + mbedtls_pk_context pk; + + mbedtls_pk_init(&pk); + USE_PSA_INIT(); + + TEST_ASSERT(mbedtls_pk_parse_key_pkcs8_encrypted_der(&pk, buf->x, buf->len, + pass->x, pass->len, + mbedtls_test_rnd_std_rand, + NULL) == result); + +exit: + mbedtls_pk_free(&pk); + USE_PSA_DONE(); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_PK_WRITE_C */ void pk_parse_fix_montgomery(data_t *input_key, data_t *exp_output) { From 8d83b05ee004f243caefbe27c93ed3757646ced3 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 20 Sep 2023 18:42:05 +0100 Subject: [PATCH 5/9] Add changelog entry for switching pkparse to new pbe functions Signed-off-by: Waleed Elmelegy --- ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt diff --git a/ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt b/ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt new file mode 100644 index 0000000000..6ceab6eaf9 --- /dev/null +++ b/ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt @@ -0,0 +1,11 @@ +New deprecations + * mbedtls_pkcs5_pbes2() and mbedtls_pkcs12_pbe() functions are now + deprecated in favor of mbedtls_pkcs5_pbes2_ext() and + mbedtls_pkcs12_pbe_ext() as they offer more security by checking + for overflow of the output buffer and reporting the actual length + of the output. + +Bugfix + * Pass real length of key to pk_parse_key_pkcs8_unencrypted_der() + after decrypting the key to avoid trailing padding data which are not + part of the original key before encrypting. From 15bcf38e8821e57e02dc318a360d402b14161c36 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 20 Sep 2023 20:02:16 +0100 Subject: [PATCH 6/9] Add test pkparse test dependencies Signed-off-by: Waleed Elmelegy --- tests/suites/test_suite_pkparse.data | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data index 40449e0968..15efdb6a71 100644 --- a/tests/suites/test_suite_pkparse.data +++ b/tests/suites/test_suite_pkparse.data @@ -1220,11 +1220,11 @@ depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED pk_parse_key:"3072020101300506032b656e04220420b06d829655543a51cba36e53522bc0acfd60af59466555fb3e1e796872ab1a59a01f301d060a2a864886f70d01090914310f0c0d437572646c65204368616972738121009bc3b0e93d8233fe6a8ba6138948cc12a91362d5c2ed81584db05ab5419c9d11":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (Encrypted key PKCS5, trailing garbage data) -depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED +depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 pk_parse_key_encrypted:"307C304006092A864886F70D01050D3033301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC3949100438AD100BAC552FD0AE70BECAFA60F5E519B6180C77E8DB0B9ECC6F23FEDD30AB9BDCA2AF9F97BC470FC3A82DCA2364E22642DE0AF9275A82CB":"AAAAAAAAAAAAAAAAAA":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH Key ASN1 (Encrypted key PKCS12, trailing garbage data) -depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED +depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 pk_parse_key_encrypted:"3058301C060A2A864886F70D010C0103300E0409CCCCCCCCCCCCCCCCCC02010A04380A8CAF39C4FA001884D0583B323C5E70942444FBE1F650B92F8ADF4AD7BD5049B4748F53A2531139EBF253FE01E8FC925C82C759C944B4D0":"AAAAAAAAAAAAAAAAAA":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH # From RFC8410 Appendix A but made into version 0 From 9d4d8ebaf27475bc832927f3083cf9196255773b Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 21 Sep 2023 08:27:39 +0100 Subject: [PATCH 7/9] Add PKCS5/12 dependecies to pkparse tests Signed-off-by: Waleed Elmelegy --- tests/suites/test_suite_pkparse.data | 4 ++-- tests/suites/test_suite_pkparse.function | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data index 15efdb6a71..a4af3ed86c 100644 --- a/tests/suites/test_suite_pkparse.data +++ b/tests/suites/test_suite_pkparse.data @@ -1220,11 +1220,11 @@ depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED pk_parse_key:"3072020101300506032b656e04220420b06d829655543a51cba36e53522bc0acfd60af59466555fb3e1e796872ab1a59a01f301d060a2a864886f70d01090914310f0c0d437572646c65204368616972738121009bc3b0e93d8233fe6a8ba6138948cc12a91362d5c2ed81584db05ab5419c9d11":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (Encrypted key PKCS5, trailing garbage data) -depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 +depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7:MBEDTLS_PKCS5_C pk_parse_key_encrypted:"307C304006092A864886F70D01050D3033301B06092A864886F70D01050C300E04082ED7F24A1D516DD702020800301406082A864886F70D030704088A4FCC9DCC3949100438AD100BAC552FD0AE70BECAFA60F5E519B6180C77E8DB0B9ECC6F23FEDD30AB9BDCA2AF9F97BC470FC3A82DCA2364E22642DE0AF9275A82CB":"AAAAAAAAAAAAAAAAAA":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH Key ASN1 (Encrypted key PKCS12, trailing garbage data) -depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7 +depends_on:MBEDTLS_PK_HAVE_ECC_KEYS:MBEDTLS_ECP_DP_CURVE25519_ENABLED:MBEDTLS_MD_CAN_SHA1:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7:MBEDTLS_PKCS12_C pk_parse_key_encrypted:"3058301C060A2A864886F70D010C0103300E0409CCCCCCCCCCCCCCCCCC02010A04380A8CAF39C4FA001884D0583B323C5E70942444FBE1F650B92F8ADF4AD7BD5049B4748F53A2531139EBF253FE01E8FC925C82C759C944B4D0":"AAAAAAAAAAAAAAAAAA":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH # From RFC8410 Appendix A but made into version 0 diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function index 257e115554..91c4aebca3 100644 --- a/tests/suites/test_suite_pkparse.function +++ b/tests/suites/test_suite_pkparse.function @@ -157,13 +157,18 @@ void pk_parse_key_encrypted(data_t *buf, data_t *pass, int result) mbedtls_pk_init(&pk); USE_PSA_INIT(); - - TEST_ASSERT(mbedtls_pk_parse_key_pkcs8_encrypted_der(&pk, buf->x, buf->len, +#if defined(MBEDTLS_PKCS12_C) || defined(MBEDTLS_PKCS5_C) + TEST_EQUAL(mbedtls_pk_parse_key_pkcs8_encrypted_der(&pk, buf->x, buf->len, pass->x, pass->len, mbedtls_test_rnd_std_rand, - NULL) == result); - + NULL), result); exit: +#else + (void) buf; + (void) pass; + (void) result; +#endif + mbedtls_pk_free(&pk); USE_PSA_DONE(); } From 556a0790f6d1ecd15bcc26e747395d6a745f17bc Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 21 Sep 2023 09:19:56 +0100 Subject: [PATCH 8/9] Fix code style in pkparse tests Signed-off-by: Waleed Elmelegy --- tests/suites/test_suite_pkparse.function | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function index 91c4aebca3..02ef47594f 100644 --- a/tests/suites/test_suite_pkparse.function +++ b/tests/suites/test_suite_pkparse.function @@ -159,9 +159,9 @@ void pk_parse_key_encrypted(data_t *buf, data_t *pass, int result) USE_PSA_INIT(); #if defined(MBEDTLS_PKCS12_C) || defined(MBEDTLS_PKCS5_C) TEST_EQUAL(mbedtls_pk_parse_key_pkcs8_encrypted_der(&pk, buf->x, buf->len, - pass->x, pass->len, - mbedtls_test_rnd_std_rand, - NULL), result); + pass->x, pass->len, + mbedtls_test_rnd_std_rand, + NULL), result); exit: #else (void) buf; From 38202a2b180b7d6fbe13d7119b9703246c95b9b0 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 21 Sep 2023 15:21:10 +0100 Subject: [PATCH 9/9] Improve pkparse test dependencies and changelog Signed-off-by: Waleed Elmelegy --- .../Switch-pkparse-to-new-pbe-funsctions.txt | 4 +--- tests/suites/test_suite_pkparse.function | 15 +++++++-------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt b/ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt index 6ceab6eaf9..d819e8293f 100644 --- a/ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt +++ b/ChangeLog.d/Switch-pkparse-to-new-pbe-funsctions.txt @@ -6,6 +6,4 @@ New deprecations of the output. Bugfix - * Pass real length of key to pk_parse_key_pkcs8_unencrypted_der() - after decrypting the key to avoid trailing padding data which are not - part of the original key before encrypting. + * mbedtls_pk_parse_key() now rejects trailing garbage in encrypted keys. diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function index 02ef47594f..64a3175bc2 100644 --- a/tests/suites/test_suite_pkparse.function +++ b/tests/suites/test_suite_pkparse.function @@ -5,6 +5,11 @@ #include "mbedtls/ecp.h" #include "mbedtls/psa_util.h" #include "pk_internal.h" + +#if defined(MBEDTLS_PKCS12_C) || defined(MBEDTLS_PKCS5_C) +#define HAVE_mbedtls_pk_parse_key_pkcs8_encrypted_der +#endif + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -150,25 +155,19 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS:HAVE_mbedtls_pk_parse_key_pkcs8_encrypted_der */ void pk_parse_key_encrypted(data_t *buf, data_t *pass, int result) { mbedtls_pk_context pk; mbedtls_pk_init(&pk); USE_PSA_INIT(); -#if defined(MBEDTLS_PKCS12_C) || defined(MBEDTLS_PKCS5_C) + TEST_EQUAL(mbedtls_pk_parse_key_pkcs8_encrypted_der(&pk, buf->x, buf->len, pass->x, pass->len, mbedtls_test_rnd_std_rand, NULL), result); exit: -#else - (void) buf; - (void) pass; - (void) result; -#endif - mbedtls_pk_free(&pk); USE_PSA_DONE(); }