Merge commit from fork

* Define a new max_fragmented_message_size limit on fragmented WebSocket frames

* Set a default max frame size of 8MB

* Disallow zero length non-fin continuation frames

Author: mtrudel

lib/bandit.ex

@@ -191,7 +191,10 @@ defmodule Bandit do
 
   * `enabled`: Whether or not to serve WebSocket upgrade requests. Defaults to true
   * `max_frame_size`: The maximum size of a single WebSocket frame (expressed as
-    a number of bytes on the wire). Defaults to 0 (no limit)
+    a number of bytes on the wire). Use a value of 0 for no limit. Defaults to 8_000_000
+  * `max_fragmented_message_size`: The maximum size of a WebSocket message delivered across
+    multiple continuation frames (expressed as a number of bytes on the wire). Does NOT affect the
+    handling of single-frame messages. Use a value of 0 for no limit. Defaults to 8_000_000
   * `validate_text_frames`: Whether or not to validate text frames as being UTF-8. Strictly
     speaking this is required per RFC6455§5.6, however it can be an expensive operation and one
     that may be safely skipped in some situations. Defaults to true
@@ -209,6 +212,7 @@ defmodule Bandit do
   @type websocket_options :: [
           {:enabled, boolean()}
           | {:max_frame_size, pos_integer()}
+          | {:max_fragmented_message_size, pos_integer()}
           | {:validate_text_frames, boolean()}
           | {:compress, boolean()}
           | {:deflate_options, deflate_options()}
@@ -249,7 +253,7 @@ defmodule Bandit do
   @http_keys ~w(compress response_encodings deflate_options zstd_options log_exceptions_with_status_codes log_protocol_errors log_client_closures)a
   @http_1_keys ~w(enabled max_request_line_length max_header_length max_header_count max_requests clear_process_dict gc_every_n_keepalive_requests log_unknown_messages)a
   @http_2_keys ~w(enabled max_header_block_size max_requests max_reset_stream_rate sendfile_chunk_size default_local_settings)a
-  @websocket_keys ~w(enabled max_frame_size validate_text_frames compress deflate_options max_inflate_ratio primitive_ops_module)a
+  @websocket_keys ~w(enabled max_frame_size max_fragmented_message_size validate_text_frames compress deflate_options max_inflate_ratio primitive_ops_module)a
   @thousand_island_keys ThousandIsland.ServerConfig.__struct__()
                         |> Map.from_struct()
                         |> Map.keys()

lib/bandit/extractor.ex

@@ -33,7 +33,7 @@ defmodule Bandit.Extractor do
 
   @spec new(module(), module(), Keyword.t()) :: t()
   def new(frame_parser, primitive_ops_module, opts) do
-    max_frame_size = Keyword.get(opts, :max_frame_size, 0)
+    max_frame_size = Keyword.get(opts, :max_frame_size, 8_000_000)
 
     %__MODULE__{
       max_frame_size: max_frame_size,

lib/bandit/websocket/connection.ex

@@ -94,13 +94,26 @@ defmodule Bandit.WebSocket.Connection do
     case frame do
       %Frame.Continuation{fin: true} = frame ->
         data = IO.iodata_to_binary([connection.fragment_frame.data | frame.data])
-        frame = %{connection.fragment_frame | fin: true, data: data}
-        handle_frame(frame, socket, %{connection | fragment_frame: nil})
+
+        if oversize_message?(data, connection.opts) do
+          do_error(1009, "Received oversize fragmented message", socket, connection)
+        else
+          frame = %{connection.fragment_frame | fin: true, data: data}
+          handle_frame(frame, socket, %{connection | fragment_frame: nil})
+        end
 
       %Frame.Continuation{fin: false} = frame ->
-        data = [connection.fragment_frame.data | frame.data]
-        frame = %{connection.fragment_frame | fin: true, data: data}
-        {:continue, %{connection | fragment_frame: frame}}
+        if IO.iodata_length(frame.data) == 0 do
+          do_error(1008, "Received zero byte non-fin continuation frame", socket, connection)
+        else
+          data = [connection.fragment_frame.data | frame.data]
+
+          if oversize_message?(data, connection.opts) do
+            do_error(1009, "Received oversize fragmented message", socket, connection)
+          else
+            {:continue, %{connection | fragment_frame: %{connection.fragment_frame | data: data}}}
+          end
+        end
 
       %Frame.Text{} ->
         do_error(1002, "Received unexpected text frame (RFC6455§5.4)", socket, connection)
@@ -113,6 +126,13 @@ defmodule Bandit.WebSocket.Connection do
     end
   end
 
+  defp oversize_message?(data, opts) do
+    case Keyword.get(opts, :max_fragmented_message_size, 8_000_000) do
+      0 -> false
+      max_fragmented_message_size -> IO.iodata_length(data) > max_fragmented_message_size
+    end
+  end
+
   defp handle_control_frame(frame, socket, connection) do
     case frame do
       %Frame.ConnectionClose{} = frame ->

test/bandit/websocket/protocol_test.exs

@@ -150,6 +150,106 @@ defmodule WebSocketProtocolTest do
       expected_payload = String.duplicate(payload, 3)
       assert SimpleWebSocketClient.recv_binary_frame(client) == {:ok, expected_payload}
     end
+
+    test "zero byte fin continuation frames are accepted", context do
+      client = SimpleWebSocketClient.tcp_client(context)
+      SimpleWebSocketClient.http1_handshake(client, EchoWebSock)
+
+      payload = String.duplicate("0123456789", 1_000)
+      SimpleWebSocketClient.send_binary_frame(client, payload, 0x0)
+      SimpleWebSocketClient.send_continuation_frame(client, payload, 0x0)
+      SimpleWebSocketClient.send_continuation_frame(client, <<>>)
+
+      expected_payload = String.duplicate(payload, 2)
+      assert SimpleWebSocketClient.recv_binary_frame(client) == {:ok, expected_payload}
+    end
+
+    test "zero byte non-fin continuation frames are rejected", context do
+      output =
+        capture_log(fn ->
+          client = SimpleWebSocketClient.tcp_client(context)
+          SimpleWebSocketClient.http1_handshake(client, TerminateWebSock)
+
+          SimpleWebSocketClient.send_binary_frame(client, "0123456789", 0x0)
+          SimpleWebSocketClient.send_continuation_frame(client, <<>>, 0x0)
+
+          # Get the error that terminate saw, to ensure we're closing for the expected reason
+          assert_receive {:error, "Received zero byte non-fin continuation frame"}, 500
+
+          assert SimpleWebSocketClient.recv_connection_close_frame(client) == {:ok, <<1008::16>>}
+
+          # Verify that the server didn't send any extraneous frames
+          assert SimpleWebSocketClient.connection_closed_for_reading?(client)
+          Process.sleep(500)
+        end)
+
+      assert output =~ "Received zero byte non-fin continuation frame"
+    end
+
+    test "max_fragmented_message_size enforced for continuation frames", context do
+      output =
+        capture_log(fn ->
+          context =
+            http_server(context, websocket_options: [max_fragmented_message_size: 2_000_000])
+
+          client = SimpleWebSocketClient.tcp_client(context)
+          SimpleWebSocketClient.http1_handshake(client, TerminateWebSock)
+
+          payload = String.duplicate("0123456789", 99_999)
+          SimpleWebSocketClient.send_binary_frame(client, payload, 0x0)
+          SimpleWebSocketClient.send_continuation_frame(client, payload, 0x0)
+
+          # We should still be alive here
+          refute_receive {:error, "Received oversize fragmented message"}, 50
+
+          # This should send us over the edge
+          SimpleWebSocketClient.send_continuation_frame(client, payload, 0x0)
+
+          # Get the error that terminate saw, to ensure we're closing for the expected reason
+          assert_receive {:error, "Received oversize fragmented message"}, 500
+
+          assert SimpleWebSocketClient.recv_connection_close_frame(client) == {:ok, <<1009::16>>}
+
+          # Verify that the server didn't send any extraneous frames
+          assert SimpleWebSocketClient.connection_closed_for_reading?(client)
+          Process.sleep(500)
+        end)
+
+      assert output =~ "Received oversize fragmented message"
+    end
+
+    test "max_fragmented_message_size enforced for continuation fin frames", context do
+      output =
+        capture_log(fn ->
+          context =
+            http_server(context, websocket_options: [max_fragmented_message_size: 2_000_000])
+
+          client = SimpleWebSocketClient.tcp_client(context)
+          SimpleWebSocketClient.http1_handshake(client, TerminateWebSock)
+
+          payload = String.duplicate("0123456789", 99_999)
+          SimpleWebSocketClient.send_binary_frame(client, payload, 0x0)
+
+          SimpleWebSocketClient.send_continuation_frame(client, payload, 0x0)
+
+          # We should still be alive here
+          refute_receive {:error, "Received oversize fragmented message"}, 50
+
+          # This should send us over the edge
+          SimpleWebSocketClient.send_continuation_frame(client, payload)
+
+          # Get the error that terminate saw, to ensure we're closing for the expected reason
+          assert_receive {:error, "Received oversize fragmented message"}, 500
+
+          assert SimpleWebSocketClient.recv_connection_close_frame(client) == {:ok, <<1009::16>>}
+
+          # Verify that the server didn't send any extraneous frames
+          assert SimpleWebSocketClient.connection_closed_for_reading?(client)
+          Process.sleep(500)
+        end)
+
+      assert output =~ "Received oversize fragmented message"
+    end
   end
 
   describe "compressed frames" do