Skip to content

openssl: fix DER buffer leak in Apple SecTrust verification - #22631

Closed
tcpreplay-dev wants to merge 2 commits into
curl:masterfrom
tcpreplay-dev:sectrust_der_memory_leak
Closed

tcpreplay-dev wants to merge 2 commits into
curl:masterfrom
tcpreplay-dev:sectrust_der_memory_leak

Conversation

@tcpreplay-dev

Copy link
Copy Markdown
Contributor

ossl_chain_get_der() allocates a DER encoding of each peer certificate via i2d_X509(), but Curl_vtls_apple_verify() only copies it into a CFData and never frees the original. This leaks per certificate, per handshake, whenever USE_APPLE_SECTRUST is used with the OpenSSL/LibreSSL/BoringSSL backend.

Fix frees the buffer inside openssl.c itself, so the GnuTLS backend (which borrows rather than allocates) is unaffected.

In our macOS application this patch reduced memory growth from ~65 - 67 MB/day to ~0.6 - 0.7 MB/day.

`ossl_chain_get_der()` allocates a DER encoding of each peer certificate via
`i2d_X509()`, but `Curl_vtls_apple_verify()` only copies it into a CFData and
never frees the original. This leaks per certificate, per handshake, whenever
USE_APPLE_SECTRUST is used with the OpenSSL/LibreSSL/BoringSSL backend.

Fix frees the buffer inside openssl.c itself, so the GnuTLS backend
(which borrows rather than allocates) is unaffected.
@github-actions github-actions Bot added the TLS label Aug 20, 2026
Comment thread lib/vtls/openssl.c Outdated
Only called twice, so no need for a function call.

As per code review.
@tcpreplay-dev
tcpreplay-dev force-pushed the sectrust_der_memory_leak branch from d0198df to 56177d7 Compare August 20, 2026 23:12
@bagder

bagder commented Aug 21, 2026

Copy link
Copy Markdown
Member

@vszakats this CM integration windows-2022 fail looks like a new problem (and is not the fault of this PR)

Copilot AI 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.

Pull request overview

Fixes a DER certificate buffer leak during OpenSSL-backed Apple SecTrust verification.

Changes:

  • Tracks the most recently allocated DER buffer.
  • Frees DER memory between certificates and after verification.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@vszakats

vszakats commented Aug 21, 2026

Copy link
Copy Markdown
Member

@vszakats this CM integration windows-2022 fail looks like a new problem (and is not the fault of this PR)

Yes, it's an expected fallout (only when it hits was the question, and which jobs it affects) due to upstream changes.
Merging the fix soon via #22632

@bagder bagder closed this in 961c95f Aug 21, 2026
@bagder

bagder commented Aug 21, 2026

Copy link
Copy Markdown
Member

Thanks!

@tcpreplay-dev

tcpreplay-dev commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

@vszakats this CM integration windows-2022 fail looks like a new problem (and is not the fault of this PR)

Yes, it's an expected fallout (only when it hits was the question, and which jobs it affects) due to upstream changes. Merging the fix soon via #22632

I would recommend a cherry-pick of 7f964b into that PR. It is cleaner than the suggested fixes (no copy/paste) and a cherry-pick gives proper credit.

@vszakats

Copy link
Copy Markdown
Member

@vszakats this CM integration windows-2022 fail looks like a new problem (and is not the fault of this PR)

Yes, it's an expected fallout (only when it hits was the question, and which jobs it affects) due to upstream changes. Merging the fix soon via #22632

I would recommend a cherry-pick of 7f964b into that PR. It is cleaner than the suggested fixes (no copy/paste) and a cherry-pick gives proper credit.

I don't follow, is this regarding the GitHub fix? can you say what to cherry-pick and whom to credit?

@tcpreplay-dev

Copy link
Copy Markdown
Contributor Author

#22632

@vszakats this CM integration windows-2022 fail looks like a new problem (and is not the fault of this PR)

Yes, it's an expected fallout (only when it hits was the question, and which jobs it affects) due to upstream changes. Merging the fix soon via #22632

I would recommend a cherry-pick of 7f964b into that PR. It is cleaner than the suggested fixes (no copy/paste) and a cherry-pick gives proper credit.

I don't follow, is this regarding the GitHub fix? can you say what to cherry-pick and whom to credit?

Oh, I think I misunderstood the process. I think you are saying that even though this is closed, it will still land. Disregard my comments.

@jay

jay commented Aug 22, 2026

Copy link
Copy Markdown
Member

already landed 961c95f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

5 participants