[libsoup/carlosgc/websockets-invalid-encode-length: 2/2] WebSockets: fail and close the connection in case of invalid payload length

[Carlos Garcia Campos <carlosgc src gnome org>]

commit 8d6898ed6c46b68a11544ca50c2b6371a08be0ca
Author: Carlos Garcia Campos <cgarcia igalia com>
Date:   Tue Jun 18 17:42:05 2019 +0200

    WebSockets: fail and close the connection in case of invalid payload length
    
    RFC 6455:
    
    The length of the "Payload data", in bytes: if 0-125, that is the
    payload length.  If 126, the following 2 bytes interpreted as a
    16-bit unsigned integer are the payload length.  If 127, the
    following 8 bytes interpreted as a 64-bit unsigned integer (the
    most significant bit MUST be 0) are the payload length.  Multibyte
    length quantities are expressed in network byte order.  Note that
    in all cases, the minimal number of bytes MUST be used to encode
    the length, for example, the length of a 124-byte-long string
    can't be encoded as the sequence 126, 0, 124.

 libsoup/soup-websocket-connection.c |  19 +++++++
 tests/websocket-test.c              | 103 ++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)
---
diff --git a/libsoup/soup-websocket-connection.c b/libsoup/soup-websocket-connection.c
index 97866fe7..a753ed6e 100644
--- a/libsoup/soup-websocket-connection.c
+++ b/libsoup/soup-websocket-connection.c
@@ -839,13 +839,26 @@ process_frame (SoupWebsocketConnection *self)
 
        switch (header[1] & 0x7f) {
        case 126:
+               /* If 126, the following 2 bytes interpreted as a 16-bit
+                * unsigned integer are the payload length.
+                */
                at = 4;
                if (len < at)
                        return FALSE; /* need more data */
                payload_len = (((guint16)header[2] << 8) |
                               ((guint16)header[3] << 0));
+
+               /* The minimal number of bytes MUST be used to encode the length. */
+               if (payload_len <= 125) {
+                       protocol_error_and_close (self);
+                       return FALSE;
+               }
                break;
        case 127:
+               /* If 127, the following 8 bytes interpreted as a 64-bit
+                * unsigned integer (the most significant bit MUST be 0)
+                * are the payload length.
+                */
                at = 10;
                if (len < at)
                        return FALSE; /* need more data */
@@ -857,6 +870,12 @@ process_frame (SoupWebsocketConnection *self)
                               ((guint64)header[7] << 16) |
                               ((guint64)header[8] << 8) |
                               ((guint64)header[9] << 0));
+
+               /* The minimal number of bytes MUST be used to encode the length. */
+               if (payload_len <= G_MAXUINT16) {
+                       protocol_error_and_close (self);
+                       return FALSE;
+               }
                break;
        default:
                payload_len = header[1] & 0x7f;
diff --git a/tests/websocket-test.c b/tests/websocket-test.c
index 2347ed5c..60f2065b 100644
--- a/tests/websocket-test.c
+++ b/tests/websocket-test.c
@@ -843,6 +843,99 @@ test_receive_fragmented (Test *test,
        WAIT_UNTIL (soup_websocket_connection_get_state (test->client) == SOUP_WEBSOCKET_STATE_CLOSED);
 }
 
+typedef struct {
+       Test *test;
+       const char *header;
+       GString *payload;
+} InvalidEncodeLengthTest;
+
+static gpointer
+send_invalid_encode_length_server_thread (gpointer user_data)
+{
+       InvalidEncodeLengthTest *test = user_data;
+       gsize header_size;
+       gsize written;
+       GError *error = NULL;
+
+       header_size = test->payload->len == 125 ? 6 : 10;
+       g_output_stream_write_all (g_io_stream_get_output_stream (test->test->raw_server),
+                                  test->header, header_size, &written, NULL, &error);
+       g_assert_no_error (error);
+       g_assert_cmpuint (written, ==, header_size);
+
+       g_output_stream_write_all (g_io_stream_get_output_stream (test->test->raw_server),
+                                  test->payload->str, test->payload->len, &written, NULL, &error);
+       g_assert_no_error (error);
+       g_assert_cmpuint (written, ==, test->payload->len);
+
+       g_io_stream_close (test->test->raw_server, NULL, &error);
+       g_assert_no_error (error);
+
+       return NULL;
+}
+
+static void
+test_receive_invalid_encode_length_16 (Test *test,
+                                      gconstpointer data)
+{
+       GThread *thread;
+       GBytes *received = NULL;
+       GError *error = NULL;
+       InvalidEncodeLengthTest context = { test, NULL };
+       guint i;
+
+       g_signal_connect (test->client, "error", G_CALLBACK (on_error_copy), &error);
+       g_signal_connect (test->client, "message", G_CALLBACK (on_binary_message), &received);
+
+       /* We use 126(~) as payload length with 125 extended length */
+       context.header = "\x82~\x00}";
+       context.payload = g_string_new (NULL);
+       for (i = 0; i < 125; i++)
+               g_string_append (context.payload, "X");
+       thread = g_thread_new ("invalid-encode-length-thread", send_invalid_encode_length_server_thread, 
&context);
+
+       WAIT_UNTIL (error != NULL || received != NULL);
+       g_assert_error (error, SOUP_WEBSOCKET_ERROR, SOUP_WEBSOCKET_CLOSE_PROTOCOL_ERROR);
+       g_clear_error (&error);
+       g_assert_null (received);
+
+       g_thread_join (thread);
+       g_string_free (context.payload, TRUE);
+
+       WAIT_UNTIL (soup_websocket_connection_get_state (test->client) == SOUP_WEBSOCKET_STATE_CLOSED);
+}
+
+static void
+test_receive_invalid_encode_length_64 (Test *test,
+                                      gconstpointer data)
+{
+       GThread *thread;
+       GBytes *received = NULL;
+       GError *error = NULL;
+       InvalidEncodeLengthTest context = { test, NULL };
+       guint i;
+
+       g_signal_connect (test->client, "error", G_CALLBACK (on_error_copy), &error);
+       g_signal_connect (test->client, "message", G_CALLBACK (on_binary_message), &received);
+
+       /* We use 127(\x7f) as payload length with 65535 extended length */
+       context.header = "\x82\x7f\x00\x00\x00\x00\x00\x00\xff\xff";
+       context.payload = g_string_new (NULL);
+       for (i = 0; i < 65535; i++)
+               g_string_append (context.payload, "X");
+       thread = g_thread_new ("invalid-encode-length-thread", send_invalid_encode_length_server_thread, 
&context);
+
+       WAIT_UNTIL (error != NULL || received != NULL);
+       g_assert_error (error, SOUP_WEBSOCKET_ERROR, SOUP_WEBSOCKET_CLOSE_PROTOCOL_ERROR);
+       g_clear_error (&error);
+       g_assert_null (received);
+
+        g_thread_join (thread);
+       g_string_free (context.payload, TRUE);
+
+       WAIT_UNTIL (soup_websocket_connection_get_state (test->client) == SOUP_WEBSOCKET_STATE_CLOSED);
+}
+
 static void
 test_client_context_got_server_connection (SoupServer              *server,
                                           SoupWebsocketConnection *connection,
@@ -1006,6 +1099,16 @@ main (int argc,
                    test_receive_fragmented,
                    teardown_direct_connection);
 
+       g_test_add ("/websocket/direct/receive-invalid-encode-length-16", Test, NULL,
+                   setup_half_direct_connection,
+                   test_receive_invalid_encode_length_16,
+                   teardown_direct_connection);
+
+       g_test_add ("/websocket/direct/receive-invalid-encode-length-64", Test, NULL,
+                   setup_half_direct_connection,
+                   test_receive_invalid_encode_length_64,
+                   teardown_direct_connection);
+
        if (g_test_slow ()) {
                g_test_add ("/websocket/direct/close-after-timeout", Test, NULL,
                            setup_half_direct_connection,