Fix DTLS 1.2 session resumption #5915
Conversation
Previously, the transforms were populated before extension parsing, which resulted in the client rejecting a server hello that contained a connection ID. Signed-off-by: Andrzej Kurek <[email protected]>
AndrzejKurek
added
bug
needs-review
Signed-off-by: Andrzej Kurek <[email protected]>
|
Changelog entry added, I'll add a backport once there's at least one approval on this approach. |
library/ssl_tls12_client.c
Outdated
| @@ -1654,6 +1644,19 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) | |||
| } | |||
| } | |||
|
|
|||
| if( ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) | |||
There was a problem hiding this comment.
| if( ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) | |
| /* | |
| * mbedtls_ssl_derive_keys() has to be called after the parsing of the | |
| * extensions. It sets the transform data for the resumed session which in | |
| * case of DTLS includes the server CID extracted from the CID extension. | |
| */ | |
| if( ssl->handshake->resume ) |
There was a problem hiding this comment.
I'm not really sure if only session resumption was affected. The set of conditions that could lead to the early key derivation was a bit longer:
ssl->handshake->resume == 0 || n == 0 ||
#if defined(MBEDTLS_SSL_RENEGOTIATION)
ssl->renego_status != MBEDTLS_SSL_INITIAL_HANDSHAKE ||
#endif
ssl->session_negotiate->ciphersuite != i ||
ssl->session_negotiate->compression != comp ||
ssl->session_negotiate->id_len != n ||
memcmp( ssl->session_negotiate->id, buf + 35, n ) != 0 )
Can there be a different set of extensions with, for example, renegotiation here?
There was a problem hiding this comment.
I think it is ok and clearer to check on the ssl->handshake->resume flag. The checks you mention just above are just all the necessary conditions to go ahead with a session resumption. That's why if the check fails ssl->handshake->resume is reset to zero.
ChangeLog.d/resumption_cid.txt
Outdated
| @@ -0,0 +1,4 @@ | |||
| Bugfix | |||
| * Fix a bug where the connection ID extension on client side would not work | |||
There was a problem hiding this comment.
| * Fix a bug where the connection ID extension on client side would not work | |
| * Fix server connection identifier setting for outgoing encrypted records | |
| on DTLS 1.2 session resumption. After DTLS 1.2 session resumption with | |
| connection identifier, the Mbed TLS client now sends properly the server | |
| connection identifier in encrypted record headers. Fix #5872. |
Co-authored-by: Ronald Cron <[email protected]> Signed-off-by: Andrzej Kurek <[email protected]>
…parsing Use a more straightforward condition to note that session resumption is happening. Co-authored-by: Ronald Cron <[email protected]> Signed-off-by: Andrzej Kurek <[email protected]>
ChangeLog.d/resumption_cid.txt
Outdated
| Bugfix | ||
| * Fix server connection identifier setting for outgoing encrypted records | ||
| on DTLS 1.2 session resumption. After DTLS 1.2 session resumption with | ||
| connection identifier, the Mbed TLS client now sends properly the server |
There was a problem hiding this comment.
(minor)
| connection identifier, the Mbed TLS client now sends properly the server | |
| connection identifier, the Mbed TLS client now properly sends the server |
or: should it be "sends the correct server connection identifier"? Not sure what it is doing wrong currently...
There was a problem hiding this comment.
I'd suggest not to restart the CI cycle just for that.
There was a problem hiding this comment.
agreed (hence "minor")
There was a problem hiding this comment.
Do we require a CI rerun if the change will contain just this rewording?
There was a problem hiding this comment.
That's indeed an option: not require a CI rerun. I am ok with that.
There was a problem hiding this comment.
Not really. There is presumably a backport forthcoming as well?
|
The Internal CI ran successfully. |
ronald-cron-arm
removed
needs-ci
AndrzejKurek
changed the title
Signed-off-by: Andrzej Kurek <[email protected]>
|
Backport added here: #6067. Sorry for not doing this earlier! |
|
The internal CI has run successfully on this PR and its backport. One more review on the backport needed and then both can be merged. |
Previously, the transforms were populated before extension parsing, which resulted in the client rejecting a server hello that contained a connection ID.
Fixes #5872.