From 5caab1a6673aa8737184104c2289803a7a8ad0ba Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Fri, 14 Apr 2023 12:12:28 +0200 Subject: [PATCH] smb2: Limit per-connection read/ioctl/tree state Users on Slack observed memory growth in an environment with a lot of SMB traffic. jeprof memory profiling pointed at the offset and fid maps kept per-connection for smb2 read requests. These maps can grow unbounded if responses are seen before requests, there's packet drops, just one side of the connection is visible, or we fail to parse responses properly. Forcefully wipe out these maps when they grow too large and raise smb2_discarded_messages_state() to notify script land about this. --- scripts/base/init-bare.zeek | 10 ++++++++++ src/analyzer/protocol/smb/consts.bif | 3 ++- src/analyzer/protocol/smb/smb2-com-ioctl.pac | 11 +++++++++++ src/analyzer/protocol/smb/smb2-com-read.pac | 11 +++++++++++ src/analyzer/protocol/smb/smb2-protocol.pac | 10 ++++++++++ src/analyzer/protocol/smb/smb2_events.bif | 13 +++++++++++++ 6 files changed, 57 insertions(+), 1 deletion(-) diff --git a/scripts/base/init-bare.zeek b/scripts/base/init-bare.zeek index 2723c8bc2d..7220c538f9 100644 --- a/scripts/base/init-bare.zeek +++ b/scripts/base/init-bare.zeek @@ -2988,6 +2988,16 @@ export { ## ## .. zeek:see:: smb_pipe_connect_heuristic const SMB::pipe_filenames: set[string] &redef; + + ## The maximum number of messages for which to retain state + ## about offsets, fids, or tree ids within the parser. When + ## the limit is reached, internal parser state is discarded + ## and :zeek:see:`smb2_discarded_messages_state` raised. + ## + ## Setting this to zero will disable the functionality. + ## + ## .. zeek:see:: smb2_discarded_messages_state + const SMB::max_pending_messages = 1000 &redef; } module SMB1; diff --git a/src/analyzer/protocol/smb/consts.bif b/src/analyzer/protocol/smb/consts.bif index 321875b43d..6acd464f6a 100644 --- a/src/analyzer/protocol/smb/consts.bif +++ b/src/analyzer/protocol/smb/consts.bif @@ -1 +1,2 @@ -const SMB::pipe_filenames: string_set; \ No newline at end of file +const SMB::pipe_filenames: string_set; +const SMB::max_pending_messages: count; diff --git a/src/analyzer/protocol/smb/smb2-com-ioctl.pac b/src/analyzer/protocol/smb/smb2-com-ioctl.pac index 8d65312f9d..d37320ae68 100644 --- a/src/analyzer/protocol/smb/smb2-com-ioctl.pac +++ b/src/analyzer/protocol/smb/smb2-com-ioctl.pac @@ -17,6 +17,17 @@ refine connection SMB_Conn += { function proc_smb2_ioctl_request(val: SMB2_ioctl_request) : bool %{ + if ( zeek::BifConst::SMB::max_pending_messages > 0 && + smb2_ioctl_fids.size() >= zeek::BifConst::SMB::max_pending_messages ) + { + if ( smb2_discarded_messages_state ) + zeek::BifEvent::enqueue_smb2_discarded_messages_state(zeek_analyzer(), zeek_analyzer()->Conn(), + zeek::make_intrusive("ioctl")); + + + smb2_ioctl_fids.clear(); + } + smb2_ioctl_fids[${val.header.message_id}] = ${val.file_id.persistent} + ${val.file_id._volatile}; return true; %} diff --git a/src/analyzer/protocol/smb/smb2-com-read.pac b/src/analyzer/protocol/smb/smb2-com-read.pac index 04679d804f..d9b2d7cf7f 100644 --- a/src/analyzer/protocol/smb/smb2-com-read.pac +++ b/src/analyzer/protocol/smb/smb2-com-read.pac @@ -34,6 +34,17 @@ refine connection SMB_Conn += { ${val.read_len}); } + if ( zeek::BifConst::SMB::max_pending_messages > 0 && + (smb2_read_offsets.size() >= zeek::BifConst::SMB::max_pending_messages || + smb2_read_fids.size() >= zeek::BifConst::SMB::max_pending_messages) ) + { + if ( smb2_discarded_messages_state ) + zeek::BifEvent::enqueue_smb2_discarded_messages_state(zeek_analyzer(), zeek_analyzer()->Conn(), + zeek::make_intrusive("read")); + smb2_read_offsets.clear(); + smb2_read_fids.clear(); + } + smb2_read_offsets[${h.message_id}] = ${val.offset}; smb2_read_fids[${h.message_id}] = ${val.file_id.persistent} + ${val.file_id._volatile}; diff --git a/src/analyzer/protocol/smb/smb2-protocol.pac b/src/analyzer/protocol/smb/smb2-protocol.pac index 3c354d3216..f8126ba3bf 100644 --- a/src/analyzer/protocol/smb/smb2-protocol.pac +++ b/src/analyzer/protocol/smb/smb2-protocol.pac @@ -230,6 +230,16 @@ refine connection SMB_Conn += { %{ if ( is_orig ) { + if ( zeek::BifConst::SMB::max_pending_messages > 0 && + smb2_request_tree_id.size() >= zeek::BifConst::SMB::max_pending_messages ) + { + if ( smb2_discarded_messages_state ) + zeek::BifEvent::enqueue_smb2_discarded_messages_state(zeek_analyzer(), zeek_analyzer()->Conn(), + zeek::make_intrusive("tree")); + + smb2_request_tree_id.clear(); + } + // Store the tree_id smb2_request_tree_id[${h.message_id}] = ${h.tree_id}; } diff --git a/src/analyzer/protocol/smb/smb2_events.bif b/src/analyzer/protocol/smb/smb2_events.bif index 2071a0600e..9ef661df77 100644 --- a/src/analyzer/protocol/smb/smb2_events.bif +++ b/src/analyzer/protocol/smb/smb2_events.bif @@ -15,3 +15,16 @@ ## ## .. zeek:see:: smb1_message event smb2_message%(c: connection, hdr: SMB2::Header, is_orig: bool%); + +## Generated for :abbr:`SMB (Server Message Block)`/:abbr:`CIFS (Common Internet File System)` +## version 2 connections for which pending read, ioctl or tree requests exceeds +## the :zeek:see:`SMB::max_pending_messages` setting. This event indicates either +## traffic loss, traffic load-balancing issues, or failures to parse or match +## SMB responses with SMB requests. When this event is raised, internal per-connection +## parser state has been reset. +## +## c: The affected connection. +## +## state: String describing what kind of state was affected. +## One of read, ioctl or tree. +event smb2_discarded_messages_state%(c: connection, state: string%);