Open
Conversation
When a server uses a HRR to negotiate the key exchange group to use, the selected group is advertised in the HRR key share extension. Furthermore, this group is also stored in the Cookie that is sent to the client. When the server receives the second CH, the group used in the key share extension MUST be the one of the HRR. For stateless DTLS servers, the handling of this check had a bug. The key share group of the HRR is stored in the ssl->hrr_keyshare_group variable and is checked against the received key share of the second CH. However, in the stateless server case, another CH message may be received inbetween the two CH message of the desired client, potentially overwriting the ssl->hrr_keyshare_group variable. This then causes handshake failures when the ssl->hrr_keyshare_group variable contains another group than the second CH message of the desired client. To fix this, two changes are conducted: 1. Disable the ssl->hrr_keyshare_group check for stateless DTLS 1.3 servers. As long as the server is stateless, CHs from multiple clients may be received that individually cause HRRs with different groups. For each of these clients, the HRR group is properly stored in the cookie. 2. When a valid cookie is received from the client, the server becomes stateful. In this case, we now parse the cookie for a stored HRR group. If present, we reset the ssl->hrr_keyshare_group variable to this group to ensure the error checks succeed. A new test is added to check for this behavior.
rizlik
requested changes
Mar 2, 2026
src/dtls.c
Outdated
Comment on lines
+301
to
+311
| int ret = 0; | ||
| word16 len; | ||
| const byte* cookie = NULL; | ||
|
|
||
| if (ch->cookieExt.size < OPAQUE16_LEN + 1) | ||
| return BUFFER_E; | ||
| ato16(ch->cookieExt.elements, &len); | ||
| if (ch->cookieExt.size - OPAQUE16_LEN != len) | ||
| return BUFFER_E; | ||
| cookie = ch->cookieExt.elements + OPAQUE16_LEN; | ||
| ret = TlsCheckCookie(ssl, cookie, len); |
Contributor
There was a problem hiding this comment.
At this point, the cookie is already parsed and validated, don't re-parse it.
I suggest to refactor the function in something like get_keyshare_from_cookie(cookie, cookie_sz)
Contributor
Author
There was a problem hiding this comment.
Fixed by simplifying the logic in the new TLSX_Cookie_RestoreHrrGroup()
src/dtls.c
Outdated
| e = Dtls13GetEpoch(ssl, ssl->keys.curEpoch64); | ||
| if (e != NULL) | ||
| XMEMSET(e->window, 0xFF, sizeof(e->window)); | ||
|
|
Contributor
There was a problem hiding this comment.
here we already know that the ticket is good and we already parsed it in ch.
It's probably better to just extract the keyshare instead of re-parsing the cookie
julek-wolfssl
requested changes
Mar 2, 2026
Member
julek-wolfssl
left a comment
julek-wolfssl
left a comment
There was a problem hiding this comment.
With this PR, we don't need the ssl->hrr_keyshare_group variable anymore.
* Move new test to the test_wolfSSL_dtls_stateless_xxx group * Move cookie parsing to stateful DTLS processing * Remove unnecessary redundant cookie parsing logic
Contributor
Author
|
@julek-wolfssl @rizlik I addressed all your comments. |
julek-wolfssl
requested changes
Mar 3, 2026
Comment on lines
+10278
to
+10289
| #ifdef WOLFSSL_DTLS | ||
| /* In case of stateless DTLSv1.3, the ssl->hrr_keyshare_group variable | ||
| * may have already been set without further proceeding the handshake, | ||
| * so we never received the second ClientHello with the selected group. | ||
| * Hence, when we receive another ClientHello message, we do not | ||
| * consider that as an error here. | ||
| */ | ||
| if (ssl->options.dtls && IsAtLeastTLSv1_3(ssl->version) && | ||
| !ssl->options.dtlsStateful) { | ||
| return 0; | ||
| } | ||
| #endif |
Member
There was a problem hiding this comment.
I would prefer to error out when ssl->options.dtls because ssl->hrr_keyshare_group should not be set before anyway right? Am I missing an edge case?
| * ssl The SSL/TLS object. | ||
| * returns 0 on success and other values indicate failure. | ||
| */ | ||
| static int TLSX_Cookie_RestoreHrrGroup(WOLFSSL* ssl) |
Member
There was a problem hiding this comment.
Could this be restored in RestartHandshakeHashWithCookie? That will combine the cookie parsing logic in one place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a server uses an HRR to negotiate the key exchange group to use, the selected group is advertised in the HRR key share extension. Furthermore, this group is also stored in the Cookie that is sent to the client. When the server receives the second CH, the group used in the key share extension MUST be the one of the HRR.
For stateless DTLS servers, the handling of this check had a bug. The key share group of the HRR is stored in the ssl->hrr_keyshare_group variable and is checked against the received key share of the second CH. However, in the stateless server case, another CH message may be received inbetween the two CH messages of the desired client, potentially overwriting the ssl->hrr_keyshare_group variable. This then causes handshake failures when the ssl->hrr_keyshare_group variable contains another group than the second CH message of the desired client.
To fix this, two changes are conducted:
A new test is added to check for this behavior.
Identified while working on #9732.