Skip to content

Use refcount on DER buffer to prevent UAF #10860

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 4 commits into from
+148 −40
Closed

Use refcount on DER buffer to prevent UAF #10860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60b5dfc
Add test case to swap certificates in SNI callback
padelsbach Jul 7, 2026
b493831
Use refcount on DER buffer to prevent UAF
padelsbach Jul 8, 2026
a76d50d
PR feedback regarding return code checking
padelsbach Jul 8, 2026
0b2304f
Fix coding-no test
padelsbach Jul 9, 2026
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
14 changes: 14 additions & 0 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7173,7 +7173,15 @@ 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;
if (!RefDer(ssl->buffers.certificate)) {
ssl->buffers.certificate = NULL;
return BAD_MUTEX_E;
}
ssl->buffers.certChain = ctx->certChain;
if (!RefDer(ssl->buffers.certChain)) {
ssl->buffers.certChain = NULL;
return BAD_MUTEX_E;
}
#endif
ssl->buffers.certChainCnt = ctx->certChainCnt;
#ifndef WOLFSSL_BLIND_PRIVATE_KEY
Expand Down Expand Up @@ -9218,6 +9226,12 @@ void wolfSSL_ResourceFree(WOLFSSL* ssl)
#ifndef NO_CERTS
ssl->keepCert = 0; /* make sure certificate is free'd */
wolfSSL_UnloadCertsKeys(ssl);
/* Release references on cert buffers aliased from the context. Owned
* buffers were already freed and cleared by wolfSSL_UnloadCertsKeys. */
FreeDer(&ssl->buffers.certificate);
ssl->buffers.weOwnCert = 0;
FreeDer(&ssl->buffers.certChain);
ssl->buffers.weOwnCertChain = 0;
#endif
#ifndef NO_RSA
FreeKey(ssl, DYNAMIC_TYPE_RSA, (void**)&ssl->peerRsaKey);
Expand Down
27 changes: 16 additions & 11 deletions src/ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12727,17 +12727,10 @@ void wolfSSL_certs_clear(WOLFSSL* ssl)
if (ssl == NULL)
return;

/* ctx still owns certificate, certChain, key, dh, and cm */
if (ssl->buffers.weOwnCert) {
FreeDer(&ssl->buffers.certificate);
ssl->buffers.weOwnCert = 0;
}
ssl->buffers.certificate = NULL;
if (ssl->buffers.weOwnCertChain) {
FreeDer(&ssl->buffers.certChain);
ssl->buffers.weOwnCertChain = 0;
}
ssl->buffers.certChain = NULL;
FreeDer(&ssl->buffers.certificate);
ssl->buffers.weOwnCert = 0;
FreeDer(&ssl->buffers.certChain);
ssl->buffers.weOwnCertChain = 0;
#ifdef WOLFSSL_TLS13
ssl->buffers.certChainCnt = 0;
#endif
Expand Down Expand Up @@ -13200,8 +13193,20 @@ WOLFSSL_CTX* wolfSSL_set_SSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
}
#else
/* ctx owns certificate, certChain and key */
FreeDer(&ssl->buffers.certificate);
ssl->buffers.weOwnCert = 0;
FreeDer(&ssl->buffers.certChain);
ssl->buffers.weOwnCertChain = 0;
ssl->buffers.certificate = ctx->certificate;
if (!RefDer(ssl->buffers.certificate)) {
ssl->buffers.certificate = NULL;
return NULL;
}
ssl->buffers.certChain = ctx->certChain;
if (!RefDer(ssl->buffers.certChain)) {
ssl->buffers.certChain = NULL;
return NULL;
}
#endif
#ifdef WOLFSSL_TLS13
ssl->buffers.certChainCnt = ctx->certChainCnt;
Expand Down
12 changes: 4 additions & 8 deletions src/ssl_load.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,7 @@ static int ProcessUserChainRetain(WOLFSSL_CTX* ctx, WOLFSSL* ssl,

/* Store in SSL object if available. */
if (ssl != NULL) {
/* Dispose of old chain if not reference to context's. */
if (ssl->buffers.weOwnCertChain) {
FreeDer(&ssl->buffers.certChain);
}
FreeDer(&ssl->buffers.certChain);
/* Allocate and copy the buffer into SSL object. */
ret = AllocCopyDer(&ssl->buffers.certChain, chainBuffer, len, type,
heap);
Expand Down Expand Up @@ -2224,15 +2221,14 @@ static int ProcessBufferCertHandleDer(WOLFSSL_CTX* ctx, WOLFSSL* ssl,
/* Leaf certificate - our certificate. */
else if (type == CERT_TYPE) {
if (ssl != NULL) {
/* Free previous certificate if we own it. */
#ifdef KEEP_OUR_CERT
if (ssl->buffers.weOwnCert) {
FreeDer(&ssl->buffers.certificate);
#ifdef KEEP_OUR_CERT
/* Dispose of X509 version of certificate. */
wolfSSL_X509_free(ssl->ourCert);
ssl->ourCert = NULL;
#endif
}
#endif
FreeDer(&ssl->buffers.certificate);
/* Store certificate as ours. */
ssl->buffers.certificate = der;
#ifdef KEEP_OUR_CERT
Expand Down
17 changes: 5 additions & 12 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3956,25 +3956,18 @@ static int test_wolfSSL_add_to_chain_overflow(void)
if (EXPECT_SUCCESS()) {
/* Now ctx->certificate is set, next add goes to certChain via
* wolfssl_add_to_chain. Fake a chain whose length is near UINT32_MAX
* so the size calculation (len + CERT_HEADER_SZ + certSz) overflows. */
fakeChain = (DerBuffer*)XMALLOC(sizeof(DerBuffer) + 16, ctx->heap,
DYNAMIC_TYPE_CERT);
ExpectNotNull(fakeChain);
* so the size calculation (len + CERT_HEADER_SZ + certSz) overflows.
* Use AllocDer so the buffer refcount is initialized. */
ExpectIntEQ(AllocDer(&fakeChain, 16, CERT_TYPE, ctx->heap), 0);
}
if (EXPECT_SUCCESS()) {
XMEMSET(fakeChain, 0, sizeof(DerBuffer) + 16);
fakeChain->buffer = (byte*)(fakeChain + 1);
fakeChain->length = WOLFSSL_MAX_32BIT - 2; /* will overflow with any cert */
fakeChain->type = CERT_TYPE;
fakeChain->dynType = DYNAMIC_TYPE_CERT;
/* Replace the real chain with our fake one. */
if (ctx->certChain != NULL) {
XFREE(ctx->certChain, ctx->heap, DYNAMIC_TYPE_CERT);
}
FreeDer(&ctx->certChain);
ctx->certChain = fakeChain;
}
else {
XFREE(fakeChain, ctx ? ctx->heap : NULL, DYNAMIC_TYPE_CERT);
FreeDer(&fakeChain);
}

/* Try to add another cert - this MUST fail due to overflow guard. */
Expand Down
64 changes: 64 additions & 0 deletions tests/api/test_tls13.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3627,6 +3627,70 @@ int test_tls13_pha(void)
}


#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \

@dgarske dgarske Jul 9, 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] No test coverage for add0/add1_chain_cert on an SSL that aliases the CTX chain · Missing Tests

The added test exercises only the leaf-certificate reload path (wolfSSL_CTX_use_certificate_file in the SNI callback), which correctly decrements the aliased reference via ProcessBufferCertHandleDer's now-unconditional FreeDer. The refcount change also affects the cert-CHAIN aliasing path, but there is no test that appends to an SSL's aliased chain via wolfSSL_add1_chain_cert / wolfSSL_add0_chain_cert: create an SSL from a CTX that loaded an intermediate chain (so ssl->buffers.certChain aliases ctx->certChain with refcount 2), then call wolfSSL_add1_chain_cert(ssl, x509) and free both under leak detection. That is exactly the path where the wolfssl_add_to_chain leak (see High finding) survives, which is why the leak was not caught. Severity: review Medium/SUGGEST, review-security Low; stricter view kept.

Fix: Add a test under OPENSSL_EXTRA + KEEP_OUR_CERT that (1) loads a certificate chain on the CTX, (2) creates a WOLFSSL so ssl->buffers.certChain aliases ctx->certChain, (3) calls wolfSSL_add1_chain_cert(ssl, x509) (or wolfSSL_add0_chain_cert), and (4) frees ssl then ctx, verifying no leak under ASAN/valgrind. This validates the chain-alias refcount balance and guards the fix for the High finding.

defined(HAVE_SNI) && !defined(WOLFSSL_COPY_CERT) && \
!defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) && \
!defined(NO_RSA) && !defined(NO_CERTS)
/* React to the client SNI by reloading the certificate on the existing CTX.
* Because the SSL was created without WOLFSSL_COPY_CERT, the cert pointer is
* identical between the CTX and SSL objects. This test validates the
* cert file replacement.
* Note the updated cert will not be used for this connection. */
static int test_cert_alias_sni_cb(WOLFSSL* ssl, int* ad, void* arg)
{
WOLFSSL_CTX* ctx = wolfSSL_get_SSL_CTX(ssl);
int* cbRet = (int*)arg;
(void)ad;
/* Feed the reload result back to the parent function below. */
*cbRet = wolfSSL_CTX_use_certificate_file(ctx, svrCertFile, CERT_FILETYPE);
/* 0 means ack the servername and continue the handshake. */
return 0;
}
#endif

int test_tls13_cert_alias_uaf_sni(void)
{
EXPECT_DECLS;
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \
defined(HAVE_SNI) && !defined(WOLFSSL_COPY_CERT) && \
!defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) && \
!defined(NO_RSA) && !defined(NO_CERTS)
WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
struct test_memio_ctx test_ctx;
const char* host = "example.com";
int cbRet = WOLFSSL_FATAL_ERROR;

XMEMSET(&test_ctx, 0, sizeof(test_ctx));
/* Server cert/key are loaded on ctx_s, so ssl_s->buffers.certificate
* aliases ctx_s->certificate. */
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0);

/* Server swaps its CTX certificate when the SNI arrives. */
wolfSSL_CTX_set_servername_callback(ctx_s, test_cert_alias_sni_cb);
ExpectIntEQ(wolfSSL_CTX_set_servername_arg(ctx_s, &cbRet), WOLFSSL_SUCCESS);

/* Client offers SNI so the server callback fires while parsing the
* ClientHello. */
ExpectIntEQ(wolfSSL_UseSNI(ssl_c, WOLFSSL_SNI_HOST_NAME, host,
(word16)XSTRLEN(host)), WOLFSSL_SUCCESS);

/* The callback loads a new certificate on the server CTX, which is
* aliased by the SSL. The handshake must complete without a UAF. */
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
/* Fail the test if the callback's cert reload failed or didn't run. */
ExpectIntEQ(cbRet, WOLFSSL_SUCCESS);

wolfSSL_free(ssl_c);
wolfSSL_free(ssl_s);
wolfSSL_CTX_free(ctx_c);
wolfSSL_CTX_free(ctx_s);
#endif
return EXPECT_RESULT();
}


#if defined(HAVE_IO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \
defined(WOLFSSL_HAVE_MLKEM) && !defined(WOLFSSL_MLKEM_NO_ENCAPSULATE) && \
!defined(WOLFSSL_MLKEM_NO_DECAPSULATE) && \
Expand Down
2 changes: 2 additions & 0 deletions tests/api/test_tls13.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ int test_tls13_bad_psk_binder(void);
int test_tls13_rpk_handshake(void);
int test_tls13_rpk_handshake_no_negotiation(void);
int test_tls13_pha(void);
int test_tls13_cert_alias_uaf_sni(void);
int test_tls13_pq_groups(void);
int test_tls13_multi_pqc_key_share(void);
int test_tls13_early_data(void);
Expand Down Expand Up @@ -108,6 +109,7 @@ int test_tls13_pqc_hybrid_async_server(void);
TEST_DECL_GROUP("tls13", test_tls13_rpk_handshake), \
TEST_DECL_GROUP("tls13", test_tls13_rpk_handshake_no_negotiation), \
TEST_DECL_GROUP("tls13", test_tls13_pha), \
TEST_DECL_GROUP("tls13", test_tls13_cert_alias_uaf_sni), \
TEST_DECL_GROUP("tls13", test_tls13_pq_groups), \
TEST_DECL_GROUP("tls13", test_tls13_multi_pqc_key_share), \
TEST_DECL_GROUP("tls13", test_tls13_early_data), \
Expand Down
46 changes: 39 additions & 7 deletions wolfcrypt/src/asn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24693,13 +24693,39 @@ int AllocDer(DerBuffer** pDer, word32 length, int type, void* heap)
der->heap = heap;
der->buffer = (byte*)der + sizeof(DerBuffer);
der->length = length;
wolfSSL_RefInit(&der->ref, &ret);
#ifdef WOLFSSL_REFCNT_ERROR_RETURN
if (ret != 0) {
XFREE(der, heap, dynType);
*pDer = NULL;
return ret;
}
#endif
ret = 0; /* Success */
} else {
ret = BAD_FUNC_ARG;
}
return ret;
}

/* Increments the reference count on a shared DER buffer.
*
* A mutex-backed refcount can fail to lock, leaving the count un-incremented;
* return failure so callers don't proceed with an un-referenced alias.
*
* @param [in, out] der DER buffer. May be NULL.
* @return 1 on success
* @return 0 on error
*/
int RefDer(DerBuffer* der)

@dgarske dgarske Jul 9, 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] RefDer treats NULL as success; FreeDer discards mutex-lock failure (leak-on-lock-failure) · Style

RefDer(NULL) returns 1 (success), which is intentional so aliasing a NULL ctx->certificate is a no-op; callers rely on this. Worth a brief note only: in the mutex build, a lock failure inside FreeDer's wolfSSL_RefDec sets isZero=0 and the buffer is intentionally not freed (leak-on-lock-failure), and ref_err is discarded via (void)ref_err;. This matches the library's existing refcount conventions (session/ctx refcounts), so no change is required, but the discarded error is easy to misread.

Fix: No functional change needed; consider a one-line comment at the FreeDer (void)ref_err; noting the deliberate leak-on-lock-failure behavior for parity with other refcounted frees.

{
int err = 0;
if (der != NULL) {
wolfSSL_RefInc(&der->ref, &err);
}
return !err;
}

int AllocCopyDer(DerBuffer** pDer, const unsigned char* buff, word32 length,
int type, void* heap)
{
Expand All @@ -24715,15 +24741,21 @@ void FreeDer(DerBuffer** pDer)
{
if (pDer && *pDer) {
DerBuffer* der = (DerBuffer*)*pDer;
int isZero = 1;
int ref_err = 0;

/* ForceZero private keys */
if (((der->type == PRIVATEKEY_TYPE) ||
(der->type == ALT_PRIVATEKEY_TYPE)) && der->buffer != NULL) {
ForceZero(der->buffer, der->length);
wolfSSL_RefDec(&der->ref, &isZero, &ref_err);
(void)ref_err;
if (isZero) {
if (((der->type == PRIVATEKEY_TYPE) ||
(der->type == ALT_PRIVATEKEY_TYPE)) && der->buffer != NULL) {
ForceZero(der->buffer, der->length);
}
der->buffer = NULL;
der->length = 0;
wolfSSL_RefFree(&der->ref);
XFREE(der, der->heap, der->dynType);
}
der->buffer = NULL;
der->length = 0;
XFREE(der, der->heap, der->dynType);

*pDer = NULL;
}
Expand Down
1 change: 1 addition & 0 deletions wolfssl/wolfcrypt/asn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2811,6 +2811,7 @@ WOLFSSL_API int AllocDer(DerBuffer** der, word32 length, int type,
WOLFSSL_LOCAL int AllocCopyDer(DerBuffer** der, const unsigned char* buff,
word32 length, int type, void* heap);
WOLFSSL_API void FreeDer(DerBuffer** der);
WOLFSSL_LOCAL int RefDer(DerBuffer* der);

#ifdef WOLFSSL_ASN_PARSE_KEYUSAGE
WOLFSSL_LOCAL int ParseKeyUsageStr(const char* value, word16* keyUsage,
Expand Down
5 changes: 3 additions & 2 deletions wolfssl/wolfcrypt/asn_public.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,9 @@ typedef struct DerBuffer {
byte* buffer;

@dgarske dgarske Jul 9, 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] Public DerBuffer struct grows a new member (ABI/size change; FreeDer now requires AllocDer-initialized ref) · API / ABI

The public DerBuffer struct gains a wolfSSL_Ref ref member, correctly appended at the end (per wolfSSL struct convention, avoiding mid-struct ABI breakage). FreeDer now calls wolfSSL_RefDec(&der->ref, ...) on every buffer. Two consequences: (1) sizeof(DerBuffer) changes, so any application or FIPS-boundary build that allocates/embeds DerBuffer directly, or links against a prebuilt shared library, must be recompiled - a classic shared-library ABI break. (2) In mutex-backed refcount builds (!SINGLE_THREADED && !WOLFSSL_ATOMIC_OPS), a DerBuffer NOT created via AllocDer (which now runs wolfSSL_RefInit) has an uninitialized mutex and hits UB when freed with wc_FreeDer; every cert/key DerBuffer also now embeds and init/frees a mutex per AllocDer/FreeDer. All in-tree allocations go through AllocDer (verified: no sizeof(DerBuffer) allocation or raw XFREE of a DerBuffer struct exists outside wolfcrypt/src/asn.c), and the PR fixed the one internal offender (test_wolfSSL_add_to_chain_overflow now uses AllocDer/FreeDer); the stack DerBuffer rawDer in wolfcrypt/src/evp_pk.c is safe because it is never passed to FreeDer. Severity: review Medium/SUGGEST, review-security Info; stricter view kept.

Fix: Document in the ChangeLog/release notes the sizeof(DerBuffer) ABI change so downstream users rebuild rather than mix object files across the boundary, and add a contract note in the DerBuffer/FreeDer doxygen that a DerBuffer must be obtained from AllocDer/wc_AllocDer before being freed with FreeDer/wc_FreeDer. No functional code change required.

void* heap;
word32 length;
int type; /* enum CertType */
int dynType; /* DYNAMIC_TYPE_* */
int type; /* enum CertType */
int dynType; /* DYNAMIC_TYPE_* */
wolfSSL_Ref ref; /* refcount for aliased pointers to the der */

@dgarske dgarske Jul 9, 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.

Do we really need to always add "wolfSSL_Ref ref;" on all builds? I am concerned about portability and code size growth.

} DerBuffer;

typedef struct WOLFSSL_ASN1_TIME {
Expand Down
Loading