Skip to content

Commit ba20e38

Browse files
embhornembhorn
committed
Fix DupSSL issue with Poly1305 auth
1 parent 1c9555c commit ba20e38

2 files changed

Lines changed: 212 additions & 5 deletions

File tree

‎src/ssl.c‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -863,6 +863,7 @@ void FreeWriteDup(WOLFSSL* ssl)
863863
#endif /* WOLFSSL_TLS13 && WOLFSSL_POST_HANDSHAKE_AUTH */
864864
wc_FreeMutex(&ssl->dupWrite->dupMutex);
865865
XFREE(ssl->dupWrite, ssl->heap, DYNAMIC_TYPE_WRITEDUP);
866+
ssl->dupWrite = NULL;
866867
WOLFSSL_MSG("Did WriteDup full free, count to zero");
867868
}
868869
}
@@ -879,6 +880,11 @@ void FreeWriteDup(WOLFSSL* ssl)
879880
static int DupSSL(WOLFSSL* dup, WOLFSSL* ssl)
880881
{
881882
word16 tmp_weOwnRng;
883+
#ifdef HAVE_ONE_TIME_AUTH
884+
#ifdef HAVE_POLY1305
885+
Poly1305* tmp_poly1305 = NULL;
886+
#endif
887+
#endif
882888

883889
/* shared dupWrite setup */
884890
ssl->dupWrite = (WriteDup*)XMALLOC(sizeof(WriteDup), ssl->heap,
@@ -893,6 +899,27 @@ static int DupSSL(WOLFSSL* dup, WOLFSSL* ssl)
893899
ssl->dupWrite = NULL;
894900
return BAD_MUTEX_E;
895901
}
902+
903+
/* Pre-allocate any objects that can fail BEFORE performing destructive
904+
* state mutations on ssl, so an allocation failure cannot leave ssl
905+
* with a zeroed encrypt context and a poisoned dupWrite.
906+
* dup->heap == ssl->heap here because dup was initialised with ssl->ctx;
907+
* use ssl->heap consistently for cleanup symmetry. */
908+
#ifdef HAVE_ONE_TIME_AUTH
909+
#ifdef HAVE_POLY1305
910+
if (ssl->auth.setup && ssl->auth.poly1305 != NULL) {
911+
tmp_poly1305 = (Poly1305*)XMALLOC(sizeof(Poly1305), ssl->heap,
912+
DYNAMIC_TYPE_CIPHER);
913+
if (tmp_poly1305 == NULL) {
914+
wc_FreeMutex(&ssl->dupWrite->dupMutex);
915+
XFREE(ssl->dupWrite, ssl->heap, DYNAMIC_TYPE_WRITEDUP);
916+
ssl->dupWrite = NULL;
917+
return MEMORY_E;
918+
}
919+
}
920+
#endif
921+
#endif
922+
896923
ssl->dupWrite->dupCount = 2; /* both sides have a count to start */
897924
dup->dupWrite = ssl->dupWrite; /* each side uses */
898925

@@ -911,11 +938,8 @@ static int DupSSL(WOLFSSL* dup, WOLFSSL* ssl)
911938

912939
#ifdef HAVE_ONE_TIME_AUTH
913940
#ifdef HAVE_POLY1305
914-
if (ssl->auth.setup && ssl->auth.poly1305 != NULL) {
915-
dup->auth.poly1305 = (Poly1305*)XMALLOC(sizeof(Poly1305), dup->heap,
916-
DYNAMIC_TYPE_CIPHER);
917-
if (dup->auth.poly1305 == NULL)
918-
return MEMORY_E;
941+
if (tmp_poly1305 != NULL) {
942+
dup->auth.poly1305 = tmp_poly1305;
919943
dup->auth.setup = 1;
920944
}
921945
#endif

‎tests/api.c‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34824,6 +34824,188 @@ static int test_write_dup_want_write_simul(void)
3482434824
return EXPECT_RESULT();
3482534825
}
3482634826

34827+
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(HAVE_WRITE_DUP) && \
34828+
defined(HAVE_CHACHA) && defined(HAVE_POLY1305) && !defined(NO_SHA256) && \
34829+
(defined(HAVE_ECC) || defined(HAVE_CURVE25519) || \
34830+
defined(HAVE_CURVE448)) && \
34831+
!defined(NO_RSA) && defined(USE_WOLFSSL_MEMORY) && \
34832+
!defined(WOLFSSL_NO_MALLOC) && !defined(WOLFSSL_STATIC_MEMORY) && \
34833+
!defined(WOLFSSL_KERNEL_MODE) && !defined(WOLFSSL_NO_TLS12) && \
34834+
!defined(NO_TLS)
34835+
#include <wolfssl/wolfcrypt/poly1305.h>
34836+
34837+
/* Custom allocator state for test_write_dup_oom.
34838+
* The allocator counts allocations of size oom_match_size (set to
34839+
* sizeof(Poly1305) by the test). The Nth such allocation is forced to
34840+
* fail if oom_fail_at_match == N; oom_failed is then set so callers can
34841+
* confirm the fault was actually injected. The test runs in two phases:
34842+
* 1) Measure: oom_fail_at_match = 0 (no failures), record oom_match_count
34843+
* after a successful wolfSSL_write_dup(). The Poly1305 alloc in
34844+
* DupSSL() is the LAST sizeof(Poly1305) allocation in the call path,
34845+
* so that count is the index that targets it.
34846+
* 2) Trigger: oom_fail_at_match = recorded count, run write_dup again on
34847+
* a fresh handshake. The Nth match is the Poly1305 alloc, so the
34848+
* failure exercises exactly the bug under test. */
34849+
static size_t oom_match_size = 0;
34850+
static int oom_match_count = 0;
34851+
static int oom_fail_at_match = 0;
34852+
static int oom_failed = 0;
34853+
34854+
#ifdef WOLFSSL_DEBUG_MEMORY
34855+
static void* oom_malloc_cb(size_t size, const char* func, unsigned int line)
34856+
{
34857+
(void)func;
34858+
(void)line;
34859+
#else
34860+
static void* oom_malloc_cb(size_t size)
34861+
{
34862+
#endif
34863+
if (!oom_failed && oom_match_size != 0 && size == oom_match_size) {
34864+
oom_match_count++;
34865+
if (oom_fail_at_match != 0 && oom_match_count == oom_fail_at_match) {
34866+
oom_failed = 1;
34867+
return NULL;
34868+
}
34869+
}
34870+
return malloc(size);
34871+
}
34872+
34873+
#ifdef WOLFSSL_DEBUG_MEMORY
34874+
static void oom_free_cb(void* ptr, const char* func, unsigned int line)
34875+
{
34876+
(void)func;
34877+
(void)line;
34878+
#else
34879+
static void oom_free_cb(void* ptr)
34880+
{
34881+
#endif
34882+
free(ptr);
34883+
}
34884+
34885+
#ifdef WOLFSSL_DEBUG_MEMORY
34886+
static void* oom_realloc_cb(void* ptr, size_t size, const char* func,
34887+
unsigned int line)
34888+
{
34889+
(void)func;
34890+
(void)line;
34891+
#else
34892+
static void* oom_realloc_cb(void* ptr, size_t size)
34893+
{
34894+
#endif
34895+
return realloc(ptr, size);
34896+
}
34897+
#endif
34898+
34899+
/* Regression test for the DupSSL() error-path bug: an allocation failure on
34900+
* the Poly1305 alloc must not leave the original ssl object with a zeroed
34901+
* encrypt context or a non-NULL ssl->dupWrite.
34902+
*
34903+
* Pass 0 measures the count of sizeof(Poly1305) allocations during a
34904+
* successful wolfSSL_write_dup(); pass 1 fails the same-indexed allocation
34905+
* on a fresh handshake to deterministically target the DupSSL() Poly1305
34906+
* alloc regardless of any incidental size collisions. */
34907+
static int test_write_dup_oom(void)
34908+
{
34909+
EXPECT_DECLS;
34910+
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(HAVE_WRITE_DUP) && \
34911+
defined(HAVE_CHACHA) && defined(HAVE_POLY1305) && !defined(NO_SHA256) && \
34912+
(defined(HAVE_ECC) || defined(HAVE_CURVE25519) || \
34913+
defined(HAVE_CURVE448)) && \
34914+
!defined(NO_RSA) && defined(USE_WOLFSSL_MEMORY) && \
34915+
!defined(WOLFSSL_NO_MALLOC) && !defined(WOLFSSL_STATIC_MEMORY) && \
34916+
!defined(WOLFSSL_KERNEL_MODE) && !defined(WOLFSSL_NO_TLS12) && \
34917+
!defined(NO_TLS)
34918+
char hiWorld[] = "dup message";
34919+
char readData[sizeof(hiWorld) + 5];
34920+
wolfSSL_Malloc_cb prev_mc = NULL;
34921+
wolfSSL_Free_cb prev_fc = NULL;
34922+
wolfSSL_Realloc_cb prev_rc = NULL;
34923+
int allocators_set = 0;
34924+
int target_match = 0;
34925+
int pass;
34926+
34927+
ExpectIntEQ(wolfSSL_GetAllocators(&prev_mc, &prev_fc, &prev_rc), 0);
34928+
ExpectIntEQ(wolfSSL_SetAllocators(oom_malloc_cb, oom_free_cb,
34929+
oom_realloc_cb), 0);
34930+
if (EXPECT_SUCCESS())
34931+
allocators_set = 1;
34932+
34933+
for (pass = 0; pass < 2 && EXPECT_SUCCESS(); pass++) {
34934+
struct test_memio_ctx test_ctx;
34935+
WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
34936+
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
34937+
WOLFSSL *ssl_c2 = NULL;
34938+
34939+
XMEMSET(&test_ctx, 0, sizeof(test_ctx));
34940+
test_ctx.c_ciphers = test_ctx.s_ciphers =
34941+
"ECDHE-RSA-CHACHA20-POLY1305";
34942+
34943+
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
34944+
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
34945+
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
34946+
34947+
/* Start counting sizeof(Poly1305) allocations only now, after the
34948+
* handshake, so the count reflects only allocations on the
34949+
* wolfSSL_write_dup() code path. */
34950+
oom_match_size = sizeof(Poly1305);
34951+
oom_match_count = 0;
34952+
oom_failed = 0;
34953+
oom_fail_at_match = (pass == 0) ? 0 : target_match;
34954+
34955+
if (pass == 0) {
34956+
ExpectNotNull(ssl_c2 = wolfSSL_write_dup(ssl_c));
34957+
target_match = oom_match_count;
34958+
/* Sanity: at least one Poly1305 alloc must occur on this path,
34959+
* otherwise the test setup was wrong (feature compiled out, or
34960+
* negotiated cipher does not use Poly1305). */
34961+
ExpectIntGE(target_match, 1);
34962+
}
34963+
else {
34964+
ExpectNull(wolfSSL_write_dup(ssl_c));
34965+
/* Confirm the targeted Poly1305 allocation actually failed. */
34966+
ExpectIntEQ(oom_failed, 1);
34967+
/* Stop further failures so the recovery path can run. */
34968+
oom_fail_at_match = 0;
34969+
34970+
/* The original ssl_c must NOT be poisoned: the WriteDup must have
34971+
* been cleaned up, the encrypt cipher context must still be
34972+
* intact, and a fresh wolfSSL_write_dup() must succeed. */
34973+
ExpectNull(ssl_c->dupWrite);
34974+
ExpectNotNull(ssl_c2 = wolfSSL_write_dup(ssl_c));
34975+
34976+
/* Round-trip data both directions to confirm both sides still
34977+
* encrypt with the original ssl_c context preserved. */
34978+
ExpectIntEQ(wolfSSL_write(ssl_s, hiWorld, sizeof(hiWorld)),
34979+
sizeof(hiWorld));
34980+
ExpectIntEQ(wolfSSL_read(ssl_c, readData, sizeof(readData)),
34981+
sizeof(hiWorld));
34982+
ExpectIntEQ(wolfSSL_write(ssl_c2, hiWorld, sizeof(hiWorld)),
34983+
sizeof(hiWorld));
34984+
ExpectIntEQ(wolfSSL_read(ssl_s, readData, sizeof(readData)),
34985+
sizeof(hiWorld));
34986+
}
34987+
34988+
/* Disable matching/failure during teardown so frees and any internal
34989+
* allocations of the same size during cleanup are unaffected. */
34990+
oom_match_size = 0;
34991+
oom_fail_at_match = 0;
34992+
34993+
wolfSSL_free(ssl_c);
34994+
wolfSSL_free(ssl_c2);
34995+
wolfSSL_free(ssl_s);
34996+
wolfSSL_CTX_free(ctx_c);
34997+
wolfSSL_CTX_free(ctx_s);
34998+
}
34999+
35000+
/* Restore previous allocators only after every object allocated under the
35001+
* test allocators has been freed, so allocator bookkeeping (in builds
35002+
* that wrap the default allocators) is not desynchronised. */
35003+
if (allocators_set)
35004+
(void)wolfSSL_SetAllocators(prev_mc, prev_fc, prev_rc);
35005+
#endif
35006+
return EXPECT_RESULT();
35007+
}
35008+
3482735009
static int test_read_write_hs(void)
3482835010
{
3482935011

@@ -37660,6 +37842,7 @@ TEST_CASE testCases[] = {
3766037842
TEST_DECL(test_write_dup),
3766137843
TEST_DECL(test_write_dup_want_write),
3766237844
TEST_DECL(test_write_dup_want_write_simul),
37845+
TEST_DECL(test_write_dup_oom),
3766337846
TEST_DECL(test_read_write_hs),
3766437847
TEST_DECL(test_get_signature_nid),
3766537848
#ifndef WOLFSSL_TEST_APPLE_NATIVE_CERT_VALIDATION

0 commit comments

Comments
 (0)