Merge pull request #10218 from julek-wolfssl/zd/21535

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

Author: SparkiDev

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)
@@ -1455,7 +1466,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;
@@ -1470,6 +1481,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);
@@ -1518,6 +1532,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

@@ -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

@@ -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

@@ -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 */