wolfSSL/wolfssl
1
0
Fork 1
mirror of https://github.com/wolfSSL/wolfssl.git synced 2026-07-05 11:50:52 +02:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
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 commit is contained in:
Juliusz Sosinowicz
2026-04-14 11:57:48 +00:00
parent 9176185d66
commit 4a36d16b30
4 changed files with 115 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/crl.c
+16 -1
View File
@@ -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);
src/keys.c
+8 -7
View File
@@ -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);
}
src/ssl_certman.c
+83 -56
View File
@@ -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);
wolfssl/internal.h
+8
View File
@@ -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 */