From 1dff76782b635be6d96ecf46652c4864eda77d8c Mon Sep 17 00:00:00 2001 From: Anthony Hu Date: Fri, 28 Mar 2025 18:53:50 -0400 Subject: [PATCH 1/2] Check for duplicate extensions in a CRL --- tests/api.c | 39 +++++++++++++++++++ wolfcrypt/src/asn.c | 95 ++++++++++++++++++++++++++++----------------- 2 files changed, 98 insertions(+), 36 deletions(-) diff --git a/tests/api.c b/tests/api.c index 14295bbd6..8fa3e585b 100644 --- a/tests/api.c +++ b/tests/api.c @@ -4256,6 +4256,44 @@ static int test_wolfSSL_CertManagerNameConstraint5(void) return EXPECT_RESULT(); } +static int test_wolfSSL_CRL_duplicate_extensions(void) +{ + EXPECT_DECLS; +#if defined(WOLFSSL_ASN_TEMPLATE) && !defined(NO_CERTS) && \ + defined(HAVE_CRL) && !defined(NO_RSA) && !defined(WOLFSSL_NO_ASN_STRICT) + const unsigned char crl_duplicate_akd[] = + "-----BEGIN X509 CRL-----\n" + "MIICCDCB8QIBATANBgkqhkiG9w0BAQsFADB5MQswCQYDVQQGEwJVUzETMBEGA1UE\n" + "CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzETMBEGA1UECgwK\n" + "TXkgQ29tcGFueTETMBEGA1UEAwwKTXkgUm9vdCBDQTETMBEGA1UECwwKTXkgUm9v\n" + "dCBDQRcNMjQwOTAxMDAwMDAwWhcNMjUxMjAxMDAwMDAwWqBEMEIwHwYDVR0jBBgw\n" + "FoAU72ng99Ud5pns3G3Q9+K5XGRxgzUwHwYDVR0jBBgwFoAU72ng99Ud5pns3G3Q\n" + "9+K5XGRxgzUwDQYJKoZIhvcNAQELBQADggEBAIFVw4jrS4taSXR/9gPzqGrqFeHr\n" + "IXCnFtHJTLxqa8vUOAqSwqysvNpepVKioMVoGrLjFMjANjWQqTEiMROAnLfJ/+L8\n" + "FHZkV/mZwOKAXMhIC9MrJzifxBICwmvD028qnwQm09EP8z4ICZptD6wPdRTDzduc\n" + "KBuAX+zn8pNrJgyrheRKpPgno9KsbCzK4D/RIt1sTK2M3vVOtY+vpsN70QYUXvQ4\n" + "r2RZac3omlT43x5lddPxIlcouQpwWcVvr/K+Va770MRrjn88PBrJmvsEw/QYVBXp\n" + "Gxv2b78HFDacba80sMIm8ltRdqUCa5qIc6OATsz7izCQXEbkTEeESrcK1MA=\n" + "-----END X509 CRL-----\n"; + + WOLFSSL_CERT_MANAGER* cm = NULL; + int ret; + + cm = wolfSSL_CertManagerNew(); + ExpectNotNull(cm); + + /* Test loading CRL with duplicate extensions */ + WOLFSSL_MSG("Testing CRL with duplicate Authority Key Identifier extensions"); + ret = wolfSSL_CertManagerLoadCRLBuffer(cm, crl_duplicate_akd, + sizeof(crl_duplicate_akd), + WOLFSSL_FILETYPE_PEM); + ExpectIntEQ(ret, ASN_PARSE_E); + + wolfSSL_CertManagerFree(cm); +#endif + return EXPECT_RESULT(); +} + static int test_wolfSSL_CertManagerCRL(void) { EXPECT_DECLS; @@ -68143,6 +68181,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_wolfSSL_CertManagerNameConstraint4), TEST_DECL(test_wolfSSL_CertManagerNameConstraint5), TEST_DECL(test_wolfSSL_CertManagerCRL), + TEST_DECL(test_wolfSSL_CRL_duplicate_extensions), TEST_DECL(test_wolfSSL_CertManagerCheckOCSPResponse), TEST_DECL(test_wolfSSL_CheckOCSPResponse), #ifdef HAVE_CERT_CHAIN_VALIDATION diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index e89f3627b..4ec0ada59 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -40540,6 +40540,9 @@ static int ParseCRL_Extensions(DecodedCRL* dcrl, const byte* buf, word32 idx, { DECL_ASNGETDATA(dataASN, certExtASN_Length); int ret = 0; + /* Track if we've seen these extensions already */ + word32 seenAuthKey = 0; + word32 seenCrlNum = 0; ALLOC_ASNGETDATA(dataASN, certExtASN_Length, ret, dcrl->heap); @@ -40562,49 +40565,69 @@ static int ParseCRL_Extensions(DecodedCRL* dcrl, const byte* buf, word32 idx, /* Length of extension data. */ int length = (int)dataASN[CERTEXTASN_IDX_VAL].length; - if (oid == AUTH_KEY_OID) { - #ifndef NO_SKID - /* Parse Authority Key Id extension. - * idx is at start of OCTET_STRING data. */ - ret = ParseCRL_AuthKeyIdExt(buf + localIdx, length, dcrl); - if (ret != 0) { - WOLFSSL_MSG("\tcouldn't parse AuthKeyId extension"); - } - #endif + /* Check for duplicate extension */ + if ((oid == AUTH_KEY_OID && seenAuthKey) || + (oid == CRL_NUMBER_OID && seenCrlNum)) { + WOLFSSL_MSG("Duplicate CRL extension found"); + /* Gating !WOLFSSL_NO_ASN_STRICT will allow wolfCLU to have same + * behaviour as OpenSSL */ +#ifndef WOLFSSL_NO_ASN_STRICT + ret = ASN_PARSE_E; +#endif } - else if (oid == CRL_NUMBER_OID) { - DECL_MP_INT_SIZE_DYN(m, CRL_MAX_NUM_SZ * CHAR_BIT, - CRL_MAX_NUM_SZ * CHAR_BIT); - NEW_MP_INT_SIZE(m, CRL_MAX_NUM_SZ * CHAR_BIT, NULL, - DYNAMIC_TYPE_TMP_BUFFER); - #ifdef MP_INT_SIZE_CHECK_NULL - if (m == NULL) { - ret = MEMORY_E; + /* Track this extension if no duplicate found */ + if (ret == 0) { + if (oid == AUTH_KEY_OID) + seenAuthKey = 1; + else if (oid == CRL_NUMBER_OID) + seenCrlNum = 1; + } + + if (ret == 0) { + if (oid == AUTH_KEY_OID) { + #ifndef NO_SKID + /* Parse Authority Key Id extension. + * idx is at start of OCTET_STRING data. */ + ret = ParseCRL_AuthKeyIdExt(buf + localIdx, length, dcrl); + if (ret != 0) { + WOLFSSL_MSG("\tcouldn't parse AuthKeyId extension"); + } + #endif } - #endif + else if (oid == CRL_NUMBER_OID) { + DECL_MP_INT_SIZE_DYN(m, CRL_MAX_NUM_SZ * CHAR_BIT, + CRL_MAX_NUM_SZ * CHAR_BIT); + NEW_MP_INT_SIZE(m, CRL_MAX_NUM_SZ * CHAR_BIT, NULL, + DYNAMIC_TYPE_TMP_BUFFER); - if (ret == 0 && (INIT_MP_INT_SIZE(m, CRL_MAX_NUM_SZ * CHAR_BIT) - != MP_OKAY)) { + #ifdef MP_INT_SIZE_CHECK_NULL + if (m == NULL) { + ret = MEMORY_E; + } + #endif + + if (ret == 0 && (INIT_MP_INT_SIZE(m, CRL_MAX_NUM_SZ * + CHAR_BIT) != MP_OKAY)) { ret = MP_INIT_E; + } + + if (ret == 0) { + ret = GetInt(m, buf, &localIdx, maxIdx); + } + + if (ret == 0 && mp_toradix(m, (char*)dcrl->crlNumber, + MP_RADIX_HEX) != MP_OKAY) + ret = BUFFER_E; + + if (ret == 0) { + dcrl->crlNumberSet = 1; + } + + mp_free(m); + FREE_MP_INT_SIZE(m, NULL, DYNAMIC_TYPE_TMP_BUFFER); } - - if (ret == 0) { - ret = GetInt(m, buf, &localIdx, maxIdx); - } - - if (ret == 0 && mp_toradix(m, (char*)dcrl->crlNumber, - MP_RADIX_HEX) != MP_OKAY) - ret = BUFFER_E; - - if (ret == 0) { - dcrl->crlNumberSet = 1; - } - - mp_free(m); - FREE_MP_INT_SIZE(m, NULL, DYNAMIC_TYPE_TMP_BUFFER); } - /* TODO: check criticality */ /* Move index on to next extension. */ idx += (word32)length; } From a0cd18daead4257dace0be5eaa2178686f319c95 Mon Sep 17 00:00:00 2001 From: Anthony Hu Date: Wed, 25 Jun 2025 16:01:04 -0400 Subject: [PATCH 2/2] Add back a removed comment and give RFC reference. --- wolfcrypt/src/asn.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 4ec0ada59..3ea6fc4c5 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -40565,7 +40565,10 @@ static int ParseCRL_Extensions(DecodedCRL* dcrl, const byte* buf, word32 idx, /* Length of extension data. */ int length = (int)dataASN[CERTEXTASN_IDX_VAL].length; - /* Check for duplicate extension */ + /* Check for duplicate extension. RFC 5280 Section 4.2 states that + * a certificate must not include more than one instance of a + * particular extension. Note that the same guidance does not appear + * for CRLs but the same reasoning should apply. */ if ((oid == AUTH_KEY_OID && seenAuthKey) || (oid == CRL_NUMBER_OID && seenCrlNum)) { WOLFSSL_MSG("Duplicate CRL extension found"); @@ -40628,6 +40631,7 @@ static int ParseCRL_Extensions(DecodedCRL* dcrl, const byte* buf, word32 idx, FREE_MP_INT_SIZE(m, NULL, DYNAMIC_TYPE_TMP_BUFFER); } } + /* TODO: check criticality */ /* Move index on to next extension. */ idx += (word32)length; }