From 4a36d16b30ca77151c4e699733a24334fd60df8a Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Tue, 14 Apr 2026 11:57:48 +0000 Subject: [PATCH] Fix bugs found in crl.c, keys.c, and ssl_certman.c review 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. --- src/crl.c | 17 +++++- src/keys.c | 15 ++--- src/ssl_certman.c | 139 +++++++++++++++++++++++++++------------------ wolfssl/internal.h | 8 +++ 4 files changed, 115 insertions(+), 64 deletions(-) diff --git a/src/crl.c b/src/crl.c index 62c207de526..62e2af98d4a 100644 --- a/src/crl.c +++ b/src/crl.c @@ -93,7 +93,18 @@ int InitCRL(WOLFSSL_CRL* crl, WOLFSSL_CERT_MANAGER* cm) { int ret; wolfSSL_RefInit(&crl->ref, &ret); + #ifdef WOLFSSL_REFCNT_ERROR_RETURN + if (ret != 0) { + WOLFSSL_MSG("wolfSSL_RefInit failed"); + wc_FreeRwLock(&crl->crlLock); + #ifdef HAVE_CRL_MONITOR + wolfSSL_CondFree(&crl->cond); + #endif + return ret; + } + #else (void)ret; + #endif } #endif #if defined(OPENSSL_EXTRA) @@ -1451,7 +1462,7 @@ static int DupX509_CRL(WOLFSSL_X509_CRL *dupl, const WOLFSSL_X509_CRL* crl) #endif dupl->crlList = DupCRL_list(crl->crlList, dupl->heap); - if (dupl->crlList == NULL) + if (crl->crlList != NULL && dupl->crlList == NULL) return MEMORY_E; #ifdef HAVE_CRL_IO dupl->crlIOCb = crl->crlIOCb; @@ -1466,6 +1477,9 @@ WOLFSSL_X509_CRL* wolfSSL_X509_CRL_dup(const WOLFSSL_X509_CRL* crl) WOLFSSL_ENTER("wolfSSL_X509_CRL_dup"); + if (crl == NULL) + return NULL; + ret = wolfSSL_X509_crl_new(crl->cm); if (ret != NULL && DupX509_CRL(ret, crl) != 0) { FreeCRL(ret, 1); @@ -1514,6 +1528,7 @@ int wolfSSL_X509_STORE_add_crl(WOLFSSL_X509_STORE *store, WOLFSSL_X509_CRL *newc } if (wc_LockRwLock_Rd(&newcrl->crlLock) != 0) { WOLFSSL_MSG("wc_LockRwLock_Rd failed"); + FreeCRL(crl, 1); return BAD_MUTEX_E; } ret = DupX509_CRL(crl, newcrl); diff --git a/src/keys.c b/src/keys.c index 190a5d4dd3d..7f70a733c1c 100644 --- a/src/keys.c +++ b/src/keys.c @@ -525,8 +525,6 @@ int GetCipherSpec(word16 side, byte cipherSuite0, byte cipherSuite, specs->block_size = WC_AES_BLOCK_SIZE; specs->iv_size = AES_IV_SIZE; - if (opts != NULL) - opts->usingPSK_cipher = 1; if (opts != NULL) opts->usingPSK_cipher = 1; break; @@ -1374,7 +1372,8 @@ int GetCipherSpec(word16 side, byte cipherSuite0, byte cipherSuite, #endif #endif /* WOLFSSL_TLS13 */ default: - break; + WOLFSSL_MSG("Unsupported cipher suite, SetCipherSpecs TLS 1.3"); + return UNSUPPORTED_SUITE; } } @@ -1405,7 +1404,8 @@ int GetCipherSpec(word16 side, byte cipherSuite0, byte cipherSuite, #endif default: - break; + WOLFSSL_MSG("Unsupported cipher suite, SetCipherSpecs ECDHE_PSK"); + return UNSUPPORTED_SUITE; } } @@ -1466,7 +1466,8 @@ int GetCipherSpec(word16 side, byte cipherSuite0, byte cipherSuite, #endif default: - break; + WOLFSSL_MSG("Unsupported cipher suite, SetCipherSpecs SM"); + return UNSUPPORTED_SUITE; } } @@ -2799,7 +2800,7 @@ int SetKeys(Ciphers* enc, Ciphers* dec, Keys* keys, CipherSpecs* specs, if (dec->aes == NULL) { dec->aes = (Aes*)XMALLOC(sizeof(Aes), heap, DYNAMIC_TYPE_CIPHER); if (dec->aes == NULL) - return MEMORY_E; + return MEMORY_E; } else { wc_AesFree(dec->aes); } @@ -3247,7 +3248,7 @@ int SetKeys(Ciphers* enc, Ciphers* dec, Keys* keys, CipherSpecs* specs, dec->sm4 = (wc_Sm4*)XMALLOC(sizeof(wc_Sm4), heap, DYNAMIC_TYPE_CIPHER); if (dec->sm4 == NULL) - return MEMORY_E; + return MEMORY_E; } else { wc_Sm4Free(dec->sm4); } diff --git a/src/ssl_certman.c b/src/ssl_certman.c index 00acd003714..e4bad5400b8 100644 --- a/src/ssl_certman.c +++ b/src/ssl_certman.c @@ -74,6 +74,8 @@ static WC_INLINE WOLFSSL_METHOD* cm_pick_method(void* heap) #endif } +static void DoCertManagerFree(WOLFSSL_CERT_MANAGER* cm); + /* Create a new certificate manager with a heap hint. * * @param [in] heap Heap hint. @@ -107,12 +109,17 @@ WOLFSSL_CERT_MANAGER* wolfSSL_CertManagerNew_ex(void* heap) if (!err) { /* Reset all fields. */ XMEMSET(cm, 0, sizeof(WOLFSSL_CERT_MANAGER)); + /* Set heap hint early so cleanup can use it. */ + cm->heap = heap; /* Create a mutex for use when modify table of stored CAs. */ if (wc_InitMutex(&cm->caLock) != 0) { WOLFSSL_MSG("Bad mutex init"); err = 1; } + else { + cm->caLockInit = 1; + } } if (!err) { /* Initialize reference count. */ @@ -121,13 +128,23 @@ WOLFSSL_CERT_MANAGER* wolfSSL_CertManagerNew_ex(void* heap) if (err != 0) { WOLFSSL_MSG("Bad reference count init"); } + else { + cm->refInit = 1; + } + #else + cm->refInit = 1; #endif } #ifdef WOLFSSL_TRUST_PEER_CERT - /* Create a mutex for use when modify table of trusted peers. */ - if ((!err) && (wc_InitMutex(&cm->tpLock) != 0)) { - WOLFSSL_MSG("Bad mutex init"); - err = 1; + if (!err) { + /* Create a mutex for use when modify table of trusted peers. */ + if (wc_InitMutex(&cm->tpLock) != 0) { + WOLFSSL_MSG("Bad mutex init"); + err = 1; + } + else { + cm->tpLockInit = 1; + } } #endif if (!err) { @@ -144,14 +161,12 @@ WOLFSSL_CERT_MANAGER* wolfSSL_CertManagerNew_ex(void* heap) #ifdef HAVE_DILITHIUM cm->minDilithiumKeySz = MIN_DILITHIUMKEY_SZ; #endif /* HAVE_DILITHIUM */ - - /* Set heap hint to use in certificate manager operations. */ - cm->heap = heap; } - /* Dispose of certificate manager on error. */ + /* Dispose of certificate manager on error. The reference count may not + * have been initialized, so bypass the ref check and free directly. */ if (err && (cm != NULL)) { - wolfSSL_CertManagerFree(cm); + DoCertManagerFree(cm); cm = NULL; } return cm; @@ -168,6 +183,63 @@ WOLFSSL_CERT_MANAGER* wolfSSL_CertManagerNew(void) return wolfSSL_CertManagerNew_ex(NULL); } +/* Unconditionally dispose of all resources owned by the certificate manager + * and free cm itself, bypassing any reference count check. Only frees the + * sub-resources that are marked as initialized in the cm bitfield, so it is + * safe to call on a cm that was only partially initialized by + * wolfSSL_CertManagerNew_ex. + * + * @param [in, out] cm Certificate manager (must be non-NULL). + */ +static void DoCertManagerFree(WOLFSSL_CERT_MANAGER* cm) +{ +#ifdef HAVE_CRL + /* Dispose of CRL handler. */ + if (cm->crl != NULL) { + /* Dispose of CRL object - indicating dynamically allocated. */ + FreeCRL(cm->crl, 1); + } +#endif + +#ifdef HAVE_OCSP + /* Dispose of OCSP handler. */ + if (cm->ocsp != NULL) { + FreeOCSP(cm->ocsp, 1); + } + /* Dispose of URL. */ + XFREE(cm->ocspOverrideURL, cm->heap, DYNAMIC_TYPE_URL); +#if !defined(NO_WOLFSSL_SERVER) && \ + (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ + defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2)) + /* Dispose of OCSP stapling handler. */ + if (cm->ocsp_stapling) { + FreeOCSP(cm->ocsp_stapling, 1); + } +#endif +#endif /* HAVE_OCSP */ + + /* Dispose of CA table and mutex. */ + FreeSignerTable(cm->caTable, CA_TABLE_SIZE, cm->heap); + if (cm->caLockInit) { + wc_FreeMutex(&cm->caLock); + } + +#ifdef WOLFSSL_TRUST_PEER_CERT + /* Dispose of trusted peer table and mutex. */ + FreeTrustedPeerTable(cm->tpTable, TP_TABLE_SIZE, cm->heap); + if (cm->tpLockInit) { + wc_FreeMutex(&cm->tpLock); + } +#endif + + /* Dispose of reference count. */ + if (cm->refInit) { + wolfSSL_RefFree(&cm->ref); + } + /* Dispose of certificate manager memory. */ + XFREE(cm, cm->heap, DYNAMIC_TYPE_CERT_MANAGER); +} + /* Dispose of certificate manager. * * @param [in, out] cm Certificate manager. @@ -191,45 +263,7 @@ void wolfSSL_CertManagerFree(WOLFSSL_CERT_MANAGER* cm) (void)ret; #endif if (doFree) { - #ifdef HAVE_CRL - /* Dispose of CRL handler. */ - if (cm->crl != NULL) { - /* Dispose of CRL object - indicating dynamically allocated. */ - FreeCRL(cm->crl, 1); - } - #endif - - #ifdef HAVE_OCSP - /* Dispose of OCSP handler. */ - if (cm->ocsp != NULL) { - FreeOCSP(cm->ocsp, 1); - } - /* Dispose of URL. */ - XFREE(cm->ocspOverrideURL, cm->heap, DYNAMIC_TYPE_URL); - #if !defined(NO_WOLFSSL_SERVER) && \ - (defined(HAVE_CERTIFICATE_STATUS_REQUEST) || \ - defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2)) - /* Dispose of OCSP stapling handler. */ - if (cm->ocsp_stapling) { - FreeOCSP(cm->ocsp_stapling, 1); - } - #endif - #endif /* HAVE_OCSP */ - - /* Dispose of CA table and mutex. */ - FreeSignerTable(cm->caTable, CA_TABLE_SIZE, cm->heap); - wc_FreeMutex(&cm->caLock); - - #ifdef WOLFSSL_TRUST_PEER_CERT - /* Dispose of trusted peer table and mutex. */ - FreeTrustedPeerTable(cm->tpTable, TP_TABLE_SIZE, cm->heap); - wc_FreeMutex(&cm->tpLock); - #endif - - /* Dispose of reference count. */ - wolfSSL_RefFree(&cm->ref); - /* Dispose of certificate manager memory. */ - XFREE(cm, cm->heap, DYNAMIC_TYPE_CERT_MANAGER); + DoCertManagerFree(cm); } } } @@ -2859,7 +2893,7 @@ int AddTrustedPeer(WOLFSSL_CERT_MANAGER* cm, DerBuffer** pDer, int verify) InitDecodedCert(cert, der->buffer, der->length, cm->heap); if ((ret = ParseCert(cert, TRUSTED_PEER_TYPE, verify, cm)) != 0) { FreeDecodedCert(cert); - XFREE(cert, NULL, DYNAMIC_TYPE_DCERT); + XFREE(cert, cm->heap, DYNAMIC_TYPE_DCERT); FreeDer(&der); return ret; } @@ -2875,13 +2909,6 @@ int AddTrustedPeer(WOLFSSL_CERT_MANAGER* cm, DerBuffer** pDer, int verify) } XMEMSET(peerCert, 0, sizeof(TrustedPeerCert)); - #ifndef IGNORE_NAME_CONSTRAINTS - if (peerCert->permittedNames) - FreeNameSubtrees(peerCert->permittedNames, cm->heap); - if (peerCert->excludedNames) - FreeNameSubtrees(peerCert->excludedNames, cm->heap); - #endif - if (AlreadyTrustedPeer(cm, cert)) { WOLFSSL_MSG("\tAlready have this CA, not adding again"); FreeTrustedPeer(peerCert, cm->heap); diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 822c0722f67..f27d2344073 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -2647,6 +2647,14 @@ struct WOLFSSL_CERT_MANAGER { || defined(HAVE_CERTIFICATE_STATUS_REQUEST_V2) byte ocspMustStaple:1; /* server must respond with staple */ #endif + /* Tracks which resources were successfully initialized so that + * DoCertManagerFree can dispose of them safely even when construction + * fails partway through. */ + WC_BITFIELD caLockInit:1; /* caLock has been initialized */ +#ifdef WOLFSSL_TRUST_PEER_CERT + WC_BITFIELD tpLockInit:1; /* tpLock has been initialized */ +#endif + WC_BITFIELD refInit:1; /* ref has been initialized */ #ifndef NO_RSA short minRsaKeySz; /* minimum allowed RSA key size */