Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
julek-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
+115 −64
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/crl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 8 additions & 7 deletions src/keys.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down
139 changes: 83 additions & 56 deletions src/ssl_certman.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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. */
Expand All @@ -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) {
Expand All @@ -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;
Expand All @@ -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.
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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);
Expand Down
8 changes: 8 additions & 0 deletions wolfssl/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
Loading