From 015a7c5fbcb1efcc9904f855798d41afec12d0ed Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 22 Jan 2024 15:01:35 +0100 Subject: [PATCH] websocket: Address review feedback for BinPac code * Rename mask_ to masking_key_ * Fold FrameHeaderFixed into FrameHeader directly * Drop WebSocket_FramePayloadUnmask type Thanks a bunch @ckreibich! --- .../protocol/websocket/websocket-analyzer.pac | 41 +++------- .../protocol/websocket/websocket-protocol.pac | 79 +++++++++---------- .../out-coalesced | 4 +- .../out-separate | 4 +- .../out | 14 ++-- 5 files changed, 60 insertions(+), 82 deletions(-) diff --git a/src/analyzer/protocol/websocket/websocket-analyzer.pac b/src/analyzer/protocol/websocket/websocket-analyzer.pac index c57dd9a651..fc60538161 100644 --- a/src/analyzer/protocol/websocket/websocket-analyzer.pac +++ b/src/analyzer/protocol/websocket/websocket-analyzer.pac @@ -27,30 +27,15 @@ refine flow WebSocket_Flow += { zeek::BifEvent::enqueue_websocket_frame(connection()->zeek_analyzer(), connection()->zeek_analyzer()->Conn(), is_orig(), - ${hdr.b.fin}, - ${hdr.b.reserved}, - ${hdr.b.opcode}, + ${hdr.fin}, + ${hdr.reserved}, + ${hdr.opcode}, ${hdr.payload_len}); } return true; %} - function process_payload_unmask(chunk: WebSocket_FramePayloadUnmask): bool - %{ - auto& data = ${chunk.data}; - - // In-place unmasking if frame payload is masked. - if ( has_mask_ ) - { - auto *d = data.data(); - for ( int i = 0; i < data.length(); i++ ) - d[i] = d[i] ^ mask_[mask_idx_++ % mask_.size()]; - } - - return true; - %} - function process_payload_close(close: WebSocket_FramePayloadClose): bool %{ if ( websocket_close ) @@ -84,7 +69,15 @@ refine flow WebSocket_Flow += { function process_payload_chunk(chunk: WebSocket_FramePayloadChunk): bool %{ - auto& data = ${chunk.unmask.data}; + auto& data = ${chunk.data}; + + // In-place unmasking if frame payload is masked. + if ( has_mask_ ) + { + auto *d = data.data(); + for ( int i = 0; i < data.length(); i++ ) + d[i] = d[i] ^ masking_key_[masking_key_idx_++ % masking_key_.size()]; + } if ( websocket_frame_data ) { @@ -96,7 +89,7 @@ refine flow WebSocket_Flow += { } // Forward text and binary data to downstream analyzers. - if ( ${chunk.hdr.b.opcode} == OPCODE_TEXT|| ${chunk.hdr.b.opcode} == OPCODE_BINARY) + if ( ${chunk.hdr.opcode} == OPCODE_TEXT|| ${chunk.hdr.opcode} == OPCODE_BINARY) connection()->zeek_analyzer()->ForwardStream(data.length(), data.data(), is_orig()); @@ -113,14 +106,6 @@ refine typeattr WebSocket_FrameHeader += &let { proc_header = $context.flow.process_header(this); }; -refine typeattr WebSocket_FramePayloadUnmask += &let { - proc_payload_unmask = $context.flow.process_payload_unmask(this); -}; - refine typeattr WebSocket_FramePayloadClose += &let { proc_payload_close = $context.flow.process_payload_close(this); }; - -refine typeattr WebSocket_FramePayloadChunk += &let { - proc_payload_chunk = $context.flow.process_payload_chunk(this); -}; diff --git a/src/analyzer/protocol/websocket/websocket-protocol.pac b/src/analyzer/protocol/websocket/websocket-protocol.pac index 93cd4eb94a..1abd317859 100644 --- a/src/analyzer/protocol/websocket/websocket-protocol.pac +++ b/src/analyzer/protocol/websocket/websocket-protocol.pac @@ -13,54 +13,47 @@ enum Opcodes { OPCODE_PONG = 0x0a, } -type WebSocket_FrameHeaderFixed(first_frame: bool) = record { - # First frame in message cannot be continuation, following - # frames are only expected to be continuations. - b: uint16 &enforce((first_frame && opcode != 0) || (!first_frame && opcode == 0)); -} &let { - fin: bool = (b & 0x8000) ? true : false; - reserved: uint8 = ((b & 0x7000) >> 12); - opcode: uint8 = (b & 0x0f00) >> 8; - has_mask: bool = (b & 0x0080) ? true : false; - payload_len1: uint8 = (b & 0x007f); - rest_header_len: uint64 = (has_mask ? 4 : 0) + (payload_len1 < 126 ? 0 : (payload_len1 == 126 ? 2 : 8)); -} &length=2; - -type WebSocket_FrameHeader(b: WebSocket_FrameHeaderFixed) = record { - maybe_more_len: case b.payload_len1 of { +type WebSocket_FrameHeader(first_frame: bool) = record { + first2: uint16 &enforce((first_frame && opcode != 0) || (!first_frame && opcode == 0)); + maybe_more_len: case payload_len1 of { 126 -> payload_len2: uint16; 127 -> payload_len8: uint64; default -> short_len: empty; }; - maybe_mask: case b.has_mask of { - true -> mask: bytestring &length=4; - false -> no_mask: empty; + maybe_masking_key: case has_mask of { + true -> masking_key: bytestring &length=4; + false -> no_masking_key: empty; }; } &let { - payload_len: uint64 = b.payload_len1 < 126 ? b.payload_len1 : (b.payload_len1 == 126 ? payload_len2 : payload_len8); - new_frame_payload = $context.flow.new_frame_payload(this); -} &length=b.rest_header_len; + fin: bool = (first2 & 0x8000) ? true : false; + reserved: uint8 = ((first2 & 0x7000) >> 12); + opcode: uint8 = (first2 & 0x0f00) >> 8; + payload_len1: uint8 = (first2 & 0x007f); + has_mask: bool = (first2 & 0x0080) ? true : false; -type WebSocket_FramePayloadClose(hdr: WebSocket_FrameHeader) = record { + # Derived fields. + rest_header_len: uint64 = (has_mask ? 4 : 0) + (payload_len1 < 126 ? 0 : (payload_len1 == 126 ? 2 : 8)); + payload_len: uint64 = payload_len1 < 126 ? payload_len1 : (payload_len1 == 126 ? payload_len2 : payload_len8); + + new_frame_payload = $context.flow.new_frame_payload(this); +} &length=2+rest_header_len; + +type WebSocket_FramePayloadClose = record { status: uint16; reason: bytestring &restofdata; } &byteorder=bigendian; -type WebSocket_FramePayloadUnmask(hdr: WebSocket_FrameHeader) = record { - data: bytestring &restofdata; -}; - type WebSocket_FramePayloadChunk(len: uint64, hdr: WebSocket_FrameHeader) = record { - unmask: WebSocket_FramePayloadUnmask(hdr); + data: bytestring &restofdata; } &let { - consumed_payload = $context.flow.consumed_chunk(len); - close_payload: WebSocket_FramePayloadClose(hdr) withinput unmask.data &length=len &if(hdr.b.opcode == OPCODE_CLOSE); + consumed_payload = $context.flow.consumed_chunk_len(len); + payload_chunk = $context.flow.process_payload_chunk(this); # unmasks if needed + close_payload: WebSocket_FramePayloadClose withinput data &length=len &if(hdr.opcode == OPCODE_CLOSE); } &length=len; type WebSocket_Frame(first_frame: bool, msg: WebSocket_Message) = record { - b: WebSocket_FrameHeaderFixed(first_frame); - hdr: WebSocket_FrameHeader(b); + hdr: WebSocket_FrameHeader(first_frame); # This is implementing frame payload chunking so that we do not # attempt to buffer huge frames and forward data to downstream @@ -73,17 +66,17 @@ type WebSocket_Frame(first_frame: bool, msg: WebSocket_Message) = record { } &let { # If we find a close frame without payload, raise the event here # as the close won't have been parsed via chunks. - empty_close = $context.flow.process_empty_close(hdr) &if(b.opcode == OPCODE_CLOSE) && hdr.payload_len == 0; + empty_close = $context.flow.process_empty_close(hdr) &if(hdr.opcode == OPCODE_CLOSE) && hdr.payload_len == 0; }; type WebSocket_Message = record { first_frame: WebSocket_Frame(true, this); - optional_more_frames: case first_frame.hdr.b.fin of { + optional_more_frames: case first_frame.hdr.fin of { true -> no_more_frames: empty; - false -> more_frames: WebSocket_Frame(false, this)[] &until($element.hdr.b.fin); + false -> more_frames: WebSocket_Frame(false, this)[] &until($element.hdr.fin); }; } &let { - opcode = first_frame.hdr.b.opcode; + opcode = first_frame.hdr.opcode; } &byteorder=bigendian; flow WebSocket_Flow(is_orig: bool) { @@ -91,14 +84,14 @@ flow WebSocket_Flow(is_orig: bool) { %member{ bool has_mask_; - uint64_t mask_idx_; + uint64_t masking_key_idx_; uint64_t frame_payload_len_; - std::array mask_; + std::array masking_key_; %} %init{ has_mask_ = false; - mask_idx_ = 0; + masking_key_idx_ = 0; frame_payload_len_ = 0; %} @@ -108,11 +101,11 @@ flow WebSocket_Flow(is_orig: bool) { connection()->zeek_analyzer()->Weird("websocket_frame_not_consumed"); frame_payload_len_ = ${hdr.payload_len}; - has_mask_ = ${hdr.b.has_mask}; - mask_idx_ = 0; + has_mask_ = ${hdr.has_mask}; + masking_key_idx_ = 0; if ( has_mask_ ) { - assert(${hdr.mask}.length() == static_cast(mask_.size())); - memcpy(mask_.data(), ${hdr.mask}.data(), mask_.size()); + assert(${hdr.masking_key}.length() == static_cast(masking_key_.size())); + memcpy(masking_key_.data(), ${hdr.masking_key}.data(), masking_key_.size()); } return frame_payload_len_; %} @@ -122,7 +115,7 @@ flow WebSocket_Flow(is_orig: bool) { return frame_payload_len_; %} - function consumed_chunk(len: uint64): uint64 + function consumed_chunk_len(len: uint64): uint64 %{ if ( len > frame_payload_len_ ) { connection()->zeek_analyzer()->Weird("websocket_frame_consuming_too_much"); diff --git a/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-coalesced b/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-coalesced index 8a28458914..bb559170db 100644 --- a/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-coalesced +++ b/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-coalesced @@ -9,8 +9,8 @@ websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 11, data, Hello Zeek! websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, text, payload_len, 12 websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 12, data, Hello there! websocket_frame, CHhAvVGS1DHFjwGM9, T, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, diff --git a/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-separate b/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-separate index 8a28458914..bb559170db 100644 --- a/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-separate +++ b/testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-separate @@ -9,8 +9,8 @@ websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 11, data, Hello Zeek! websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, text, payload_len, 12 websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 12, data, Hello there! websocket_frame, CHhAvVGS1DHFjwGM9, T, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, diff --git a/testing/btest/Baseline/scripts.base.protocols.websocket.events/out b/testing/btest/Baseline/scripts.base.protocols.websocket.events/out index 41bb69dcbe..af9e938aaf 100644 --- a/testing/btest/Baseline/scripts.base.protocols.websocket.events/out +++ b/testing/btest/Baseline/scripts.base.protocols.websocket.events/out @@ -52,12 +52,12 @@ websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, binary, payload_l websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 409, data, HTTP/1.1 301 Moved Permanently\x0d\x0aServer: nginx\x0d\x0aDate: Fri, 12 Jan 2024 17:15:32 GMT\x0d\x0aContent-Type: text/html\x0d\x0aContent-Len websocket_message, CHhAvVGS1DHFjwGM9, F, opcode, binary websocket_frame, CHhAvVGS1DHFjwGM9, T, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_message, CHhAvVGS1DHFjwGM9, T, opcode, close websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_message, CHhAvVGS1DHFjwGM9, F, opcode, close broker-websocket.pcap websocket_established, CHhAvVGS1DHFjwGM9, 7, [ts=XXXXXXXXXX.XXXXXX, uid=CHhAvVGS1DHFjwGM9, id=[orig_h=127.0.0.1, orig_p=38776/tcp, resp_h=127.0.0.1, resp_p=27599/tcp], host=localhost:27599, uri=/v1/messages/json, user_agent=Python/3.10 websockets/12.0, subprotocol=, client_protocols=, server_extensions=, client_extensions=[permessage-deflate; client_max_window_bits], client_key=E58pVwft35HPkD/MFCjtEA==, server_accept=HxOmr1a2nvOOc4Qiv7Ou3wrCsJc=] @@ -86,8 +86,8 @@ websocket_frame, CHhAvVGS1DHFjwGM9, T, fin, T, rsv, 0, opcode, text, payload_len websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 361, data, {"type": "data-message", "topic": "/zeek/event/my_topic", "@data-type": "vector", "data": [{"@data-type": "count", "data websocket_message, CHhAvVGS1DHFjwGM9, T, opcode, text websocket_frame, CHhAvVGS1DHFjwGM9, T, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_message, CHhAvVGS1DHFjwGM9, T, opcode, close message-too-big-status.pcap websocket_established, CHhAvVGS1DHFjwGM9, 7, [ts=XXXXXXXXXX.XXXXXX, uid=CHhAvVGS1DHFjwGM9, id=[orig_h=127.0.0.1, orig_p=60956/tcp, resp_h=127.0.0.1, resp_p=8080/tcp], host=localhost:8080, uri=/, user_agent=Python/3.10 websockets/12.0, subprotocol=v1, client_protocols=[v1], server_extensions=, client_extensions=[permessage-deflate; client_max_window_bits], client_key=iTel1Ova5Nhz/G7VlI2qKg==, server_accept=YsQYYLj7ZCpzTLsVLb+w/ydy79E=] @@ -95,12 +95,12 @@ websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, ping, payload_len websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 4, data, Zeek websocket_message, CHhAvVGS1DHFjwGM9, F, opcode, ping websocket_frame, CHhAvVGS1DHFjwGM9, T, fin, T, rsv, 0, opcode, close, payload_len, 31 -websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1009, reason, over size limit (4 > 2 bytes) websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 31, data, \x03\xf1over size limit (4 > 2 bytes) +websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1009, reason, over size limit (4 > 2 bytes) websocket_message, CHhAvVGS1DHFjwGM9, T, opcode, close websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_message, CHhAvVGS1DHFjwGM9, F, opcode, close two-binary-fragments.pcap websocket_established, CHhAvVGS1DHFjwGM9, 7, [ts=XXXXXXXXXX.XXXXXX, uid=CHhAvVGS1DHFjwGM9, id=[orig_h=127.0.0.1, orig_p=50198/tcp, resp_h=127.0.0.1, resp_p=8080/tcp], host=localhost:8080, uri=/, user_agent=Python/3.10 websockets/12.0, subprotocol=v1, client_protocols=[v1], server_extensions=, client_extensions=[permessage-deflate; client_max_window_bits], client_key=cQGA5Z1nvyUJ9XOVIaLaQA==, server_accept=zWaHVUKxEGPDs+xJeKtzkE1bm54=] @@ -119,10 +119,10 @@ websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, continuation, pay websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 7, data, there! websocket_message, CHhAvVGS1DHFjwGM9, F, opcode, binary websocket_frame, CHhAvVGS1DHFjwGM9, T, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, T, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, T, status, 1000, reason, websocket_message, CHhAvVGS1DHFjwGM9, T, opcode, close websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, close, payload_len, 2 -websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_frame_data, CHhAvVGS1DHFjwGM9, F, len, 2, data, \x03\xe8 +websocket_close, CHhAvVGS1DHFjwGM9, F, status, 1000, reason, websocket_message, CHhAvVGS1DHFjwGM9, F, opcode, close