Security #7183
closedTask #5682: tracking: smb performance issues
Optimization #5672: smb: avoid unbounded hash maps
smb: hashmap entries not removed for error responses
Description
SMB tracks multiple hashmaps:
ssnguid2vec_map - only used by dcerpc frag reassembly
guid2name_map - mapping guid to name
TODO: never freed, insert/get only. Except at EOS
ssn2vec_map - mapping ssn to guid/fid?
smb2: added by SMB2_COMMAND_CREATE command, removed by success response
smb2: added by SMB2_COMMAND_WRITE, removed by successful SMB2_COMMAND_WRITE (by response)
smb1: added by SMB1_COMMAND_NT_CREATE_ANDX, removed by success response
smb1: added by SMB1_COMMAND_CLOSE, removed by success response
smb1: added by SMB1_COMMAND_TRANS, removed by success response
TODO: freeing depends on success responses (or EOS)
ssn2vecoffset_map - store fid+offset for session
smb1: SMB1_COMMAND_READ_ANDX insert, removed on success response (but error case missing?)
smb2: SMB2_COMMAND_READ insert, only removed on SMB_NTSTATUS_END_OF_FILE|SMB_NTSTATUS_SUCCESS|SMB_NTSTATUS_BUFFER_OVERFLOW, except with errors
ssn2tree_map - stores tree name by tree key
smb1: SMB1_COMMAND_TREE_DISCONNECT removes, request/response
smb1: SMB1_COMMAND_TREE_CONNECT_ANDX inserts
smb2: SMB2_COMMAND_READ response for dcerpc can add ("fake tree for dcerpc")
smb2: SMB2_COMMAND_WRITE request for dcerpc can add ("fake tree for dcerpc")
smb2: SMB2_COMMAND_TREE_DISCONNECT request/response removes
smb2: SMB2_COMMAND_TREE_CONNECT success response inserts
Additionally, if we don't see a response (e.g. GAP), won't remove either.
Updated by Philippe Antoine 8 months ago
- Status changed from New to Closed
- Target version changed from TBD to 8.0.0-beta1
Fixed by 23f2317c6a1fbd3e2cbd6b41f2116f824019d5f5 85987aaad658d28d3d99b4da3fa9d3fb7c6449d5 right ?
Updated by Victor Julien 8 months ago
I think this a related issue, but a bit different. Several error paths will not clean up the hash map. So they will grow to the limit.
IIRC what I was thinking about is:
request: adds something to hashmap, expecting response to remove it
response: some error, leaves data in hashmap
Now that maps are bounded it is less of an issue, but it's still something I'd like to look into more.
Updated by Victor Julien 8 months ago
I think errors here are more about protocol reporting errors than parsing errors. More a "access denied" style thing. Most logic only checks for success currently, and does no clean up on receiving an error.