fix: plug pkey/ca leak in CA setup error paths#266
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes memory leaks in wolfCLU_CASetup() error paths by ensuring CA cert (ca) and private key (pkey) are properly freed when ownership transfer to the signer does not occur.
Changes:
- NULL out
pkey/caimmediately after transferring ownership to the signer. - Unconditionally free
caandpkey(when non-NULL) in the cleanup block to cover pre-transfer error paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (selfSigned) { | ||
| wolfCLU_CertSignSetCA(signer, x509, pkey, | ||
| wolfCLU_GetTypeFromPKEY(pkey)); | ||
| pkey = NULL; /* ownership transferred to signer */ | ||
| } |
| else { | ||
| wolfCLU_CertSignSetCA(signer, ca, pkey, | ||
| wolfCLU_GetTypeFromPKEY(pkey)); | ||
| ca = NULL; /* ownership transferred to signer */ | ||
| pkey = NULL; /* ownership transferred to signer */ | ||
| } |
| if (selfSigned) { | ||
| wolfCLU_CertSignSetCA(signer, x509, pkey, | ||
| wolfCLU_GetTypeFromPKEY(pkey)); | ||
| pkey = NULL; /* ownership transferred to signer */ | ||
| } |
|
The problem is that assigning NULL outside wolfCLU_CertSignSetCA() splits ownership handling across two locations. If ownership is transferred inside wolfCLU_CertSignSetCA(), the corresponding cleanup should also be handled there. More generally, clearing variables in this manner can hide use-after-free and corruption bugs from Valgrind and similar tools. If project policy requires pointers to be set to NULL in release builds, this should be implemented through a macro that expands to the assignment in release builds but leaves the original value intact during development builds so diagnostic tools can detect invalid access. wolfSSL already distinguishes development and release builds through Autoconf: builds made from a Git checkout are treated differently from release tarballs produced by make dist. There appear to be several similar cases in this change. There is also no test case, so it is not clear what execution path triggers the issue or how the proposed change addresses it. |
Bug: In
wolfCLU_CASetup()(src/x509/clu_ca_setup.c), the localpkey(allocated bywolfSSL_PEM_read_bio_PrivateKey) and the localca(-CAcert) are transferred to the signer struct only inside theif (ret == WOLFCLU_SUCCESS && (pkey != NULL || ca != NULL || ...))block.wolfCLU_CertSignSetCAtakes ownership (see/* take ownership of ca or key passed in */in clu_x509_sign.c), andwolfCLU_CertSignFreefrees them.If
retgoes to an error state before that transfer block runs — e.g.wolfCLU_CertSignAppendOutfails, or the PEM/d2i X509_REQ read yields NULL — the transfer is skipped. The cleanup section then had nowolfSSL_EVP_PKEY_free(pkey)at all, and freedcaonly under the(selfSigned || altSign)guard, so the normal CA-signing error path leaked both.Fix: NULL out
pkey/caimmediately after each ownership transfer, then free them unconditionally (when non-NULL) in cleanup. A transferred pointer is NULL by the time cleanup runs, so it is freed bywolfCLU_CertSignFreeinstead — no double free. Pre-transfer error paths now free via cleanup.Double-free audit: normal success -> both NULL, freed by CertSignFree; selfSigned success -> pkey NULL (freed by signer), unused
-CAcert freed by cleanup; altSign -> pkey never allocated, ca borrowed then freed by cleanup; any pre-transfer error -> both freed by cleanup. No path frees twice.Out of scope (pre-existing):
wolfCLU_CertSignSetCAdefault keytype branch neither stores nor frees the key, so a non-RSA/ECDSAwolfCLU_GetTypeFromPKEYresult would orphan the key regardless of this change. A successfully PEM-read RSA/ECC key returns RSAk/ECDSAk, so this fix is unaffected.Build-verified: compiled the modified translation unit against a prebuilt wolfSSL (gcc -c, exit 0); preprocessed output confirms
wolfSSL_EVP_PKEY_free(pkey)is now present (was absent).Reported by static analysis (Fenrir finding 662).
[fenrir-sweep:held]