Skip to content

Commit c278b61

Browse files
dgarskedgarske
authored
Merge pull request #10337 from embhorn/zd21709
Fix DupSSL issue with Poly1305 auth
2 parents d793452 + ba20e38 commit c278b61

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
@@ -860,6 +860,7 @@ void FreeWriteDup(WOLFSSL* ssl)
860860
#endif /* WOLFSSL_TLS13 && WOLFSSL_POST_HANDSHAKE_AUTH */
861861
wc_FreeMutex(&ssl->dupWrite->dupMutex);
862862
XFREE(ssl->dupWrite, ssl->heap, DYNAMIC_TYPE_WRITEDUP);
863+
ssl->dupWrite = NULL;
863864
WOLFSSL_MSG("Did WriteDup full free, count to zero");
864865
}
865866
}
@@ -876,6 +877,11 @@ void FreeWriteDup(WOLFSSL* ssl)
876877
static int DupSSL(WOLFSSL* dup, WOLFSSL* ssl)
877878
{
878879
word16 tmp_weOwnRng;
880+
#ifdef HAVE_ONE_TIME_AUTH
881+
#ifdef HAVE_POLY1305
882+
Poly1305* tmp_poly1305 = NULL;
883+
#endif
884+
#endif
879885

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

@@ -908,11 +935,8 @@ static int DupSSL(WOLFSSL* dup, WOLFSSL* ssl)
908935

909936
#ifdef HAVE_ONE_TIME_AUTH
910937
#ifdef HAVE_POLY1305
911-
if (ssl->auth.setup && ssl->auth.poly1305 != NULL) {
912-
dup->auth.poly1305 = (Poly1305*)XMALLOC(sizeof(Poly1305), dup->heap,
913-
DYNAMIC_TYPE_CIPHER);
914-
if (dup->auth.poly1305 == NULL)
915-
return MEMORY_E;
938+
if (tmp_poly1305 != NULL) {
939+
dup->auth.poly1305 = tmp_poly1305;
916940
dup->auth.setup = 1;
917941
}
918942
#endif

‎tests/api.c‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34909,6 +34909,188 @@ static int test_write_dup_want_write_simul(void)
3490934909
return EXPECT_RESULT();
3491034910
}
3491134911

34912+
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(HAVE_WRITE_DUP) && \
34913+
defined(HAVE_CHACHA) && defined(HAVE_POLY1305) && !defined(NO_SHA256) && \
34914+
(defined(HAVE_ECC) || defined(HAVE_CURVE25519) || \
34915+
defined(HAVE_CURVE448)) && \
34916+
!defined(NO_RSA) && defined(USE_WOLFSSL_MEMORY) && \
34917+
!defined(WOLFSSL_NO_MALLOC) && !defined(WOLFSSL_STATIC_MEMORY) && \
34918+
!defined(WOLFSSL_KERNEL_MODE) && !defined(WOLFSSL_NO_TLS12) && \
34919+
!defined(NO_TLS)
34920+
#include <wolfssl/wolfcrypt/poly1305.h>
34921+
34922+
/* Custom allocator state for test_write_dup_oom.
34923+
* The allocator counts allocations of size oom_match_size (set to
34924+
* sizeof(Poly1305) by the test). The Nth such allocation is forced to
34925+
* fail if oom_fail_at_match == N; oom_failed is then set so callers can
34926+
* confirm the fault was actually injected. The test runs in two phases:
34927+
* 1) Measure: oom_fail_at_match = 0 (no failures), record oom_match_count
34928+
* after a successful wolfSSL_write_dup(). The Poly1305 alloc in
34929+
* DupSSL() is the LAST sizeof(Poly1305) allocation in the call path,
34930+
* so that count is the index that targets it.
34931+
* 2) Trigger: oom_fail_at_match = recorded count, run write_dup again on
34932+
* a fresh handshake. The Nth match is the Poly1305 alloc, so the
34933+
* failure exercises exactly the bug under test. */
34934+
static size_t oom_match_size = 0;
34935+
static int oom_match_count = 0;
34936+
static int oom_fail_at_match = 0;
34937+
static int oom_failed = 0;
34938+
34939+
#ifdef WOLFSSL_DEBUG_MEMORY
34940+
static void* oom_malloc_cb(size_t size, const char* func, unsigned int line)
34941+
{
34942+
(void)func;
34943+
(void)line;
34944+
#else
34945+
static void* oom_malloc_cb(size_t size)
34946+
{
34947+
#endif
34948+
if (!oom_failed && oom_match_size != 0 && size == oom_match_size) {
34949+
oom_match_count++;
34950+
if (oom_fail_at_match != 0 && oom_match_count == oom_fail_at_match) {
34951+
oom_failed = 1;
34952+
return NULL;
34953+
}
34954+
}
34955+
return malloc(size);
34956+
}
34957+
34958+
#ifdef WOLFSSL_DEBUG_MEMORY
34959+
static void oom_free_cb(void* ptr, const char* func, unsigned int line)
34960+
{
34961+
(void)func;
34962+
(void)line;
34963+
#else
34964+
static void oom_free_cb(void* ptr)
34965+
{
34966+
#endif
34967+
free(ptr);
34968+
}
34969+
34970+
#ifdef WOLFSSL_DEBUG_MEMORY
34971+
static void* oom_realloc_cb(void* ptr, size_t size, const char* func,
34972+
unsigned int line)
34973+
{
34974+
(void)func;
34975+
(void)line;
34976+
#else
34977+
static void* oom_realloc_cb(void* ptr, size_t size)
34978+
{
34979+
#endif
34980+
return realloc(ptr, size);
34981+
}
34982+
#endif
34983+
34984+
/* Regression test for the DupSSL() error-path bug: an allocation failure on
34985+
* the Poly1305 alloc must not leave the original ssl object with a zeroed
34986+
* encrypt context or a non-NULL ssl->dupWrite.
34987+
*
34988+
* Pass 0 measures the count of sizeof(Poly1305) allocations during a
34989+
* successful wolfSSL_write_dup(); pass 1 fails the same-indexed allocation
34990+
* on a fresh handshake to deterministically target the DupSSL() Poly1305
34991+
* alloc regardless of any incidental size collisions. */
34992+
static int test_write_dup_oom(void)
34993+
{
34994+
EXPECT_DECLS;
34995+
#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(HAVE_WRITE_DUP) && \
34996+
defined(HAVE_CHACHA) && defined(HAVE_POLY1305) && !defined(NO_SHA256) && \
34997+
(defined(HAVE_ECC) || defined(HAVE_CURVE25519) || \
34998+
defined(HAVE_CURVE448)) && \
34999+
!defined(NO_RSA) && defined(USE_WOLFSSL_MEMORY) && \
35000+
!defined(WOLFSSL_NO_MALLOC) && !defined(WOLFSSL_STATIC_MEMORY) && \
35001+
!defined(WOLFSSL_KERNEL_MODE) && !defined(WOLFSSL_NO_TLS12) && \
35002+
!defined(NO_TLS)
35003+
char hiWorld[] = "dup message";
35004+
char readData[sizeof(hiWorld) + 5];
35005+
wolfSSL_Malloc_cb prev_mc = NULL;
35006+
wolfSSL_Free_cb prev_fc = NULL;
35007+
wolfSSL_Realloc_cb prev_rc = NULL;
35008+
int allocators_set = 0;
35009+
int target_match = 0;
35010+
int pass;
35011+
35012+
ExpectIntEQ(wolfSSL_GetAllocators(&prev_mc, &prev_fc, &prev_rc), 0);
35013+
ExpectIntEQ(wolfSSL_SetAllocators(oom_malloc_cb, oom_free_cb,
35014+
oom_realloc_cb), 0);
35015+
if (EXPECT_SUCCESS())
35016+
allocators_set = 1;
35017+
35018+
for (pass = 0; pass < 2 && EXPECT_SUCCESS(); pass++) {
35019+
struct test_memio_ctx test_ctx;
35020+
WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
35021+
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
35022+
WOLFSSL *ssl_c2 = NULL;
35023+
35024+
XMEMSET(&test_ctx, 0, sizeof(test_ctx));
35025+
test_ctx.c_ciphers = test_ctx.s_ciphers =
35026+
"ECDHE-RSA-CHACHA20-POLY1305";
35027+
35028+
ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
35029+
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);
35030+
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
35031+
35032+
/* Start counting sizeof(Poly1305) allocations only now, after the
35033+
* handshake, so the count reflects only allocations on the
35034+
* wolfSSL_write_dup() code path. */
35035+
oom_match_size = sizeof(Poly1305);
35036+
oom_match_count = 0;
35037+
oom_failed = 0;
35038+
oom_fail_at_match = (pass == 0) ? 0 : target_match;
35039+
35040+
if (pass == 0) {
35041+
ExpectNotNull(ssl_c2 = wolfSSL_write_dup(ssl_c));
35042+
target_match = oom_match_count;
35043+
/* Sanity: at least one Poly1305 alloc must occur on this path,
35044+
* otherwise the test setup was wrong (feature compiled out, or
35045+
* negotiated cipher does not use Poly1305). */
35046+
ExpectIntGE(target_match, 1);
35047+
}
35048+
else {
35049+
ExpectNull(wolfSSL_write_dup(ssl_c));
35050+
/* Confirm the targeted Poly1305 allocation actually failed. */
35051+
ExpectIntEQ(oom_failed, 1);
35052+
/* Stop further failures so the recovery path can run. */
35053+
oom_fail_at_match = 0;
35054+
35055+
/* The original ssl_c must NOT be poisoned: the WriteDup must have
35056+
* been cleaned up, the encrypt cipher context must still be
35057+
* intact, and a fresh wolfSSL_write_dup() must succeed. */
35058+
ExpectNull(ssl_c->dupWrite);
35059+
ExpectNotNull(ssl_c2 = wolfSSL_write_dup(ssl_c));
35060+
35061+
/* Round-trip data both directions to confirm both sides still
35062+
* encrypt with the original ssl_c context preserved. */
35063+
ExpectIntEQ(wolfSSL_write(ssl_s, hiWorld, sizeof(hiWorld)),
35064+
sizeof(hiWorld));
35065+
ExpectIntEQ(wolfSSL_read(ssl_c, readData, sizeof(readData)),
35066+
sizeof(hiWorld));
35067+
ExpectIntEQ(wolfSSL_write(ssl_c2, hiWorld, sizeof(hiWorld)),
35068+
sizeof(hiWorld));
35069+
ExpectIntEQ(wolfSSL_read(ssl_s, readData, sizeof(readData)),
35070+
sizeof(hiWorld));
35071+
}
35072+
35073+
/* Disable matching/failure during teardown so frees and any internal
35074+
* allocations of the same size during cleanup are unaffected. */
35075+
oom_match_size = 0;
35076+
oom_fail_at_match = 0;
35077+
35078+
wolfSSL_free(ssl_c);
35079+
wolfSSL_free(ssl_c2);
35080+
wolfSSL_free(ssl_s);
35081+
wolfSSL_CTX_free(ctx_c);
35082+
wolfSSL_CTX_free(ctx_s);
35083+
}
35084+
35085+
/* Restore previous allocators only after every object allocated under the
35086+
* test allocators has been freed, so allocator bookkeeping (in builds
35087+
* that wrap the default allocators) is not desynchronised. */
35088+
if (allocators_set)
35089+
(void)wolfSSL_SetAllocators(prev_mc, prev_fc, prev_rc);
35090+
#endif
35091+
return EXPECT_RESULT();
35092+
}
35093+
3491235094
static int test_read_write_hs(void)
3491335095
{
3491435096

@@ -37742,6 +37924,7 @@ TEST_CASE testCases[] = {
3774237924
TEST_DECL(test_write_dup),
3774337925
TEST_DECL(test_write_dup_want_write),
3774437926
TEST_DECL(test_write_dup_want_write_simul),
37927+
TEST_DECL(test_write_dup_oom),
3774537928
TEST_DECL(test_read_write_hs),
3774637929
TEST_DECL(test_get_signature_nid),
3774737930
#ifndef WOLFSSL_TEST_APPLE_NATIVE_CERT_VALIDATION

0 commit comments

Comments
 (0)