From e85901c8e5d6360c191193396c0d070a4f87a5da Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 21 Aug 2023 13:49:44 +0200 Subject: [PATCH 1/4] Only list supported sigalgs in certreq --- src/internal.c | 12 +++---- tests/api.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index 722355090..f74b96726 100644 --- a/src/internal.c +++ b/src/internal.c @@ -25906,7 +25906,7 @@ int SetCipherList(WOLFSSL_CTX* ctx, Suites* suites, const char* list) #endif #if defined(HAVE_ECC) || defined(HAVE_ED25519) || \ defined(HAVE_ED448) - if (((haveSig && SIG_ECDSA) == 0) && XSTRSTR(name, "ECDSA")) + if (XSTRSTR(name, "ECDSA")) haveSig |= SIG_ECDSA; else #endif @@ -25915,11 +25915,11 @@ int SetCipherList(WOLFSSL_CTX* ctx, Suites* suites, const char* list) haveSig |= SIG_ANON; else #endif - if (((haveSig & SIG_RSA) == 0) - #ifndef NO_PSK - && (XSTRSTR(name, "PSK") == NULL) - #endif - ) { + #ifndef NO_PSK + if (XSTRSTR(name, "PSK") == NULL) + #endif + { + /* Fall back to RSA */ haveSig |= SIG_RSA; } diff --git a/tests/api.c b/tests/api.c index 9579f9076..d0ebd4fbd 100644 --- a/tests/api.c +++ b/tests/api.c @@ -64893,6 +64893,90 @@ static int test_dtls_client_hello_timeout(void) return EXPECT_RESULT(); } +/** + * Make sure we don't send RSA Signature Hash Algorithms in the + * CertificateRequest when we don't have any such ciphers set. + * @return EXPECT_RESULT() + */ +static int test_certreq_sighash_algos(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) + WOLFSSL_CTX *ctx_c = NULL; + WOLFSSL_CTX *ctx_s = NULL; + WOLFSSL *ssl_c = NULL; + WOLFSSL *ssl_s = NULL; + struct test_memio_ctx test_ctx; + int idx = 0; + int maxIdx = 0; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + test_ctx.c_ciphers = test_ctx.s_ciphers = "TLS_ECDHE_ECDSA_WITH_NULL_SHA:" + "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384"; + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); + + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_c, + "./certs/ca-ecc-cert.pem", NULL), WOLFSSL_SUCCESS); + + wolfSSL_set_verify(ssl_s, SSL_VERIFY_PEER, NULL); + ExpectIntEQ(wolfSSL_use_PrivateKey_file(ssl_s, "./certs/ecc-key.pem", + WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_use_certificate_file(ssl_s, "./certs/server-ecc.pem", + WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + + ExpectIntEQ(wolfSSL_connect(ssl_c), WOLFSSL_FATAL_ERROR); + ExpectIntEQ(wolfSSL_get_error(ssl_c, WOLFSSL_FATAL_ERROR), + WOLFSSL_ERROR_WANT_READ); + + ExpectIntEQ(wolfSSL_accept(ssl_s), WOLFSSL_FATAL_ERROR); + ExpectIntEQ(wolfSSL_get_error(ssl_s, WOLFSSL_FATAL_ERROR), + WOLFSSL_ERROR_WANT_READ); + + /* Find the CertificateRequest message */ + for (idx = 0; idx < test_ctx.c_len && EXPECT_SUCCESS();) { + word16 len; + ExpectIntEQ(test_ctx.c_buff[idx++], handshake); + ExpectIntEQ(test_ctx.c_buff[idx++], SSLv3_MAJOR); + ExpectIntEQ(test_ctx.c_buff[idx++], TLSv1_2_MINOR); + ato16(test_ctx.c_buff + idx, &len); + idx += OPAQUE16_LEN; + if (test_ctx.c_buff[idx] == certificate_request) { + idx++; + /* length */ + idx += OPAQUE24_LEN; + /* cert types */ + idx += 1 + test_ctx.c_buff[idx]; + /* Sig algos */ + ato16(test_ctx.c_buff + idx, &len); + idx += OPAQUE16_LEN; + maxIdx = idx + (int)len; + for (; idx < maxIdx && EXPECT_SUCCESS(); idx += OPAQUE16_LEN) { + if (test_ctx.c_buff[idx+1] == ED25519_SA_MINOR || + test_ctx.c_buff[idx+1] == ED448_SA_MINOR) + ExpectIntEQ(test_ctx.c_buff[idx], NEW_SA_MAJOR); + else + ExpectIntEQ(test_ctx.c_buff[idx+1], ecc_dsa_sa_algo); + } + break; + } + else { + idx += (int)len; + } + } + ExpectIntLT(idx, test_ctx.c_len); + + + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + /*----------------------------------------------------------------------------* | Main *----------------------------------------------------------------------------*/ @@ -66155,6 +66239,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_dtls_downgrade_scr), TEST_DECL(test_dtls_client_hello_timeout_downgrade), TEST_DECL(test_dtls_client_hello_timeout), + TEST_DECL(test_certreq_sighash_algos), /* This test needs to stay at the end to clean up any caches allocated. */ TEST_DECL(test_wolfSSL_Cleanup) }; From abfcda8750a76d4a47630593c45c9c40399e261f Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Mon, 21 Aug 2023 17:31:31 +0200 Subject: [PATCH 2/4] Decode the key usage extension as LE not BE --- wolfcrypt/src/asn.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 6184f2e88..a4b92c17d 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -19247,14 +19247,24 @@ static int DecodeKeyUsage(const byte* input, word32 sz, DecodedCert* cert) #else ASNGetData dataASN[keyUsageASN_Length]; word32 idx = 0; + byte keyUsage[OPAQUE16_LEN]; + word32 keyUsageSz = sizeof(keyUsage); + int ret; WOLFSSL_ENTER("DecodeKeyUsage"); /* Clear dynamic data and set where to store extended key usage. */ XMEMSET(dataASN, 0, sizeof(dataASN)); - GetASN_Int16Bit(&dataASN[KEYUSAGEASN_IDX_STR], &cert->extKeyUsage); + GetASN_Buffer(&dataASN[KEYUSAGEASN_IDX_STR], keyUsage, &keyUsageSz); /* Parse key usage. */ - return GetASN_Items(keyUsageASN, dataASN, keyUsageASN_Length, 0, input, + ret = GetASN_Items(keyUsageASN, dataASN, keyUsageASN_Length, 0, input, &idx, sz); + if (ret == 0) { + /* Decode the bit string number as LE */ + cert->extKeyUsage = (word16)(keyUsage[0]); + if (keyUsageSz == 2) + cert->extKeyUsage |= (word16)(keyUsage[1] << 8); + } + return ret; #endif /* WOLFSSL_ASN_TEMPLATE */ } From 57ce894393d96455684a2fe610d6085e690ab9f3 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Wed, 23 Aug 2023 16:05:10 +0200 Subject: [PATCH 3/4] CRL refactor - CheckCertCRLList: check all entries in case a single issuer has multiple CRL's loaded - test_multiple_crls_same_issuer: testing two different certificates forcing the client to check both CRL's from the same issuer - CRL_Entry - use a lock instead of a mutex to allow multiple threads to access the same list simultaneously - add a verifyMutex when doing verification so that we don't have to release the crlLock - Add allocation and free functions for CRL_Entry - DupCRL_Entry: simplify copying by copying all static fields in one memcpy --- certs/crl/extra-crls/general-server-crl.pem | 13 + certs/crl/gencrls.sh | 36 ++- src/crl.c | 307 ++++++++------------ tests/api.c | 51 +++- wolfcrypt/src/asn.c | 2 +- wolfssl/internal.h | 14 +- 6 files changed, 218 insertions(+), 205 deletions(-) create mode 100644 certs/crl/extra-crls/general-server-crl.pem diff --git a/certs/crl/extra-crls/general-server-crl.pem b/certs/crl/extra-crls/general-server-crl.pem new file mode 100644 index 000000000..25b3fdf5e --- /dev/null +++ b/certs/crl/extra-crls/general-server-crl.pem @@ -0,0 +1,13 @@ +-----BEGIN X509 CRL----- +MIICBDCB7QIBATANBgkqhkiG9w0BAQsFADCBlDELMAkGA1UEBhMCVVMxEDAOBgNV +BAgMB01vbnRhbmExEDAOBgNVBAcMB0JvemVtYW4xETAPBgNVBAoMCFNhd3Rvb3Ro +MRMwEQYDVQQLDApDb25zdWx0aW5nMRgwFgYDVQQDDA93d3cud29sZnNzbC5jb20x +HzAdBgkqhkiG9w0BCQEWEGluZm9Ad29sZnNzbC5jb20XDTIzMDgyMjE3NDgzNloX +DTI2MDUxODE3NDgzNlowFDASAgEBFw0yMzA4MjIxNzQ4MjlaoA4wDDAKBgNVHRQE +AwIBAzANBgkqhkiG9w0BAQsFAAOCAQEArb/WuyC0mGBJXNdWFACKd8t3xHP1ypbH +IkRyTBXGgsb7zjCiwraMxNBwaypaDURv3uVBIjSF+toJYnEB2cCj8K6VBeMOeqz7 +9l7gsP9xy6LP2YosqiN1MuGZP8SxUxBX9RlHPXO4i85s2DKwdBftg0rXdXLbhafx +m6F3+CIIG+J6BO6D9KOrfaNcLZOgY3LTF2Rc1Y9qH2CUNBgfGMalFt1c13MsP2Oa +Z22HWuJbiLPdeyEsFNy/4ROshgB85kMwZWZQA0LnD5gedwRuaAmlFwSuayl3epwE +v0SQy1Kcp6UbZFTELiIoCNC8y9hL56okbux/TtiukU6mQkvIQBidtQ== +-----END X509 CRL----- diff --git a/certs/crl/gencrls.sh b/certs/crl/gencrls.sh index 4f2e22ad0..453f3d807 100755 --- a/certs/crl/gencrls.sh +++ b/certs/crl/gencrls.sh @@ -92,6 +92,18 @@ mv tmp crl.revoked #cp crl.revoked ~/wolfssl/certs/crl/crl.revoked +# remove revoked so next time through the normal CA won't have server revoked +cp blank.index.txt demoCA/index.txt + +# revoke the general server cert +echo "Step 10" +openssl ca -config ../renewcerts/wolfssl.cnf -revoke ../server-cert.pem -keyfile ../ca-key.pem -cert ../ca-cert.pem +check_result $? + +echo "Step 11" +openssl ca -config ../renewcerts/wolfssl.cnf -gencrl -crldays 1000 -out extra-crls/general-server-crl.pem -keyfile ../ca-key.pem -cert ../ca-cert.pem +check_result $? + # remove revoked so next time through the normal CA won't have server revoked cp blank.index.txt demoCA/index.txt @@ -105,7 +117,7 @@ openssl ca -config ../renewcerts/wolfssl.cnf -gencrl -crldays 1000 -out caEccCrl check_result $? # metadata -echo "Step 12" +echo "Step 13" openssl crl -in caEccCrl.pem -text > tmp check_result $? mv tmp caEccCrl.pem @@ -116,12 +128,12 @@ mv tmp caEccCrl.pem # server-revoked-cert.pem is already revoked in Step 10 #openssl ca -config ../renewcerts/wolfssl.cnf -revoke ../server-revoked-cert.pem -keyfile ../ca-ecc384-key.pem -cert ../ca-ecc384-cert.pem -echo "Step 13" +echo "Step 14" openssl ca -config ../renewcerts/wolfssl.cnf -gencrl -crldays 1000 -out caEcc384Crl.pem -keyfile ../ca-ecc384-key.pem -cert ../ca-ecc384-cert.pem check_result $? # metadata -echo "Step 14" +echo "Step 15" openssl crl -in caEcc384Crl.pem -text > tmp check_result $? mv tmp caEcc384Crl.pem @@ -129,12 +141,12 @@ mv tmp caEcc384Crl.pem #cp caEcc384Crl.pem ~/wolfssl/certs/crl/caEcc384Crl.pem # cliCrl -echo "Step 15" +echo "Step 16" openssl ca -config ../renewcerts/wolfssl.cnf -gencrl -crldays 1000 -out cliCrl.pem -keyfile ../client-key.pem -cert ../client-cert.pem check_result $? # metadata -echo "Step 16" +echo "Step 17" openssl crl -in cliCrl.pem -text > tmp check_result $? mv tmp cliCrl.pem @@ -142,12 +154,12 @@ mv tmp cliCrl.pem #cp cliCrl.pem ~/wolfssl/certs/crl/cliCrl.pem # eccCliCRL -echo "Step 17" +echo "Step 18" openssl ca -config ../renewcerts/wolfssl.cnf -gencrl -crldays 1000 -out eccCliCRL.pem -keyfile ../ecc-client-key.pem -cert ../client-ecc-cert.pem check_result $? # metadata -echo "Step 18" +echo "Step 19" openssl crl -in eccCliCRL.pem -text > tmp check_result $? mv tmp eccCliCRL.pem @@ -155,12 +167,12 @@ mv tmp eccCliCRL.pem #cp eccCliCRL.pem ~/wolfssl/certs/crl/eccCliCRL.pem # eccSrvCRL -echo "Step 19" +echo "Step 20" openssl ca -config ../renewcerts/wolfssl.cnf -gencrl -crldays 1000 -out eccSrvCRL.pem -keyfile ../ecc-key.pem -cert ../server-ecc.pem check_result $? # metadata -echo "Step 20" +echo "Step 21" openssl crl -in eccSrvCRL.pem -text > tmp check_result $? mv tmp eccSrvCRL.pem @@ -168,17 +180,17 @@ mv tmp eccSrvCRL.pem #cp eccSrvCRL.pem ~/wolfssl/certs/crl/eccSrvCRL.pem # caEccCrl -echo "Step 21" +echo "Step 22" openssl ca -config ./wolfssl.cnf -gencrl -crldays 1000 -out caEccCrl.pem -keyfile ../ca-ecc-key.pem -cert ../ca-ecc-cert.pem check_result $? # ca-ecc384-cert -echo "Step 22" +echo "Step 23" openssl ca -config ./wolfssl.cnf -gencrl -crldays 1000 -out caEcc384Crl.pem -keyfile ../ca-ecc384-key.pem -cert ../ca-ecc384-cert.pem check_result $? # create crl and crl2 der files for unit test -echo "Step 23" +echo "Step 24" openssl crl -in crl.pem -inform PEM -out crl.der -outform DER openssl crl -in crl2.pem -inform PEM -out crl2.der -outform DER diff --git a/src/crl.c b/src/crl.c index 20160e3d3..bcafc309c 100644 --- a/src/crl.c +++ b/src/crl.c @@ -83,7 +83,7 @@ int InitCRL(WOLFSSL_CRL* crl, WOLFSSL_CERT_MANAGER* cm) #ifdef HAVE_CRL_IO crl->crlIOCb = NULL; #endif - if (wc_InitMutex(&crl->crlLock) != 0) { + if (wc_InitRwLock(&crl->crlLock) != 0) { WOLFSSL_MSG("Init Mutex failed"); return BAD_MUTEX_E; } @@ -168,9 +168,23 @@ static int InitCRL_Entry(CRL_Entry* crle, DecodedCRL* dcrl, const byte* buff, return 0; } +static CRL_Entry* CRL_Entry_new(void* heap) +{ + CRL_Entry* crle = (CRL_Entry*)XMALLOC(sizeof(CRL_Entry), heap, + DYNAMIC_TYPE_CRL_ENTRY); + if (crle != NULL) { + XMEMSET(crle, 0, sizeof(CRL_Entry)); + if (wc_InitMutex(&crle->verifyMutex) != 0) { + XFREE(crle, heap, DYNAMIC_TYPE_CRL_ENTRY); + crle = NULL; + } + } + (void)heap; + return crle; +} /* Free all CRL Entry resources */ -static void FreeCRL_Entry(CRL_Entry* crle, void* heap) +static void CRL_Entry_free(CRL_Entry* crle, void* heap) { #ifdef CRL_STATIC_REVOKED_LIST if (crle != NULL) { @@ -198,11 +212,12 @@ static void FreeCRL_Entry(CRL_Entry* crle, void* heap) XFREE(crle->issuer, heap, DYNAMIC_TYPE_X509); } #endif + wc_FreeMutex(&crle->verifyMutex); + XFREE(crle, heap, DYNAMIC_TYPE_CRL_ENTRY); (void)heap; } - /* Free all CRL resources */ void FreeCRL(WOLFSSL_CRL* crl, int dynamic) { @@ -219,8 +234,7 @@ void FreeCRL(WOLFSSL_CRL* crl, int dynamic) crl->currentEntry = NULL; while(tmp) { CRL_Entry* next = tmp->next; - FreeCRL_Entry(tmp, crl->heap); - XFREE(tmp, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); + CRL_Entry_free(tmp, crl->heap); tmp = next; } @@ -238,7 +252,7 @@ void FreeCRL(WOLFSSL_CRL* crl, int dynamic) if (wolfSSL_CondFree(&crl->cond) != 0) WOLFSSL_MSG("wolfSSL_CondFree failed in FreeCRL"); #endif - wc_FreeMutex(&crl->crlLock); + wc_FreeRwLock(&crl->crlLock); if (dynamic) /* free self */ XFREE(crl, crl->heap, DYNAMIC_TYPE_CRL); } @@ -285,108 +299,71 @@ static int FindRevokedSerial(DecodedCert* cert, RevokedCert* rc, int totalCerts) #endif return ret; } + +static int VerifyCRLE(const WOLFSSL_CRL* crl, CRL_Entry* crle) +{ + Signer* ca = NULL; + SignatureCtx sigCtx; + int ret = 0; + +#ifndef NO_SKID + if (crle->extAuthKeyIdSet) + ca = GetCA(crl->cm, crle->extAuthKeyId); + if (ca == NULL) + ca = GetCAByName(crl->cm, crle->issuerHash); +#else /* NO_SKID */ + ca = GetCA(crl->cm, crle->issuerHash); +#endif /* NO_SKID */ + if (ca == NULL) { + WOLFSSL_MSG("Did NOT find CRL issuer CA"); + return ASN_CRL_NO_SIGNER_E; + } + + ret = VerifyCRL_Signature(&sigCtx, crle->toBeSigned, crle->tbsSz, + crle->signature, crle->signatureSz, crle->signatureOID, ca, + crl->heap); + + if (ret == 0) + crle->verified = 1; + else + crle->verified = ret; + + return ret; +} + static int CheckCertCRLList(WOLFSSL_CRL* crl, DecodedCert* cert, int *pFoundEntry) { CRL_Entry* crle; int foundEntry = 0; int ret = 0; - if (wc_LockMutex(&crl->crlLock) != 0) { - WOLFSSL_MSG("wc_LockMutex failed"); + if (wc_LockRwLock_Rd(&crl->crlLock) != 0) { + WOLFSSL_MSG("wc_LockRwLock_Rd failed"); return BAD_MUTEX_E; } - crle = crl->crlList; - - while (crle) { + for (crle = crl->crlList; crle != NULL; crle = crle->next) { if (XMEMCMP(crle->issuerHash, cert->issuerHash, CRL_DIGEST_SIZE) == 0) { WOLFSSL_MSG("Found CRL Entry on list"); if (crle->verified == 0) { - Signer* ca = NULL; - #ifndef NO_SKID - byte extAuthKeyId[KEYID_SIZE]; - #endif - byte issuerHash[CRL_DIGEST_SIZE]; - byte* tbs; - word32 tbsSz = crle->tbsSz; - byte* sig = NULL; - word32 sigSz = crle->signatureSz; - word32 sigOID = crle->signatureOID; - SignatureCtx sigCtx; - - tbs = (byte*)XMALLOC(tbsSz, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - if (tbs == NULL) { - wc_UnLockMutex(&crl->crlLock); - return MEMORY_E; - } - sig = (byte*)XMALLOC(sigSz, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - if (sig == NULL) { - XFREE(tbs, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - wc_UnLockMutex(&crl->crlLock); - return MEMORY_E; - } - - XMEMCPY(tbs, crle->toBeSigned, tbsSz); - XMEMCPY(sig, crle->signature, sigSz); - #ifndef NO_SKID - XMEMCPY(extAuthKeyId, crle->extAuthKeyId, - sizeof(extAuthKeyId)); - #endif - XMEMCPY(issuerHash, crle->issuerHash, sizeof(issuerHash)); - - wc_UnLockMutex(&crl->crlLock); - - #ifndef NO_SKID - if (crle->extAuthKeyIdSet) - ca = GetCA(crl->cm, extAuthKeyId); - if (ca == NULL) - ca = GetCAByName(crl->cm, issuerHash); - #else /* NO_SKID */ - ca = GetCA(crl->cm, issuerHash); - #endif /* NO_SKID */ - if (ca == NULL) { - XFREE(sig, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - XFREE(tbs, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - WOLFSSL_MSG("Did NOT find CRL issuer CA"); - return ASN_CRL_NO_SIGNER_E; - } - - ret = VerifyCRL_Signature(&sigCtx, tbs, tbsSz, sig, sigSz, - sigOID, ca, crl->heap); - - XFREE(sig, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - XFREE(tbs, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - - if (wc_LockMutex(&crl->crlLock) != 0) { + if (wc_LockMutex(&crle->verifyMutex) != 0) { WOLFSSL_MSG("wc_LockMutex failed"); - return BAD_MUTEX_E; + break; } - crle = crl->crlList; - while (crle) { - if (XMEMCMP(crle->issuerHash, cert->issuerHash, - CRL_DIGEST_SIZE) == 0) { + /* A different thread may have verified the entry while we were + * waiting for the mutex. */ + if (crle->verified == 0) + ret = VerifyCRLE(crl, crle); - if (ret == 0) - crle->verified = 1; - else - crle->verified = ret; + wc_UnLockMutex(&crle->verifyMutex); - XFREE(crle->toBeSigned, crl->heap, - DYNAMIC_TYPE_CRL_ENTRY); - crle->toBeSigned = NULL; - XFREE(crle->signature, crl->heap, - DYNAMIC_TYPE_CRL_ENTRY); - crle->signature = NULL; - break; - } - crle = crle->next; - } - if (crle == NULL || crle->verified < 0) + if (ret != 0) break; } - else if (crle->verified < 0) { + + if (crle->verified < 0) { WOLFSSL_MSG("Cannot use CRL as it didn't verify"); ret = crle->verified; break; @@ -407,17 +384,14 @@ static int CheckCertCRLList(WOLFSSL_CRL* crl, DecodedCert* cert, int *pFoundEntr } if (ret == 0) { foundEntry = 1; + ret = FindRevokedSerial(cert, crle->certs, crle->totalCerts); + if (ret != 0) + break; } - break; } - crle = crle->next; } - if (foundEntry) { - ret = FindRevokedSerial(cert, crle->certs, crle->totalCerts); - } - - wc_UnLockMutex(&crl->crlLock); + wc_UnLockRwLock(&crl->crlLock); *pFoundEntry = foundEntry; @@ -520,8 +494,7 @@ static int AddCRL(WOLFSSL_CRL* crl, DecodedCRL* dcrl, const byte* buff, crle = crl->currentEntry; if (crle == NULL) { - crle = (CRL_Entry*)XMALLOC(sizeof(CRL_Entry), crl->heap, - DYNAMIC_TYPE_CRL_ENTRY); + crle = CRL_Entry_new(crl->heap); if (crle == NULL) { WOLFSSL_MSG("alloc CRL Entry failed"); return MEMORY_E; @@ -530,25 +503,19 @@ static int AddCRL(WOLFSSL_CRL* crl, DecodedCRL* dcrl, const byte* buff, if (InitCRL_Entry(crle, dcrl, buff, verified, crl->heap) < 0) { WOLFSSL_MSG("Init CRL Entry failed"); - FreeCRL_Entry(crle, crl->heap); - if (crle != crl->currentEntry) { - XFREE(crle, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - } + CRL_Entry_free(crle, crl->heap); return -1; } - if (wc_LockMutex(&crl->crlLock) != 0) { - WOLFSSL_MSG("wc_LockMutex failed"); - FreeCRL_Entry(crle, crl->heap); - if (crle != crl->currentEntry) { - XFREE(crle, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); - } + if (wc_LockRwLock_Wr(&crl->crlLock) != 0) { + WOLFSSL_MSG("wc_LockRwLock_Wr failed"); + CRL_Entry_free(crle, crl->heap); return BAD_MUTEX_E; } crle->next = crl->crlList; crl->crlList = crle; - wc_UnLockMutex(&crl->crlLock); + wc_UnLockRwLock(&crl->crlLock); /* Avoid heap-use-after-free after crl->crlList is released */ crl->currentEntry = NULL; @@ -599,8 +566,7 @@ int BufferLoadCRL(WOLFSSL_CRL* crl, const byte* buff, long sz, int type, } #endif - crl->currentEntry = (CRL_Entry*)XMALLOC(sizeof(CRL_Entry), crl->heap, - DYNAMIC_TYPE_CRL_ENTRY); + crl->currentEntry = CRL_Entry_new(crl->heap); if (crl->currentEntry == NULL) { WOLFSSL_MSG("alloc CRL Entry failed"); #ifdef WOLFSSL_SMALL_STACK @@ -609,14 +575,13 @@ int BufferLoadCRL(WOLFSSL_CRL* crl, const byte* buff, long sz, int type, FreeDer(&der); return MEMORY_E; } - XMEMSET(crl->currentEntry, 0, sizeof(CRL_Entry)); InitDecodedCRL(dcrl, crl->heap); ret = ParseCRL(crl->currentEntry->certs, dcrl, myBuffer, (word32)sz, verify, crl->cm); if (ret != 0 && !(ret == ASN_CRL_NO_SIGNER_E && verify == NO_VERIFY)) { WOLFSSL_MSG("ParseCRL error"); - XFREE(crl->currentEntry, crl->heap, DYNAMIC_TYPE_CRL_ENTRY); + CRL_Entry_free(crl->currentEntry, crl->heap); crl->currentEntry = NULL; } else { @@ -701,75 +666,49 @@ static RevokedCert *DupRevokedCertList(RevokedCert* in, void* heap) static CRL_Entry* DupCRL_Entry(const CRL_Entry* ent, void* heap) { CRL_Entry *dupl; + const size_t copyOffset = OFFSETOF(CRL_Entry, next) + + sizeof(ent->next); #ifdef CRL_STATIC_REVOKED_LIST if (ent->totalCerts > CRL_MAX_REVOKED_CERTS) { return NULL; } #endif - dupl = (CRL_Entry*)XMALLOC(sizeof(CRL_Entry), heap, DYNAMIC_TYPE_CRL_ENTRY); + dupl = CRL_Entry_new(heap); if (dupl == NULL) { WOLFSSL_MSG("alloc CRL Entry failed"); return NULL; } - XMEMSET(dupl, 0, sizeof(CRL_Entry)); - XMEMCPY(dupl->issuerHash, ent->issuerHash, CRL_DIGEST_SIZE); - XMEMCPY(dupl->lastDate, ent->lastDate, MAX_DATE_SIZE); - XMEMCPY(dupl->nextDate, ent->nextDate, MAX_DATE_SIZE); - dupl->lastDateFormat = ent->lastDateFormat; - dupl->nextDateFormat = ent->nextDateFormat; + XMEMCPY((byte*)dupl + copyOffset, (byte*)ent + copyOffset, + sizeof(CRL_Entry) - copyOffset); -#if defined(OPENSSL_EXTRA) - dupl->lastDateAsn1.length = MAX_DATE_SIZE; - XMEMCPY (dupl->lastDateAsn1.data, dupl->lastDate, - dupl->lastDateAsn1.length); - dupl->lastDateAsn1.type = dupl->lastDateFormat; - dupl->nextDateAsn1.length = MAX_DATE_SIZE; - XMEMCPY (dupl->nextDateAsn1.data, dupl->nextDate, - dupl->nextDateAsn1.length); - dupl->nextDateAsn1.type = dupl->nextDateFormat; -#endif - -#ifdef CRL_STATIC_REVOKED_LIST - XMEMCPY(dupl->certs, ent->certs, ent->totalCerts*sizeof(RevokedCert)); -#else +#ifndef CRL_STATIC_REVOKED_LIST dupl->certs = DupRevokedCertList(ent->certs, heap); #endif - dupl->totalCerts = ent->totalCerts; - dupl->verified = ent->verified; +#ifdef OPENSSL_EXTRA + dupl->issuer = wolfSSL_X509_NAME_dup(ent->issuer); +#endif if (!ent->verified) { - dupl->tbsSz = ent->tbsSz; - dupl->signatureSz = ent->signatureSz; - dupl->signatureOID = ent->signatureOID; dupl->toBeSigned = (byte*)XMALLOC(dupl->tbsSz, heap, DYNAMIC_TYPE_CRL_ENTRY); - if (dupl->toBeSigned == NULL) { - FreeCRL_Entry(dupl, heap); - XFREE(dupl, heap, DYNAMIC_TYPE_CRL_ENTRY); - return NULL; - } - dupl->signature = (byte*)XMALLOC(dupl->signatureSz, heap, DYNAMIC_TYPE_CRL_ENTRY); - if (dupl->signature == NULL) { - FreeCRL_Entry(dupl, heap); - XFREE(dupl, heap, DYNAMIC_TYPE_CRL_ENTRY); + if (dupl->toBeSigned == NULL || dupl->signature == NULL) { + CRL_Entry_free(dupl, heap); return NULL; } XMEMCPY(dupl->toBeSigned, ent->toBeSigned, dupl->tbsSz); XMEMCPY(dupl->signature, ent->signature, dupl->signatureSz); - #ifndef NO_SKID - dupl->extAuthKeyIdSet = ent->extAuthKeyIdSet; - if (dupl->extAuthKeyIdSet) - XMEMCPY(dupl->extAuthKeyId, ent->extAuthKeyId, KEYID_SIZE); - #endif } else { dupl->toBeSigned = NULL; dupl->tbsSz = 0; dupl->signature = NULL; dupl->signatureSz = 0; +#if !defined(NO_SKID) && !defined(NO_ASN) + dupl->extAuthKeyIdSet = 0; +#endif } return dupl; @@ -781,33 +720,26 @@ static CRL_Entry* DupCRL_list(CRL_Entry* crl, void* heap) { CRL_Entry* current; CRL_Entry* head = NULL; - CRL_Entry* prev = NULL; + CRL_Entry** prev = &head; - current = crl; - while (current != NULL) { + for (current = crl; current != NULL; current = current->next) { CRL_Entry* tmp = DupCRL_Entry(current, heap); if (tmp != NULL) { - tmp->next = NULL; - if (head == NULL) - head = tmp; - if (prev != NULL) - prev->next = tmp; - prev = tmp; + *prev = tmp; + prev = &tmp->next; } else { WOLFSSL_MSG("Failed to allocate new CRL_Entry structure"); /* free up any existing list */ while (head != NULL) { - current = head; - head = head->next; - FreeCRL_Entry(current, heap); - XFREE(current, heap, DYNAMIC_TYPE_CRL_ENTRY); + CRL_Entry* next = head->next; + CRL_Entry_free(head, heap); + head = next; } - return NULL; } - current = current->next; } + return head; } @@ -860,7 +792,6 @@ static int DupX509_CRL(WOLFSSL_X509_CRL *dupl, const WOLFSSL_X509_CRL* crl) /* returns WOLFSSL_SUCCESS on success. Does not take ownership of newcrl */ int wolfSSL_X509_STORE_add_crl(WOLFSSL_X509_STORE *store, WOLFSSL_X509_CRL *newcrl) { - CRL_Entry *crle; WOLFSSL_X509_CRL *crl; WOLFSSL_ENTER("wolfSSL_X509_STORE_add_crl"); @@ -872,11 +803,16 @@ int wolfSSL_X509_STORE_add_crl(WOLFSSL_X509_STORE *store, WOLFSSL_X509_CRL *newc if (crl == NULL) { return WOLFSSL_FAILURE; } + if (wc_LockRwLock_Rd(&newcrl->crlLock) != 0) { + WOLFSSL_MSG("wc_LockRwLock_Rd failed"); + return BAD_MUTEX_E; + } if (DupX509_CRL(crl, newcrl) != 0) { if (crl != NULL) FreeCRL(crl, 1); return WOLFSSL_FAILURE; } + wc_UnLockRwLock(&newcrl->crlLock); store->crl = store->cm->crl = crl; if (wolfSSL_CertManagerEnableCRL(store->cm, WOLFSSL_CRL_CHECKALL) != WOLFSSL_SUCCESS) { @@ -888,26 +824,29 @@ int wolfSSL_X509_STORE_add_crl(WOLFSSL_X509_STORE *store, WOLFSSL_X509_CRL *newc /* find tail of current list and add new list */ crl = store->cm->crl; - crle = crl->crlList; if (newcrl->crlList != NULL) { - CRL_Entry *tail = crle; + CRL_Entry **tail; CRL_Entry *toAdd; - if (wc_LockMutex(&crl->crlLock) != 0) - { - WOLFSSL_MSG("wc_LockMutex failed"); + if (wc_LockRwLock_Wr(&crl->crlLock) != 0) { + WOLFSSL_MSG("wc_LockRwLock_Wr failed"); return BAD_MUTEX_E; } + if (crl != newcrl && wc_LockRwLock_Rd(&newcrl->crlLock) != 0) { + WOLFSSL_MSG("wc_LockRwLock_Wr failed"); + wc_UnLockRwLock(&crl->crlLock); + return BAD_MUTEX_E; + } toAdd = DupCRL_list(newcrl->crlList, crl->heap); - if (tail == NULL) { - crl->crlList = toAdd; - } - else { - while (tail->next != NULL) tail = tail->next; - tail->next = toAdd; - } - wc_UnLockMutex(&crl->crlLock); + if (crl != newcrl) + wc_UnLockRwLock(&newcrl->crlLock); + + tail = &crl->crlList; + while (*tail != NULL) + tail = &(*tail)->next; + *tail = toAdd; + wc_UnLockRwLock(&crl->crlLock); } if (wolfSSL_CertManagerEnableCRL(store->cm, WOLFSSL_CRL_CHECKALL) @@ -994,8 +933,8 @@ static int SwapLists(WOLFSSL_CRL* crl) } } - if (wc_LockMutex(&crl->crlLock) != 0) { - WOLFSSL_MSG("wc_LockMutex failed"); + if (wc_LockRwLock_Wr(&crl->crlLock) != 0) { + WOLFSSL_MSG("wc_LockRwLock_Wr failed"); FreeCRL(tmp, 0); #ifdef WOLFSSL_SMALL_STACK XFREE(tmp, NULL, DYNAMIC_TYPE_TMP_BUFFER); @@ -1009,7 +948,7 @@ static int SwapLists(WOLFSSL_CRL* crl) tmp->crlList = crl->crlList; crl->crlList = newList; - wc_UnLockMutex(&crl->crlLock); + wc_UnLockRwLock(&crl->crlLock); FreeCRL(tmp, 0); diff --git a/tests/api.c b/tests/api.c index d0ebd4fbd..565ecac16 100644 --- a/tests/api.c +++ b/tests/api.c @@ -57789,6 +57789,48 @@ static int test_wolfSSL_CTX_LoadCRL(void) return EXPECT_RESULT(); } +#if defined(HAVE_SSL_MEMIO_TESTS_DEPENDENCIES) && defined(HAVE_CRL) +static int test_multiple_crls_same_issuer_ctx_ready(WOLFSSL_CTX* ctx) +{ + EXPECT_DECLS; + wolfSSL_CTX_set_verify(ctx, WOLFSSL_VERIFY_PEER, NULL); + ExpectIntEQ(wolfSSL_CTX_LoadCRLFile(ctx, "./certs/crl/crl.pem", + WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); + return EXPECT_RESULT(); +} +#endif + +static int test_multiple_crls_same_issuer(void) +{ + EXPECT_DECLS; +#if defined(HAVE_SSL_MEMIO_TESTS_DEPENDENCIES) && defined(HAVE_CRL) + test_ssl_cbf client_cbs, server_cbs; + struct { + const char* server_cert; + const char* server_key; + } test_params[] = { + { "./certs/server-cert.pem", "./certs/server-key.pem" }, + { "./certs/server-revoked-cert.pem", "./certs/server-revoked-key.pem" } + }; + size_t i; + + for (i = 0; i < (sizeof(test_params)/sizeof(*test_params)); i++) { + XMEMSET(&client_cbs, 0, sizeof(client_cbs)); + XMEMSET(&server_cbs, 0, sizeof(server_cbs)); + + server_cbs.certPemFile = test_params[i].server_cert; + server_cbs.keyPemFile = test_params[i].server_key; + client_cbs.crlPemFile = "./certs/crl/extra-crls/general-server-crl.pem"; + + client_cbs.ctx_ready = test_multiple_crls_same_issuer_ctx_ready; + + ExpectIntEQ(test_wolfSSL_client_server_nofail_memio(&client_cbs, + &server_cbs, NULL), TEST_FAIL); + } +#endif + return EXPECT_RESULT(); +} + static int test_SetTmpEC_DHE_Sz(void) { EXPECT_DECLS; @@ -64901,7 +64943,10 @@ static int test_dtls_client_hello_timeout(void) static int test_certreq_sighash_algos(void) { EXPECT_DECLS; -#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + !defined(WOLFSSL_MAX_STRENGTH) && defined(HAVE_ECC) && \ + defined(WOLFSSL_SHA384) && defined(WOLFSSL_AES_256) && \ + defined(HAVE_AES_CBC) WOLFSSL_CTX *ctx_c = NULL; WOLFSSL_CTX *ctx_s = NULL; WOLFSSL *ssl_c = NULL; @@ -64911,7 +64956,8 @@ static int test_certreq_sighash_algos(void) int maxIdx = 0; XMEMSET(&test_ctx, 0, sizeof(test_ctx)); - test_ctx.c_ciphers = test_ctx.s_ciphers = "TLS_ECDHE_ECDSA_WITH_NULL_SHA:" + test_ctx.c_ciphers = test_ctx.s_ciphers = + "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA:" "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384"; ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); @@ -66048,6 +66094,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_wolfSSL_CTX_use_certificate_chain_file_format), TEST_DECL(test_wolfSSL_CTX_trust_peer_cert), TEST_DECL(test_wolfSSL_CTX_LoadCRL), + TEST_DECL(test_multiple_crls_same_issuer), TEST_DECL(test_wolfSSL_CTX_SetTmpDH_file), TEST_DECL(test_wolfSSL_CTX_SetTmpDH_buffer), TEST_DECL(test_wolfSSL_CTX_SetMinMaxDhKey_Sz), diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index a4b92c17d..08af5b48c 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -19247,7 +19247,7 @@ static int DecodeKeyUsage(const byte* input, word32 sz, DecodedCert* cert) #else ASNGetData dataASN[keyUsageASN_Length]; word32 idx = 0; - byte keyUsage[OPAQUE16_LEN]; + byte keyUsage[2]; word32 keyUsageSz = sizeof(keyUsage); int ret; WOLFSSL_ENTER("DecodeKeyUsage"); diff --git a/wolfssl/internal.h b/wolfssl/internal.h index c7777b5f2..a46e7c6ac 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -2464,7 +2464,14 @@ typedef struct CRL_Entry CRL_Entry; #endif /* Complete CRL */ struct CRL_Entry { + wolfSSL_Mutex verifyMutex; + byte* toBeSigned; + byte* signature; +#if defined(OPENSSL_EXTRA) + WOLFSSL_X509_NAME* issuer; /* X509_NAME type issuer */ +#endif CRL_Entry* next; /* next entry */ + /* DupCRL_Entry copies data after the `next` member */ byte issuerHash[CRL_DIGEST_SIZE]; /* issuer hash */ /* byte crlHash[CRL_DIGEST_SIZE]; raw crl data hash */ /* restore the hash here if needed for optimized comparisons */ @@ -2484,9 +2491,7 @@ struct CRL_Entry { int totalCerts; /* number on list */ int version; /* version of certificate */ int verified; - byte* toBeSigned; word32 tbsSz; - byte* signature; word32 signatureSz; word32 signatureOID; #if !defined(NO_SKID) && !defined(NO_ASN) @@ -2494,9 +2499,6 @@ struct CRL_Entry { byte extAuthKeyId[KEYID_SIZE]; #endif int crlNumber; /* CRL number extension */ -#if defined(OPENSSL_EXTRA) - WOLFSSL_X509_NAME* issuer; /* X509_NAME type issuer */ -#endif }; @@ -2534,7 +2536,7 @@ struct WOLFSSL_CRL { #ifdef HAVE_CRL_IO CbCrlIO crlIOCb; #endif - wolfSSL_Mutex crlLock; /* CRL list lock */ + wolfSSL_RwLock crlLock; /* CRL list lock */ CRL_Monitor monitors[WOLFSSL_CRL_MONITORS_LEN]; #ifdef HAVE_CRL_MONITOR COND_TYPE cond; /* condition to signal setup */ From b02fe0853fbaba78166c4bbebea73f15b9892f88 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 24 Aug 2023 14:34:14 +0200 Subject: [PATCH 4/4] CI fixes --- src/crl.c | 4 ++-- tests/api.c | 10 +++++----- wolfssl/internal.h | 6 ++++-- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/crl.c b/src/crl.c index bcafc309c..8c7630c05 100644 --- a/src/crl.c +++ b/src/crl.c @@ -666,8 +666,8 @@ static RevokedCert *DupRevokedCertList(RevokedCert* in, void* heap) static CRL_Entry* DupCRL_Entry(const CRL_Entry* ent, void* heap) { CRL_Entry *dupl; - const size_t copyOffset = OFFSETOF(CRL_Entry, next) + - sizeof(ent->next); + const size_t copyOffset = OFFSETOF(CRL_Entry, verifyMutex) + + sizeof(ent->verifyMutex); #ifdef CRL_STATIC_REVOKED_LIST if (ent->totalCerts > CRL_MAX_REVOKED_CERTS) { return NULL; diff --git a/tests/api.c b/tests/api.c index 565ecac16..ddb7b025f 100644 --- a/tests/api.c +++ b/tests/api.c @@ -366,7 +366,8 @@ defined(HAVE_SESSION_TICKET) || (defined(OPENSSL_EXTRA) && \ defined(WOLFSSL_CERT_EXT) && defined(WOLFSSL_CERT_GEN)) || \ defined(WOLFSSL_TEST_STATIC_BUILD) || defined(WOLFSSL_DTLS) || \ - defined(HAVE_ECH) || defined(HAVE_EX_DATA) || !defined(NO_SESSION_CACHE) + defined(HAVE_ECH) || defined(HAVE_EX_DATA) || !defined(NO_SESSION_CACHE) \ + || !defined(WOLFSSL_NO_TLS12) /* for testing SSL_get_peer_cert_chain, or SESSION_TICKET_HINT_DEFAULT, * for setting authKeyIdSrc in WOLFSSL_X509, or testing DTLS sequence * number tracking */ @@ -64946,7 +64947,7 @@ static int test_certreq_sighash_algos(void) #if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ !defined(WOLFSSL_MAX_STRENGTH) && defined(HAVE_ECC) && \ defined(WOLFSSL_SHA384) && defined(WOLFSSL_AES_256) && \ - defined(HAVE_AES_CBC) + defined(HAVE_AES_CBC) && !defined(WOLFSSL_NO_TLS12) WOLFSSL_CTX *ctx_c = NULL; WOLFSSL_CTX *ctx_s = NULL; WOLFSSL *ssl_c = NULL; @@ -64957,15 +64958,14 @@ static int test_certreq_sighash_algos(void) XMEMSET(&test_ctx, 0, sizeof(test_ctx)); test_ctx.c_ciphers = test_ctx.s_ciphers = - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA:" - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384"; + "ECDHE-ECDSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA384"; ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_c, "./certs/ca-ecc-cert.pem", NULL), WOLFSSL_SUCCESS); - wolfSSL_set_verify(ssl_s, SSL_VERIFY_PEER, NULL); + wolfSSL_set_verify(ssl_s, WOLFSSL_VERIFY_PEER, NULL); ExpectIntEQ(wolfSSL_use_PrivateKey_file(ssl_s, "./certs/ecc-key.pem", WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); ExpectIntEQ(wolfSSL_use_certificate_file(ssl_s, "./certs/server-ecc.pem", diff --git a/wolfssl/internal.h b/wolfssl/internal.h index a46e7c6ac..c983a39e1 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -2464,14 +2464,16 @@ typedef struct CRL_Entry CRL_Entry; #endif /* Complete CRL */ struct CRL_Entry { - wolfSSL_Mutex verifyMutex; byte* toBeSigned; byte* signature; #if defined(OPENSSL_EXTRA) WOLFSSL_X509_NAME* issuer; /* X509_NAME type issuer */ #endif CRL_Entry* next; /* next entry */ - /* DupCRL_Entry copies data after the `next` member */ + wolfSSL_Mutex verifyMutex; + /* DupCRL_Entry copies data after the `verifyMutex` member. Using the mutex + * as the marker because clang-tidy doesn't like taking the sizeof a + * pointer. */ byte issuerHash[CRL_DIGEST_SIZE]; /* issuer hash */ /* byte crlHash[CRL_DIGEST_SIZE]; raw crl data hash */ /* restore the hash here if needed for optimized comparisons */