fix: Send zero-length NST when session key is expired (#4532)

* send zero-length NST when session key is expired * add another to test the impact on tls 1.3 path * check if tls1.3 is fully supported * use shorter variable names * modify test setups * address comment * add compliance comments * add unit test for s2n_server_send * address clang format issues * edit rfc comment format * fix rfc number * remove unnecessary expect_false() * add test case for sending non-zero nst * use suggested comment Co-authored-by: maddeleine <[email protected]> * remove unnecessary test case * check all data is read * give better test description * add compliance comment to the test * fix comment format * verify ticket length and and lifetime hint is 0 * fix input orders * address PR feedbacks * send zero-length nst upon failing to retrieve encrypt key * address linter issue * address redundant code * remove unnecessary test setup * change return value to S2N_SUCCESS * remove unnecessary #define Co-authored-by: Lindsay Stewart <[email protected]> --------- Co-authored-by: maddeleine <[email protected]> Co-authored-by: Lindsay Stewart <[email protected]>
aws · May 15, 2024 · 885b607 · 885b607
1 parent 15311dc
commit 885b607
Show file tree

Hide file tree

Showing 4 changed files with 136 additions and 7 deletions.
diff --git a/tests/unit/s2n_server_new_session_ticket_test.c b/tests/unit/s2n_server_new_session_ticket_test.c
@@ -34,7 +34,6 @@
 #define MAX_TEST_SESSION_SIZE 300
 
 #define EXPECT_TICKETS_SENT(conn, count) EXPECT_OK(s2n_assert_tickets_sent(conn, count))
-
 static S2N_RESULT s2n_assert_tickets_sent(struct s2n_connection *conn, uint16_t expected_tickets_sent)
 {
     uint16_t tickets_sent = 0;
@@ -801,6 +800,68 @@ int main(int argc, char **argv)
         };
     };
 
+    /* s2n_server_nst_send */
+    {
+        /* s2n_server_nst_send writes a non-zero ticket when a valid encryption key exists */
+        {
+            DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
+            EXPECT_NOT_NULL(config);
+            DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(conn);
+            EXPECT_OK(s2n_resumption_test_ticket_key_setup(config));
+            EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
+
+            conn->session_ticket_status = S2N_NEW_TICKET;
+
+            EXPECT_SUCCESS(s2n_server_nst_send(conn));
+            EXPECT_TICKETS_SENT(conn, 1);
+
+            uint32_t lifetime = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint32(&conn->handshake.io, &lifetime));
+            EXPECT_TRUE(lifetime > 0);
+
+            uint16_t ticket_len = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint16(&conn->handshake.io, &ticket_len));
+            EXPECT_TRUE(ticket_len > 0);
+
+            EXPECT_EQUAL(s2n_stuffer_data_available(&conn->handshake.io),
+                    S2N_TLS12_TICKET_SIZE_IN_BYTES);
+        };
+
+        /* s2n_server_nst_send writes a zero-length ticket when no valid encryption key exists
+         *
+         *= https://www.rfc-editor.org/rfc/rfc5077#section-3.3
+         *= type=test
+         *# If the server determines that it does not want to include a
+         *# ticket after it has included the SessionTicket extension in the
+         *# ServerHello, then it sends a zero-length ticket in the
+         *# NewSessionTicket handshake message.
+         **/
+        {
+            DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
+            EXPECT_NOT_NULL(config);
+            DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(conn);
+            EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
+
+            conn->session_ticket_status = S2N_NEW_TICKET;
+
+            EXPECT_SUCCESS(s2n_server_nst_send(conn));
+            EXPECT_TICKETS_SENT(conn, 0);
+
+            uint32_t lifetime = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint32(&conn->handshake.io, &lifetime));
+            EXPECT_TRUE(lifetime == 0);
+
+            uint16_t ticket_len = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint16(&conn->handshake.io, &ticket_len));
+            EXPECT_TRUE(ticket_len == 0);
+            EXPECT_EQUAL(s2n_stuffer_data_available(&conn->handshake.io), 0);
+        };
+    }
+
     /* s2n_tls13_server_nst_send */
     {
         /* Mode is not server */

diff --git a/tests/unit/s2n_session_ticket_test.c b/tests/unit/s2n_session_ticket_test.c
@@ -1416,6 +1416,67 @@ int main(int argc, char **argv)
         EXPECT_TRUE(IS_RESUMPTION_HANDSHAKE(server));
     }
 
+    /* Test TLS 1.2 Server sends a zero-length ticket in the NewSessionTicket handshake
+     * if the ticket key was expired after SERVER_HELLO
+     */
+    {
+        DEFER_CLEANUP(struct s2n_config *client_configuration = s2n_config_new(),
+                s2n_config_ptr_free);
+        EXPECT_NOT_NULL(client_configuration);
+        EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(client_configuration, 1));
+        EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_configuration));
+
+        DEFER_CLEANUP(struct s2n_config *server_configuration = s2n_config_new(),
+                s2n_config_ptr_free);
+        EXPECT_NOT_NULL(server_configuration);
+        EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(server_configuration, 1));
+        EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_configuration,
+                chain_and_key));
+
+        EXPECT_SUCCESS(s2n_config_add_ticket_crypto_key(server_configuration, ticket_key_name1,
+                s2n_array_len(ticket_key_name1), ticket_key1, s2n_array_len(ticket_key1), 0));
+
+        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(client);
+        EXPECT_SUCCESS(s2n_connection_set_config(client, client_configuration));
+
+        DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(server);
+        EXPECT_SUCCESS(s2n_connection_set_config(server, server_configuration));
+
+        DEFER_CLEANUP(struct s2n_test_io_stuffer_pair test_io = { 0 },
+                s2n_io_stuffer_pair_free);
+        EXPECT_OK(s2n_io_stuffer_pair_init(&test_io));
+        EXPECT_OK(s2n_connections_set_io_stuffer_pair(client, server, &test_io));
+
+        /* Stop the handshake after the peers have established that a ticket 
+         * will be sent in this handshake. 
+         */
+        EXPECT_OK(s2n_negotiate_test_server_and_client_until_message(server, client,
+                CLIENT_FINISHED));
+
+        /* Expire current session ticket key so that server no longer holds a valid key */
+        uint64_t mock_delay = server_configuration->encrypt_decrypt_key_lifetime_in_nanos;
+        EXPECT_SUCCESS(s2n_config_set_wall_clock(server_configuration, mock_nanoseconds_since_epoch,
+                &mock_delay));
+
+        /* Attempt to send a NewSessionTicket. This should send a zero-length NST message */
+        EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server, client));
+
+        /* Verify that TLS1.2 was negotiated */
+        EXPECT_EQUAL(client->actual_protocol_version, S2N_TLS12);
+        EXPECT_EQUAL(server->actual_protocol_version, S2N_TLS12);
+
+        /* Verify that the server issued zero-length session ticket */
+        EXPECT_TRUE(IS_ISSUING_NEW_SESSION_TICKET(server));
+
+        /* Client does not have a session ticket since it received zero-length NST message */
+        EXPECT_EQUAL(client->client_ticket.size, 0);
+        EXPECT_EQUAL(client->ticket_lifetime_hint, 0);
+    }
+
     EXPECT_SUCCESS(s2n_io_pair_close(&io_pair));
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(chain_and_key));
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(ecdsa_chain_and_key));

diff --git a/tls/s2n_resume.c b/tls/s2n_resume.c
@@ -817,7 +817,7 @@ int s2n_encrypt_session_ticket(struct s2n_connection *conn, struct s2n_stuffer *
     POSIX_GUARD(s2n_aes256_gcm.destroy_key(&aes_ticket_key));
     POSIX_GUARD(s2n_session_key_free(&aes_ticket_key));
 
-    return 0;
+    return S2N_SUCCESS;
 }
 
 int s2n_decrypt_session_ticket(struct s2n_connection *conn, struct s2n_stuffer *from)

diff --git a/tls/s2n_server_new_session_ticket.c b/tls/s2n_server_new_session_ticket.c
@@ -82,23 +82,30 @@ int s2n_server_nst_send(struct s2n_connection *conn)
     uint32_t lifetime_hint_in_secs =
             (conn->config->encrypt_decrypt_key_lifetime_in_nanos + conn->config->decrypt_key_lifetime_in_nanos) / ONE_SEC_IN_NANOS;
 
-    /* When server changes it's mind mid handshake send lifetime hint and session ticket length as zero */
-    if (!conn->config->use_tickets) {
+    /* Send a zero-length ticket in the NewSessionTicket message if the server changes 
+     * its mind mid-handshake or if there are no valid encrypt keys currently available. 
+     *
+     *= https://www.rfc-editor.org/rfc/rfc5077#section-3.3
+     *# If the server determines that it does not want to include a
+     *# ticket after it has included the SessionTicket extension in the
+     *# ServerHello, then it sends a zero-length ticket in the
+     *# NewSessionTicket handshake message.
+     **/
+    POSIX_GUARD(s2n_stuffer_init(&to, &entry));
+    if (!conn->config->use_tickets || s2n_encrypt_session_ticket(conn, &to) != S2N_SUCCESS) {
         POSIX_GUARD(s2n_stuffer_write_uint32(&conn->handshake.io, 0));
         POSIX_GUARD(s2n_stuffer_write_uint16(&conn->handshake.io, 0));
 
-        return 0;
+        return S2N_SUCCESS;
     }
 
     if (!s2n_server_sending_nst(conn)) {
         POSIX_BAIL(S2N_ERR_SENDING_NST);
     }
 
-    POSIX_GUARD(s2n_stuffer_init(&to, &entry));
     POSIX_GUARD(s2n_stuffer_write_uint32(&conn->handshake.io, lifetime_hint_in_secs));
     POSIX_GUARD(s2n_stuffer_write_uint16(&conn->handshake.io, session_ticket_len));
 
-    POSIX_GUARD(s2n_encrypt_session_ticket(conn, &to));
     POSIX_GUARD(s2n_stuffer_write(&conn->handshake.io, &to.blob));
 
     /* For parity with TLS1.3, track the single ticket sent.