From adb3cc4d433466f00ba827d5288577e367fbf1f3 Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Tue, 17 Oct 2023 11:50:50 +0200 Subject: [PATCH 01/10] Fixes #8377. Signed-off-by: Matthias Schulz --- library/x509_csr.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/x509_csr.c b/library/x509_csr.c index 0b2bb6f3bf..bd45c56657 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -78,6 +78,7 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, int ret; size_t len; unsigned char *end_ext_data; + int critical; while (*p < end) { mbedtls_x509_buf extn_oid = { 0, 0, NULL }; int ext_type = 0; @@ -100,6 +101,9 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, extn_oid.p = *p; *p += extn_oid.len; + /* Get and ignore optional critical flag */ + (void)mbedtls_asn1_get_bool(p, end_ext_data, &critical); + /* Data should be octet string type */ if ((ret = mbedtls_asn1_get_tag(p, end_ext_data, &len, MBEDTLS_ASN1_OCTET_STRING)) != 0) { From cc923f307ed3b99299034d17ed042151e4cc0712 Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Tue, 17 Oct 2023 12:36:23 +0200 Subject: [PATCH 02/10] Added missing like between variables and function body. Signed-off-by: Matthias Schulz --- library/x509_csr.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/x509_csr.c b/library/x509_csr.c index bd45c56657..ce4c081e39 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -79,6 +79,7 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, size_t len; unsigned char *end_ext_data; int critical; + while (*p < end) { mbedtls_x509_buf extn_oid = { 0, 0, NULL }; int ext_type = 0; From 0ca58e3c1000d067fb28bfb41dc90a1db9308b0d Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Tue, 17 Oct 2023 13:11:52 +0200 Subject: [PATCH 03/10] Added testcase with certificate that contains extensions with critical fields. Signed-off-by: Matthias Schulz --- tests/suites/test_suite_x509parse.data | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 4b75f17639..a38a3ff32e 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2940,6 +2940,10 @@ X509 CSR ASN.1 (OK) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA1:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_parse:"308201183081bf0201003034310b3009060355040613024e4c3111300f060355040a1308506f6c617253534c31123010060355040313096c6f63616c686f73743059301306072a8648ce3d020106082a8648ce3d0301070342000437cc56d976091e5a723ec7592dff206eee7cf9069174d0ad14b5f768225962924ee500d82311ffea2fd2345d5d16bd8a88c26b770d55cd8a2a0efa01c8b4edffa029302706092a864886f70d01090e311a301830090603551d1304023000300b0603551d0f0404030205e0300906072a8648ce3d04010349003046022100b49fd8c8f77abfa871908dfbe684a08a793d0f490a43d86fcf2086e4f24bb0c2022100f829d5ccd3742369299e6294394717c4b723a0f68b44e831b6e6c3bcabf97243":"CSR version \: 1\nsubject name \: C=NL, O=PolarSSL, CN=localhost\nsigned using \: ECDSA with SHA1\nEC key size \: 256 bits\n\nkey usage \: Digital Signature, Non Repudiation, Key Encipherment\n":0 +X509 CSR ASN.1 (critical extensions) +depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO +mbedtls_x509_csr_parse:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"CSR version \: 1\nsubject name \: CN=Self signed test, C=DE, O=AuthCrtDB Test\nsigned using \: ECDSA with SHA256\nEC key size \: 256 bits\n":0 + X509 CSR ASN.1 (bad first tag) mbedtls_x509_csr_parse:"3100":"":MBEDTLS_ERR_X509_INVALID_FORMAT From 873a202d18915abe1ca15976389768eb3c1e18ba Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Tue, 17 Oct 2023 16:02:20 +0200 Subject: [PATCH 04/10] Now handling critical extensions similarly to how its done in x509_get_crt_ext just without the callback function to handle unknown extensions. Signed-off-by: Matthias Schulz --- library/x509_csr.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/library/x509_csr.c b/library/x509_csr.c index ce4c081e39..baf260664a 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -75,13 +75,13 @@ static int x509_csr_get_version(unsigned char **p, static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, unsigned char **p, const unsigned char *end) { - int ret; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t len; unsigned char *end_ext_data; - int critical; while (*p < end) { mbedtls_x509_buf extn_oid = { 0, 0, NULL }; + int is_critical = 0; /* DEFAULT FALSE */ int ext_type = 0; /* Read sequence tag */ @@ -102,8 +102,11 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, extn_oid.p = *p; *p += extn_oid.len; - /* Get and ignore optional critical flag */ - (void)mbedtls_asn1_get_bool(p, end_ext_data, &critical); + /* Get optional critical */ + if ((ret = mbedtls_asn1_get_bool(p, end_ext_data, &is_critical)) != 0 && + (ret != MBEDTLS_ERR_ASN1_UNEXPECTED_TAG)) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); + } /* Data should be octet string type */ if ((ret = mbedtls_asn1_get_tag(p, end_ext_data, &len, @@ -157,6 +160,12 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, default: break; } + } else { + if (is_critical) { + /* Data is marked as critical: fail */ + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); + } } *p = end_ext_data; } From ab4082290ee9e1a175255940ebba6af90827ba4f Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Wed, 18 Oct 2023 13:20:59 +0200 Subject: [PATCH 05/10] Added parameters to add callback function to handle unsupported extensions. Similar to how the callback functions work when parsing certificates. Also added new test cases. Signed-off-by: Matthias Schulz --- include/mbedtls/x509_csr.h | 58 +++++++++ library/x509_csr.c | 136 ++++++++++++++------- tests/suites/test_suite_x509parse.data | 12 +- tests/suites/test_suite_x509parse.function | 57 +++++++++ 4 files changed, 218 insertions(+), 45 deletions(-) diff --git a/include/mbedtls/x509_csr.h b/include/mbedtls/x509_csr.h index 513a83edd0..a4bdc9413b 100644 --- a/include/mbedtls/x509_csr.h +++ b/include/mbedtls/x509_csr.h @@ -102,6 +102,64 @@ mbedtls_x509write_csr; int mbedtls_x509_csr_parse_der(mbedtls_x509_csr *csr, const unsigned char *buf, size_t buflen); +/** + * \brief The type of certificate extension callbacks. + * + * Callbacks of this type are passed to and used by the + * mbedtls_x509_csr_parse_der_with_ext_cb() routine when + * it encounters either an unsupported extension. + * Future versions of the library may invoke the callback + * in other cases, if and when the need arises. + * + * \param p_ctx An opaque context passed to the callback. + * \param csr The CSR being parsed. + * \param oid The OID of the extension. + * \param critical Whether the extension is critical. + * \param p Pointer to the start of the extension value + * (the content of the OCTET STRING). + * \param end End of extension value. + * + * \note The callback must fail and return a negative error code + * if it can not parse or does not support the extension. + * When the callback fails to parse a critical extension + * mbedtls_x509_csr_parse_der_with_ext_cb() also fails. + * When the callback fails to parse a non critical extension + * mbedtls_x509_csr_parse_der_with_ext_cb() simply skips + * the extension and continues parsing. + * + * \return \c 0 on success. + * \return A negative error code on failure. + */ +typedef int (*mbedtls_x509_csr_ext_cb_t)(void *p_ctx, + mbedtls_x509_csr const *csr, + mbedtls_x509_buf const *oid, + int critical, + const unsigned char *p, + const unsigned char *end); + +/** + * \brief Load a Certificate Signing Request (CSR) in DER format + * + * \note CSR attributes (if any) are currently silently ignored. + * + * \note If #MBEDTLS_USE_PSA_CRYPTO is enabled, the PSA crypto + * subsystem must have been initialized by calling + * psa_crypto_init() before calling this function. + * + * \param csr CSR context to fill + * \param buf buffer holding the CRL data + * \param buflen size of the buffer + * \param cb A callback invoked for every unsupported certificate + * extension. + * \param p_ctx An opaque context passed to the callback. + * + * \return 0 if successful, or a specific X509 error code + */ +int mbedtls_x509_csr_parse_der_with_ext_cb(mbedtls_x509_csr *csr, + const unsigned char *buf, size_t buflen, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx); + /** * \brief Load a Certificate Signing Request (CSR), DER or PEM format * diff --git a/library/x509_csr.c b/library/x509_csr.c index baf260664a..9e4a01ab6c 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -73,11 +73,13 @@ static int x509_csr_get_version(unsigned char **p, * Parse CSR extension requests in DER format */ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, - unsigned char **p, const unsigned char *end) + unsigned char **p, const unsigned char *end, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t len; - unsigned char *end_ext_data; + unsigned char *end_ext_data, *end_ext_octet; while (*p < end) { mbedtls_x509_buf extn_oid = { 0, 0, NULL }; @@ -114,7 +116,9 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); } - if (*p + len != end_ext_data) { + end_ext_octet = *p + len; + + if (end_ext_octet != end_ext_data) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); } @@ -124,50 +128,72 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, */ ret = mbedtls_oid_get_x509_ext_type(&extn_oid, &ext_type); - if (ret == 0) { - /* Forbid repeated extensions */ - if ((csr->ext_types & ext_type) != 0) { - return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, - MBEDTLS_ERR_ASN1_INVALID_DATA); + if (ret != 0) { + /* Give the callback (if any) a chance to handle the extension */ + if (cb != NULL) { + ret = cb(p_ctx, csr, &extn_oid, is_critical, *p, end_ext_octet); + if (ret != 0 && is_critical) { + return ret; + } + *p = end_ext_octet; + continue; } - csr->ext_types |= ext_type; + /* No parser found, skip extension */ + *p = end_ext_octet; - switch (ext_type) { - case MBEDTLS_X509_EXT_KEY_USAGE: - /* Parse key usage */ - if ((ret = mbedtls_x509_get_key_usage(p, end_ext_data, - &csr->key_usage)) != 0) { - return ret; - } - break; - - case MBEDTLS_X509_EXT_SUBJECT_ALT_NAME: - /* Parse subject alt name */ - if ((ret = mbedtls_x509_get_subject_alt_name(p, end_ext_data, - &csr->subject_alt_names)) != 0) { - return ret; - } - break; - - case MBEDTLS_X509_EXT_NS_CERT_TYPE: - /* Parse netscape certificate type */ - if ((ret = mbedtls_x509_get_ns_cert_type(p, end_ext_data, - &csr->ns_cert_type)) != 0) { - return ret; - } - break; - default: - break; - } - } else { if (is_critical) { /* Data is marked as critical: fail */ return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); } + continue; + } + + /* Forbid repeated extensions */ + if ((csr->ext_types & ext_type) != 0) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, + MBEDTLS_ERR_ASN1_INVALID_DATA); + } + + csr->ext_types |= ext_type; + + switch (ext_type) { + case MBEDTLS_X509_EXT_KEY_USAGE: + /* Parse key usage */ + if ((ret = mbedtls_x509_get_key_usage(p, end_ext_data, + &csr->key_usage)) != 0) { + return ret; + } + break; + + case MBEDTLS_X509_EXT_SUBJECT_ALT_NAME: + /* Parse subject alt name */ + if ((ret = mbedtls_x509_get_subject_alt_name(p, end_ext_data, + &csr->subject_alt_names)) != 0) { + return ret; + } + break; + + case MBEDTLS_X509_EXT_NS_CERT_TYPE: + /* Parse netscape certificate type */ + if ((ret = mbedtls_x509_get_ns_cert_type(p, end_ext_data, + &csr->ns_cert_type)) != 0) { + return ret; + } + break; + default: + /* + * If this is a non-critical extension, which the oid layer + * supports, but there isn't an x509 parser for it, + * skip the extension. + */ + if (is_critical) { + return MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE; + } else { + *p = end_ext_octet; + } } - *p = end_ext_data; } if (*p != end) { @@ -182,7 +208,9 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, * Parse CSR attributes in DER format */ static int x509_csr_parse_attributes(mbedtls_x509_csr *csr, - const unsigned char *start, const unsigned char *end) + const unsigned char *start, const unsigned char *end, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx) { int ret; size_t len; @@ -221,7 +249,7 @@ static int x509_csr_parse_attributes(mbedtls_x509_csr *csr, return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); } - if ((ret = x509_csr_parse_extensions(csr, p, *p + len)) != 0) { + if ((ret = x509_csr_parse_extensions(csr, p, *p + len, cb, p_ctx)) != 0) { return ret; } @@ -245,8 +273,10 @@ static int x509_csr_parse_attributes(mbedtls_x509_csr *csr, /* * Parse a CSR in DER format */ -int mbedtls_x509_csr_parse_der(mbedtls_x509_csr *csr, - const unsigned char *buf, size_t buflen) +static int mbedtls_x509_csr_parse_der_internal(mbedtls_x509_csr *csr, + const unsigned char *buf, size_t buflen, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t len; @@ -370,7 +400,7 @@ int mbedtls_x509_csr_parse_der(mbedtls_x509_csr *csr, return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_FORMAT, ret); } - if ((ret = x509_csr_parse_attributes(csr, p, p + len)) != 0) { + if ((ret = x509_csr_parse_attributes(csr, p, p + len, cb, p_ctx)) != 0) { mbedtls_x509_csr_free(csr); return ret; } @@ -409,6 +439,26 @@ int mbedtls_x509_csr_parse_der(mbedtls_x509_csr *csr, return 0; } +/* + * Parse a CSR in DER format + */ +int mbedtls_x509_csr_parse_der(mbedtls_x509_csr *csr, + const unsigned char *buf, size_t buflen) +{ + return mbedtls_x509_csr_parse_der_internal(csr, buf, buflen, NULL, NULL); +} + +/* + * Parse a CSR in DER format with callback for unknown extensions + */ +int mbedtls_x509_csr_parse_der_with_ext_cb(mbedtls_x509_csr *csr, + const unsigned char *buf, size_t buflen, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx) +{ + return mbedtls_x509_csr_parse_der_internal(csr, buf, buflen, cb, p_ctx); +} + /* * Parse a CSR, allowing for PEM or raw DER encoding */ diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index a38a3ff32e..9817536d7d 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2940,9 +2940,17 @@ X509 CSR ASN.1 (OK) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA1:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_parse:"308201183081bf0201003034310b3009060355040613024e4c3111300f060355040a1308506f6c617253534c31123010060355040313096c6f63616c686f73743059301306072a8648ce3d020106082a8648ce3d0301070342000437cc56d976091e5a723ec7592dff206eee7cf9069174d0ad14b5f768225962924ee500d82311ffea2fd2345d5d16bd8a88c26b770d55cd8a2a0efa01c8b4edffa029302706092a864886f70d01090e311a301830090603551d1304023000300b0603551d0f0404030205e0300906072a8648ce3d04010349003046022100b49fd8c8f77abfa871908dfbe684a08a793d0f490a43d86fcf2086e4f24bb0c2022100f829d5ccd3742369299e6294394717c4b723a0f68b44e831b6e6c3bcabf97243":"CSR version \: 1\nsubject name \: C=NL, O=PolarSSL, CN=localhost\nsigned using \: ECDSA with SHA1\nEC key size \: 256 bits\n\nkey usage \: Digital Signature, Non Repudiation, Key Encipherment\n":0 -X509 CSR ASN.1 (critical extensions) +X509 CSR ASN.1 (Unsupported critical extension) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO -mbedtls_x509_csr_parse:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"CSR version \: 1\nsubject name \: CN=Self signed test, C=DE, O=AuthCrtDB Test\nsigned using \: ECDSA with SHA256\nEC key size \: 256 bits\n":0 +mbedtls_x509_csr_parse:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS+MBEDTLS_ERR_ASN1_UNEXPECTED_TAG + +X509 CSR ASN.1 (Unsupported critical extension accepted by callback) +depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO +mbedtls_x509_csr_parse_with_ext_cb:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"CSR version \: 1\nsubject name \: CN=Self signed test, C=DE, O=AuthCrtDB Test\nsigned using \: ECDSA with SHA256\nEC key size \: 256 bits\n":0:1 + +X509 CSR ASN.1 (Unsupported critical extension rejected by callback) +depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO +mbedtls_x509_csr_parse_with_ext_cb:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS+MBEDTLS_ERR_ASN1_UNEXPECTED_TAG:0 X509 CSR ASN.1 (bad first tag) mbedtls_x509_csr_parse:"3100":"":MBEDTLS_ERR_X509_INVALID_FORMAT diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 114bd52776..8cdca82e08 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -412,6 +412,33 @@ int parse_crt_ext_cb(void *p_ctx, mbedtls_x509_crt const *crt, mbedtls_x509_buf MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); } } + +int parse_csr_ext_accept_cb(void *p_ctx, mbedtls_x509_csr const *csr, mbedtls_x509_buf const *oid, + int critical, const unsigned char *cp, const unsigned char *end) +{ + (void) p_ctx; + (void) csr; + (void) oid; + (void) critical; + (void) cp; + (void) end; + + return 0; +} + +int parse_csr_ext_reject_cb(void *p_ctx, mbedtls_x509_csr const *csr, mbedtls_x509_buf const *oid, + int critical, const unsigned char *cp, const unsigned char *end) +{ + (void) p_ctx; + (void) csr; + (void) oid; + (void) critical; + (void) cp; + (void) end; + + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); +} #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* END_HEADER */ @@ -1245,6 +1272,36 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_X509_CSR_PARSE_C:!MBEDTLS_X509_REMOVE_INFO */ +void mbedtls_x509_csr_parse_with_ext_cb(data_t *csr_der, char *ref_out, int ref_ret, int accept) +{ + mbedtls_x509_csr csr; + char my_out[1000]; + int my_ret; + + mbedtls_x509_csr_init(&csr); + USE_PSA_INIT(); + + memset(my_out, 0, sizeof(my_out)); + + my_ret = mbedtls_x509_csr_parse_der_with_ext_cb(&csr, csr_der->x, csr_der->len, + accept ? parse_csr_ext_accept_cb : + parse_csr_ext_reject_cb, + NULL); + TEST_EQUAL(my_ret, ref_ret); + + if (ref_ret == 0) { + size_t my_out_len = mbedtls_x509_csr_info(my_out, sizeof(my_out), "", &csr); + TEST_EQUAL(my_out_len, strlen(ref_out)); + TEST_EQUAL(strcmp(my_out, ref_out), 0); + } + +exit: + mbedtls_x509_csr_free(&csr); + USE_PSA_DONE(); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_FS_IO:MBEDTLS_X509_CSR_PARSE_C:!MBEDTLS_X509_REMOVE_INFO */ void mbedtls_x509_csr_parse_file(char *csr_file, char *ref_out, int ref_ret) { From 03bd095a76bbc46ba73f30d6a0cb9e4176c424b9 Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Thu, 19 Oct 2023 09:52:59 +0200 Subject: [PATCH 06/10] Fix dependency check for helper functions. Signed-off-by: Matthias Schulz --- tests/suites/test_suite_x509parse.function | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 8cdca82e08..cdbbcfd733 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -412,7 +412,9 @@ int parse_crt_ext_cb(void *p_ctx, mbedtls_x509_crt const *crt, mbedtls_x509_buf MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); } } +#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#if defined(MBEDTLS_X509_CSR_PARSE_C) int parse_csr_ext_accept_cb(void *p_ctx, mbedtls_x509_csr const *csr, mbedtls_x509_buf const *oid, int critical, const unsigned char *cp, const unsigned char *end) { @@ -439,7 +441,7 @@ int parse_csr_ext_reject_cb(void *p_ctx, mbedtls_x509_csr const *csr, mbedtls_x5 return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG); } -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif /* MBEDTLS_X509_CSR_PARSE_C */ /* END_HEADER */ /* BEGIN_CASE depends_on:MBEDTLS_X509_CRT_PARSE_C */ From edc32eaf1ac8d967d093692c36902d1b3186cd3f Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Thu, 19 Oct 2023 16:09:08 +0200 Subject: [PATCH 07/10] Uncrustified Signed-off-by: Matthias Schulz --- include/mbedtls/x509_csr.h | 6 ++--- library/x509_csr.c | 28 +++++++++++----------- tests/suites/test_suite_x509parse.function | 9 +++---- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/include/mbedtls/x509_csr.h b/include/mbedtls/x509_csr.h index a4bdc9413b..f3ac570454 100644 --- a/include/mbedtls/x509_csr.h +++ b/include/mbedtls/x509_csr.h @@ -156,9 +156,9 @@ typedef int (*mbedtls_x509_csr_ext_cb_t)(void *p_ctx, * \return 0 if successful, or a specific X509 error code */ int mbedtls_x509_csr_parse_der_with_ext_cb(mbedtls_x509_csr *csr, - const unsigned char *buf, size_t buflen, - mbedtls_x509_csr_ext_cb_t cb, - void *p_ctx); + const unsigned char *buf, size_t buflen, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx); /** * \brief Load a Certificate Signing Request (CSR), DER or PEM format diff --git a/library/x509_csr.c b/library/x509_csr.c index 9e4a01ab6c..a8fef7c54d 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -153,7 +153,7 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, /* Forbid repeated extensions */ if ((csr->ext_types & ext_type) != 0) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, - MBEDTLS_ERR_ASN1_INVALID_DATA); + MBEDTLS_ERR_ASN1_INVALID_DATA); } csr->ext_types |= ext_type; @@ -162,7 +162,7 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, case MBEDTLS_X509_EXT_KEY_USAGE: /* Parse key usage */ if ((ret = mbedtls_x509_get_key_usage(p, end_ext_data, - &csr->key_usage)) != 0) { + &csr->key_usage)) != 0) { return ret; } break; @@ -170,7 +170,7 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, case MBEDTLS_X509_EXT_SUBJECT_ALT_NAME: /* Parse subject alt name */ if ((ret = mbedtls_x509_get_subject_alt_name(p, end_ext_data, - &csr->subject_alt_names)) != 0) { + &csr->subject_alt_names)) != 0) { return ret; } break; @@ -178,16 +178,16 @@ static int x509_csr_parse_extensions(mbedtls_x509_csr *csr, case MBEDTLS_X509_EXT_NS_CERT_TYPE: /* Parse netscape certificate type */ if ((ret = mbedtls_x509_get_ns_cert_type(p, end_ext_data, - &csr->ns_cert_type)) != 0) { + &csr->ns_cert_type)) != 0) { return ret; } break; default: /* - * If this is a non-critical extension, which the oid layer - * supports, but there isn't an x509 parser for it, - * skip the extension. - */ + * If this is a non-critical extension, which the oid layer + * supports, but there isn't an x509 parser for it, + * skip the extension. + */ if (is_critical) { return MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE; } else { @@ -274,9 +274,9 @@ static int x509_csr_parse_attributes(mbedtls_x509_csr *csr, * Parse a CSR in DER format */ static int mbedtls_x509_csr_parse_der_internal(mbedtls_x509_csr *csr, - const unsigned char *buf, size_t buflen, - mbedtls_x509_csr_ext_cb_t cb, - void *p_ctx) + const unsigned char *buf, size_t buflen, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t len; @@ -452,9 +452,9 @@ int mbedtls_x509_csr_parse_der(mbedtls_x509_csr *csr, * Parse a CSR in DER format with callback for unknown extensions */ int mbedtls_x509_csr_parse_der_with_ext_cb(mbedtls_x509_csr *csr, - const unsigned char *buf, size_t buflen, - mbedtls_x509_csr_ext_cb_t cb, - void *p_ctx) + const unsigned char *buf, size_t buflen, + mbedtls_x509_csr_ext_cb_t cb, + void *p_ctx) { return mbedtls_x509_csr_parse_der_internal(csr, buf, buflen, cb, p_ctx); } diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index cdbbcfd733..67d93d03aa 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -250,7 +250,8 @@ int verify_parse_san(mbedtls_x509_subject_alternative_name *san, ret = mbedtls_oid_get_numeric_string(p, n, - &san->san.other_name.value.hardware_module_name.oid); + &san->san.other_name.value.hardware_module_name + .oid); MBEDTLS_X509_SAFE_SNPRINTF; ret = mbedtls_snprintf(p, n, ", hardware serial number : "); @@ -416,7 +417,7 @@ int parse_crt_ext_cb(void *p_ctx, mbedtls_x509_crt const *crt, mbedtls_x509_buf #if defined(MBEDTLS_X509_CSR_PARSE_C) int parse_csr_ext_accept_cb(void *p_ctx, mbedtls_x509_csr const *csr, mbedtls_x509_buf const *oid, - int critical, const unsigned char *cp, const unsigned char *end) + int critical, const unsigned char *cp, const unsigned char *end) { (void) p_ctx; (void) csr; @@ -429,7 +430,7 @@ int parse_csr_ext_accept_cb(void *p_ctx, mbedtls_x509_csr const *csr, mbedtls_x5 } int parse_csr_ext_reject_cb(void *p_ctx, mbedtls_x509_csr const *csr, mbedtls_x509_buf const *oid, - int critical, const unsigned char *cp, const unsigned char *end) + int critical, const unsigned char *cp, const unsigned char *end) { (void) p_ctx; (void) csr; @@ -1288,7 +1289,7 @@ void mbedtls_x509_csr_parse_with_ext_cb(data_t *csr_der, char *ref_out, int ref_ my_ret = mbedtls_x509_csr_parse_der_with_ext_cb(&csr, csr_der->x, csr_der->len, accept ? parse_csr_ext_accept_cb : - parse_csr_ext_reject_cb, + parse_csr_ext_reject_cb, NULL); TEST_EQUAL(my_ret, ref_ret); From 83d0dbf087dcb8ba8ce5b910606f38aeb597fe2e Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Thu, 19 Oct 2023 16:25:53 +0200 Subject: [PATCH 08/10] Added changelog. Signed-off-by: Matthias Schulz --- ChangeLog.d/fix-csr-parsing-with-critical-fields-fails.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/fix-csr-parsing-with-critical-fields-fails.txt diff --git a/ChangeLog.d/fix-csr-parsing-with-critical-fields-fails.txt b/ChangeLog.d/fix-csr-parsing-with-critical-fields-fails.txt new file mode 100644 index 0000000000..5b155121f8 --- /dev/null +++ b/ChangeLog.d/fix-csr-parsing-with-critical-fields-fails.txt @@ -0,0 +1,6 @@ +Features + * Add new mbedtls_x509_csr_parse_der_with_ext_cb() routine which allows + parsing unsupported certificate extensions via user provided callback. + +Bugfix + * Fix parsing of CSRs with critical extensions. From e92f6dcf5cce6b33c57de75e2b77cba54853e081 Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Tue, 7 Nov 2023 15:16:35 +0100 Subject: [PATCH 09/10] New test cases requested in https://github.com/Mbed-TLS/mbedtls/pull/8378#discussion_r1383779861 Signed-off-by: Matthias Schulz --- tests/suites/test_suite_x509parse.data | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 9817536d7d..261c220ee6 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2940,15 +2940,23 @@ X509 CSR ASN.1 (OK) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA1:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_parse:"308201183081bf0201003034310b3009060355040613024e4c3111300f060355040a1308506f6c617253534c31123010060355040313096c6f63616c686f73743059301306072a8648ce3d020106082a8648ce3d0301070342000437cc56d976091e5a723ec7592dff206eee7cf9069174d0ad14b5f768225962924ee500d82311ffea2fd2345d5d16bd8a88c26b770d55cd8a2a0efa01c8b4edffa029302706092a864886f70d01090e311a301830090603551d1304023000300b0603551d0f0404030205e0300906072a8648ce3d04010349003046022100b49fd8c8f77abfa871908dfbe684a08a793d0f490a43d86fcf2086e4f24bb0c2022100f829d5ccd3742369299e6294394717c4b723a0f68b44e831b6e6c3bcabf97243":"CSR version \: 1\nsubject name \: C=NL, O=PolarSSL, CN=localhost\nsigned using \: ECDSA with SHA1\nEC key size \: 256 bits\n\nkey usage \: Digital Signature, Non Repudiation, Key Encipherment\n":0 -X509 CSR ASN.1 (Unsupported critical extension) +X509 CSR ASN.1 (Unsupported critical extension, critical=true) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_parse:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS+MBEDTLS_ERR_ASN1_UNEXPECTED_TAG -X509 CSR ASN.1 (Unsupported critical extension accepted by callback) +X509 CSR ASN.1 (Unsupported non-critical extension, critical=false) +depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO +mbedtls_x509_csr_parse:"308201243081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004b5e718a7df6b21912733e77c42f266b8283e6cae7adf8afd56b990c1c6232ea0a2a46097c218353bc948444aea3d00423e84802e28c48099641fe9977cdfb505a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101000403010101300a06082a8648ce3d0403020348003045022100f0ba9a0846ad0a7cefd0a61d5fc92194dc06037a44158de2d0c569912c058d430220421f27a9f249c1687e2fa34db3f543c8512fd925dfe5ae00867f13963ffd4f8d":"CSR version \: 1\nsubject name \: CN=Self signed test, C=DE, O=AuthCrtDB Test\nsigned using \: ECDSA with SHA256\nEC key size \: 256 bits\n":0 + +X509 CSR ASN.1 (Unsupported non-critical extension, critical undefined) +depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO +mbedtls_x509_csr_parse:"308201223081c802010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d030107034200045f94b28d133418833bf10c442d91306459d3925e7cea06ebb9220932e7de116fb671c5d2d6c0a3784a12897217aef8432e7228fcea0ab016bdb67b67ced4c612a025302306092a864886f70d01090e311630143012060b2b0601040183890c8622020403010101300a06082a8648ce3d04030203490030460221009b1e8b25775c18525e96753e1ed55875f8d62f026c5b7f70eb5037ad27dc92de022100ba1dfe14de6af6a603f763563fd046b1cd3714b54d6daf5d8a72076497f11014":"CSR version \: 1\nsubject name \: CN=Self signed test, C=DE, O=AuthCrtDB Test\nsigned using \: ECDSA with SHA256\nEC key size \: 256 bits\n":0 + +X509 CSR ASN.1 (Unsupported critical extension accepted by callback, critical=true) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_parse_with_ext_cb:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"CSR version \: 1\nsubject name \: CN=Self signed test, C=DE, O=AuthCrtDB Test\nsigned using \: ECDSA with SHA256\nEC key size \: 256 bits\n":0:1 -X509 CSR ASN.1 (Unsupported critical extension rejected by callback) +X509 CSR ASN.1 (Unsupported critical extension rejected by callback, critical=true) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_parse_with_ext_cb:"308201233081cb02010030413119301706035504030c1053656c66207369676e65642074657374310b300906035504061302444531173015060355040a0c0e41757468437274444220546573743059301306072a8648ce3d020106082a8648ce3d03010703420004c11ebb9951848a436ca2c8a73382f24bbb6c28a92e401d4889b0c361f377b92a8b0497ff2f5a5f6057ae85f704ab1850bef075914f68ed3aeb15a1ff1ebc0dc6a028302606092a864886f70d01090e311930173015060b2b0601040183890c8622020101ff0403010101300a06082a8648ce3d040302034700304402200c4108fd098525993d3fd5b113f0a1ead8750852baf55a2f8e670a22cabc0ba1022034db93a0fcb993912adcf2ea8cb4b66389af30e264d43c0daea03255e45d2ccc":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS+MBEDTLS_ERR_ASN1_UNEXPECTED_TAG:0 From c55b50034343d733f77441172ddda3cf2a9a9a89 Mon Sep 17 00:00:00 2001 From: Matthias Schulz Date: Tue, 7 Nov 2023 16:47:37 +0100 Subject: [PATCH 10/10] Changed notes in x509_csr.h to better describe the behavior of mbedtls_x509_csr_parse_der and mbedtls_x509_csr_parse_der_with_ext_cb. Signed-off-by: Matthias Schulz --- include/mbedtls/x509_csr.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/x509_csr.h b/include/mbedtls/x509_csr.h index f3ac570454..dc4f86de46 100644 --- a/include/mbedtls/x509_csr.h +++ b/include/mbedtls/x509_csr.h @@ -87,7 +87,9 @@ mbedtls_x509write_csr; /** * \brief Load a Certificate Signing Request (CSR) in DER format * - * \note CSR attributes (if any) are currently silently ignored. + * \note Any unsupported requested extensions are silently + * ignored, unless the critical flag is set, in which case + * the CSR is rejected. * * \note If #MBEDTLS_USE_PSA_CRYPTO is enabled, the PSA crypto * subsystem must have been initialized by calling @@ -140,7 +142,10 @@ typedef int (*mbedtls_x509_csr_ext_cb_t)(void *p_ctx, /** * \brief Load a Certificate Signing Request (CSR) in DER format * - * \note CSR attributes (if any) are currently silently ignored. + * \note Any unsupported requested extensions are silently + * ignored, unless the critical flag is set, in which case + * the result of the callback function decides whether + * CSR is rejected. * * \note If #MBEDTLS_USE_PSA_CRYPTO is enabled, the PSA crypto * subsystem must have been initialized by calling