Fix bugs found in crl.c, keys.c, and ssl_certman.c review#10218
Open
julek-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
Open
Fix bugs found in crl.c, keys.c, and ssl_certman.c review#10218julek-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
julek-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses several correctness issues across CRL handling, cipher suite parsing, and certificate manager lifetime management (primarily around error paths and resource cleanup).
Changes:
- CRL: fix NULL dereference, correct empty-list duplication behavior, plug a lock-failure leak, and propagate refcount init failures with proper cleanup.
- Cipher suites / key setup: remove redundant PSK flag assignment, return
UNSUPPORTED_SUITEfor unknown TLS 1.3 / ECDHE_PSK / SM suite bytes, and fix misleading indentation inSetKeys. - Cert manager: remove dead code in
AddTrustedPeer, fix heap usage on a free path, and refactor cert manager disposal into a helper to bypass refcount checks during init failure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ssl_certman.c | Refactors cert manager free logic and fixes cleanup paths in wolfSSL_CertManagerNew_ex / AddTrustedPeer. |
| src/keys.c | Improves cipher suite default handling and cleans up minor logic/indentation issues in key setup. |
| src/crl.c | Strengthens CRL duplication and store-add behavior, especially on error paths and NULL inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crl.c: - wolfSSL_X509_CRL_dup: add NULL check on input before dereferencing crl->cm - DupX509_CRL: distinguish empty source CRL list from allocation failure so duplicating a CRL with no entries no longer returns MEMORY_E - wolfSSL_X509_STORE_add_crl: free newly-allocated CRL when wc_LockRwLock_Rd fails to avoid leaking it - InitCRL: propagate wolfSSL_RefInit failure in OPENSSL_ALL + WOLFSSL_REFCNT_ERROR_RETURN builds, freeing crlLock (and cond when HAVE_CRL_MONITOR is enabled) on the error path keys.c: - GetCipherSpec: remove duplicate usingPSK_cipher assignment in BUILD_TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA256 case - GetCipherSpec: return UNSUPPORTED_SUITE for unknown cipher suite bytes in the TLS13_BYTE, ECDHE_PSK_BYTE, and SM_BYTE switch blocks, matching the behavior of the ECC_BYTE, CHACHA_BYTE, and normal suite switches - SetKeys: fix misleading indentation on the AESCCM and SM4-CCM dec->aes NULL-check return statements ssl_certman.c / internal.h: - AddTrustedPeer: remove dead code that checked peerCert->permittedNames and peerCert->excludedNames immediately after XMEMSET zeroed the struct - AddTrustedPeer: use cm->heap (matching allocation) instead of NULL when freeing cert on the ParseCert failure path - Extract the body of wolfSSL_CertManagerFree into a new static helper DoCertManagerFree that unconditionally disposes of the certificate manager, bypassing the reference count check. wolfSSL_CertManagerFree now delegates to it after the RefDec check. - Add caLockInit, tpLockInit, and refInit bitfield members to WOLFSSL_CERT_MANAGER that track which sub-resources were successfully initialized. DoCertManagerFree consults these flags so that it only destroys mutexes and the reference count that were actually set up, which makes partial-construction cleanup safe without relying on platform-specific behavior of free-on-zeroed-storage. - wolfSSL_CertManagerNew_ex: set the init flags as each sub-resource is initialized, and on failure call DoCertManagerFree directly to free exactly the resources that succeeded. Set cm->heap immediately after XMEMSET so the forceful free path can use it.
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.
crl.c:
duplicating a CRL with no entries no longer returns MEMORY_E
fails to avoid leaking it
WOLFSSL_REFCNT_ERROR_RETURN builds, freeing crlLock (and cond when
HAVE_CRL_MONITOR is enabled) on the error path
keys.c:
BUILD_TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA256 case
the TLS13_BYTE, ECDHE_PSK_BYTE, and SM_BYTE switch blocks, matching the
behavior of the ECC_BYTE, CHACHA_BYTE, and normal suite switches
NULL-check return statements
ssl_certman.c:
and peerCert->excludedNames immediately after XMEMSET zeroed the struct
freeing cert on the ParseCert failure path
DoCertManagerFree that unconditionally disposes of the certificate
manager, bypassing the reference count check. wolfSSL_CertManagerFree
now delegates to it after the RefDec check.
directly to avoid leaking cm when the reference count was never
initialized or failed to initialize. Set cm->heap immediately after
XMEMSET so the forceful free path can use it.