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.
There was a problem hiding this comment.
added documentation
| * @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.
There was a problem hiding this comment.
reworded comment
| * @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.
There was a problem hiding this comment.
Not a public API, no need to mirror the public one. Documentation already in place
|
|
jenkins retest this please |
d0f1aea to
3d4fb40
Compare
3d4fb40 to
ce5508b
Compare
| if (((der->type == PRIVATEKEY_TYPE) || | ||
| (der->type == ALT_PRIVATEKEY_TYPE)) && der->buffer != NULL) { | ||
| ForceZero(der->buffer, der->length); | ||
| if (counted) { |
There was a problem hiding this comment.
dumb question: what scenario is this guarding against?
A scenario like this
- ctx created
i. ssl_1(ctx) created
ii. ssl_2(ctx) created (possibly on different threads)
iii. etc. - do work & cleanup ssls
i. free(ssl_1)
ii. free(ssl_2) (possibly on different threads)
iii. etc - free(ctx)
is already safe even with multithreading because ssl cleanup guards the FreeDer(ssl->thing) with ssl->weOwnThing.
If someone is modifying a ctx out from under an ssl, then that's naughty and we're not obligated to defensively code against it!
If we did want to, this would only be enough for serial protection, and we would need a mutex in the DerBuffer for concurrent protection. (the wolfSSL_Ref* stuff defers to atomics when available first, and then mutexes).
|
Not much benefit, #11191 has the fix. Closing |
4 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