x509: harden wolfSSL_X509_verify_cert() against alloc failure and stack pollution

Robustness fixes in the OpenSSL-compatibility certificate verifier, independent
of the depth-exhaustion fix:

- Fail closed on allocation failure. When the failedCerts working stack could
  not be allocated, the function fell through to exit with ret still set to
  WOLFSSL_SUCCESS and reported the chain as verified without checking anything
  (a fail-open regression from the leak fix that turned the early return into a
  goto exit). Also check the ctx->chain allocation. Both now set an error.

- Remove caller-supplied intermediates from the correct stack. The intermediates
  appended to the working cert list during chain building were popped from
  ctx->store->certs by count, but they are appended to whichever stack is in use
  - which may be the caller's setTrustedSk (X509_STORE_CTX_set0_trusted_stack).
  Remove them by pointer identity from that same stack, recomputed from
  ctxIntermediates. Identity removal also survives the chain-building retries
  that reorder the stack, where a positional pop could drop a legitimate trusted
  entry and leave an injected intermediate behind - which a later verification
  reusing the store/ctx would then snapshot as a trust anchor. The removal helper
  walks the list once (O(n)) rather than indexing per position.

- NULL-guard ctx->store->param before dereferencing its flags in the
  partial-chain check.

Add regression tests covering: the trusted stack being restored after
verification, and the retry path (tampered plus genuine same-subject
intermediates, both orderings) leaving the store clean for later use.

Author: Frauschi

src/x509_str.c

@@ -531,6 +531,36 @@ static int X509StoreMoveCert(WOLFSSL_STACK *certs_stack,
     return WOLFSSL_FAILURE;
 }
 
+/* Remove the first node referencing `cert` (by pointer identity) from `stack`.
+ * The certificate object itself is not freed - the stack only holds a borrowed
+ * reference. Returns WOLFSSL_SUCCESS if a node was removed, WOLFSSL_FAILURE if
+ * `cert` was not present, or WOLFSSL_FATAL_ERROR if `stack`/`cert` is NULL.
+ * The only caller performs best-effort cleanup and intentionally ignores the
+ * return value.
+ *
+ * Walks the linked list once (O(n)) rather than indexing with
+ * wolfSSL_sk_X509_value() per position (which would re-walk from the head each
+ * time, O(n^2)). */
+static int X509StoreRemoveCert(WOLFSSL_STACK *stack, WOLFSSL_X509 *cert) {
+    WOLFSSL_STACK* node;
+    int idx;
+    int num;
+
+    if (stack == NULL || cert == NULL)
+        return WOLFSSL_FATAL_ERROR;
+
+    num = wolfSSL_sk_X509_num(stack);
+    for (node = stack, idx = 0; idx < num && node != NULL;
+            node = node->next, idx++) {
+        if (node->data.x509 == cert) {
+            (void)wolfSSL_sk_pop_node(stack, idx);
+            return WOLFSSL_SUCCESS;
+        }
+    }
+
+    return WOLFSSL_FAILURE;
+}
+
 
 /* Current certificate failed, but it is possible there is an
  * alternative cert with the same subject key which will work.
@@ -608,7 +638,6 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
     int done = 0;
     int added = 0;
     int i = 0;
-    int numInterAdd = 0;
     int numFailedCerts = 0;
     int depth = 0;
     int origDepth = 0;
@@ -666,8 +695,9 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
             }
         }
         /* Add the intermediates provided on init to the list of untrusted
-         * intermediates to be used */
-        ret = addAllButSelfSigned(certs, ctx->ctxIntermediates, &numInterAdd);
+         * intermediates to be used.  They are removed again from `certs` in the
+         * exit cleanup (by identity, recomputed from ctxIntermediates). */
+        ret = addAllButSelfSigned(certs, ctx->ctxIntermediates, NULL);
     }
     if (ret != WOLFSSL_SUCCESS) {
         goto exit;
@@ -677,10 +707,19 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
         wolfSSL_sk_X509_free(ctx->chain);
     }
     ctx->chain = wolfSSL_sk_X509_new_null();
+    if (ctx->chain == NULL) {
+        ret = WOLFSSL_FAILURE;
+        goto exit;
+    }
 
     failedCerts = wolfSSL_sk_X509_new_null();
-    if (!failedCerts)
+    if (!failedCerts) {
+        /* Fail closed: ret is still WOLFSSL_SUCCESS from the checks above, so
+         * an unset error here would make the function report a verified chain
+         * after an allocation failure. */
+        ret = WOLFSSL_FAILURE;
         goto exit;
+    }
 
     if (ctx->depth > 0) {
         depth = ctx->depth + 1;
@@ -779,7 +818,8 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
                  * chains that never reach a trust anchor.  Verify that
                  * ctx->current_cert is itself in the original trust set. */
                 if (((ctx->flags & WOLFSSL_PARTIAL_CHAIN) ||
-                     (ctx->store->param->flags & WOLFSSL_PARTIAL_CHAIN)) &&
+                     (ctx->store->param != NULL &&
+                      (ctx->store->param->flags & WOLFSSL_PARTIAL_CHAIN))) &&
                     X509StoreCertIsTrusted(ctx->store, ctx->current_cert,
                         origTrustedSk)) {
                     wolfSSL_sk_X509_push(ctx->chain, ctx->current_cert);
@@ -847,10 +887,30 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
     }
     wolfSSL_sk_X509_pop_free(failedCerts, NULL);
 
-    /* Remove additional intermediates from init from the store */
-    if (ctx != NULL && numInterAdd > 0) {
-        for (i = 0; i < numInterAdd; i++) {
-            wolfSSL_sk_X509_pop(ctx->store->certs);
+    /* Remove the caller-supplied intermediates that addAllButSelfSigned
+     * appended to `certs` during chain building, restoring it to its original
+     * contents.  Remove them by pointer identity from the same stack they were
+     * added to (store->certs in the common case, or the caller's setTrustedSk
+     * via X509_STORE_CTX_set0_trusted_stack), recomputed from ctxIntermediates
+     * with the same self-signed filter as the add.
+     *
+     * Identity removal - not a saved count + positional pop - is required:
+     * X509VerifyCertSetupRetry reorders `certs` during chain building, so
+     * popping N entries off the top could drop a legitimate trusted entry and
+     * leave an injected intermediate behind, which a later verification reusing
+     * this store/ctx would then snapshot as a trust anchor.  certsToUse is the
+     * throwaway certs==NULL path and is freed wholesale below, so skip it. */
+    if (ctx != NULL && certsToUse == NULL && certs != NULL &&
+            ctx->ctxIntermediates != NULL) {
+        int n = wolfSSL_sk_X509_num(ctx->ctxIntermediates);
+        for (i = 0; i < n; i++) {
+            WOLFSSL_X509* inter =
+                wolfSSL_sk_X509_value(ctx->ctxIntermediates, i);
+            if (inter != NULL &&
+                    wolfSSL_X509_NAME_cmp(&inter->issuer, &inter->subject)
+                        != 0) {
+                X509StoreRemoveCert(certs, inter);
+            }
         }
     }
     /* Remove intermediates that were added to CM */

tests/api/test_ossl_x509_str.c

@@ -1301,6 +1301,129 @@ static int test_untrusted_inter_depth_exhaustion(X509* leafDeep, X509* inter,
     sk_X509_free(untrusted);
     return EXPECT_RESULT();
 }
+
+/* Intermediate-stack cleanup: the caller-supplied intermediates that the
+ * verifier temporarily appends to its working cert list must be removed from
+ * the exact stack they were added to once verification finishes.  When a
+ * trusted_stack is in use (X509_STORE_CTX_set0_trusted_stack), they are
+ * appended to that caller-owned stack; if they are not removed again, a later
+ * verification reusing the stack/ctx would snapshot them as trust anchors.
+ *
+ *     leaf <- int-ca <- root, with root supplied via the trusted_stack.
+ *
+ * Verify the chain (which reaches root in the trusted stack), then assert the
+ * trusted stack is left exactly as the caller supplied it: only root, with the
+ * injected intermediate removed again. */
+static int test_untrusted_inter_trusted_stack_cleanup(X509* leaf, X509* inter,
+    X509* root)
+{
+    EXPECT_DECLS;
+    X509_STORE* store = NULL;
+    X509_STORE_CTX* ctx = NULL;
+    STACK_OF(X509)* trusted = NULL;
+    STACK_OF(X509)* untrusted = NULL;
+
+    ExpectNotNull(store = X509_STORE_new());
+    ExpectNotNull(trusted = sk_X509_new_null());
+    ExpectIntGT(sk_X509_push(trusted, root), 0);
+    ExpectNotNull(untrusted = sk_X509_new_null());
+    ExpectIntGT(sk_X509_push(untrusted, inter), 0);
+    ExpectNotNull(ctx = X509_STORE_CTX_new());
+    ExpectIntEQ(X509_STORE_CTX_init(ctx, store, leaf, untrusted), 1);
+    X509_STORE_CTX_trusted_stack(ctx, trusted);
+    /* Chain reaches root in the trusted stack -> verifies. */
+    ExpectIntEQ(X509_verify_cert(ctx), 1);
+    ExpectIntEQ(X509_STORE_CTX_get_error(ctx), X509_V_OK);
+    /* The trusted stack must be restored: the injected intermediate appended
+     * during verification must have been removed, leaving only root. */
+    ExpectIntEQ(sk_X509_num(trusted), 1);
+    ExpectPtrEq(sk_X509_value(trusted, 0), root);
+    X509_STORE_CTX_free(ctx);
+    X509_STORE_free(store);
+    sk_X509_free(untrusted);
+    sk_X509_free(trusted);
+    return EXPECT_RESULT();
+}
+
+/* One mixed-candidate verification: leaf with both the tampered intermediate
+ * (broken outer signature, same subject as int-ca) and the genuine int-ca in
+ * the untrusted stack, in the given push order.  The chain must verify - the
+ * verifier tries the candidates and recovers via X509VerifyCertSetupRetry when
+ * it hits the tampered one first.  Only the return value is asserted: when the
+ * tampered candidate is tried first wolfSSL intentionally preserves its error
+ * code even though the chain recovers (see the "worst-seen error must persist"
+ * behaviour exercised by test_X509_STORE_InvalidCa), so the error after success
+ * is order-dependent and not asserted here. */
+static int untrusted_inter_retry_one(X509_STORE* store, X509* leaf,
+    X509* first, X509* second)
+{
+    EXPECT_DECLS;
+    X509_STORE_CTX* ctx = NULL;
+    STACK_OF(X509)* mixed = NULL;
+
+    ExpectNotNull(mixed = sk_X509_new_null());
+    ExpectIntGT(sk_X509_push(mixed, first), 0);
+    ExpectIntGT(sk_X509_push(mixed, second), 0);
+    ExpectNotNull(ctx = X509_STORE_CTX_new());
+    ExpectIntEQ(X509_STORE_CTX_init(ctx, store, leaf, mixed), 1);
+    ExpectIntEQ(X509_verify_cert(ctx), 1);
+    X509_STORE_CTX_free(ctx);
+    sk_X509_free(mixed);
+    return EXPECT_RESULT();
+}
+
+/* Retry path: when several caller-supplied candidates share a subject, the
+ * verifier tries one, fails, and retries with another
+ * (X509VerifyCertSetupRetry), moving entries through an internal "failed" list.
+ * Exercise BOTH push orderings so that, whichever order the verifier enumerates
+ * same-subject candidates, at least one ordering forces it to hit the tampered
+ * candidate first and recover.  Then prove the store is left clean: a later
+ * verification on the same store with only the tampered intermediate must fail
+ * (no genuine int-ca left behind), and one with the genuine intermediate must
+ * still succeed. */
+static int test_untrusted_inter_retry(X509* leaf, X509* inter,
+    X509* tamperedInter, X509* root)
+{
+    EXPECT_DECLS;
+    X509_STORE* store = NULL;
+    X509_STORE_CTX* ctx = NULL;
+    STACK_OF(X509)* goodOnly = NULL;
+    STACK_OF(X509)* badOnly = NULL;
+
+    ExpectNotNull(store = X509_STORE_new());
+    ExpectIntEQ(X509_STORE_add_cert(store, root), 1);
+
+    /* Both orderings verify and report X509_V_OK. */
+    ExpectIntEQ(untrusted_inter_retry_one(store, leaf, tamperedInter, inter), 1);
+    ExpectIntEQ(untrusted_inter_retry_one(store, leaf, inter, tamperedInter), 1);
+
+    /* Reuse the SAME store with only the tampered intermediate: must fail.  If
+     * a retry above left the genuine int-ca behind in the store, this would
+     * wrongly succeed. */
+    ExpectNotNull(badOnly = sk_X509_new_null());
+    ExpectIntGT(sk_X509_push(badOnly, tamperedInter), 0);
+    ExpectNotNull(ctx = X509_STORE_CTX_new());
+    ExpectIntEQ(X509_STORE_CTX_init(ctx, store, leaf, badOnly), 1);
+    ExpectIntEQ(X509_verify_cert(ctx), 0);
+    ExpectIntNE(X509_STORE_CTX_get_error(ctx), X509_V_OK);
+    X509_STORE_CTX_free(ctx);
+    ctx = NULL;
+
+    /* Reuse the SAME store with the genuine intermediate: must still succeed,
+     * proving the retry left no stale state that breaks later verifications. */
+    ExpectNotNull(goodOnly = sk_X509_new_null());
+    ExpectIntGT(sk_X509_push(goodOnly, inter), 0);
+    ExpectNotNull(ctx = X509_STORE_CTX_new());
+    ExpectIntEQ(X509_STORE_CTX_init(ctx, store, leaf, goodOnly), 1);
+    ExpectIntEQ(X509_verify_cert(ctx), 1);
+    ExpectIntEQ(X509_STORE_CTX_get_error(ctx), X509_V_OK);
+
+    X509_STORE_CTX_free(ctx);
+    X509_STORE_free(store);
+    sk_X509_free(goodOnly);
+    sk_X509_free(badOnly);
+    return EXPECT_RESULT();
+}
 #endif /* OPENSSL_EXTRA && !NO_RSA && !NO_CERTS && !NO_FILESYSTEM */
 
 int test_X509_verify_cert_untrusted_inter(void)
@@ -1324,6 +1447,8 @@ int test_X509_verify_cert_untrusted_inter(void)
     int reusedStoreRes = 0;
     int noStaleRes = 0;
     int depthExhaustRes = 0;
+    int trustedStackCleanupRes = 0;
+    int retryRes = 0;
 
     ExpectNotNull(leaf = untrusted_inter_load(UA_CERT_DIR "leaf-cert.pem"));
     ExpectNotNull(leafDeep =
@@ -1355,6 +1480,9 @@ int test_X509_verify_cert_untrusted_inter(void)
                             root);
         depthExhaustRes = test_untrusted_inter_depth_exhaustion(leafDeep,
                             inter, inter2, root);
+        trustedStackCleanupRes = test_untrusted_inter_trusted_stack_cleanup(
+                            leaf, inter, root);
+        retryRes = test_untrusted_inter_retry(leaf, inter, tamperedInter, root);
         ExpectIntEQ(sanityRes, 1);
         ExpectIntEQ(twoLevelRes, 1);
         ExpectIntEQ(emptyStoreRes, 1);
@@ -1363,6 +1491,8 @@ int test_X509_verify_cert_untrusted_inter(void)
         ExpectIntEQ(reusedStoreRes, 1);
         ExpectIntEQ(noStaleRes, 1);
         ExpectIntEQ(depthExhaustRes, 1);
+        ExpectIntEQ(trustedStackCleanupRes, 1);
+        ExpectIntEQ(retryRes, 1);
     }
 
     X509_free(leaf);