Ensure non-zero record protocol version

tests/unit/s2n_record_write_test.c

@@ -0,0 +1,87 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include "s2n_test.h"
+#include "testlib/s2n_testlib.h"
+#include "tls/s2n_record.h"
+#include "tls/s2n_tls.h"
+#include "utils/s2n_safety.h"
+
+int main(int argc, char *argv[])
+{
+    BEGIN_TEST();
+
+    /* Test: Records sent before the ServerHello include a sane protocol version */
+    {
+        const uint8_t expected_version = S2N_TLS10;
+
+        /* Test: ClientHellos include a sane protocol version */
+        {
+            DEFER_CLEANUP(struct s2n_connection *client_conn = s2n_connection_new(S2N_CLIENT),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(client_conn);
+
+            DEFER_CLEANUP(struct s2n_stuffer out = { 0 }, s2n_stuffer_free);
+            EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&out, 0));
+            EXPECT_SUCCESS(s2n_connection_set_send_io_stuffer(&out, client_conn));
+
+            s2n_blocked_status blocked = S2N_NOT_BLOCKED;
+            EXPECT_OK(s2n_negotiate_until_message(client_conn, &blocked, SERVER_HELLO));
+
+            uint8_t content_type = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint8(&out, &content_type));
+            EXPECT_EQUAL(content_type, TLS_HANDSHAKE);
+
+            uint8_t version_high = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint8(&out, &version_high));
+            EXPECT_EQUAL(version_high, expected_version / 10);
+
+            uint8_t version_low = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint8(&out, &version_low));
+            EXPECT_EQUAL(version_low, expected_version % 10);
+        }
+
+        /* Test: Alerts include a sane protocol version */
+        {
+            DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(server_conn);
+
+            DEFER_CLEANUP(struct s2n_stuffer out = { 0 }, s2n_stuffer_free);
+            EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&out, 0));
+            EXPECT_SUCCESS(s2n_connection_set_send_io_stuffer(&out, server_conn));
+
+            EXPECT_SUCCESS(s2n_queue_writer_close_alert_warning(server_conn));
+
+            s2n_blocked_status blocked = S2N_NOT_BLOCKED;
+            EXPECT_ERROR_WITH_ERRNO(s2n_negotiate_until_message(server_conn, &blocked, SERVER_HELLO),
+                    S2N_ERR_CLOSED);
+
+            uint8_t content_type = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint8(&out, &content_type));
+            EXPECT_EQUAL(content_type, TLS_ALERT);
+
+            uint8_t version_high = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint8(&out, &version_high));
+            EXPECT_EQUAL(version_high, expected_version / 10);
+
+            uint8_t version_low = 0;
+            EXPECT_SUCCESS(s2n_stuffer_read_uint8(&out, &version_low));
+            EXPECT_EQUAL(version_low, expected_version % 10);
+        }
+    }
+
+    END_TEST();
+}

tls/s2n_record_write.c

@@ -157,20 +157,42 @@ S2N_RESULT s2n_record_min_write_payload_size(struct s2n_connection *conn, uint16
 int s2n_record_write_protocol_version(struct s2n_connection *conn, struct s2n_stuffer *out)
 {
     uint8_t record_protocol_version = conn->actual_protocol_version;
+
+    /**
+     *= https://www.rfc-editor.org/rfc/rfc8446#section-5.1
+     *# This version value is historical, deriving from the use of 0x0301 for
+     *# TLS 1.0 and 0x0300 for SSL 3.0.  In order to maximize backward
+     *# compatibility, a record containing an initial ClientHello SHOULD have
+     *# version 0x0301 (reflecting TLS 1.0)
+     *
+     * We set actual_protocol_version early for clients, but we do not
+     * use that assumed value here in case we are talking to a legacy
+     * server that expects TLS1.0.
+     *
+     * If we are requesting early data, we can assume that we aren't talking to
+     * a legacy server as a legacy server would not know how to handle early data.
+     **/
     if (conn->server_protocol_version == s2n_unknown_protocol_version
             && conn->early_data_state != S2N_EARLY_DATA_REQUESTED) {
-        /* Some legacy TLS implementations can't handle records with protocol version higher than TLS1.0.
-         * To provide maximum compatibility, send record version as TLS1.0 if server protocol version isn't
-         * established yet, which happens only during ClientHello message. Note, this has no effect on
-         * protocol version in ClientHello, so we're still able to negotiate protocol versions above TLS1.0 */
         record_protocol_version = MIN(record_protocol_version, S2N_TLS10);
     }
 
-    /* In accordance to TLS 1.3 spec, https://tools.ietf.org/html/rfc8446#section-5.1
-     * tls record version should never be greater than 33 (legacy TLS 1.2 version).
-     */
+    /**
+     *= https://www.rfc-editor.org/rfc/rfc8446#section-5.1
+     *# legacy_record_version:  MUST be set to 0x0303 for all records
+     *#    generated by a TLS 1.3 implementation other than an initial
+     *#    ClientHello (i.e., one not generated after a HelloRetryRequest),
+     *#    where it MAY also be 0x0301 for compatibility purposes.
+     **/
     record_protocol_version = MIN(record_protocol_version, S2N_TLS12);
 
+    /* Never send an empty protocol version.
+     * If the protocol version is unknown, default to TLS1.0 like we do for initial ClientHellos.
+     */
+    if (record_protocol_version == s2n_unknown_protocol_version) {
+        record_protocol_version = S2N_TLS10;
+    }
+
     uint8_t protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN];
     protocol_version[0] = record_protocol_version / 10;
     protocol_version[1] = record_protocol_version % 10;