From eee0c8d7c2fa314a80dc28087942353d4590591c Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Fri, 14 Aug 2026 18:18:08 +0000 Subject: [PATCH] DTLS 1.3: only ack handshake records that were processed or buffered F-8926 RFC 9147 Sec 7 forbids acking a record whose handshake message was neither processed nor buffered: the peer stops retransmitting it and the handshake deadlocks. Dtls13RecordRecvd() added the ack at the record layer, before _Dtls13HandshakeRecv() had a chance to drop the message (oversized plaintext message, non-ClientHello before the ClientHello is verified, fragmented plaintext message we don't reassemble, a buffering failure, or a handler returning WANT_WRITE). DtlsMsgStore() now reports whether the fragment was stored so the DTLS 1.3 caller can tell "buffered" from "dropped", and the three DtlsMsgSet() paths that dropped a fragment on an allocation failure while returning success now return MEMORY_ERROR. The DTLS 1.2 callers keep ignoring the value, matching their current behaviour. A retransmission of an already processed message is still acked. That message was processed, and the ack is what stops a peer that retransmitted because our previous ack was lost. The post-handshake CertificateRequest stays implicitly acked by the flight it triggers. Acks are keyed by record, so a record packing a dropped message behind a kept one is still acked. Closing that needs record-scoped state in DoProcessReplyEx and is not worth it for a peer that only hurts itself. --- src/dtls13.c | 48 +++++++++++++++++++++++++++++++++------------- src/internal.c | 37 ++++++++++++++++++++++------------- wolfssl/internal.h | 2 +- 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/dtls13.c b/src/dtls13.c index 32b4e00df0..5d01f5b099 100644 --- a/src/dtls13.c +++ b/src/dtls13.c @@ -803,6 +803,18 @@ int Dtls13RtxAddAck(WOLFSSL* ssl, w64wrapper epoch, w64wrapper seq) return 0; } +/* RFC 9147 Sec 7: only ack a record whose message we processed or buffered. + * Acking a dropped message stops the peer retransmitting it and deadlocks. */ +static void Dtls13RtxAddAckForCurRecord(WOLFSSL* ssl) +{ + /* Stateless processing must not touch the ssl object. */ + if (!ssl->options.dtlsStateful) + return; + + if (Dtls13RtxAddAck(ssl, ssl->keys.curEpoch64, ssl->keys.curSeq) != 0) + WOLFSSL_MSG("can't save ack fragment"); +} + static void Dtls13RtxFlushAcks(WOLFSSL* ssl) { Dtls13RecordNumber *list, *rn; @@ -934,8 +946,9 @@ static void Dtls13SaveOrFlushClientHello(WOLFSSL* ssl) } } +/* implicitAck is set when the flight we send already acks this record. */ static int Dtls13RtxMsgRecvd(WOLFSSL* ssl, enum HandShakeType hs, - word32 fragOffset) + word32 fragOffset, byte* implicitAck) { WOLFSSL_ENTER("Dtls13RtxMsgRecvd"); @@ -965,6 +978,10 @@ static int Dtls13RtxMsgRecvd(WOLFSSL* ssl, enum HandShakeType hs, /* retransmission detected. */ ssl->dtls13Rtx.retransmit = 1; + /* Already processed, so acking is allowed. It stops a peer that + * retransmitted because our earlier ack was lost. */ + Dtls13RtxAddAckForCurRecord(ssl); + /* the other peer may have retransmitted because an ACK for a flight that needs explicit ACK was lost.*/ if (ssl->dtls13Rtx.seenRecords != NULL) @@ -985,6 +1002,7 @@ static int Dtls13RtxMsgRecvd(WOLFSSL* ssl, enum HandShakeType hs, should be rare and simplifies the code. Otherwise, it would be necessary to track which record number contained a CertificateRequest with a particular context id */ + *implicitAck = 1; Dtls13RtxRemoveCurAck(ssl); } @@ -1616,19 +1634,14 @@ int Dtls13ParseUnifiedRecordLayer(WOLFSSL* ssl, const byte* input, int Dtls13RecordRecvd(WOLFSSL* ssl) { - int ret; - if (ssl->curRL.type != handshake) return 0; if (!ssl->options.dtls13SendMoreAcks) ssl->dtls13FastTimeout = 1; - ret = Dtls13RtxAddAck(ssl, ssl->keys.curEpoch64, ssl->keys.curSeq); - if (ret != 0) - WOLFSSL_MSG("can't save ack fragment"); - - return ret; + /* Acking happens in Dtls13RtxAddAckForCurRecord(). */ + return 0; } static void Dtls13RtxMoveToEndOfList(WOLFSSL* ssl, Dtls13RtxRecord** prevNext, @@ -1874,10 +1887,12 @@ static int _Dtls13HandshakeRecv(WOLFSSL* ssl, byte* input, word32 size, byte usingAsyncCrypto; word32 messageLength; byte handshakeType; + byte implicitAck; word32 idx; int ret; idx = 0; + implicitAck = 0; ret = GetDtlsHandShakeHeader(ssl, input, &idx, &handshakeType, &messageLength, &fragOff, &fragLength, size); if (ret != 0) @@ -1937,7 +1952,8 @@ static int _Dtls13HandshakeRecv(WOLFSSL* ssl, byte* input, word32 size, if (fragOff + fragLength > messageLength) return BUFFER_ERROR; - ret = Dtls13RtxMsgRecvd(ssl, (enum HandShakeType)handshakeType, fragOff); + ret = Dtls13RtxMsgRecvd(ssl, (enum HandShakeType)handshakeType, fragOff, + &implicitAck); if (ret != 0) return ret; @@ -2001,10 +2017,13 @@ static int _Dtls13HandshakeRecv(WOLFSSL* ssl, byte* input, word32 size, ssl->keys.dtls_expected_peer_handshake_number || usingAsyncCrypto) { if (ssl->dtls_rx_msg_list_sz < DTLS_POOL_SZ) { - DtlsMsgStore(ssl, (word16)w64GetLow32(ssl->keys.curEpoch64), - ssl->keys.dtls_peer_handshake_number, - input + DTLS_HANDSHAKE_HEADER_SZ, messageLength, handshakeType, - fragOff, fragLength, ssl->heap); + /* Only ack a fragment we really buffered. */ + if (DtlsMsgStore(ssl, (word16)w64GetLow32(ssl->keys.curEpoch64), + ssl->keys.dtls_peer_handshake_number, + input + DTLS_HANDSHAKE_HEADER_SZ, messageLength, + handshakeType, fragOff, fragLength, ssl->heap) == 0) { + Dtls13RtxAddAckForCurRecord(ssl); + } } else { /* DTLS_POOL_SZ outstanding messages is way more than enough for any @@ -2025,6 +2044,9 @@ static int _Dtls13HandshakeRecv(WOLFSSL* ssl, byte* input, word32 size, if (ret != 0) return ret; + if (!implicitAck) + Dtls13RtxAddAckForCurRecord(ssl); + Dtls13MsgWasProcessed(ssl, (enum HandShakeType)handshakeType); /* check if we have buffered some message */ diff --git a/src/internal.c b/src/internal.c index 004943c4e7..e19b171f1a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -10717,10 +10717,10 @@ int DtlsMsgSet(DtlsMsg* msg, word32 seq, word16 epoch, const byte* data, byte ty } prev->m.m.next = DtlsMsgCreateFragBucket(fragOffset, data, fragSz, heap); - if (prev->m.m.next != NULL) { - msg->bytesReceived += fragSz; - msg->fragBucketListCount++; - } + if (prev->m.m.next == NULL) + return MEMORY_ERROR; + msg->bytesReceived += fragSz; + msg->fragBucketListCount++; } else if (fragOffsetEnd < cur->m.m.offset) { /* Fragment is entirely before cur with a gap */ @@ -10741,6 +10741,7 @@ int DtlsMsgSet(DtlsMsg* msg, word32 seq, word16 epoch, const byte* data, byte ty else { /* reset on error */ *prev_next = cur; + return MEMORY_ERROR; } } else { @@ -10754,8 +10755,11 @@ int DtlsMsgSet(DtlsMsg* msg, word32 seq, word16 epoch, const byte* data, byte ty /* We can combine the buckets */ *prev_next = DtlsMsgCombineFragBuckets(msg, cur, next, fragOffset, data, fragSz, heap); - if (*prev_next == NULL) /* reset on error */ + if (*prev_next == NULL) { + /* reset on error */ *prev_next = cur; + return MEMORY_ERROR; + } } } } @@ -10777,7 +10781,8 @@ DtlsMsg* DtlsMsgFind(DtlsMsg* head, word16 epoch, word32 seq) } -void DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, +/* Returns 0 when the fragment was stored, negative when it was dropped. */ +int DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, word32 dataSz, byte type, word32 fragOffset, word32 fragSz, void* heap) { /* See if seq exists in the list. If it isn't in the list, make @@ -10799,6 +10804,7 @@ void DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, DtlsMsg* head = ssl->dtls_rx_msg_list; byte encrypted = ssl->keys.decryptedCur == 1; + int ret = 0; WOLFSSL_ENTER("DtlsMsgStore"); if (head != NULL) { @@ -10807,11 +10813,13 @@ void DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, cur = DtlsMsgNew(dataSz, 0, heap); if (cur == NULL) { WOLFSSL_MSG("DtlsMsgNew allocation failed"); - ssl->error = MEMORY_E; + ret = MEMORY_E; + ssl->error = ret; } else { - if (DtlsMsgSet(cur, seq, epoch, data, type, - fragOffset, fragSz, heap, dataSz, encrypted) < 0) { + ret = DtlsMsgSet(cur, seq, epoch, data, type, + fragOffset, fragSz, heap, dataSz, encrypted); + if (ret < 0) { DtlsMsgDelete(cur, heap); } else { @@ -10822,7 +10830,7 @@ void DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, } else { /* If this fails, the data is just dropped. */ - DtlsMsgSet(cur, seq, epoch, data, type, fragOffset, + ret = DtlsMsgSet(cur, seq, epoch, data, type, fragOffset, fragSz, heap, dataSz, encrypted); } } @@ -10830,10 +10838,11 @@ void DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, head = DtlsMsgNew(dataSz, 0, heap); if (head == NULL) { WOLFSSL_MSG("DtlsMsgNew allocation failed"); - ssl->error = MEMORY_E; + ret = MEMORY_E; + ssl->error = ret; } - else if (DtlsMsgSet(head, seq, epoch, data, type, fragOffset, - fragSz, heap, dataSz, encrypted) < 0) { + else if ((ret = DtlsMsgSet(head, seq, epoch, data, type, fragOffset, + fragSz, heap, dataSz, encrypted)) < 0) { DtlsMsgDelete(head, heap); head = NULL; } @@ -10843,6 +10852,8 @@ void DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, } ssl->dtls_rx_msg_list = head; + + return ret; } diff --git a/wolfssl/internal.h b/wolfssl/internal.h index dd20de7d6c..410a56be69 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -7431,7 +7431,7 @@ WOLFSSL_LOCAL word32 MacSize(const WOLFSSL* ssl); word32 totalLen, byte encrypted); WOLFSSL_TEST_VIS DtlsMsg* DtlsMsgFind(DtlsMsg* head, word16 epoch, word32 seq); - WOLFSSL_TEST_VIS void DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, + WOLFSSL_TEST_VIS int DtlsMsgStore(WOLFSSL* ssl, word16 epoch, word32 seq, const byte* data, word32 dataSz, byte type, word32 fragOffset, word32 fragSz, void* heap);