Project

General

Profile

Actions
Copy link

Security #7183

closed
VJ OD

Task #5682: tracking: smb performance issues

Optimization #5672: smb: avoid unbounded hash maps

smb: hashmap entries not removed for error responses

Security #7183: smb: hashmap entries not removed for error responses

Added by Victor Julien over 1 year ago. Updated 9 months ago.

Status:
Closed
Priority:
Normal
Assignee:
OISF Dev
Target version:
8.0.0-beta1
Affected Versions:
Label:
CVE:
Git IDs:
Severity:
MODERATE
Disclosure Date:

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.

PA Updated by Philippe Antoine over 1 year ago Actions
Copy link
 #1

  • Status changed from New to Closed
  • Target version changed from TBD to 8.0.0-beta1

Fixed by 23f2317c6a1fbd3e2cbd6b41f2116f824019d5f5 85987aaad658d28d3d99b4da3fa9d3fb7c6449d5 right ?

VJ Updated by Victor Julien over 1 year ago Actions
Copy link
 #2

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.

VJ Updated by Victor Julien over 1 year ago Actions
Copy link
 #3

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.

JI Updated by Jason Ish 9 months ago Actions
Copy link
 #4

  • Private changed from Yes to No
Actions
Copy link

Also available in: PDF Atom