Intentionally disable fragmenting KeyUpdates

api/s2n.h

@@ -854,7 +854,7 @@ S2N_API extern int s2n_config_set_verify_after_sign(struct s2n_config *config, s
  *
  * Less memory can be allocated for the send buffer, but this will result in
  * smaller, more fragmented records and increased overhead. While the absolute
- * minimum size required is 1031 bytes, at least 2K bytes is recommended for
+ * minimum size required is 1034 bytes, at least 2K bytes is recommended for
  * reasonable record sizes.
  *
  * More memory can be allocated for the send buffer. This will result in s2n-tls

tests/integrationv2/test_buffered_send.py

@@ -15,7 +15,7 @@
 SEND_DATA_STRING = to_string(SEND_DATA)
 
 K_BYTES = 1024
-SEND_BUFFER_SIZE_MIN = 1031
+SEND_BUFFER_SIZE_MIN = 1034
 SEND_BUFFER_SIZE_MIN_RECOMMENDED = 2 * K_BYTES
 SEND_BUFFER_SIZE_MULTI_RECORD = 17 * K_BYTES
 SEND_BUFFER_SIZE_PREFER_THROUGHPUT = 35 * K_BYTES

tests/unit/s2n_config_test.c

@@ -460,15 +460,13 @@ int main(int argc, char **argv)
 
     /* Test s2n_config_set_send_buffer_size */
     {
-        const uint32_t min_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MAX_FRAGMENT_LENGTH_MIN);
-
         /* Safety */
         {
             DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
             EXPECT_NOT_NULL(config);
 
             EXPECT_EQUAL(config->send_buffer_size_override, 0);
-            EXPECT_FAILURE_WITH_ERRNO(s2n_config_set_send_buffer_size(NULL, min_size), S2N_ERR_NULL);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_config_set_send_buffer_size(NULL, S2N_MIN_SEND_BUFFER_SIZE), S2N_ERR_NULL);
             EXPECT_FAILURE_WITH_ERRNO(s2n_config_set_send_buffer_size(config, 0), S2N_ERR_INVALID_ARGUMENT);
             EXPECT_EQUAL(config->send_buffer_size_override, 0);
         };
@@ -490,13 +488,13 @@ int main(int argc, char **argv)
         {
             DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
             EXPECT_NOT_NULL(config);
-            EXPECT_SUCCESS(s2n_config_set_send_buffer_size(config, min_size));
+            EXPECT_SUCCESS(s2n_config_set_send_buffer_size(config, S2N_MIN_SEND_BUFFER_SIZE));
 
             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));
 
-            EXPECT_EQUAL(config->send_buffer_size_override, min_size);
+            EXPECT_EQUAL(config->send_buffer_size_override, S2N_MIN_SEND_BUFFER_SIZE);
             EXPECT_TRUE(conn->multirecord_send);
         };
     };

tests/unit/s2n_key_update_test.c

@@ -460,6 +460,45 @@ int main(int argc, char **argv)
         };
     };
 
+    /* Test: KeyUpdate fails if fragmentation required */
+    {
+        const size_t key_update_record_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_KEY_UPDATE_MESSAGE_SIZE);
+
+        /* Test: send buffer cannot be set smaller than a KeyUpdate record */
+        {
+            DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
+            EXPECT_FAILURE_WITH_ERRNO(s2n_config_set_send_buffer_size(config, key_update_record_size - 1),
+                    S2N_ERR_INVALID_ARGUMENT);
+        };
+
+        /* Test: send fails if send buffer is too small for a KeyUpdate record */
+        {
+            DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
+            s2n_blocked_status blocked = S2N_NOT_BLOCKED;
+
+            DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);
+            EXPECT_OK(s2n_connection_set_secrets(conn));
+
+            DEFER_CLEANUP(struct s2n_stuffer stuffer = { 0 }, s2n_stuffer_free);
+            EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&stuffer, 0));
+            EXPECT_SUCCESS(s2n_connection_set_io_stuffers(&stuffer, &stuffer, conn));
+
+            /* Sanity check: send buffer just large enough for KeyUpdate record */
+            config->send_buffer_size_override = key_update_record_size;
+            EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
+            conn->key_update_pending = true;
+            EXPECT_SUCCESS(s2n_key_update_send(conn, &blocked));
+
+            EXPECT_SUCCESS(s2n_connection_release_buffers(conn));
+
+            /* Test: send buffer too small for KeyUpdate record */
+            config->send_buffer_size_override = key_update_record_size - 1;
+            EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
+            conn->key_update_pending = true;
+            EXPECT_FAILURE_WITH_ERRNO(s2n_key_update_send(conn, &blocked), S2N_ERR_FRAGMENT_LENGTH_TOO_LARGE);
+        };
+    };
+
     /* Test: all cipher suites must have record limits set.
      *
      * If this ever changes, then s2n_check_record_limit needs to consider

tests/unit/s2n_post_handshake_recv_test.c

@@ -205,7 +205,9 @@ int main(int argc, char **argv)
     config->initial_tickets_to_send = 0;
 
     const uint32_t fragment_sizes[] = {
-        S2N_MAX_FRAGMENT_LENGTH_MIN,
+        1,
+        2,
+        S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE,
         TLS_HANDSHAKE_HEADER_LENGTH,
         TLS_HANDSHAKE_HEADER_LENGTH + 1,
         S2N_DEFAULT_FRAGMENT_LENGTH,

tests/unit/s2n_record_size_test.c

@@ -169,7 +169,7 @@ int main(int argc, char **argv)
 
         /* Min buffer size */
         {
-            const uint32_t buffer_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MAX_FRAGMENT_LENGTH_MIN);
+            const uint32_t buffer_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE);
             EXPECT_SUCCESS(s2n_config_set_send_buffer_size(config, buffer_size));
 
             DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);
@@ -178,7 +178,7 @@ int main(int argc, char **argv)
 
             uint16_t size = 0;
             EXPECT_OK(s2n_record_max_write_payload_size(conn, &size));
-            EXPECT_EQUAL(size, S2N_MAX_FRAGMENT_LENGTH_MIN);
+            EXPECT_EQUAL(size, S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE);
         };
 
         /* Small buffer size */

tests/unit/s2n_renegotiate_test.c

@@ -198,7 +198,7 @@ int main(int argc, char *argv[])
 
             EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server_conn, client_conn));
 
-            size_t small_frag_len = S2N_MAX_FRAGMENT_LENGTH_MIN;
+            const size_t small_frag_len = S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE;
             client_conn->max_outgoing_fragment_length = small_frag_len;
 
             EXPECT_EQUAL(s2n_send(client_conn, app_data, sizeof(app_data), &blocked), sizeof(app_data));

tests/unit/s2n_send_multirecord_test.c

@@ -149,7 +149,7 @@ int main(int argc, char **argv)
                     s2n_connection_ptr_free);
             EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
 
-            /* Unitialized buffer */
+            /* Uninitialized buffer */
             EXPECT_FALSE(s2n_should_flush(conn, buffer_size));
 
             /* Empty buffer */
@@ -194,18 +194,16 @@ int main(int argc, char **argv)
 
     /* Send buffer was configured too small for even a single record.
      * Send smaller records.
+     *
+     * The minimum buffer size we allow generates a fragment size of 5, to prevent
+     * fragmenting KeyUpdate messages, which are always 5 bytes. At this minimum size,
+     * application data is also fragmented into 5 byte chunks, which is pretty silly,
+     * but is an edge case.
      */
     {
-        /* The minimum buffer size we allow generates a fragment size of 2, to prevent
-         * fragmenting alert messages, which are always 2 bytes. At this minimum size,
-         * application data is also fragmented into 2 byte chucks, which is pretty silly,
-         * but is an edge case.
-         */
-        uint32_t min_buffer_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MAX_FRAGMENT_LENGTH_MIN);
-
         DEFER_CLEANUP(struct s2n_config *min_buffer_config = s2n_config_new(), s2n_config_ptr_free);
         EXPECT_NOT_NULL(min_buffer_config);
-        EXPECT_SUCCESS(s2n_config_set_send_buffer_size(min_buffer_config, min_buffer_size));
+        EXPECT_SUCCESS(s2n_config_set_send_buffer_size(min_buffer_config, S2N_MIN_SEND_BUFFER_SIZE));
 
         DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
                 s2n_connection_ptr_free);
@@ -217,19 +215,20 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_connection_set_send_cb(conn, s2n_test_send_cb));
         EXPECT_SUCCESS(s2n_connection_set_send_ctx(conn, (void *) &context));
 
-        ssize_t send_size = 5;
+        ssize_t send_size = sizeof(test_data);
         s2n_blocked_status blocked = 0;
         EXPECT_EQUAL(s2n_send(conn, test_data, send_size, &blocked), send_size);
 
         /* Since each record only contains two bytes of payload,
          * we need to send a number of records equal to our total send ceil(size / 2).
          */
-        EXPECT_EQUAL(context.calls, (send_size + 1) / 2);
+        uint8_t remainder = (send_size % S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE) ? 1 : 0;
+        EXPECT_EQUAL(context.calls, (send_size / S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE) + remainder);
         EXPECT_EQUAL(context.bytes_sent, conn->wire_bytes_out);
         EXPECT_TRUE(context.bytes_sent > send_size);
 
         /* Verify output buffer */
-        EXPECT_EQUAL(conn->out.blob.size, min_buffer_size);
+        EXPECT_EQUAL(conn->out.blob.size, S2N_MIN_SEND_BUFFER_SIZE);
     };
 
     /* Total data fits in multiple records.
@@ -453,5 +452,48 @@ int main(int argc, char **argv)
         EXPECT_EQUAL(conn->out.blob.size, buffer_size);
     };
 
+    /* Test: Alert records are not buffered with ApplicationData records
+     *
+     * We flush before sending an alert, even if there is sufficient
+     * space for the alert record in the send buffer.
+     *
+     * If this behavior changed, then s2n_should_flush would need to consider
+     * the size of a possible alert.
+     */
+    {
+        DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(conn);
+        EXPECT_OK(s2n_connection_set_secrets(conn));
+        EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
+
+        struct s2n_send_context context = context_all_ok;
+        EXPECT_SUCCESS(s2n_connection_set_send_ctx(conn, (void *) &context));
+        EXPECT_SUCCESS(s2n_connection_set_send_cb(conn, s2n_test_send_cb));
+
+        const uint32_t send_size = 10;
+        const uint32_t max_app_data_record_size = S2N_TLS_MAX_RECORD_LEN_FOR(send_size);
+        const uint32_t max_alert_record_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_ALERT_LENGTH);
+        const uint32_t min_send_buffer_size = max_app_data_record_size + max_alert_record_size;
+        EXPECT_TRUE(min_send_buffer_size <= buffer_size);
+
+        /* Queue the alert */
+        EXPECT_SUCCESS(s2n_queue_writer_close_alert_warning(conn));
+
+        /* Send the Application Data and Alert */
+        s2n_blocked_status blocked = 0;
+        EXPECT_EQUAL(s2n_send(conn, large_test_data, send_size, &blocked), send_size);
+
+        /* We expect two separate send calls: one for the application data record
+         * and one for the alert record.
+         */
+        EXPECT_EQUAL(context.calls, 2);
+
+        /* We expect that the output buffer never contained all data sent,
+         * since that data was split between two records.
+         */
+        EXPECT_TRUE(conn->out.high_water_mark < context.bytes_sent);
+    };
+
     END_TEST();
 }

tls/s2n_config.c

@@ -998,7 +998,7 @@ int s2n_config_client_hello_cb_enable_poll(struct s2n_config *config)
 int s2n_config_set_send_buffer_size(struct s2n_config *config, uint32_t size)
 {
     POSIX_ENSURE_REF(config);
-    POSIX_ENSURE(size >= S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MAX_FRAGMENT_LENGTH_MIN), S2N_ERR_INVALID_ARGUMENT);
+    POSIX_ENSURE(size >= S2N_MIN_SEND_BUFFER_SIZE, S2N_ERR_INVALID_ARGUMENT);
     config->send_buffer_size_override = size;
     return S2N_SUCCESS;
 }

tls/s2n_config.h

@@ -15,20 +15,37 @@
 
 #pragma once
 
+#include <sys/param.h>
+
 #include "api/s2n.h"
 #include "crypto/s2n_certificate.h"
 #include "crypto/s2n_dhe.h"
 #include "tls/s2n_crl.h"
+#include "tls/s2n_key_update.h"
 #include "tls/s2n_psk.h"
+#include "tls/s2n_record.h"
 #include "tls/s2n_renegotiate.h"
 #include "tls/s2n_resume.h"
+#include "tls/s2n_tls_parameters.h"
 #include "tls/s2n_x509_validator.h"
 #include "utils/s2n_blob.h"
 #include "utils/s2n_set.h"
 
 #define S2N_MAX_TICKET_KEYS       48
 #define S2N_MAX_TICKET_KEY_HASHES 500 /* 10KB */
 
+/*
+ * TLS1.3 does not allow alert messages to be fragmented, and some TLS
+ * implementations (for example, GnuTLS) reject fragmented TLS1.2 alerts.
+ * The send buffer must be able to hold an unfragmented alert message.
+ *
+ * We choose not to fragment KeyUpdate messages to keep our post-handshake
+ * fragmentation logic simple and consistent across message types.
+ * The send buffer must be able to hold an unfragmented KeyUpdate message.
+ */
+#define S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE MAX(S2N_KEY_UPDATE_MESSAGE_SIZE, S2N_ALERT_LENGTH)
+#define S2N_MIN_SEND_BUFFER_SIZE          S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE)
+
 struct s2n_cipher_preferences;
 
 typedef enum {

tls/s2n_connection.h

@@ -35,6 +35,7 @@
 #include "tls/s2n_prf.h"
 #include "tls/s2n_quic_support.h"
 #include "tls/s2n_record.h"
+#include "tls/s2n_resume.h"
 #include "tls/s2n_security_policies.h"
 #include "tls/s2n_tls_parameters.h"
 #include "tls/s2n_x509_validator.h"

tls/s2n_crypto.h

@@ -24,7 +24,6 @@
 #include "crypto/s2n_pkey.h"
 #include "crypto/s2n_signature.h"
 #include "crypto/s2n_tls13_keys.h"
-#include "tls/s2n_config.h"
 #include "tls/s2n_crypto_constants.h"
 #include "tls/s2n_kem.h"
 #include "tls/s2n_signature_scheme.h"

tls/s2n_record.h

@@ -40,18 +40,6 @@
  */
 #define S2N_TLS_MAXIMUM_FRAGMENT_LENGTH (1 << 14)
 
-/*
- * The minimum amount of space we need to reserve for a message
- * fragment. We cannot fragment alert messages because not all peer
- * implementations accept them, even in TLS1.2 where it is not
- * disallowed by RFC5246.
- * 
- * Specificity we found that GnuTLS rejects fragmented alert messages.
- * This is a simple solution for the Alert Attack, although it is
- * strictly speaking a violation of the standard.
- */
-#define S2N_MAX_FRAGMENT_LENGTH_MIN 2
-
 /* The TLS1.2 record length allows for 1024 bytes of compression expansion and
  * 1024 bytes of encryption expansion and padding.
  * Since S2N does not support compression, we can ignore the compression overhead.

tls/s2n_record_write.c

@@ -72,6 +72,7 @@ S2N_RESULT s2n_record_max_write_payload_size(struct s2n_connection *conn, uint16
     RESULT_ENSURE_REF(conn);
     RESULT_ENSURE_REF(conn->config);
     RESULT_ENSURE_MUT(max_fragment_size);
+    RESULT_ENSURE(conn->max_outgoing_fragment_length > 0, S2N_ERR_FRAGMENT_LENGTH_TOO_SMALL);
 
     *max_fragment_size = MIN(conn->max_outgoing_fragment_length, S2N_TLS_MAXIMUM_FRAGMENT_LENGTH);
 
@@ -89,9 +90,6 @@ S2N_RESULT s2n_record_max_write_payload_size(struct s2n_connection *conn, uint16
         }
     }
 
-    /* Ensure that we don't reserve too little space.  */
-    RESULT_ENSURE(*max_fragment_size >= S2N_MAX_FRAGMENT_LENGTH_MIN, S2N_ERR_FRAGMENT_LENGTH_TOO_SMALL);
-
     return S2N_RESULT_OK;
 }
 

tls/s2n_send.c

@@ -29,6 +29,16 @@
 #include "utils/s2n_blob.h"
 #include "utils/s2n_safety.h"
 
+/*
+ * Determine whether there is currently sufficient space in the send buffer to construct
+ * another record, or if we need to flush now.
+ *
+ * We only buffer multiple records when sending application data, NOT when
+ * sending handshake messages or alerts. If the next record is a post-handshake message
+ * or an alert, then the send buffer will be flushed regardless of the result of this method.
+ * Therefore we don't need to consider the size of any potential KeyUpdate messages,
+ * NewSessionTicket messages, or Alerts.
+ */
 bool s2n_should_flush(struct s2n_connection *conn, ssize_t total_message_size)
 {
     /* Always flush if not buffering multiple records. */