Skip to content

Fix bugs found in crl.c, keys.c, and ssl_certman.c review#10218

Open
julek-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
julek-wolfssl:zd/21535
Open

Fix bugs found in crl.c, keys.c, and ssl_certman.c review#10218
julek-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
julek-wolfssl:zd/21535

Conversation

@julek-wolfssl
Copy link
Copy Markdown
Member

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:

  • 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.
  • wolfSSL_CertManagerNew_ex: on init failure, call DoCertManagerFree
    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.

@julek-wolfssl julek-wolfssl self-assigned this Apr 14, 2026
Copilot AI review requested due to automatic review settings April 14, 2026 12:39
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_SUITE for unknown TLS 1.3 / ECDHE_PSK / SM suite bytes, and fix misleading indentation in SetKeys.
  • 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants