fortra / impacket

Impacket is a collection of Python classes for working with network protocols.
https://www.coresecurity.com
Other
13.59k stars 3.59k forks source link

smbserver clears Session Id on LogOff Response breaking signature verification #1829

Open jborean93 opened 1 month ago

jborean93 commented 1 month ago

Configuration

impacket version: 0.12.0 (All affected) Python version: Any Target OS: Any

Debug Output With Command String

There is no debug command to show this change. When an SMB2 LogOff Request is sent the smbserver.py implementation clears out the SessionId value causing the SMB2 Header to have 0 as the SessionId field.

https://github.com/fortra/impacket/blob/65b774ded17a79f1041397202852eab0c24cd039/impacket/smbserver.py#L3818

You can see the packet capture showing that the Session Id value is set to 0 on this message.

image

While some clients might be able to just find the request the response is for and use the request's session id as the session lookup some clients may just use the value in the response header which will fail. The response from the server should continue to reflect the correct UID/SessionId from the request allowing the client to process the response and validate the signature on the response.

PCAP

If applicable, add a packet capture to help explain your problem.

Additional context

Space for additional context, investigative results, suspected issue.

jborean93 commented 1 month ago

I actually think Impacket may still be compliant with the spec and the logic in my client is incorrect. MS-SMB2 states https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/36172e53-ac81-48fb-b2e3-caa3761b9157

For SMB2 SESSION_SETUP, the client MUST retrieve SessionId from SMB2 header of the response. For all other messages, the client MUST retrieve SessionId from the corresponding Request.Message. The client MUST look up the session in the Connection.SessionTable using the SessionId.

So I need to update my logic to retrieve the session id from the associated request. It still would be nice to keep them aligned but I can understand if you don't wish to update the logic.