Skip to content

Free every OCSP chain request in TLSX_CSR_Free - #11285

Merged
philljj merged 1 commit into
wolfSSL:masterfrom
danielinux:ocsp-free-fix
Aug 26, 2026
Merged

Free every OCSP chain request in TLSX_CSR_Free#11285
philljj merged 1 commit into
wolfSSL:masterfrom
danielinux:ocsp-free-fix

Conversation

@danielinux

Copy link
Copy Markdown
Member

Description

ProcessChainOCSPRequest() stores one OcspRequest per certificate at that
certificate's position in the chain:

request = &csr->request.ocsp[i];   /* i starts at 1, index 0 is the leaf */

csr->requests, however, is a count of calls that completed successfully,
maintained independently of i.

TLSX_CSR_Free() bounded the release by that count:

for (i = 0; i < csr->requests; i++)
    FreeOcspRequest(&csr->request.ocsp[i]);

The two only coincide when index 0 was populated first. Whenever the highest
index written is at or above csr->requests, the tail entries are never
released, and the serial and URL buffers InitOcspRequest() allocated for them
become unreachable.

Valgrind reports them as definitely lost:

InitOcspRequest (asn.c)
  <- CreateOcspRequest (internal.c)
    <- ProcessChainOCSPRequest (tls.c)

This affects OCSP multi-stapling for certificate chains
(--enable-ocspstapling). Functional behaviour is unaffected — every test
passes either way — so it only surfaces under a leak checker.

Fix: walk the whole array rather than a count. FreeOcspRequest() is a no-op
on a request that was never populated, and the array is sized
MAX_CERT_EXTENSIONS, so unpopulated slots cost nothing.

Testing

Discovered through #11222

  • Reproducer that populates a request at chain position 1 with csr->requests
    still 0, then frees the WOLFSSL object, run under
    valgrind --leak-check=full:

    • before: definitely lost: 43 bytes in 2 blocks, ERROR SUMMARY: 2 errors
    • after: no leaks, ERROR SUMMARY: 0 errors

    The 20-byte and 23-byte blocks correspond to the serial and URL allocations
    in InitOcspRequest().

  • ./tests/unit.test --group ocsp — 0 failures.

  • ./wolfcrypt/test/testwolfcrypt — exit 0.

  • Configuration used:
    --enable-ocsp --enable-ocspstapling --enable-ocspstapling2 --enable-tls13

ProcessChainOCSPRequest() stores each request at the certificate's position
in the chain, counting from 1 for the first intermediate, while csr->requests
is a count of successful calls. TLSX_CSR_Free() bounded the free by that
count, so any entry written at an index at or above it was never released:
valgrind reports the serial and URL buffers InitOcspRequest() allocated as
definitely lost.

Walk the whole array instead. FreeOcspRequest() is a no-op on a request that
was never populated, and the array is sized MAX_CERT_EXTENSIONS.
Copilot AI lite review requested due to automatic review settings August 26, 2026 12:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@danielinux danielinux self-assigned this Aug 26, 2026
@wolfSSL-Bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #11285

Scan targets checked: wolfssl-bugs, wolfssl-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

@danielinux danielinux assigned wolfSSL-Bot and unassigned danielinux Aug 26, 2026
@philljj
philljj requested review from philljj and a balanced review from Copilot August 26, 2026 14:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@philljj

philljj commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Retest this please.

(raise TLSAbruptCloseError() in multi-msg-record test)

@philljj

philljj commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

note: will fix valgrind leak reported in:

@philljj

philljj commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Retest this please.

PRB preflight hung

@philljj
philljj merged commit 4645779 into wolfSSL:master Aug 26, 2026
381 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants