fix: parse fragmented sslv2 client hellos (#4425)

tests/unit/s2n_client_hello_recv_test.c

@@ -509,6 +509,45 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_disable_tls13_in_test());
     };
 
+    /* Test: Parse fragmented sslv2 client hello.
+     *
+     * Even if the sslv2 client hello is sent in one packet, there is no requirement
+     * that our first call to conn->recv returns the whole message. sslv2 uses separate
+     * record parsing code, so we need to ensure that those paths can handle partial reads.
+     */
+    {
+        DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
+                s2n_connection_ptr_free);
+        EXPECT_SUCCESS(s2n_connection_set_config(server, tls12_config));
+
+        struct s2n_stuffer server_in = { 0 };
+        uint8_t sslv2_client_hello[] = {
+            SSLv2_CLIENT_HELLO_HEADER,
+            SSLv2_CLIENT_HELLO_PREFIX,
+            SSLv2_CLIENT_HELLO_CIPHER_SUITES,
+            SSLv2_CLIENT_HELLO_CHALLENGE,
+        };
+        EXPECT_SUCCESS(s2n_blob_init(&server_in.blob,
+                sslv2_client_hello, sizeof(sslv2_client_hello)));
+        EXPECT_SUCCESS(s2n_connection_set_recv_io_stuffer(&server_in, server));
+
+        /* Read message one byte at a time */
+        s2n_blocked_status blocked = S2N_NOT_BLOCKED;
+        for (size_t i = 0; i < sizeof(sslv2_client_hello); i++) {
+            EXPECT_ERROR_WITH_ERRNO(
+                    s2n_negotiate_until_message(server, &blocked, SERVER_HELLO),
+                    S2N_ERR_IO_BLOCKED);
+            EXPECT_EQUAL(blocked, S2N_BLOCKED_ON_READ);
+            EXPECT_SUCCESS(s2n_stuffer_skip_write(&server_in, 1));
+        }
+
+        /* Successfully read the full message */
+        EXPECT_OK(s2n_negotiate_until_message(server, &blocked, SERVER_HELLO));
+        EXPECT_EQUAL(server->client_protocol_version, S2N_TLS12);
+        EXPECT_EQUAL(server->client_hello_version, S2N_SSLv2);
+        EXPECT_TRUE(server->client_hello.sslv2);
+    };
+
     s2n_config_free(tls12_config);
     s2n_config_free(tls13_config);
     s2n_cert_chain_and_key_free(chain_and_key);

tests/unit/s2n_record_read_test.c

@@ -0,0 +1,163 @@
+/*
+ * 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"
+
+#define SSLV2_MIN_SIZE 3
+
+int main(int argc, char *argv[])
+{
+    BEGIN_TEST();
+
+    /* Test s2n_sslv2_record_header_parse */
+    {
+        const struct {
+            uint8_t bytes[S2N_TLS_RECORD_HEADER_LENGTH];
+            uint16_t length;
+            uint8_t type;
+            uint8_t version;
+        } test_cases[] = {
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, SSLV2_MIN_SIZE, TLS_CLIENT_HELLO, 0x03, 0x03 },
+                    .length = 0,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, SSLV2_MIN_SIZE + 1, TLS_CLIENT_HELLO, 0x03, 0x03 },
+                    .length = 1,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, 0xFF, TLS_CLIENT_HELLO, 0x03, 0x03 },
+                    .length = 0xFF - SSLV2_MIN_SIZE,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, 0x84, TLS_CLIENT_HELLO, 0x03, 0x03 },
+                    .length = 0x84 - SSLV2_MIN_SIZE,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { 0x84, 0x84, TLS_CLIENT_HELLO, 0x03, 0x03 },
+                    .length = 0x484 - SSLV2_MIN_SIZE,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { 0xFF, 0xFF, TLS_CLIENT_HELLO, 0x03, 0x03 },
+                    .length = 0x7FFF - SSLV2_MIN_SIZE,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, SSLV2_MIN_SIZE, 0, 0x03, 0x03 },
+                    .length = 0,
+                    .type = 0,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, SSLV2_MIN_SIZE, 77, 0x03, 0x03 },
+                    .length = 0,
+                    .type = 77,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, SSLV2_MIN_SIZE, TLS_SERVER_HELLO, 0x03, 0x03 },
+                    .length = 0,
+                    .type = TLS_SERVER_HELLO,
+                    .version = S2N_TLS12,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, SSLV2_MIN_SIZE, TLS_CLIENT_HELLO, 0x03, 0x04 },
+                    .length = 0,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = S2N_TLS13,
+            },
+            {
+                    .bytes = { S2N_TLS_SSLV2_HEADER_FLAG, SSLV2_MIN_SIZE, TLS_CLIENT_HELLO, 0, 0 },
+                    .length = 0,
+                    .type = TLS_CLIENT_HELLO,
+                    .version = 0,
+            },
+        };
+
+        /* Test: parse valid record headers */
+        for (size_t i = 0; i < s2n_array_len(test_cases); i++) {
+            DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(conn);
+
+            EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->header_in,
+                    test_cases[i].bytes, sizeof(test_cases[i].bytes)));
+
+            uint8_t type = 0, version = 0;
+            uint16_t length = 0;
+            EXPECT_SUCCESS(s2n_sslv2_record_header_parse(conn, &type, &version, &length));
+            EXPECT_EQUAL(test_cases[i].type, type);
+            EXPECT_EQUAL(test_cases[i].version, version);
+            EXPECT_EQUAL(test_cases[i].length, length);
+
+            EXPECT_BYTEARRAY_EQUAL(conn->header_in_data,
+                    test_cases[i].bytes, sizeof(test_cases[i].bytes));
+
+            EXPECT_EQUAL(s2n_stuffer_data_available(&conn->header_in), S2N_TLS_RECORD_HEADER_LENGTH);
+        }
+
+        /* Test: parse header with bad length
+         *
+         * We already read 3 bytes by reading the header, so logically the message
+         * is at least 3 bytes long.
+         */
+        {
+            DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(conn);
+            EXPECT_SUCCESS(s2n_stuffer_skip_write(&conn->header_in, S2N_TLS_RECORD_HEADER_LENGTH));
+            conn->header_in_data[0] = S2N_TLS_SSLV2_HEADER_FLAG;
+
+            uint8_t type = 0, version = 0;
+            uint16_t length = 0;
+
+            conn->header_in_data[1] = SSLV2_MIN_SIZE - 1;
+            EXPECT_FAILURE_WITH_ERRNO(
+                    s2n_sslv2_record_header_parse(conn, &type, &version, &length),
+                    S2N_ERR_BAD_MESSAGE);
+            EXPECT_EQUAL(s2n_stuffer_data_available(&conn->header_in), 3);
+            EXPECT_SUCCESS(s2n_stuffer_reread(&conn->header_in));
+
+            conn->header_in_data[1] = SSLV2_MIN_SIZE;
+            EXPECT_SUCCESS(s2n_sslv2_record_header_parse(conn, &type, &version, &length));
+            EXPECT_EQUAL(s2n_stuffer_data_available(&conn->header_in), S2N_TLS_RECORD_HEADER_LENGTH);
+            EXPECT_SUCCESS(s2n_stuffer_reread(&conn->header_in));
+
+            conn->header_in_data[1] = 0;
+            EXPECT_FAILURE_WITH_ERRNO(
+                    s2n_sslv2_record_header_parse(conn, &type, &version, &length),
+                    S2N_ERR_BAD_MESSAGE);
+            EXPECT_EQUAL(s2n_stuffer_data_available(&conn->header_in), 3);
+            EXPECT_SUCCESS(s2n_stuffer_reread(&conn->header_in));
+        };
+    };
+
+    END_TEST();
+}

tests/unit/s2n_record_test.c

@@ -411,24 +411,6 @@ int main(int argc, char **argv)
         EXPECT_FAILURE_WITH_ERRNO(s2n_record_parse(conn), S2N_ERR_DECRYPT);
     };
 
-    /* Test s2n_sslv2_record_header_parse fails when fragment_length < 3 */
-    {
-        EXPECT_SUCCESS(s2n_stuffer_wipe(&conn->header_in));
-
-        uint8_t record_type = 0;
-        uint8_t protocol_version = 0;
-        uint16_t fragment_length = 0;
-
-        /* First two bytes are the fragment length */
-        uint8_t header_bytes[] = { 0x00, 0x00, 0x00, 0x00, 0x00 };
-        EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->header_in, header_bytes, sizeof(header_bytes)));
-
-        EXPECT_FAILURE_WITH_ERRNO(s2n_sslv2_record_header_parse(conn, &record_type, &protocol_version, &fragment_length), S2N_ERR_SAFETY);
-
-        /* Check the rest of the stuffer has not been read yet */
-        EXPECT_EQUAL(s2n_stuffer_data_available(&conn->header_in), 3);
-    };
-
     /* Record version is recorded for the first message received (Client Hello) */
     {
         DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER),

tls/s2n_record.h

@@ -22,6 +22,9 @@
 
 #define S2N_TLS_CONTENT_TYPE_LENGTH 1
 
+#define S2N_TLS_SSLV2_HEADER_FLAG        (0x80)
+#define S2N_TLS_SSLV2_HEADER_FLAG_UINT16 (S2N_TLS_SSLV2_HEADER_FLAG << 8)
+
 /* All versions of TLS define the record header the same:
  * ContentType + ProtocolVersion + length
  */

tls/s2n_record_read.c

@@ -34,36 +34,51 @@ int s2n_sslv2_record_header_parse(
         uint8_t *client_protocol_version,
         uint16_t *fragment_length)
 {
-    struct s2n_stuffer *in = &conn->header_in;
+    struct s2n_stuffer *header_in = &conn->header_in;
 
-    S2N_ERROR_IF(s2n_stuffer_data_available(in) < S2N_TLS_RECORD_HEADER_LENGTH, S2N_ERR_BAD_MESSAGE);
+    POSIX_ENSURE(s2n_stuffer_data_available(header_in) >= S2N_TLS_RECORD_HEADER_LENGTH,
+            S2N_ERR_BAD_MESSAGE);
 
-    POSIX_GUARD(s2n_stuffer_read_uint16(in, fragment_length));
+    POSIX_GUARD(s2n_stuffer_read_uint16(header_in, fragment_length));
 
-    /* The SSLv2 header is only a 2 byte record length (technically 3 bytes if
-     * padding is included, but s2n-tls assumes no padding).
-     * See https://www.ietf.org/archive/id/draft-hickman-netscape-ssl-00.txt.
+    /* The first bit of the SSLv2 message would usually indicate whether the
+     * length is 2 bytes long or 3 bytes long.
+     * See https://www.ietf.org/archive/id/draft-hickman-netscape-ssl-00.txt
+     *
+     * However, s2n-tls only supports SSLv2 for ClientHellos as defined in the
+     * TLS1.2 RFC. In that case, the first bit must always be set to distinguish
+     * SSLv2 from non-SSLv2 headers. The length is always 2 bytes.
+     * See https://datatracker.ietf.org/doc/html/rfc5246#appendix-E.2
+     *
+     * Since the first bit is not actually used to indicate length, we need to
+     * remove it from the length.
      *
-     * So by reading 5 bytes for a standard header we have also read the first
-     * 3 bytes of the record payload. s2n-tls only supports SSLv2 ClientHellos,
-     * so we assume that those 3 bytes are the first two fields of the
-     * SSLv2 ClientHello.
+     *= https://tools.ietf.org/rfc/rfc5246#appendix-E.2
+     *# msg_length
+     *#    The highest bit MUST be 1; the remaining bits contain the length
+     *#    of the following data in bytes.
      */
-
-    /* Because we already read 3 bytes of the record payload while trying to
-     * read a standard header, we need to adjust the length so that we only
-     * try to read the remainder of the record payload.
+    POSIX_ENSURE(*fragment_length & S2N_TLS_SSLV2_HEADER_FLAG_UINT16, S2N_ERR_BAD_MESSAGE);
+    *fragment_length ^= S2N_TLS_SSLV2_HEADER_FLAG_UINT16;
+
+    /* We read 5 bytes into header_in because we expected a standard, non-SSLv2 record header
+     * instead of an SSLv2 message. We have therefore already read 3 bytes of the payload.
+     * We need to adjust "fragment_length" to account for the bytes we have already
+     * read so that we will only attempt to read the remainder of the payload on
+     * our next call to conn->recv.
      */
-    POSIX_ENSURE_GTE(*fragment_length, 3);
-    *fragment_length -= 3;
+    POSIX_ENSURE(*fragment_length >= s2n_stuffer_data_available(header_in), S2N_ERR_BAD_MESSAGE);
+    *fragment_length -= s2n_stuffer_data_available(header_in);
 
-    /*
-     * The first field of an SSLv2 ClientHello is the msg_type.
+    /* By reading 5 bytes for a standard header we have also read the first
+     * 3 bytes of the SSLv2 ClientHello message.
+     * So we now need to parse those three bytes.
      *
+     * The first field of an SSLv2 ClientHello is the msg_type.
      * This is always '1', matching the ClientHello msg_type used by later
      * handshake messages.
      */
-    POSIX_GUARD(s2n_stuffer_read_uint8(in, record_type));
+    POSIX_GUARD(s2n_stuffer_read_uint8(header_in, record_type));
 
     /*
      * The second field of an SSLv2 ClientHello is the version.
@@ -73,10 +88,10 @@ int s2n_sslv2_record_header_parse(
      * See s2n_sslv2_client_hello_recv.
      */
     uint8_t protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN] = { 0 };
-    POSIX_GUARD(s2n_stuffer_read_bytes(in, protocol_version, S2N_TLS_PROTOCOL_VERSION_LEN));
-
+    POSIX_GUARD(s2n_stuffer_read_bytes(header_in, protocol_version, S2N_TLS_PROTOCOL_VERSION_LEN));
     *client_protocol_version = (protocol_version[0] * 10) + protocol_version[1];
 
+    POSIX_GUARD(s2n_stuffer_reread(header_in));
     return 0;
 }
 

tls/s2n_recv.c

@@ -70,15 +70,14 @@ int s2n_read_full_record(struct s2n_connection *conn, uint8_t *record_type, int
     POSIX_GUARD(s2n_stuffer_resize_if_empty(&conn->in, S2N_LARGE_FRAGMENT_LENGTH));
 
     /* Read the record until we at least have a header */
+    POSIX_GUARD(s2n_stuffer_reread(&conn->header_in));
     POSIX_GUARD_RESULT(s2n_read_in_bytes(conn, &conn->header_in, S2N_TLS_RECORD_HEADER_LENGTH));
 
     uint16_t fragment_length;
 
     /* If the first bit is set then this is an SSLv2 record */
-    if (conn->header_in.blob.data[0] & 0x80) {
-        conn->header_in.blob.data[0] &= 0x7f;
+    if (conn->header_in.blob.data[0] & S2N_TLS_SSLV2_HEADER_FLAG) {
         *isSSLv2 = 1;
-
         WITH_ERROR_BLINDING(conn, POSIX_GUARD(s2n_sslv2_record_header_parse(conn, record_type, &conn->client_protocol_version, &fragment_length)));
     } else {
         WITH_ERROR_BLINDING(conn, POSIX_GUARD(s2n_record_header_parse(conn, record_type, &fragment_length)));