Skip to content

Commit

Permalink
websocket: Address review feedback for BinPac code
Browse files Browse the repository at this point in the history 
* Rename mask_ to masking_key_
* Fold FrameHeaderFixed into FrameHeader directly
* Drop WebSocket_FramePayloadUnmask type

Thanks a bunch @ckreibich!
  • Loading branch information
@awelzel
awelzel committed Jan 22, 2024
1 parent 1775b01 commit 015a7c5
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 81 deletions.
41 changes: 13 additions & 28 deletions src/analyzer/protocol/websocket/websocket-analyzer.pac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Expand Down Expand Up @@ -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 )
{
Expand All @@ -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());
Expand All @@ -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);
};
77 changes: 35 additions & 42 deletions src/analyzer/protocol/websocket/websocket-protocol.pac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
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;

# 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=b.rest_header_len;
} &length=2+rest_header_len;

type WebSocket_FramePayloadClose(hdr: WebSocket_FrameHeader) = record {
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
Expand All @@ -73,32 +66,32 @@ 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) {
flowunit = WebSocket_Message withcontext(connection, this);

%member{
bool has_mask_;
uint64_t mask_idx_;
uint64_t masking_key_idx_;
uint64_t frame_payload_len_;
std::array<uint8_t, 4> mask_;
std::array<uint8_t, 4> masking_key_;
%}

%init{
has_mask_ = false;
mask_idx_ = 0;
masking_key_idx_ = 0;
frame_payload_len_ = 0;
%}

Expand All @@ -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<int>(mask_.size()));
memcpy(mask_.data(), ${hdr.mask}.data(), mask_.size());
assert(${hdr.masking_key}.length() == static_cast<int>(masking_key_.size()));
memcpy(masking_key_.data(), ${hdr.masking_key}.data(), masking_key_.size());
}
return frame_payload_len_;
%}
Expand All @@ -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");
Expand Down
4 changes: 2 additions & 2 deletions testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-coalesced
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
4 changes: 2 additions & 2 deletions testing/btest/Baseline/scripts.base.protocols.websocket.coalesced-reply-ping/out-separate
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
14 changes: 7 additions & 7 deletions testing/btest/Baseline/scripts.base.protocols.websocket.events/out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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=<uninitialized>, client_protocols=<uninitialized>, server_extensions=<uninitialized>, client_extensions=[permessage-deflate; client_max_window_bits], client_key=E58pVwft35HPkD/MFCjtEA==, server_accept=HxOmr1a2nvOOc4Qiv7Ou3wrCsJc=]
Expand Down Expand Up @@ -86,21 +86,21 @@ 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=<uninitialized>, client_extensions=[permessage-deflate; client_max_window_bits], client_key=iTel1Ova5Nhz/G7VlI2qKg==, server_accept=YsQYYLj7ZCpzTLsVLb+w/ydy79E=]
websocket_frame, CHhAvVGS1DHFjwGM9, F, fin, T, rsv, 0, opcode, ping, payload_len, 4
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=<uninitialized>, client_extensions=[permessage-deflate; client_max_window_bits], client_key=cQGA5Z1nvyUJ9XOVIaLaQA==, server_accept=zWaHVUKxEGPDs+xJeKtzkE1bm54=]
Expand All @@ -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

0 comments on commit 015a7c5

Please sign in to comment.