diff --git a/.github/workflows/async.yml b/.github/workflows/async.yml index 275ca63961..2d11923904 100644 --- a/.github/workflows/async.yml +++ b/.github/workflows/async.yml @@ -71,7 +71,7 @@ jobs: run: | cat > "$RUNNER_TEMP/async-configs.json" <<'EOF' [ - {"comment": "The only entry that pairs the software async simulator with --enable-all. --enable-all turns on cryptocb, which stops configure.ac from auto-enabling the simulator, so the asynccrypt-all entries below define WOLFSSL_ASYNC_CRYPT but never actually return WC_PENDING_E. Without this one nothing exercises TLS 1.3 post-handshake auth or DTLS writes against a pending crypto op.", + {"comment": "The only entry that pairs the software async simulator with --enable-all. --enable-all turns on cryptocb, which stops configure.ac from auto-enabling the simulator, so the asynccrypt-all entries below define WOLFSSL_ASYNC_CRYPT but never actually return WC_PENDING_E. Without this one nothing exercises TLS 1.3 post-handshake auth or DTLS writes against a pending crypto op. The minutes value is a projection, not a CI measurement: this config takes 1.6 min locally where the asynccrypt-all entries below take 1.4 against their declared 3. Refresh it from the first real run.", "name": "asynccrypt-sw-all-dtls13", "minutes": 3, "configure": ["--enable-asynccrypt-sw", "--enable-all", "--enable-dtls13", diff --git a/src/internal.c b/src/internal.c index ce72385c6a..4fd1768532 100644 --- a/src/internal.c +++ b/src/internal.c @@ -25481,26 +25481,26 @@ static int DoProcessReplyEx(WOLFSSL* ssl, int allowSocketErr) &ssl->buffers.inputBuffer.idx, ssl->curStartIdx + ssl->curSize); #if defined(WOLFSSL_ASYNC_CRYPT) && defined(WOLFSSL_POST_HANDSHAKE_AUTH) - /* Post-handshake authentication runs the connect state - * machine from inside this handler and resumes through - * wolfSSL_negotiate() rather than by reprocessing this - * record, which is why it leaves processReply at - * doProcessInit. Finish the record's accounting here, - * or the trailing MAC is parsed as the next record - * header and the read fails with VERSION_ERROR. An - * ordinary pending message leaves processReply at - * runProcessingOneMessage and must not be advanced. - * The content check matches the end of record test - * below: a fragmented certificate_request rewinds - * inOutIdx to reprocess the fragment, and coalesced - * handshake messages leave idx inside the record, so - * in both cases the padding must not be skipped. */ + /* Post-handshake auth resumes through + * wolfSSL_negotiate() instead of reprocessing this + * record, so it leaves processReply at doProcessInit + * (an ordinary pending message leaves it at + * runProcessingOneMessage). Finish the record here or + * the trailing MAC is read as the next record header + * and fails with VERSION_ERROR. Mirrors the end of + * record block below: resume inside the record when + * content is left, else skip the padding. */ if (ret == WC_NO_ERR_TRACE(WC_PENDING_E) && - ssl->options.processReply == doProcessInit && - (ssl->buffers.inputBuffer.idx - - ssl->curStartIdx) >= ssl->curSize && - IsEncryptionOn(ssl, 0)) { - ssl->buffers.inputBuffer.idx += ssl->keys.padSz; + ssl->options.processReply == doProcessInit) { + if ((ssl->buffers.inputBuffer.idx - + ssl->curStartIdx) < ssl->curSize) { + ssl->options.processReply = + runProcessingOneMessage; + } + else if (IsEncryptionOn(ssl, 0)) { + ssl->buffers.inputBuffer.idx += + ssl->keys.padSz; + } } #endif #ifdef WOLFSSL_EARLY_DATA @@ -26277,6 +26277,10 @@ int BuildMessage(WOLFSSL* ssl, byte* output, int outSz, const byte* input, if (ret == WC_NO_ERR_TRACE(WC_NO_PENDING_E)) #endif { + /* Note: these hit ssl->options even for a sizeOnly probe, where every + * other result goes to lcl_args, so a probe destroys the resume point + * of a suspended asynchronous build. wolfssl_local_GetRecordSize() is + * the only sizeOnly caller and restores them; a new one must too. */ ret = 0; #ifdef WOLFSSL_ASYNC_CRYPT ssl->options.buildArgsSet = 1; @@ -26294,7 +26298,10 @@ int BuildMessage(WOLFSSL* ssl, byte* output, int outSz, const byte* input, case BUILD_MSG_BEGIN: { #if defined(WOLFSSL_DTLS) && defined(HAVE_SECURE_RENEGOTIATION) - if (ssl->options.dtls && DtlsSCRKeysSet(ssl)) { + /* Skipped for a size probe: the size is the same either way, and + * SetKeysSide() would swap the active encryption state and clear + * recordSzOverhead under a suspended asynchronous build. */ + if (!sizeOnly && ssl->options.dtls && DtlsSCRKeysSet(ssl)) { /* For epochs >1 the current cipher parameters are located in * ssl->secure_renegotiation->tmp_keys. Previous cipher * parameters and for epoch 1 use ssl->keys */ @@ -45181,18 +45188,9 @@ int wolfssl_local_GetRecordSize(WOLFSSL *ssl, int payloadSz, int isEncrypted) int isDtls13 = ssl->options.dtls && ssl->options.tls1_3; #endif #ifdef WOLFSSL_ASYNC_CRYPT - /* The size probe below borrows two fields that also describe an - * asynchronous BuildMessage still in flight: buildMsgState is its - * resume point, and buildArgsSet says its arguments are live. - * SendData() calls us again on every retry, so without saving them the - * probe rewinds the suspended record to BUILD_MSG_BEGIN and clears the - * arguments flag. The resumed call then re-applies the header and - * cipher overhead to arguments that already carry it, and the record - * is rejected with BUFFER_E. */ byte savedBuildMsgState = ssl->options.buildMsgState; byte savedBuildArgsSet = ssl->options.buildArgsSet; #endif - if (ssl->specs.cipher_type == aead && ssl->recordSzOverhead != 0 #ifdef WOLFSSL_DTLS13 && (!isDtls13 || payloadSz + (int)ssl->recordSzOverhead @@ -45205,6 +45203,12 @@ int wolfssl_local_GetRecordSize(WOLFSSL *ssl, int payloadSz, int isEncrypted) recordSz = BuildMessage(ssl, NULL, 0, NULL, payloadSz, application_data, 0, 1, 0, CUR_ORDER); #ifdef WOLFSSL_ASYNC_CRYPT + /* Sizing shares the build state machine with an asynchronous + * BuildMessage that SendData() re-sizes on every retry, so the probe + * runs while that record is suspended. Restore its resume point, or + * the record is sized twice and rejected with BUFFER_E. Skipping the + * probe is not an option: wolfssl_local_GetMaxPlaintextSize() derives + * the DTLS fragment size from this result, so it must stay exact. */ ssl->options.buildMsgState = savedBuildMsgState; ssl->options.buildArgsSet = savedBuildArgsSet; #endif diff --git a/src/tls13.c b/src/tls13.c index c74e9a7d65..83ac9bd39a 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -3366,6 +3366,10 @@ int BuildTls13Message(WOLFSSL* ssl, byte* output, int outSz, const byte* input, if (ret == WC_NO_ERR_TRACE(WC_NO_PENDING_E)) #endif { + /* Note: these hit ssl->options even for a sizeOnly probe, where every + * other result goes to lcl_args, so a probe destroys the resume point + * of a suspended asynchronous build. wolfssl_local_GetRecordSize() is + * the only sizeOnly caller and restores them; a new one must too. */ ret = 0; ssl->options.buildMsgState = BUILD_MSG_BEGIN; XMEMSET(args, 0, sizeof(BuildMsg13Args)); diff --git a/tests/api/test_tls.c b/tests/api/test_tls.c index 2fa2fd2d5b..bd5e0259a0 100644 --- a/tests/api/test_tls.c +++ b/tests/api/test_tls.c @@ -2927,37 +2927,55 @@ int test_record_size_matches_build_message(void) return EXPECT_RESULT(); } -int test_record_size_preserves_build_msg_state(void) +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + defined(WOLFSSL_ASYNC_CRYPT) +/* SendData() sizes the output buffer on every retry, including while an + * asynchronous BuildMessage is suspended part way through a record. Sizing + * runs the same build state machine, so the probe must not re-enter it: it + * would rewind buildMsgState, clear buildArgsSet, and the resumed record would + * be sized a second time. Check that a suspended build survives a probe, and + * that a probe from a clean state still returns the exact record size. */ +static int record_size_state_check(method_provider client_method, + method_provider server_method) { EXPECT_DECLS; -#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ - defined(WOLFSSL_ASYNC_CRYPT) && !defined(WOLFSSL_NO_TLS12) WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; WOLFSSL *ssl_c = NULL, *ssl_s = NULL; struct test_memio_ctx test_ctx; - int sz; + int expectedSz = 0, cleanSz = 0, busySz = 0; XMEMSET(&test_ctx, 0, sizeof(test_ctx)); ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, - wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); + client_method, server_method), 0); ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); - /* SendData() re-probes the record size on every retry, which happens while - * an asynchronous BuildMessage is suspended part way through a record. The - * probe shares buildMsgState and buildArgsSet with that build, so it must - * put both back or the resumed record is sized twice. */ if (ssl_c != NULL) { + expectedSz = BuildMessage(ssl_c, NULL, 0, NULL, 256, + application_data, 0, 1, 0, CUR_ORDER); + ssl_c->options.buildMsgState = BUILD_MSG_BEGIN; + ssl_c->options.buildArgsSet = 0; + ExpectIntGT(expectedSz, 256); + + /* Clearing the cache is what forces the BuildMessage path; an AEAD + * suite would otherwise answer from ssl->recordSzOverhead and the + * assertions below would hold no matter what the probe did. */ + ssl_c->recordSzOverhead = 0; + cleanSz = wolfssl_local_GetRecordSize(ssl_c, 256, 1); + ExpectIntEQ(cleanSz, expectedSz); + + /* Same probe with a build suspended mid-record. */ + ssl_c->recordSzOverhead = 0; ssl_c->options.buildMsgState = BUILD_MSG_ENCRYPT; ssl_c->options.buildArgsSet = 1; - sz = wolfssl_local_GetRecordSize(ssl_c, 256, 1); + busySz = wolfssl_local_GetRecordSize(ssl_c, 256, 1); - ExpectIntGT(sz, 256); ExpectIntEQ(ssl_c->options.buildMsgState, BUILD_MSG_ENCRYPT); ExpectIntEQ(ssl_c->options.buildArgsSet, 1); + /* Still exact: wolfssl_local_GetMaxPlaintextSize() sizes DTLS + * fragments from this, so it may not degrade to an upper bound. */ + ExpectIntEQ(busySz, expectedSz); - /* Nothing was really built, so do not leave the flag claiming the - * async arguments are live. */ ssl_c->options.buildMsgState = BUILD_MSG_BEGIN; ssl_c->options.buildArgsSet = 0; } @@ -2966,6 +2984,25 @@ int test_record_size_preserves_build_msg_state(void) wolfSSL_free(ssl_s); wolfSSL_CTX_free(ctx_c); wolfSSL_CTX_free(ctx_s); + return EXPECT_RESULT(); +} +#endif /* HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES && WOLFSSL_ASYNC_CRYPT */ + +int test_record_size_preserves_build_msg_state(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + defined(WOLFSSL_ASYNC_CRYPT) +#ifndef WOLFSSL_NO_TLS12 + ExpectIntEQ(record_size_state_check(wolfTLSv1_2_client_method, + wolfTLSv1_2_server_method), TEST_SUCCESS); +#endif +#ifdef WOLFSSL_TLS13 + /* BuildTls13Message() clobbers buildMsgState by a different route: its + * sizeOnly return bypasses exit_buildmsg entirely. */ + ExpectIntEQ(record_size_state_check(wolfTLSv1_3_client_method, + wolfTLSv1_3_server_method), TEST_SUCCESS); +#endif #endif return EXPECT_RESULT(); } diff --git a/wolfcrypt/src/eccsi.c b/wolfcrypt/src/eccsi.c index 79ddfeb5df..2564f19c71 100644 --- a/wolfcrypt/src/eccsi.c +++ b/wolfcrypt/src/eccsi.c @@ -465,8 +465,11 @@ int wc_MakeEccsiKey(EccsiKey* key, WC_RNG* rng) err = wc_ecc_make_key_ex(rng, key->ecc.dp->size, &key->ecc, key->ecc.dp->id); #ifdef WOLFSSL_ASYNC_CRYPT - /* ECCSI has no asynchronous API, so the caller cannot resume a - * pending key generation. Complete it here. */ + /* ECCSI has no asynchronous API, so the caller cannot resume a pending + * key generation - complete it here. The key->pubkey sites in + * eccsi_make_pair() and eccsi_gen_sig() need no wait: each is preceded + * by wc_ecc_free(&key->pubkey), which clears the marker that + * _ecc_make_key_ex() gates its pending path on. */ err = wc_AsyncWait(err, &key->ecc.asyncDev, WC_ASYNC_FLAG_NONE); #endif } diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index 6988960ec7..8ad792baad 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -81235,7 +81235,7 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t cryptocb_test(void) if (ret == 0) { haveSrc = 1; ret = wc_ecc_make_key(eccRng, 32, srcKey); -#ifdef WOLFSSL_ASYNC_CRYPT +#if defined(WOLFSSL_ASYNC_CRYPT) ret = wc_AsyncWait(ret, &srcKey->asyncDev, WC_ASYNC_FLAG_NONE); #endif }