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,7 +460,7 @@ 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);
+        const uint32_t min_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE);
 
         /* Safety */
         {

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

@@ -208,7 +208,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

@@ -172,7 +172,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);
@@ -181,7 +181,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

@@ -199,7 +199,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

@@ -148,7 +148,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 */
@@ -164,6 +164,32 @@ int main(int argc, char **argv)
             EXPECT_SUCCESS(s2n_stuffer_skip_write(&conn->out, 1));
             EXPECT_TRUE(s2n_should_flush(conn, buffer_size));
         }
+
+        /* Flush if buffer can't hold alert record */
+        {
+            DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
+                    s2n_connection_ptr_free);
+            EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
+
+            /* Leave sufficient space for an alert */
+            EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&conn->out, buffer_size));
+            size_t alert_record_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_ALERT_LENGTH);
+            EXPECT_SUCCESS(s2n_stuffer_skip_write(&conn->out, buffer_size - alert_record_size));
+
+            /* Insufficient space for max size record,
+             * so set fragmentation length to minimum so that it's not a problem.
+             */
+            EXPECT_TRUE(s2n_should_flush(conn, buffer_size));
+            conn->max_outgoing_fragment_length = 1;
+            EXPECT_TRUE(conn->max_outgoing_fragment_length < S2N_ALERT_LENGTH);
+
+            /* Sufficient space for alert */
+            EXPECT_FALSE(s2n_should_flush(conn, buffer_size));
+
+            /* Insufficient space for alert */
+            EXPECT_SUCCESS(s2n_stuffer_skip_write(&conn->out, 1));
+            EXPECT_TRUE(s2n_should_flush(conn, buffer_size));
+        }
     }
 
     /* Total data fits in a single record.
@@ -195,12 +221,12 @@ int main(int argc, char **argv)
      * Send smaller records.
      */
     {
-        /* 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,
+        /* 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 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);
+        uint32_t min_buffer_size = S2N_TLS_MAX_RECORD_LEN_FOR(S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE);
 
         DEFER_CLEANUP(struct s2n_config *min_buffer_config = s2n_config_new(), s2n_config_ptr_free);
         EXPECT_NOT_NULL(min_buffer_config);
@@ -216,14 +242,15 @@ 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);
 

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_TLS_MAX_RECORD_LEN_FOR(S2N_MIN_SEND_BUFFER_FRAGMENT_SIZE), S2N_ERR_INVALID_ARGUMENT);
     config->send_buffer_size_override = size;
     return S2N_SUCCESS;
 }

tls/s2n_config.h

@@ -15,20 +15,35 @@
 
 #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_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)
+
 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_key_update.c

@@ -79,8 +79,8 @@ int s2n_key_update_send(struct s2n_connection *conn, s2n_blocked_status *blocked
         /* Write key update message */
         POSIX_GUARD(s2n_key_update_write(&key_update_blob));
 
-        /* Encrypt the message */
-        POSIX_GUARD(s2n_record_write(conn, TLS_HANDSHAKE, &key_update_blob));
+        /* Encrypt the message in a single record */
+        POSIX_GUARD_RESULT(s2n_record_write_all(conn, TLS_HANDSHAKE, &key_update_blob));
 
         /* Update encryption key */
         POSIX_GUARD(s2n_update_application_traffic_keys(conn, conn->mode, SENDING));

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.
@@ -81,6 +69,7 @@
 S2N_RESULT s2n_record_max_write_size(struct s2n_connection *conn, uint16_t max_fragment_size, uint16_t *max_record_size);
 extern S2N_RESULT s2n_record_max_write_payload_size(struct s2n_connection *conn, uint16_t *max_fragment_size);
 extern S2N_RESULT s2n_record_min_write_payload_size(struct s2n_connection *conn, uint16_t *payload_size);
+S2N_RESULT s2n_record_write_all(struct s2n_connection *conn, uint8_t content_type, struct s2n_blob *in);
 extern int s2n_record_write(struct s2n_connection *conn, uint8_t content_type, struct s2n_blob *in);
 extern int s2n_record_writev(struct s2n_connection *conn, uint8_t content_type, const struct iovec *in, int in_count, size_t offs, size_t to_write);
 extern int s2n_record_parse(struct s2n_connection *conn);

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;
 }
 
@@ -526,3 +524,11 @@ int s2n_record_write(struct s2n_connection *conn, uint8_t content_type, struct s
     iov.iov_len = in->size;
     return s2n_record_writev(conn, content_type, &iov, 1, 0, in->size);
 }
+
+S2N_RESULT s2n_record_write_all(struct s2n_connection *conn, uint8_t content_type, struct s2n_blob *in)
+{
+    int written = s2n_record_write(conn, content_type, in);
+    RESULT_GUARD_POSIX(written);
+    RESULT_ENSURE(written == in->size, S2N_ERR_FRAGMENT_LENGTH_TOO_LARGE);
+    return S2N_RESULT_OK;
+}

tls/s2n_send.c

@@ -49,6 +49,12 @@ bool s2n_should_flush(struct s2n_connection *conn, ssize_t total_message_size)
     }
     max_payload_size = MIN(max_payload_size, remaining_payload_size);
 
+    /* We can't guarantee that the next record won't be an alert.
+     * TLS1.3 does not allow alerts to be fragmented, and some TLS implementations
+     * (for example, GnuTLS) reject fragmented TLS1.2 alerts.
+     */
+    max_payload_size = MAX(max_payload_size, S2N_ALERT_LENGTH);
+
     uint16_t max_write_size = 0;
     if (!s2n_result_is_ok(s2n_record_max_write_size(conn, max_payload_size, &max_write_size))) {
         /* When in doubt, flush */
@@ -98,7 +104,7 @@ int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *blocked)
         struct s2n_blob alert = { 0 };
         alert.data = conn->reader_alert_out.blob.data;
         alert.size = 2;
-        POSIX_GUARD(s2n_record_write(conn, TLS_ALERT, &alert));
+        POSIX_GUARD_RESULT(s2n_record_write_all(conn, TLS_ALERT, &alert));
         POSIX_GUARD(s2n_stuffer_rewrite(&conn->reader_alert_out));
         POSIX_GUARD_RESULT(s2n_alerts_close_if_fatal(conn, &alert));
 
@@ -111,7 +117,7 @@ int s2n_flush(struct s2n_connection *conn, s2n_blocked_status *blocked)
         struct s2n_blob alert = { 0 };
         alert.data = conn->writer_alert_out.blob.data;
         alert.size = 2;
-        POSIX_GUARD(s2n_record_write(conn, TLS_ALERT, &alert));
+        POSIX_GUARD_RESULT(s2n_record_write_all(conn, TLS_ALERT, &alert));
         POSIX_GUARD(s2n_stuffer_rewrite(&conn->writer_alert_out));
         POSIX_GUARD_RESULT(s2n_alerts_close_if_fatal(conn, &alert));