fix: Send zero-length NST when session key is expired #4532
Conversation
Co-authored-by: maddeleine <59030281+maddeleine@users.noreply.github.com>
tls/s2n_server_new_session_ticket.c
Outdated
| *= https://www.rfc-editor.org/rfc/rfc5077#section-3.3 | ||
| *# This message MUST be sent if the server included | ||
| *# a SessionTicket extension in the ServerHello. |
There was a problem hiding this comment.
Is this the right place for this compliance comment? This code doesn't ensure that the message is sent.
There was a problem hiding this comment.
Is writing to stuffer not considered "sending a message", or are you suggesting to put this comment on top of code where I send zero-length nst message upon failing to retrieve a key like what I did in my latest change?
There was a problem hiding this comment.
Writing to a stuffer isn't automatically sending a message. This code also doesn't somehow enforce that you always write to that stuffer, just that you do in this one case.
With the way the code is currently structured, I'm not sure there's a good place for this comment.
tls/s2n_server_new_session_ticket.c
Outdated
| *= https://www.rfc-editor.org/rfc/rfc5077#section-3.3 | ||
| *# struct { | ||
| *# uint32 ticket_lifetime_hint; | ||
| *# opaque ticket<0..2^16-1>; | ||
| *# } NewSessionTicket; |
There was a problem hiding this comment.
I'm also not convinced this is the right place for this compliance comment. The struct format applies to the whole message, not just to an empty message.
There was a problem hiding this comment.
Would it make more sense to put this comment on top of this s2n_server_nst_send() function? Or is there more suitable place for this comment
There was a problem hiding this comment.
Like above: it's not clear to me that there is a suitable place to put this comment given the current structure of the code.
tls/s2n_server_new_session_ticket.c
Outdated
| **/ | ||
| if (!conn->config->use_tickets || s2n_result_is_error(s2n_config_is_encrypt_key_available(conn->config))) { |
There was a problem hiding this comment.
Does this completely solve the problem? Couldn't the key still expire between here and s2n_encrypt_session_ticket? Or couldn't s2n_config_is_encrypt_key_available still otherwise disagree with s2n_get_ticket_encrypt_decrypt_key?
To completely solve the problem, you need to write an empty message in response to actually failing to retrieve the key, not in response to a new "is there a key" check. We already made an "is there a key" check during the last message, which was probably like nanoseconds ago.
Co-authored-by: Lindsay Stewart <slindsay@amazon.com>
Resolved issues:
Resolves #4528
Description of changes:
According to RFC5077:
Currently we throw an error during the write of NewSessionTicket message if there are no valid session ticket keys available. It is guarded by s2n_encrypt_session_ticket, which fails if there is no key available. This behavior is verified and can be reproduced by running the new test added with this change reverted.
This change modifies the condition of s2n_server_nst_send in s2n-tls/tls/s2n_server_new_session_ticket.c so that it writes a zero-length session ticket if the key used to encrypt session ticket was expired between SERVER_HELLO and CLIENT_FINISHED, before writing NewSessionTicket.
This change does not impact TLS1.3, as it bail and don't send it if no key is available: https://github.com/aws/s2n-tls/blob/main/tls/s2n_server_new_session_ticket.c#L170-L172. A test to verify this is also added.
Does this change what S2N sends over the wire?
It will now correctly sends a zero-length NewSessionTicket message in the condition described above.
Does this change any public APIs? If yes, explain.
No.
Which versions of TLS will this impact?
This will impact TLS1.2 and not TLS1.3.
Call-outs:
Currently I am verifying that client received zero-length session ticket by
EXPECT_EQUAL(client_connection->client_ticket.allocated, 0);, but this may not explicitly verify the condition and there could be a better way to test it.Testing:
Confirmed all unit tests pass.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.