-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix UAF in DH buffers #11115
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.
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
base: master
Are you sure you want to change the base?
Fix UAF in DH buffers #11115
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7624,6 +7624,58 @@ 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; | ||
|
|
||
| return 0; | ||
| } | ||
| #endif /* !NO_DH */ | ||
|
|
||
| int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) | ||
| { | ||
| int ret = WOLFSSL_SUCCESS; /* set default ret */ | ||
|
|
@@ -7812,8 +7864,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) | ||
|
|
@@ -9688,7 +9741,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); | ||
|
|
@@ -10077,13 +10129,8 @@ 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) { | ||
| 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); | ||
| ssl->buffers.serverDH_P.buffer = NULL; | ||
| } | ||
| /* The parameters (p,g) are kept: a renegotiation or a reused object needs | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the point of the PR, to keep the buffers
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the point of the PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 [Low] DH p/g retention in FreeHandshakeResources is unconditional, including for objects that re-parse them · Resource leaks on error paths The p/g free was dropped for every side. A client object retains the peer's prime and generator (up to Fix: Retain p/g only where they can be reused, e.g. keep the free when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 [Low] Reused server object keeps the previous peer's negotiated FFDHE group · TLS protocol issues
Related known finding #9094 (similar but distinct): Both concern FFDHE group handling during server negotiation, but this retains a prior connection's installed DH parameters through FreeHandshakeResources/wolfSSL_clear, whereas #9094 skips offered-group enforcement for custom-DH builds in DoClientHello. They have different faulting paths and require separate lifecycle versus negotiation fixes. Fix: Re-copy the context's parameters in |
||
| * them again, and they are freed with the object. */ | ||
| #endif /* !NO_DH */ | ||
|
|
||
| #if !defined(NO_CERTS) && !defined(OPENSSL_EXTRA) && \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1592,9 +1592,14 @@ void wolfSSL_set_accept_state(WOLFSSL* ssl) | |
|
|
||
| #ifndef NO_DH | ||
| if ((!ssl->options.haveDH) && (ssl->ctx->haveDH)) { | ||
| ssl->buffers.serverDH_P = ssl->ctx->serverDH_P; | ||
| ssl->buffers.serverDH_G = ssl->ctx->serverDH_G; | ||
| ssl->options.haveDH = 1; | ||
| /* Nothing to return the failure through, so leave the object | ||
| * without DH rather than claim parameters it does not have. */ | ||
| if (CopySSL_CTX_DhParams(ssl, ssl->ctx) == 0) { | ||
| ssl->options.haveDH = 1; | ||
| } | ||
| else { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ [Info] Allocation-failure branch of the new DH copy is untested · Missing edge-case coverage on a function the PR also changed The added tests only exercise the successful copy. Neither the Fix: Add a case that forces the allocation failure and assert |
||
| WOLFSSL_MSG("Unable to copy DH parameters from context"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ [Info] New DH-copy failure paths are not exercised by the added tests · Missing edge-case coverage on a function the PR also changed
Fix: Add a case under the existing allocation-failure facility ( |
||
| } | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -810,10 +810,15 @@ int test_wolfSSL_set_accept_state_reinit(void) | |
|
|
||
| wolfSSL_set_accept_state(ssl); | ||
|
|
||
| /* Picked up as the object's own copy, not the context's buffers. */ | ||
| if ((ssl != NULL) && (ctx != NULL)) { | ||
| ExpectIntEQ(ssl->options.haveDH, 1); | ||
| ExpectPtrEq(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer); | ||
| ExpectPtrEq(ssl->buffers.serverDH_G.buffer, ctx->serverDH_G.buffer); | ||
| ExpectNotNull(ssl->buffers.serverDH_P.buffer); | ||
| ExpectNotNull(ssl->buffers.serverDH_G.buffer); | ||
| ExpectPtrNE(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer); | ||
| ExpectPtrNE(ssl->buffers.serverDH_G.buffer, ctx->serverDH_G.buffer); | ||
| ExpectIntEQ(ssl->buffers.serverDH_P.length, ctx->serverDH_P.length); | ||
| ExpectIntEQ(ssl->buffers.serverDH_G.length, ctx->serverDH_G.length); | ||
| } | ||
|
|
||
| wolfSSL_free(ssl); | ||
|
|
@@ -968,8 +973,8 @@ int test_wolfSSL_SSL_do_handshake_quic(void) | |
| /* Test that wolfSSL_set_connect_state() discards server DH parameters. | ||
| * | ||
| * A client generates its own DH parameters, so any server ones are dropped. | ||
| * Parameters the object owns are freed; parameters merely borrowed from the | ||
| * context are only unlinked, since the context still owns them. | ||
| * The object holds its own copy either way, whether set on it directly or | ||
| * taken from the context, so the context is left untouched. | ||
| * | ||
| * @return TEST_SUCCESS on success. | ||
| */ | ||
|
|
@@ -1016,7 +1021,7 @@ int test_wolfSSL_set_connect_state_dh(void) | |
| wolfSSL_CTX_free(ctx); | ||
| ctx = NULL; | ||
|
|
||
| /* Parameters borrowed from the context are left for the context to free. */ | ||
| /* Parameters taken from the context are a copy the object owns. */ | ||
| ExpectNotNull(ctx = wolfSSL_CTX_new(wolfTLSv1_2_server_method())); | ||
| ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx, svrCertFile, | ||
| WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); | ||
|
|
@@ -1026,10 +1031,11 @@ int test_wolfSSL_set_connect_state_dh(void) | |
| WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS); | ||
| ExpectNotNull(ssl = wolfSSL_new(ctx)); | ||
|
|
||
| /* Inherited from the context, so not owned here. */ | ||
| if (ssl != NULL) { | ||
| ExpectPtrEq(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer); | ||
| ExpectIntEQ(ssl->buffers.weOwnDH, 0); | ||
| /* Copied from the context, so owned here. */ | ||
| if ((ssl != NULL) && (ctx != NULL)) { | ||
| ExpectNotNull(ssl->buffers.serverDH_P.buffer); | ||
| ExpectPtrNE(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer); | ||
| ExpectIntEQ(ssl->buffers.weOwnDH, 1); | ||
| } | ||
|
|
||
| wolfSSL_set_connect_state(ssl); | ||
|
|
@@ -1044,8 +1050,9 @@ int test_wolfSSL_set_connect_state_dh(void) | |
| ExpectNotNull(ctx->serverDH_G.buffer); | ||
| } | ||
| ExpectNotNull(ssl2 = wolfSSL_new(ctx)); | ||
| if (ssl2 != NULL) { | ||
| ExpectPtrEq(ssl2->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer); | ||
| if ((ssl2 != NULL) && (ctx != NULL)) { | ||
| ExpectNotNull(ssl2->buffers.serverDH_P.buffer); | ||
| ExpectPtrNE(ssl2->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer); | ||
| } | ||
|
|
||
| wolfSSL_free(ssl2); | ||
|
|
@@ -1055,6 +1062,129 @@ int test_wolfSSL_set_connect_state_dh(void) | |
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| /* DH parameters taken from the context are the object's own copy, and they | ||
| * last as long as the object does. | ||
| * | ||
| * The copy is what lets the context replace its parameters while sessions are | ||
| * running. Since the session then holds the only copy it will ever have, the | ||
| * end of a handshake must not take it away: a reused object needs it again. | ||
| * | ||
| * @return TEST_SUCCESS on success. | ||
| */ | ||
| int test_wolfSSL_dh_ctx_params_reuse(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| /* Handing the object a second connection needs its certificate and key to | ||
| * outlast the first handshake, which only these builds arrange. */ | ||
| #if (defined(OPENSSL_EXTRA) || defined(WOLFSSL_WPAS_SMALL)) && \ | ||
| defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ | ||
| defined(BUILD_TLS_DHE_RSA_WITH_AES_128_GCM_SHA256) && \ | ||
| !defined(WOLFSSL_NO_TLS12) && !defined(NO_DH) && !defined(NO_RSA) && \ | ||
| !defined(NO_FILESYSTEM) && !defined(NO_WOLFSSL_SERVER) && \ | ||
| !defined(NO_WOLFSSL_CLIENT) | ||
| struct test_memio_ctx test_ctx; | ||
| WOLFSSL_CTX* ctx_c = NULL; | ||
| WOLFSSL_CTX* ctx_s = NULL; | ||
| WOLFSSL* ssl_c = NULL; | ||
| WOLFSSL* ssl_s = NULL; | ||
| const byte* startP = NULL; | ||
| const byte* startG = NULL; | ||
|
|
||
| XMEMSET(&test_ctx, 0, sizeof(test_ctx)); | ||
| test_ctx.c_ciphers = test_ctx.s_ciphers = "DHE-RSA-AES128-GCM-SHA256"; | ||
|
|
||
| /* The parameters go on the context before any session takes them. A | ||
| * context made here is used as-is, so it gets the credentials and the | ||
| * memio wiring that test_memio_setup would otherwise have given it. */ | ||
| ExpectNotNull(ctx_s = wolfSSL_CTX_new(wolfTLSv1_2_server_method())); | ||
| ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx_s, svrKeyFile, | ||
| CERT_FILETYPE), WOLFSSL_SUCCESS); | ||
| ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx_s, svrCertFile, | ||
| CERT_FILETYPE), WOLFSSL_SUCCESS); | ||
| ExpectIntEQ(wolfSSL_CTX_set_cipher_list(ctx_s, test_ctx.s_ciphers), | ||
| 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, CERT_FILETYPE), | ||
| WOLFSSL_SUCCESS); | ||
|
|
||
| ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, | ||
| wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); | ||
|
|
||
| /* test_memio_setup puts its own parameters on the server session, which | ||
| * would stand in for the ones under test. Take a fresh session from the | ||
| * same context so the parameters really are the context's. */ | ||
| wolfSSL_free(ssl_s); | ||
| ssl_s = NULL; | ||
| ExpectNotNull(ssl_s = wolfSSL_new(ctx_s)); | ||
| wolfSSL_SetIOWriteCtx(ssl_s, &test_ctx); | ||
| wolfSSL_SetIOReadCtx(ssl_s, &test_ctx); | ||
|
|
||
| /* Its own copy, matching the context's but not the same buffers. */ | ||
| if ((ssl_s != NULL) && (ctx_s != NULL)) { | ||
| ExpectNotNull(ssl_s->buffers.serverDH_P.buffer); | ||
| ExpectNotNull(ssl_s->buffers.serverDH_G.buffer); | ||
| ExpectPtrNE(ssl_s->buffers.serverDH_P.buffer, ctx_s->serverDH_P.buffer); | ||
| ExpectPtrNE(ssl_s->buffers.serverDH_G.buffer, ctx_s->serverDH_G.buffer); | ||
| ExpectIntEQ(ssl_s->buffers.serverDH_P.length, ctx_s->serverDH_P.length); | ||
| ExpectIntEQ(ssl_s->buffers.serverDH_G.length, ctx_s->serverDH_G.length); | ||
| ExpectIntEQ(ssl_s->buffers.weOwnDH, 1); | ||
| startP = ssl_s->buffers.serverDH_P.buffer; | ||
| startG = ssl_s->buffers.serverDH_G.buffer; | ||
| } | ||
|
|
||
| /* Offering no FFDHE group keeps the server on the parameters it was | ||
| * given, rather than switching to a named group and dropping them. A build | ||
| * without supported curves sends no groups at all, so there is nothing to | ||
| * narrow down. */ | ||
| #ifdef HAVE_SUPPORTED_CURVES | ||
| ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding missing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding missing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| WOLFSSL_SUCCESS); | ||
| #endif | ||
|
|
||
| ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); | ||
|
|
||
| /* The handshake used them and the cleanup after it left them alone. */ | ||
| if (ssl_s != NULL) { | ||
| ExpectPtrEq(ssl_s->buffers.serverDH_P.buffer, startP); | ||
| ExpectPtrEq(ssl_s->buffers.serverDH_G.buffer, startG); | ||
| } | ||
|
|
||
| /* So the object can be handed a second connection. */ | ||
| ExpectIntEQ(wolfSSL_clear(ssl_s), WOLFSSL_SUCCESS); | ||
| wolfSSL_free(ssl_c); | ||
| ssl_c = NULL; | ||
| ExpectNotNull(ssl_c = wolfSSL_new(ctx_c)); | ||
| wolfSSL_SetIOWriteCtx(ssl_c, &test_ctx); | ||
| wolfSSL_SetIOReadCtx(ssl_c, &test_ctx); | ||
| #ifdef HAVE_SUPPORTED_CURVES | ||
| ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1), | ||
| WOLFSSL_SUCCESS); | ||
| #endif | ||
| test_memio_clear_buffer(&test_ctx, 0); | ||
| test_memio_clear_buffer(&test_ctx, 1); | ||
|
|
||
| /* The second handshake ran on the parameters the first one left behind. */ | ||
| ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); | ||
| if (ssl_s != NULL) { | ||
| ExpectPtrEq(ssl_s->buffers.serverDH_P.buffer, startP); | ||
| ExpectPtrEq(ssl_s->buffers.serverDH_G.buffer, startG); | ||
| } | ||
|
|
||
| /* And the context still has its own to hand out. */ | ||
| if (ctx_s != NULL) { | ||
| ExpectNotNull(ctx_s->serverDH_P.buffer); | ||
| ExpectNotNull(ctx_s->serverDH_G.buffer); | ||
| } | ||
|
|
||
| wolfSSL_free(ssl_c); | ||
| wolfSSL_free(ssl_s); | ||
| wolfSSL_CTX_free(ctx_c); | ||
| wolfSSL_CTX_free(ctx_s); | ||
| #endif | ||
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| /* The helpers below only install I/O callbacks, so they need nothing beyond | ||
| * TLS 1.2 itself. The guard is nevertheless the union of their six callers' | ||
| * guards, so the block is neither compiled without a caller nor missing when | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 [Low] DH parameter copy reads context buffers without synchronization · Use-after-free / double-free
CopySSL_CTX_DhParams()readsctx->serverDH_P/Gunder no lock, whilewolfssl_ctx_set_tmp_dh()(src/ssl_load.c:6028-6034) frees the old buffers and then stores the new pointer and length separately. AwolfSSL_CTX_SetTmpDH*()concurrent withwolfSSL_new()makes thisXMEMCPYread freed memory, or a new pointer with the previous larger length (heap over-read into the session's prime). Adjacent to known finding #8917, which covers the certificate/private-key buffers inSetSSL_CTX_CertsAndKeys— different fields and a different fix site.Related known finding #6557 (similar but distinct): Both are unsynchronized reads of shared mutable state in src/internal.c, but this copies ctx DH buffers during CopySSL_CTX_DhParams while #6557 decrypts with ticket key state in DefTicketEncCb. The raced operations, objects, root ownership, and required locks/fixes differ.
Fix: Guard context DH parameter replacement and this copy with a CTX-level lock, or reference-count the parameter buffers.