Fix aliasing of der buffers - #11109
Conversation
43bca6b to
bbc6b0e
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 9 total — 9 posted, 0 skipped
7 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] test_scr_dhe_ctx_params_survive cannot detect the regression it is written to guard against —
tests/api/test_tls_ext.c:513-577 - [Medium] wolfSSL_set_accept_state silently swallows the DH parameter copy failure —
src/ssl_api_hs.c:1593-1598 - [Medium] FreeDer infers "is this buffer reference counted" from possibly uninitialized memory in a public struct —
wolfcrypt/src/asn.c:25304-25340 - [Medium] AliasSslDer releases the object's existing buffer before it knows the new hold can be taken —
src/internal.c:7545-7557 - [Medium] No test coverage for the reference counting mechanism itself or its conditional build paths —
tests/api/test_tls13.c:3913-4139 - [Low] internal.h comment overstates the ownership invariant —
wolfssl/internal.h:5328-5340 - [Low] FreeSslDer dereferences its out-parameters without the NULL guard FreeDer provides —
src/internal.c:7520-7532
Findings not tied to a diff line
CTX-supplied DH parameters are now freed at end of handshake, breaking WOLFSSL object reuse (NO_DH_PARAMS)
File: src/internal.c:7693-7729 (CopySSL_CTX_DhParams), src/internal.c:10192-10198 (FreeHandshakeResources)
Function: CopySSL_CTX_DhParams / FreeHandshakeResources
Severity: High
CopySSL_CTX_DhParams() gives every session its own copy of the context's DH parameters and sets ssl->buffers.weOwnDH = 1. FreeHandshakeResources() is unchanged and still frees the parameters whenever weOwnDH is set:
if (ssl->buffers.weOwnDH) {
XFREE(ssl->buffers.serverDH_G.buffer, ...); ssl->buffers.serverDH_G.buffer = NULL;
XFREE(ssl->buffers.serverDH_P.buffer, ...); ssl->buffers.serverDH_P.buffer = NULL;
}Before this PR, parameters inherited from the CTX had weOwnDH == 0, so this block skipped them and they survived the handshake. The diff even deletes the two /* parameters (p,g) may be owned by ctx */ comments that documented exactly this. Now the parameters are freed and NULLed while ssl->options.haveDH stays 1, so any subsequent use of the same WOLFSSL object hits ERROR_OUT(NO_DH_PARAMS, exit_sske) in SendServerKeyExchange (src/internal.c:39080).
Note this only reproduces when secure renegotiation is not active on the session; with SCR enabled FreeHandshakeResources() early-returns at internal.c:10044-10051 and never reaches the free. That is why the PR's own new test does not catch it (see the separate finding on test_scr_dhe_ctx_params_survive).
Empirically verified. Identical ./configure --enable-dh --enable-opensslextra --disable-shared on both commits, TLS 1.2 DHE-RSA-AES128-GCM-SHA256 server reusing one WOLFSSL object via wolfSSL_clear():
=== PR (HEAD) ===
server handshake 1: OK (err=0)
before clear: P=(nil) weOwnDH=1 haveDH=1
server handshake 2: FAILED (err=-319) <-- NO_DH_PARAMS
=== BASE (HEAD~1)…
**Recommendation:** Do not let `FreeHandshakeResources()` drop the per-session copy of the CTX's DH parameters, or re-populate them in `wolfSSL_clear()`. Whichever route is chosen, add a regression test that completes a TLS 1.2 DHE handshake *without* secure renegotiation, calls `wolfSSL_clear()`, and drives a second handshake on the same object.
_Referenced code: `src/internal.c:7693-7729 (CopySSL_CTX_DhParams), src/internal.c:10192-10198 (FreeHandshakeResources)` (15 lines)_
---
#### Two existing unit tests fail on this branch; make check is broken across every configuration tested
**File:** `tests/api/test_ssl_hs.c:815, tests/api/test_ssl_hs.c:1031`
**Function:** `test_wolfSSL_set_accept_state_reinit / test_wolfSSL_set_connect_state_dh`
**Severity:** High
`tests/api/test_ssl_hs.c` is not in the PR's changed-file list, but both of its DH ownership tests assert the pre-PR aliasing contract and now fail:
2057: test_wolfSSL_set_accept_state_reinit :
ERROR - tests/api/test_ssl_hs.c line 815 failed with:
expected: ssl->buffers.serverDH_P.buffer == ctx->serverDH_P.buffer
result: 0x5744fb32d620 != 0x5744fb328fa0
2061: test_wolfSSL_set_connect_state_dh :
ERROR - tests/api/test_ssl_hs.c line 1031 failed with:
expected: ssl->buffers.serverDH_P.buffer == ctx->serverDH_P.buffer
result: 0x5744fb32d620 != 0x5744fb328fa0
`test_wolfSSL_set_connect_state_dh` additionally asserts `ExpectIntEQ(ssl->buffers.weOwnDH, 0)` and `ExpectPtrEq(ssl2->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer)` under the comment "Parameters borrowed from the context are left for the context to free" - all three expectations are now inverted by the switch to per-session copies.
Reproduced on four configurations, all with `2/.../2175` failing and HEAD~1 passing `unit_test: Success for all configured tests`:
- `--enable-dh --enable-secure-renegotiation --enable-opensslextra`
- `--enable-dh --enable-opensslextra`
- `--enable-dh --enable-opensslextra CFLAGS=-DWOLFSSL_NO_DER_REFCOUNT`
- `--enable-experimental --enable-dual-alg-certs --enable-opensslextra`
The oversight is visible in the PR itself: the new `test_tls13_accept_state_dh_copy` in tests/api/test_tls13.c is a near-duplicate of the DH block in `test_wolfSSL_set_accept_state_reinit`, asserting `ExpectPtrNE(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer)` - the exact…
**Recommendation:** Update both tests in `tests/api/test_ssl_hs.c` to the new copy semantics and re-run `make check`. Consider folding `test_tls13_accept_state_dh_copy` into the existing `test_wolfSSL_set_accept_state_reinit` DH block rather than keeping two tests that assert opposite things about the same code path.
_Referenced code: `tests/api/test_ssl_hs.c:815, tests/api/test_ssl_hs.c:1031-1043` (13 lines)_
---
*Review generated by [Skoll](https://github.com/wolfSSL/fenrir/tree/main/skoll)*
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| /* A TLS 1.2 server whose DH parameters came from the context must still have |
There was a problem hiding this comment.
🟠 [Medium] test_scr_dhe_ctx_params_survive cannot detect the regression it is written to guard against
The test's header comment states the intent precisely: "A TLS 1.2 server whose DH parameters came from the context must still have them for a renegotiation: the first handshake's cleanup must not take them away." But the test calls wolfSSL_UseSecureRenegotiation() on both ends, and FreeHandshakeResources() bails out before ever reaching the DH block when SCR is enabled:
/* src/internal.c:10044-10051 */
#ifdef HAVE_SECURE_RENEGOTIATION
if (ssl->secure_renegotiation && ssl->secure_renegotiation->enabled &&
!ssl->secure_renegotiation->advertiseOnly) {
WOLFSSL_MSG("Secure Renegotiation needs to retain handshake resources");
return;
}
#endifSo "the first handshake's cleanup" never runs in this test, and the assertions ExpectNotNull(ssl_s->buffers.serverDH_P.buffer) hold trivially - on HEAD~1 and on HEAD alike. I confirmed with an instrumented handshake in an SCR build that the session reaches wolfSSL_clear with scrEn=1 and P still non-NULL. Meanwhile the real regression (see the NO_DH_PARAMS finding) happens on the non-SCR path this test never touches. In a default build the test is skipped entirely.
Secondly, the renegotiation assertion is weak: (void)wolfSSL_Rehandshake(ssl_c); discards the result and ExpectIntNE(wolfSSL_get_error(ssl_s, 0), NO_DH_PARAMS) passes if the renegotiation failed for any other reason, or never got far enough to try a ServerKeyExchange.
Fix: Add a non-SCR variant that drives a full DHE handshake, lets FreeHandshakeResources() run, and then asserts the parameters are still present (and that a second handshake on the same object succeeds). Also assert the wolfSSL_Rehandshake() return value instead of discarding it.
| @@ -1592,9 +1592,9 @@ void wolfSSL_set_accept_state(WOLFSSL* ssl) | |||
|
|
|||
| #ifndef NO_DH | |||
There was a problem hiding this comment.
🟠 [Medium] wolfSSL_set_accept_state silently swallows the DH parameter copy failure
The old code inherited the CTX's DH parameter pointers and unconditionally set haveDH = 1; it could not fail. CopySSL_CTX_DhParams() allocates, so it can now return MEMORY_E, and the new code drops that on the floor:
if ((!ssl->options.haveDH) && (ssl->ctx->haveDH)) {
if (CopySSL_CTX_DhParams(ssl, ssl->ctx) == 0) {
ssl->options.haveDH = 1;
}
}On allocation failure haveDH stays 0, nothing is logged, ssl->error is not set, and wolfSSL_set_accept_state() returns void. The application gets a server object that has silently lost its DHE capability and will only find out later as an obscure cipher-suite negotiation failure. Every other failure path in this function at least emits a WOLFSSL_MSG (e.g. "Unable to unmask private key", "Error initializing server side").
Fix: Log the failure with WOLFSSL_MSG and record it in ssl->error so the application can observe it via wolfSSL_get_error().
| @@ -25272,15 +25304,37 @@ void FreeDer(DerBuffer** pDer) | |||
| { | |||
There was a problem hiding this comment.
🟠 [Medium] FreeDer infers "is this buffer reference counted" from possibly uninitialized memory in a public struct
DerBuffer is a public type (wolfssl/wolfcrypt/asn_public.h) and wc_FreeDer() is a public API, so applications do construct DerBuffer objects themselves. The new code decides whether a buffer participates in reference counting by reading the count out of the object:
int counted = (wolfSSL_RefCur(der->ref) > 0);The new doxygen text in doc/dox_comments/header_files/asn_public.h states "A DerBuffer the application built itself rather than through wc_AllocDer has no hold on it and is freed here" - but that is only true if the application zero-initialized the struct. If ref.count holds uninitialized garbage greater than zero, FreeDer() decrements instead of freeing and the buffer leaks silently; if the garbage happens to be 1, wolfSSL_RefFree() runs on an uninitialized wolfSSL_Ref, which for the mutex variant means destroying a mutex that was never initialized.
The two in-tree hand-built DerBuffers (wolfcrypt/src/evp_pk.c:1926, tests/api.c:4355) both XMEMSET to zero so they are fine today, but nothing in the public contract required that before this PR.
Fix: Either document the zero-initialization requirement in the public doxygen block for wc_FreeDer/DerBuffer, or replace the count-based inference with an explicit flag that only AllocDer() sets, so the behaviour does not depend on the contents of memory wolfSSL did not initialize.
| * @return 1 on success. | ||
| * @return 0 when the hold could not be taken. | ||
| */ | ||
| int AliasSslDer(DerBuffer** pDer, byte* weOwn, DerBuffer* src) |
There was a problem hiding this comment.
🟠 [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.
| } | ||
|
|
||
|
|
||
| /* A server whose certificate has been renewed on disk reloads it on the |
There was a problem hiding this comment.
🟠 [Medium] No test coverage for the reference counting mechanism itself or its conditional build paths
The PR's core mechanism - RefDer/FreeDer counting on DerBuffer - is only observed indirectly, and several of the riskiest changed paths have no coverage at all:
test_tls13_ctx_cert_rotationis compiled out entirely unlessWOLFSSL_DER_REFCOUNT || WOLFSSL_COPY_CERT, so theWOLFSSL_NO_DER_REFCOUNTbuild (the#elsebranch ofFreeSslDer, which still gates onweOwn) is never exercised for cert rotation.- The
WOLFSSL_DUAL_ALG_CERTSalt-key swap insrc/tls13.c:10478-10499is the trickiest new code in the PR - it aliaseskeytoaltKeyandkeyMasktoaltKeyMaskand takes a hold on each - and nothing tests it. I built--enable-experimental --enable-dual-alg-certsand confirmed no test touches that swap. WOLFSSL_BLIND_PRIVATE_KEYmask handling changed in four places (wolfSSL_certs_clear,wolfSSL_set_SSL_CTXx2,ProcessBufferPrivKeyHandleDer), including newly unconditionalFreeDer(&ssl->buffers.keyMask)calls and a newly addedssl->buffers.weOwnKey = 1. No test covers this configuration.wolfssl_add_to_chain()changed signature fromint weOwntobyte* weOwnand now writes*weOwn = 1itself; no test asserts the ownership flag afterwolfSSL_add0_chain_cert/wolfSSL_CTX_add1_chain_cert.
On the positive side, I ran valgrind --leak-check=full over the whole tls13 group on this branch and it was clean (0 failures, no definite leaks), and the four new TLS 1.3 tests do run and pass.
Fix: Add a direct unit test for AllocDer/RefDer/FreeDer counting that does not depend on a TLS handshake, plus a WOLFSSL_DUAL_ALG_CERTS test that drives SendTls13CertificateVerify down the WOLFSSL_CKS_SIGSPEC_ALTERNATIVE swap. Add --enable-experimental --enable-dual-alg-certs and a WOLFSSL_NO_DER_REFCOUNT build to CI for this change.
| #endif | ||
| } Buffers; | ||
|
|
||
| /* Every DER buffer an SSL object points at is held by it: either it allocated |
There was a problem hiding this comment.
🔵 [Low] internal.h comment overstates the ownership invariant
The new block comment claims: "These two calls are the only way to change one of those pointers, so the hold is always taken and always let go." That is not accurate - several paths assign the pointers directly after a FreeSslDer(), and one bypasses both helpers entirely:
src/ssl_load.c:1413ssl->buffers.altKey = *der;src/ssl_load.c:1445ssl->buffers.key = *der;src/ssl_load.c:2475ssl->buffers.certificate = der;src/ssl_load.c:5175*chain = newChain;src/tls13.c:10485ssl->buffers.key = ssl->buffers.altKey;wolfcrypt/src/port/maxim/maxq10xx.c:2396ssl->buffers.certificate = maxq_der;src/internal.c:7566-7583theWOLFSSL_COPY_CERTpath writes straight intossl->buffers.certificateviaAllocCopyDer()with no precedingFreeSslDer()
The intended invariant is really "the pointer is only ever released through FreeSslDer, and only ever aliased through AliasSslDer" - direct assignment of a freshly allocated buffer (which already carries its own hold from AllocDer) is fine and common. Stating it accurately makes the rule easier for the next person to follow.
Fix: Reword the comment to describe the release/alias rule rather than claiming exclusivity that the code does not have.
| * @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) |
There was a problem hiding this comment.
🔵 [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.
|
2 participants
Description
Another approach for fixing ZD 22107, superseding previous PR 10905.
Fixes a family of UAF due to shared DER buffer between WOLFSSL_CTX and WOLFSSL objects. Ref counters are used on the DER objects to know when they can be finally freed. DER objects now use two new APIs:
FreeSslDer(&der, &weOwn)andAliasSslDer(&der, &weOwn, src). For DH params, the DER buffer is copied.Footprint impact:
+4 bytes on 32-bit (no tail padding there); +712 B text
auto-enable never selects the mutex variant)
context has DH params set, 257 bytes for dh2048
Testing
New unit tests.
Checklist