Fix UAF in DH buffers - #11115
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11115
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 10
10 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
|
|
||
| /* Offering no FFDHE group keeps the server on the parameters it was | ||
| * given, rather than switching to a named group and dropping them. */ | ||
| ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1), |
There was a problem hiding this comment.
🟠 [Medium] New test calls wolfSSL_UseSupportedCurve() without a HAVE_SUPPORTED_CURVES guard · Incorrect feature flag gating
wolfSSL_UseSupportedCurve() is declared only under #ifdef HAVE_SUPPORTED_CURVES (wolfssl/ssl.h:4989). The new test's guard covers DH/RSA/TLS1.2/memio but not HAVE_SUPPORTED_CURVES, so a build such as --disable-tls13 --disable-ecc --disable-curve25519 --disable-curve448 --enable-dh compiles the call with no declaration and breaks the test build.
Fix: Wrap both wolfSSL_UseSupportedCurve() calls in #ifdef HAVE_SUPPORTED_CURVES, or add defined(HAVE_SUPPORTED_CURVES) to the test's feature guard.
| XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY); | ||
| ssl->buffers.serverDH_P.buffer = NULL; | ||
| } | ||
| /* The parameters (p,g) are kept: a renegotiation or a reused object needs |
There was a problem hiding this comment.
🔵 [Low] FreeHandshakeResources retains DH p/g for every object, not just context-derived ones · Resource leaks
The free of serverDH_P/serverDH_G is removed unconditionally, so the prime and generator are now held for the whole lifetime of every WOLFSSL object. Retention is only required for the context-copied case; client objects re-parse the peer's parameters in GetDhPublicKey() on every handshake, and FFDHE servers re-derive them in TLSX_SupportedFFDHE_Set(), so those allocations are held post-handshake for no purpose.
Related known finding #3479 (similar but distinct): Both concern post-handshake resource handling in src/internal.c, but the functions differ (FreeHandshakeResources vs FreeArrays), as do the faulting operations (retaining serverDH_P/serverDH_G allocations vs failing to scrub preMasterSecret before free) and root causes. This requires restoring conditional DH-parameter frees, whereas issue 3479 concerns ForceZero coverage; one patch would not fix both.
Fix: Track whether the parameters came from the context and keep only those, freeing peer-supplied and FFDHE-derived parameters at handshake completion as before.
|
|
||
| /* Offering no FFDHE group keeps the server on the parameters it was | ||
| * given, rather than switching to a named group and dropping them. */ | ||
| ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1), |
There was a problem hiding this comment.
🟠 [Medium] New memio test calls wolfSSL_UseSupportedCurve() without HAVE_SUPPORTED_CURVES guard · Conditional compilation / build break
wolfSSL_UseSupportedCurve() is declared only under HAVE_SUPPORTED_CURVES (wolfssl/ssl.h:4989), but the test's guard omits it. A build with --enable-supportedcurves=no plus TLS 1.2 + DHE-RSA satisfies the guard and fails to compile the test suite.
Fix: Add defined(HAVE_SUPPORTED_CURVES) to the test's preprocessor guard, as test_wolfSSL_UseSupportedCurve in tests/api.c does.
| WOLFSSL_SUCCESS); | ||
| wolfSSL_SetIORecv(ctx_s, test_memio_read_cb); | ||
| wolfSSL_SetIOSend(ctx_s, test_memio_write_cb); | ||
| ExpectIntEQ(wolfSSL_CTX_SetTmpDH_file(ctx_s, dhParamFile, |
There was a problem hiding this comment.
🟠 [Medium] New memio test hardcodes WOLFSSL_FILETYPE_PEM without WOLFSSL_PEM_TO_DER in its guard · Hardcoded environment dependencies
Without WOLFSSL_PEM_TO_DER, dhParamFile, svrKeyFile and svrCertFile expand to .der paths (wolfssl/test.h:610-624), so every load call in this test passes WOLFSSL_FILETYPE_PEM for DER files and fails. The sibling test_wolfSSL_set_connect_state_dh guards on defined(WOLFSSL_PEM_TO_DER).
Fix: Use CERT_FILETYPE instead of WOLFSSL_FILETYPE_PEM for all three file loads, or add defined(WOLFSSL_PEM_TO_DER) to the guard.
| wolfSSL_SetIOReadCtx(ssl_c, &test_ctx); | ||
| ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1), | ||
| WOLFSSL_SUCCESS); | ||
| test_ctx.c_len = test_ctx.s_len = 0; |
There was a problem hiding this comment.
🔵 [Low] Second handshake reuses memio context with only lengths reset · Race conditions in setup/teardown
Only c_len/s_len are cleared before the second handshake; c_msg_pos, c_msg_count, s_msg_pos, s_msg_count and the force-want-write flags keep first-handshake state. Any residual message bookkeeping makes test_memio_read_cb index a stale msg_sizes[] entry, returning WOLFSSL_CBIO_ERR_GENERAL or a desynced record.
Fix: Call test_memio_clear_buffer(&test_ctx, 0) and test_memio_clear_buffer(&test_ctx, 1) instead.
| int test_tls13_ctx_dh_rotation(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_TLS13) && !defined(NO_DH) && !defined(NO_RSA) && \ |
There was a problem hiding this comment.
🔵 [Low] New TLS 1.3 DH tests omit !defined(NO_CERTS) from their guards · Conditional compilation / build break
Both new tests call wolfSSL_CTX_use_certificate_file() and wolfSSL_CTX_use_PrivateKey_file(), declared only under !NO_FILESYSTEM && !NO_CERTS (wolfssl/ssl.h:1352-1354), while their guards check only !NO_FILESYSTEM. test_tls13_accept_state_dh_copy at line 3969 has the same gap.
Fix: Add !defined(NO_CERTS) to both guards, matching test_tls13_pha.
| test_ctx.c_len = test_ctx.s_len = 0; | ||
|
|
||
| ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); | ||
| ExpectIntNE(wolfSSL_get_error(ssl_s, 0), WC_NO_ERR_TRACE(NO_DH_PARAMS)); |
There was a problem hiding this comment.
⚪ [Info] NO_DH_PARAMS assertion can never fail · Dead error handling
ExpectInt no-ops once a prior expectation failed, so this line runs only when the preceding test_memio_do_handshake() returned 0, at which point wolfSSL_get_error(ssl_s, 0) is 0. The check gives an appearance of coverage for the NO_DH_PARAMS regression it names without ever being able to detect it.
Related known finding #7089 (similar but distinct): Both concern always-decided checks that create illusory error coverage, but this is an ExpectInt-gated TLS handshake test assertion in test_wolfSSL_dh_ctx_params_reuse, whereas issue 7089 covers separate CMAC implementation guards; the faulting operations and required patches differ.
Fix: Drop the redundant line, or assert the retained buffer pointers still equal startP/startG after the second handshake instead.
|
|
||
| /* Offering no FFDHE group keeps the server on the parameters it was | ||
| * given, rather than switching to a named group and dropping them. */ | ||
| ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1), |
There was a problem hiding this comment.
🔵 [Low] New memio DH test calls wolfSSL_UseSupportedCurve without a HAVE_SUPPORTED_CURVES guard · Preprocessor-conditional security bypass
wolfSSL_UseSupportedCurve is declared only under HAVE_SUPPORTED_CURVES (wolfssl/ssl.h:4989), but the test's guard block requires only DHE/RSA/TLS1.2 macros. A build such as --disable-ecc --disable-curve25519 --disable-curve448 --disable-tls13 with DH enabled fails to compile the test suite.
Fix: Add defined(HAVE_SUPPORTED_CURVES) to the test's #if guard, matching test_wolfSSL_UseSupportedCurve in tests/api.c.
| int test_tls13_ctx_dh_rotation(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_TLS13) && !defined(NO_DH) && !defined(NO_RSA) && \ |
There was a problem hiding this comment.
🔵 [Low] New TLS 1.3 DH tests use certificate-file APIs without a !NO_CERTS guard · Preprocessor-conditional security bypass
wolfSSL_CTX_use_certificate_file/wolfSSL_CTX_use_PrivateKey_file are declared only under !NO_FILESYSTEM && !NO_CERTS (wolfssl/ssl.h:1352), but neither test_tls13_ctx_dh_rotation nor test_tls13_accept_state_dh_copy (line 3969) includes !defined(NO_CERTS), so a NO_CERTS build that keeps RSA and DH fails to compile.
Fix: Add !defined(NO_CERTS) to both new test guards, as every other cert-using test in test_tls13.c does.
| XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY); | ||
| ssl->buffers.serverDH_P.buffer = NULL; | ||
| } | ||
| /* The parameters (p,g) are kept: a renegotiation or a reused object needs |
There was a problem hiding this comment.
⚪ [Info] DH parameters now retained for the whole object lifetime instead of the handshake · Resource leaks
Dropping the p/g free means every session keeps its own copy (256-512 bytes for 2048-4096-bit params) after the handshake completes, where previously ctx-derived params cost nothing per session and ssl-owned ones were released. Steady-state memory grows with concurrent connection count. They are public values, so no key material is retained.
Fix: Consider freeing p/g here and re-invoking CopySSL_CTX_DhParams from ReinitSSL so reused objects still get parameters.
|
2 participants
Description
Fixes the case where
SetSSL_CTXaliases theserverDH_PandserverDH_Gbuffers withweOwnDHstill at 0. A subsequent call towolfSSL_CTX_SetTmpDH*would result in a dangling pointer.Related to #11109, but with DH buffers. Note that refcounting is not performed here since the API uses plain byte buffers instead of structs/objects.
Testing
Added unit tests.
Checklist