Repository navigation
Conversation
After a handshake, ProcessPeerCerts() adds each verified CA from the peer's chain to the certificate manager as a WOLFSSL_CHAIN_CA. The Signer kept no validity dates, so a later peer that sent only its leaf was verified with the cached CA even after that CA had expired. Sending the expired CA, or using a new context, failed with ASN_AFTER_DATE_E. Keep the DER encoded notBefore and notAfter of a chain CA in its Signer, and after parsing each peer certificate walk from its issuer up to the self-signed trust anchor, checking the dates of every chain CA on the way. The error is reported as ASN_BEFORE_DATE_E or ASN_AFTER_DATE_E on that certificate, so a verify callback can accept it as before. CAs the application loaded are trust anchors and are not checked again. The walk uses the name constraints issuer lookup, exported as wolfssl_local_FindSignerByAkidOrName(). With IGNORE_NAME_CONSTRAINTS only the direct issuer is checked. A CA renewed with the same key is not added again, so the peer sending it now updates the dates of the cached CA. Otherwise the cached copy would fail a chain that includes a valid renewed CA. wolfSSL_CertManagerVerifyBuffer() checks cached chain CAs the same way, and wolfSSL_CertManagerLoadCABufferType() stores the dates of a CA loaded as WOLFSSL_CHAIN_CA. Add test_wolfSSL_chain_ca_cached_dates, which reuses one client context for a full chain, a leaf alone before and after the CA expires, a verify callback accepting the error and a renewed CA, and checks certificate manager verification.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical flaws allow unauthorized or failed-handshake updates to extend cached CA validity, bypass issuer expiry checks, and introduce a data race in shared CA state.
7 open issues
What changed in this PR
This PR adds validity checks for cached chain CAs in TLS and certificate-manager verification, aiming to make verification consistent whether peers send their intermediates or omit them.
Changes:
- Store validity dates on cached chain signers and check them along the issuer path.
- Refresh cached dates when a verified CA renewal uses the same key.
- Add regression tests for expiration, callback overrides, renewal, and application-loaded trust anchors.
| File | Description |
|---|---|
wolfssl/wolfcrypt/asn.h |
Defines signer validity storage and shared helper declarations. |
wolfssl/internal.h |
Declares cached-CA date-check and update helpers. |
wolfcrypt/src/asn.c |
Stores and frees signer dates; exposes issuer lookup. |
tests/api/test_ssl_cert.h |
Registers the cached-CA validity test. |
tests/api/test_ssl_cert.c |
Tests cached expiration, renewal, and verification behavior. |
src/ssl_certman.c |
Checks cached issuer dates and updates stored validity. |
src/internal.c |
Integrates date checks and renewal updates into handshakes. |
Review effort: Balanced
Comment on lines
+17558
to
+17560
| if (ret == 0 && (verify == VERIFY || verify == VERIFY_NAME)) { | ||
| ret = wolfssl_local_CheckChainCADates(SSL_CM(ssl), args->dCert->ca, | ||
| extraSigners); |
Comment on lines
+19674
to
+19675
| ret = wolfssl_local_SetChainCADates(SSL_CM(ssl), | ||
| subjectHash, args->dCert); |
Comment on lines
+2862
to
+2873
| isChainCA = 0; | ||
| if (ca->validity != NULL) { | ||
| /* Another handshake can change the dates, and the application | ||
| * the type. */ | ||
| if (wc_LockMutex(&cm->caLock) != 0) { | ||
| ret = BAD_MUTEX_E; | ||
| break; | ||
| } | ||
| isChainCA = (ca->type == WOLFSSL_CHAIN_CA); | ||
| XMEMCPY(&validity, ca->validity, sizeof(validity)); | ||
| wc_UnLockMutex(&cm->caLock); | ||
| } |
Comment on lines
+2956
to
+2960
| if (signer != NULL && signer->type == WOLFSSL_CHAIN_CA && | ||
| cert->publicKey != NULL && signer->publicKey != NULL && | ||
| signer->pubKeySize == cert->pubKeySize && | ||
| XMEMCMP(signer->publicKey, cert->publicKey, | ||
| cert->pubKeySize) == 0) { |
Comment on lines
+17558
to
+17560
| if (ret == 0 && (verify == VERIFY || verify == VERIFY_NAME)) { | ||
| ret = wolfssl_local_CheckChainCADates(SSL_CM(ssl), args->dCert->ca, | ||
| extraSigners); |
Comment on lines
+19670
to
+19674
| else if (caVerifyRet == 0 && subjectHash != NULL) { | ||
| /* A CA renewed with the same key is not added | ||
| * again. Take its dates so the cached CA does | ||
| * not expire with the old certificate. */ | ||
| ret = wolfssl_local_SetChainCADates(SSL_CM(ssl), |
Comment on lines
+2890
to
+2893
| if (ca->selfSigned) | ||
| break; | ||
| #ifndef IGNORE_NAME_CONSTRAINTS | ||
| parent = wolfssl_local_FindSignerByAkidOrName(cm, pending, ca); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
After a handshake,
ProcessPeerCerts()adds each verified CA from the peer's chain to the certificate manager as aWOLFSSL_CHAIN_CA. The cachedSignerkept no validity dates. A later peer that sent only its leaf was then verified with the cached CA, and only the leaf's own dates were checked. The same chain was rejected withASN_AFTER_DATE_Ewhen the expired CA was sent, or when a new context was used, and accepted when the CA was omitted.This change makes the result independent of whether the CA is sent:
Signergains avaliditypointer that holds the DER encodednotBeforeandnotAfter. It is allocated only for aWOLFSSL_CHAIN_CA, so CAs the application loads cost one pointer each.wolfssl_local_CheckChainCADates()walks from the issuer of each peer certificate up to the self-signed trust anchor and checks the dates of every chain CA on the way. It runs inProcessPeerCertParse()after the certificate is parsed, so the error is reported asASN_BEFORE_DATE_EorASN_AFTER_DATE_Eon that certificate and goes through the verify callback like any other date error.wc_AsnSetSkipDateCheck()turns it off as it does the other date checks.WOLFSSL_USER_CA,WOLFSSL_USER_INTER) are trust anchors and are not checked again, as before. A chain CA retyped withwolfSSL_CertManagerLoadCABufferType()follows its new type.wolfssl_local_FindSignerByAkidOrName(), with the same cycle guard andWOLFSSL_MAX_CHAIN_DEPTHlimit. WithIGNORE_NAME_CONSTRAINTSonly the direct issuer is checked. Certificate Status Request v2 pending signers are searched too.AlreadySigner()keeps the old cached copy. When the peer sends a CA that is already cached as a chain CA with the same public key and it verifies,wolfssl_local_SetChainCADates()copies its dates onto the cached CA. Without this, a client whose cached CA expired would also reject a peer that sends a valid renewed CA.caLock, because another handshake can update them.wolfSSL_CertManagerVerifyBuffer()checks cached chain CAs the same way.wolfSSL_CertManagerLoadCABufferType()stores the dates of a CA loaded asWOLFSSL_CHAIN_CA, for example one saved through the CA cache callback.Notes for review:
sizeof(Signer)changes, so aPERSIST_CERT_CACHEfile saved before this change is rejected on restore, as with anySignerlayout change. A restored signer has no type or dates and is not checked, as today. Keep CA restrictions when restoring the persisted cert cache #11875 rebuilds each CA from its DER.wolfssl_local_FindSignerByAkidOrName()with an identical hunk, so the two merge cleanly in either order.Testing
New
test_wolfSSL_chain_ca_cached_datesgenerates a CA that expires before its leaf and moves the clock withwc_SetTimeCb(). One client context is reused across handshakes:ASN_AFTER_DATE_E, as beforeASN_AFTER_DATE_E(accepted without the library change)wolfSSL_CertManagerVerifyBuffer()with the CA loaded asWOLFSSL_CHAIN_CAat T1 and T2, and asWOLFSSL_USER_CAat T2The new test fails without the library change at the leaf-alone T2 case (
0 != -151). With the dates update disabled, it fails at the renewed CA case.Built with
-Werrorand ran the new test,test_wolfSSL_chain_ca_ext_key_usage,test_wolfSSL_trust_peer_cert_datesandtest_various_pathlen_chainswith: the default flags plus--enable-certgen --enable-certext;--enable-opensslextrawith-Wall -Wextra -Wdeclaration-after-statement -Wshadow -Wpedantic;IGNORE_NAME_CONSTRAINTS;--enable-asn=original;WOLFSSL_SMALL_CERT_VERIFY;NO_ASN_TIME;NO_SKID; OCSP, stapling v1 and v2 with CRL;PERSIST_CERT_CACHE;--enable-singlethreaded;--enable-c89; and--enable-all.--enable-cryptonlybuilds. The full unit test passes in the default build, and the full--apisuite passes with--enable-all.With
NO_SKID,wolfSSL_CertManagerLoadCABufferType()cannot find the CA to retype, so the twoWOLFSSL_CHAIN_CAcertificate manager checks are skipped in that build.test_wolfSSL_chain_ca_ext_key_usagealready fails on master withNO_SKID, and this change does not affect it.Tested on macOS arm64 (Apple clang).
Checklist