Skip to content

Fix aliasing of der buffers #11109

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

Closed
padelsbach wants to merge 1 commit into from
Closed

Fix aliasing of der buffers #11109

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
2 changes: 2 additions & 0 deletions .wolfssl_known_macro_extras
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,7 @@ WOLFSSL_CONTIKI
WOLFSSL_CRL_ALLOW_MISSING_CDP
WOLFSSL_DEBUG_TRACE_ERROR_CODES_SUPPORT
WOLFSSL_DILITHIUM_VERIFY_SMALLEST_MEM
WOLFSSL_DER_REFCOUNT
WOLFSSL_DISABLE_EARLY_SANITY_CHECKS
WOLFSSL_DRBG_SHA256
WOLFSSL_DTLS13_ECHO_LEGACY_SESSION_ID
Expand Down Expand Up @@ -950,6 +951,7 @@ WOLFSSL_NO_CT_MAX_MIN
WOLFSSL_NO_DEBUG_CERTS
WOLFSSL_NO_DECODE_EXTRA
WOLFSSL_NO_DEL_HANDLE
WOLFSSL_NO_DER_REFCOUNT
WOLFSSL_NO_DER_TO_PEM
WOLFSSL_NO_DH186
WOLFSSL_NO_DILITHIUM_LEGACY_GATES
Expand Down
24 changes: 24 additions & 0 deletions ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,30 @@

## Behavioral Changes

* **Behavioral change (`DerBuffer` reference counting)**: a `DerBuffer` now
carries a reference count so a context and the sessions made from it can
share one buffer safely, and the certificate or key can be replaced on the
context while sessions still hold the one they started with. It is on
wherever the count is cheap, which is any build with atomics or
`SINGLE_THREADED`, so most builds get it without asking. `wc_FreeDer()`
now lets go of one hold rather than freeing outright; the memory goes when
the last holder lets go, and the caller's pointer is cleared either way.
An application that builds a `DerBuffer` by hand rather than through
`wc_AllocDer()` and frees it with `wc_FreeDer()` must zero the structure
first: the count is what tells a shared buffer from a hand-built one, and
a count left uninitialized reads as another holder, so the buffer is not
freed. The count is appended to the structure, so the offsets of the
existing fields do not move and code that only reads a `DerBuffer` is
unaffected; the structure does not grow on a 64-bit target, where the
count lands in padding it already had, and grows by four bytes on a 32-bit
one. Define `WOLFSSL_NO_DER_REFCOUNT` to force it off, or
`WOLFSSL_DER_REFCOUNT` to force it on. A threaded build whose compiler
offers no atomics has it off, where a session shares the context's buffer
by pointer as before and the context must outlive its sessions. The hold
covers a session that already has the buffer; replacing a certificate on a
context at the same moment as `wolfSSL_new()` is still not safe, so
reloading has to be kept clear of session creation.

* **Behavioral change (`wolfSSL_shutdown` when no close_notify can be sent)**:
when the connection is already closed or reset and no close_notify was ever
sent, the shutdown exchange can never complete. That case now returns
Expand Down
7 changes: 7 additions & 0 deletions doc/dox_comments/header_files/asn_public.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1723,6 +1723,13 @@ int wc_AllocDer(DerBuffer** pDer, word32 length, int type,
\ingroup ASN
\brief Frees DER buffer allocated by wc_AllocDer or wc_PemToDer.

With WOLFSSL_DER_REFCOUNT the buffer may be shared, in which case this
lets go of one hold on it and the memory is freed once the last holder
lets go. The caller's pointer is cleared either way. A DerBuffer the
application built itself rather than through wc_AllocDer has no hold on
it and is freed here; such a buffer must have been zeroed before use, as
the reference count is what tells the two apart.

\param pDer Pointer to DerBuffer pointer to free

_Example_
Expand Down
142 changes: 130 additions & 12 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7512,6 +7512,53 @@ static int SetSSL_CTX_CheckVersion(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
#endif /* OPENSSL_EXTRA */

#ifndef NO_CERTS
/* Let go of the DER buffer an SSL object holds.
*
* @param [in, out] pDer Buffer to release. May hold NULL.
* @param [in, out] weOwn Whether the buffer was this object's own.
*/
void FreeSslDer(DerBuffer** pDer, byte* weOwn)

@dgarske dgarske Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] FreeSslDer dereferences its out-parameters without the NULL guard FreeDer provides

FreeDer(), which FreeSslDer() wraps, is defensive about its argument (if (pDer && *pDer)). FreeSslDer() unconditionally dereferences both pDer (in the non-refcount branch) and weOwn, so it is stricter than the function it forwards to:

void FreeSslDer(DerBuffer** pDer, byte* weOwn)
{
#ifdef WOLFSSL_DER_REFCOUNT
    FreeDer(pDer);
#else
    if (*weOwn) {          /* unguarded */
        FreeDer(pDer);
    }
    *pDer = NULL;          /* unguarded */
#endif
    *weOwn = 0;            /* unguarded */
}

All 21 in-tree call sites pass the address of a struct member so this cannot fire today, and for a WOLFSSL_LOCAL helper that is a defensible contract - but the asymmetry with FreeDer() is easy to misread, and the doxygen block says only "May hold NULL" (about *pDer), not that pDer and weOwn themselves must be non-NULL.

Fix: Document that pDer and weOwn must themselves be non-NULL (the same applies to AliasSslDer), so the contract is not inferred from FreeDer's looser one.

@padelsbach padelsbach Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not a public API, no need to mirror the public one. Documentation already in place

{
#ifdef WOLFSSL_DER_REFCOUNT
/* Held either way, so let go either way. */
FreeDer(pDer);
#else
/* Only the owner may free it; the context's buffer is left alone. */
if (*weOwn) {
FreeDer(pDer);
}
*pDer = NULL;
#endif
*weOwn = 0;
}

/* Point an SSL object's DER buffer at the context's, taking a hold on it.
*
* Whatever the buffer held before is let go of first.
*
* @param [in, out] pDer Buffer to point at the context's.
* @param [in, out] weOwn Whether the buffer was this object's own.
* @param [in] src Context's buffer. May be NULL.
* @return 1 on success.
* @return 0 when the hold could not be taken.
*/
int AliasSslDer(DerBuffer** pDer, byte* weOwn, DerBuffer* src)

@dgarske dgarske Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] AliasSslDer releases the object's existing buffer before it knows the new hold can be taken

AliasSslDer() frees first and acquires second:

int AliasSslDer(DerBuffer** pDer, byte* weOwn, DerBuffer* src)
{
    int ret = 1;
    FreeSslDer(pDer, weOwn);
    if (!RefDer(src)) {
        ret = 0;
    }
    else {
        *pDer = src;
    }
    return ret;
}

If RefDer() fails the object is left with *pDer == NULL - it has lost the buffer it had and gained nothing. In wolfSSL_set_SSL_CTX() the failure return is worse than that: ssl->ctx has already been swapped and the previous context already wolfSSL_CTX_free()d by the time the first AliasSslDer() runs, so the caller receives NULL from a WOLFSSL that is now missing both its old certificate and its old context.

This is only reachable with the mutex wolfSSL_Ref variant (WOLFSSL_DER_REFCOUNT forced on without atomics on a threaded build), since the atomic and single-threaded macros always report err == 0. It is nonetheless cheap to make the operation transactional.

Fix: Acquire the reference on src first and only release the previous buffer once it has succeeded, so a failed alias is a no-op rather than a destructive partial update.

@padelsbach padelsbach Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

{
int ret = 1;

/* Take the new hold before letting go of the old buffer, so a failure
* leaves the object with what it already had. */
if (!RefDer(src)) {
ret = 0;
}
else {
FreeSslDer(pDer, weOwn);
*pDer = src;
}

return ret;
}

/* Copy the certificate, certificate chain and private key buffers from the
* CTX into the SSL object. Returns 0 on success. */
static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
Expand Down Expand Up @@ -7542,8 +7589,14 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
}
#else
/* ctx still owns certificate, certChain, key, dh, and cm */
ssl->buffers.certificate = ctx->certificate;
ssl->buffers.certChain = ctx->certChain;
if (!AliasSslDer(&ssl->buffers.certificate, &ssl->buffers.weOwnCert,
ctx->certificate)) {
return BAD_MUTEX_E;
}
if (!AliasSslDer(&ssl->buffers.certChain, &ssl->buffers.weOwnCertChain,
ctx->certChain)) {
return BAD_MUTEX_E;
}
#endif
ssl->buffers.certChainCnt = ctx->certChainCnt;
#ifndef WOLFSSL_BLIND_PRIVATE_KEY
Expand All @@ -7560,11 +7613,11 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
}
ssl->buffers.weOwnKey = 1;
}
else {
ssl->buffers.key = ctx->privateKey;
}
#else
ssl->buffers.key = ctx->privateKey;
if (!AliasSslDer(&ssl->buffers.key, &ssl->buffers.weOwnKey,
ctx->privateKey)) {
return BAD_MUTEX_E;
}
#endif
#else
if (ctx->privateKey != NULL) {
Expand Down Expand Up @@ -7594,7 +7647,10 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
ssl->buffers.keyDevId = ctx->privateKeyDevId;
#ifdef WOLFSSL_DUAL_ALG_CERTS
#ifndef WOLFSSL_BLIND_PRIVATE_KEY
ssl->buffers.altKey = ctx->altPrivateKey;
if (!AliasSslDer(&ssl->buffers.altKey, &ssl->buffers.weOwnAltKey,
ctx->altPrivateKey)) {
return BAD_MUTEX_E;
}
#else
if (ctx->altPrivateKey != NULL) {
ret = AllocCopyDer(&ssl->buffers.altKey, ctx->altPrivateKey->buffer,
Expand Down Expand Up @@ -7624,6 +7680,59 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
}
#endif /* NO_CERTS */

#ifndef NO_DH
/* Give the SSL object its own copy of the context's DH parameters.
*
* The parameters are plain buffers with no reference count on them, so a
* session must not point at the context's: the context is free to replace
* them at any time. They are small and only present when the application
* asked for them, so a copy per session is the cheap way to keep them safe.
*
* @param [in, out] ssl SSL object. Any parameters it owns are let go of.
* @param [in] ctx SSL context object.
* @return 0 on success.
* @return MEMORY_E when dynamic memory allocation fails.
*/
int CopySSL_CTX_DhParams(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
{
byte* p;
byte* g;

if ((ctx->serverDH_P.buffer == NULL) || (ctx->serverDH_G.buffer == NULL)) {
return 0;
}

p = (byte*)XMALLOC(ctx->serverDH_P.length, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
g = (byte*)XMALLOC(ctx->serverDH_G.length, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
if ((p == NULL) || (g == NULL)) {
XFREE(p, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
XFREE(g, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
return MEMORY_E;
}

/* Let go of any parameters this object already had. */
if (ssl->buffers.weOwnDH) {
XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
XFREE(ssl->buffers.serverDH_G.buffer, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
}

XMEMCPY(p, ctx->serverDH_P.buffer, ctx->serverDH_P.length);
XMEMCPY(g, ctx->serverDH_G.buffer, ctx->serverDH_G.length);
ssl->buffers.serverDH_P.buffer = p;
ssl->buffers.serverDH_P.length = ctx->serverDH_P.length;
ssl->buffers.serverDH_G.buffer = g;
ssl->buffers.serverDH_G.length = ctx->serverDH_G.length;
ssl->buffers.weOwnDH = 1;
ssl->buffers.dhFromCtx = 1;

return 0;
}
#endif /* !NO_DH */

int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup)
{
int ret = WOLFSSL_SUCCESS; /* set default ret */
Expand Down Expand Up @@ -7812,8 +7921,9 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup)
!defined(HAVE_SELFTEST)
ssl->options.dhKeyTested = ctx->dhKeyTested;
#endif
ssl->buffers.serverDH_P = ctx->serverDH_P;
ssl->buffers.serverDH_G = ctx->serverDH_G;
if (CopySSL_CTX_DhParams(ssl, ctx) != 0) {
return MEMORY_E;
}
#endif

#if defined(HAVE_RPK)
Expand Down Expand Up @@ -9688,7 +9798,6 @@ void wolfSSL_ResourceFree(WOLFSSL* ssl)
}
XFREE(ssl->buffers.serverDH_Priv.buffer, ssl->heap, DYNAMIC_TYPE_PRIVATE_KEY);
XFREE(ssl->buffers.serverDH_Pub.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
/* parameters (p,g) may be owned by ctx */
if (ssl->buffers.weOwnDH) {
XFREE(ssl->buffers.serverDH_G.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
Expand All @@ -9697,6 +9806,12 @@ void wolfSSL_ResourceFree(WOLFSSL* ssl)
#ifndef NO_CERTS
ssl->keepCert = 0; /* make sure certificate is free'd */
wolfSSL_UnloadCertsKeys(ssl);
FreeSslDer(&ssl->buffers.certificate, &ssl->buffers.weOwnCert);
FreeSslDer(&ssl->buffers.certChain, &ssl->buffers.weOwnCertChain);
FreeSslDer(&ssl->buffers.key, &ssl->buffers.weOwnKey);
#ifdef WOLFSSL_DUAL_ALG_CERTS
FreeSslDer(&ssl->buffers.altKey, &ssl->buffers.weOwnAltKey);
#endif
#endif
#ifndef NO_RSA
FreeKey(ssl, DYNAMIC_TYPE_RSA, (void**)&ssl->peerRsaKey);
Expand Down Expand Up @@ -10077,8 +10192,9 @@ void FreeHandshakeResources(WOLFSSL* ssl)
ssl->buffers.serverDH_Priv.buffer = NULL;
XFREE(ssl->buffers.serverDH_Pub.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
ssl->buffers.serverDH_Pub.buffer = NULL;
/* parameters (p,g) may be owned by ctx */
if (ssl->buffers.weOwnDH) {
/* The copy taken from the context is what this object offers DHE with, so
* it has to outlive the handshake for the object to be used again. */
if (ssl->buffers.weOwnDH && (!ssl->buffers.dhFromCtx)) {
XFREE(ssl->buffers.serverDH_G.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
ssl->buffers.serverDH_G.buffer = NULL;
XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
Expand Down Expand Up @@ -35045,6 +35161,8 @@ static int GetDhPublicKey(WOLFSSL* ssl, const byte* input, word32 size,
}

}
/* Whatever arrives from the peer replaces anything taken from the ctx. */
ssl->buffers.dhFromCtx = 0;

if (ssl->buffers.serverDH_Pub.buffer) {
XFREE(ssl->buffers.serverDH_Pub.buffer, ssl->heap,
Expand Down
Loading
Loading