websocket: Tighten frame decoding to RFC 6455 requirements

Reject WebSocket frames that violate RFC 6455 instead of accepting them or
treating malformed input as an orderly tunneled TCP shutdown.

Require every client-to-server frame to be masked and every
server-to-client frame to be unmasked. Require control frames to be final
and limit their payloads to 125 bytes.

Validate CLOSE payload structure by rejecting one-byte payloads and
requiring the optional reason following the status-code bytes to be valid
UTF-8. Keep the status-code bytes opaque so the codec remains compatible
with future protocol revisions.

Distinguish a valid WebSocket CLOSE from termination of the underlying
transport. Treat a transport FIN received without CLOSE, as well as other
frame protocol errors, as an abort that closes both tunnel directions
immediately.

Add integration coverage showing that unmasked client frames, malformed
CLOSE payloads, and transport termination without CLOSE are rejected.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

Author: jrajahalme

cilium/BUILD

@@ -160,6 +160,7 @@ envoy_cc_library(
         "//cilium:accesslog_lib",
         "//cilium:filter_state_lib",
         "//cilium/api:websocket_cc_proto",
+        "@com_google_protobuf//third_party/utf8_range:utf8_validity",
         "@envoy//bazel/external/http_parser",
         "@envoy//envoy/common/crypto:crypto_interface",
         "@envoy//source/common/common:base64_lib",

cilium/websocket_codec.cc

@@ -37,6 +37,7 @@
 #include "absl/strings/string_view.h"
 #include "cilium/websocket_config.h"
 #include "cilium/websocket_protocol.h"
+#include "third_party/utf8_range/utf8_validity.h"
 
 namespace Envoy {
 namespace Cilium {
@@ -682,6 +683,12 @@ void Codec::decode(Buffer::Instance& data, bool end_stream) {
   // Handshake done, process data.
   decoder_.decode(data, end_stream);
 
+  if (decoder_.protocol_error_) {
+    config->stats_.protocol_error_.inc();
+    decoder_.drain();
+    return closeOnError("invalid WebSocket control frame");
+  }
+
   if (end_stream && !decoder_.close_received_) {
     decoder_.drain();
     return closeOnError("websocket transport closed without CLOSE");
@@ -863,6 +870,7 @@ void Codec::Decoder::decode(Buffer::Instance& data, bool end_stream) {
 
     TRY_READ_NETWORK(&frame_header);
     const uint8_t opcode = frame_header[0] & OPCODE_MASK;
+    const bool final_frame = (frame_header[0] & FIN_MASK) != 0;
     const bool masked = (frame_header[1] & MASK_MASK) != 0;
     uint64_t payload_len = frame_header[1] & PAYLOAD_LEN_MASK;
 
@@ -884,6 +892,14 @@ void Codec::Decoder::decode(Buffer::Instance& data, bool end_stream) {
     // Whole header received and decoded
     //
 
+    // RFC 6455 section 5.1 requires client to mask all frames it sends to the server,
+    // and prohibits server from ever masking any frames is sends to the client.
+    const bool expected_masked = !parent_.config()->client_;
+    if (masked != expected_masked) {
+      ENVOY_LOG(debug, "websocket decoder: invalid frame masking");
+      goto protocol_error;
+    }
+
     if (opcode < OPCODE_CLOSE) {
       // Unframe and forward all non-control frames
       ENVOY_LOG(trace, "websocket decoder: received websocket data: header {} bytes, data {} bytes",
@@ -895,6 +911,11 @@ void Codec::Decoder::decode(Buffer::Instance& data, bool end_stream) {
 
     // Terminate and respond to any control frames
 
+    // control frames are always final
+    if (!final_frame) {
+      ENVOY_LOG(debug, "websocket decoder: invalid control frame");
+      goto protocol_error;
+    }
 
     // Protect against too large control frames that could happen if the decoder ever loses
     // sync with the data stream.
@@ -924,6 +945,21 @@ void Codec::Decoder::decode(Buffer::Instance& data, bool end_stream) {
 
       // validate CLOSE payload if any
       if (payload_len > 0) {
+        if (payload_len == 1) {
+          ENVOY_LOG(debug, "websocket decoder: invalid CLOSE payload");
+          goto protocol_error;
+        }
+
+        // we do not interpret the status code to remain compatible with future revisions of the
+        // WebSocket protocol specification, but we validate that any reason is UTF-8 encoded as
+        // required.
+        const absl::string_view reason{reinterpret_cast<const char*>(payload + 2),
+                                       static_cast<size_t>(payload_len - 2)};
+        if (!utf8_range::IsStructurallyValid(reason)) {
+          ENVOY_LOG(debug, "websocket decoder: invalid CLOSE reason");
+          goto protocol_error;
+        }
+
         // store for sending the frame back
         if (!close_received_) {
           close_payload_.assign(reinterpret_cast<const char*>(payload), payload_len);
@@ -953,6 +989,7 @@ void Codec::Decoder::decode(Buffer::Instance& data, bool end_stream) {
 
 protocol_error:
   buffer_.drain(buffer_.length());
+  protocol_error_ = true;
 }
 
 } // namespace WebSocket

cilium/websocket_codec.h

@@ -76,6 +76,7 @@ class Codec : Logger::Loggable<Logger::Id::filter> {
 
     Codec& parent_;
     bool close_received_{false};
+    bool protocol_error_{false};
     std::string close_payload_;
     Buffer::OwnedImpl buffer_;  // Buffer for partial websocket frames
     Buffer::OwnedImpl decoded_; // Buffer for decoded websocket frames

cilium/websocket_protocol.h

@@ -4,7 +4,7 @@
 
 // Some sensible limits to protect against excess resource use
 #define WEBSOCKET_HANDSHAKE_MAX_SIZE 4096
-#define WEBSOCKET_CONTROL_FRAME_MAX_SIZE 256
+#define WEBSOCKET_CONTROL_FRAME_MAX_SIZE 125 // RFC 6455 §5.5
 
 /* Ref. RFC 6455 */
 

tests/cilium_websocket_decap_integration_test.cc

@@ -388,6 +388,39 @@ TEST_P(CiliumWebSocketIntegrationTest, AcceptedWebSocket) {
   codec_client_->close();
 }
 
+TEST_P(CiliumWebSocketIntegrationTest, UnmaskedClientFrameRejected) {
+  initialize();
+  auto request_headers = Http::TestRequestHeaderMapImpl{
+      {":method", "GET"},
+      {":path", "/"},
+      {":authority", "host"},
+      {"Upgrade", "websocket"},
+      {"Connection", "Upgrade"},
+      {"Origin", "jarno.cilium.rocks"},
+      {"Sec-WebSocket-Key", "dGhlIHNhbXBsZSBub25jZQ=="},
+      {"Sec-WebSocket-Version", "13"},
+      {"x-request-id", "000000ff-0000-0000-0000-000000000001"},
+      {"x-envoy-original-dst-host", original_dst_address->asString()}};
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers);
+  FakeRawConnectionPtr fake_upstream_connection;
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
+  response->waitForHeaders();
+  ASSERT_EQ("101", response->headers().getStatusValue());
+
+  // RFC 6455 section 5.1 requires every client-to-server frame to be masked.
+  const uint8_t unmasked_frame[] = {0x82, 0x05, 'h', 'e', 'l', 'l', 'o'};
+  Buffer::OwnedImpl frame_buffer(unmasked_frame, sizeof(unmasked_frame));
+  auto* client_connection = codec_client_->connection();
+  client_connection->write(frame_buffer, false);
+  client_connection->dispatcher().run(Event::Dispatcher::RunType::NonBlock);
+
+  test_server_->waitForCounterGe("websocket.protocol_error", 1);
+  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
+  ASSERT_TRUE(codec_client_->waitForDisconnect());
+}
+
 TEST_P(CiliumWebSocketIntegrationTest, CloseResponseWaitsForReverseFin) {
   enableHalfClose(true);
   initialize();
@@ -434,4 +467,37 @@ TEST_P(CiliumWebSocketIntegrationTest, CloseResponseWaitsForReverseFin) {
   codec_client_->close();
 }
 
+TEST_P(CiliumWebSocketIntegrationTest, InvalidCloseAbortsImmediately) {
+  initialize();
+  auto request_headers = Http::TestRequestHeaderMapImpl{
+      {":method", "GET"},
+      {":path", "/"},
+      {":authority", "host"},
+      {"Upgrade", "websocket"},
+      {"Connection", "Upgrade"},
+      {"Origin", "jarno.cilium.rocks"},
+      {"Sec-WebSocket-Key", "dGhlIHNhbXBsZSBub25jZQ=="},
+      {"Sec-WebSocket-Version", "13"},
+      {"x-request-id", "000000ff-0000-0000-0000-000000000001"},
+      {"x-envoy-original-dst-host", original_dst_address->asString()}};
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers);
+  FakeRawConnectionPtr fake_upstream_connection;
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
+  response->waitForHeaders();
+  ASSERT_EQ("101", response->headers().getStatusValue());
+
+  // RFC 6455 forbids a CLOSE payload of exactly one byte. A protocol error aborts both TCP sides
+  // instead of waiting for the upstream FIN used by the normal tunnel half-close path.
+  const uint8_t invalid_masked_close[] = {0x88, 0x81, 0, 0, 0, 0, 0};
+  Buffer::OwnedImpl close_buffer(invalid_masked_close, sizeof(invalid_masked_close));
+  auto* client_connection = codec_client_->connection();
+  client_connection->write(close_buffer, false);
+  client_connection->dispatcher().run(Event::Dispatcher::RunType::NonBlock);
+
+  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
+  ASSERT_TRUE(codec_client_->waitForDisconnect());
+}
+
 } // namespace Envoy

tests/cilium_websocket_encap_integration_test.cc

@@ -432,6 +432,28 @@ TEST_P(CiliumWebSocketIntegrationTest, ControlFramesAfterReceivingClose) {
   ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
 }
 
+TEST_P(CiliumWebSocketIntegrationTest, WebSocketTransportFinWithoutCloseAborts) {
+  initialize();
+  IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy"));
+  FakeRawConnectionPtr fake_upstream_connection;
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
+
+  std::string expected_handshake =
+      fmt::format(fmt::runtime(EXPECTED_HANDSHAKE_FMT), original_dst_address->asString());
+  ASSERT_TRUE(fake_upstream_connection->waitForData(expected_handshake.length()));
+
+  std::string handshake_response =
+      fmt::format(fmt::runtime(HANDSHAKE_RESPONSE_FMT), "GjgmQ9MzNsn3h7+vuIzY25rbQ9M=");
+  ASSERT_TRUE(fake_upstream_connection->write(handshake_response));
+
+  // An outer transport FIN without a WebSocket CLOSE is an abort, not a tunneled TCP FIN.
+  ASSERT_TRUE(fake_upstream_connection->write("", true));
+  // The raw downstream observes EOF under half-close even though the WebSocket transport was
+  // aborted immediately.
+  tcp_client->waitForHalfClose();
+  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
+  tcp_client->close();
+}
 
 // Test proxying data in both directions, and that all data is flushed properly
 // when the client disconnects.