rapid7 / ruby_smb

A native Ruby implementation of the SMB Protocol Family
Other
81 stars 83 forks source link

Fix Session Signing For SMB 3.1.1 #157

Closed zeroSteiner closed 4 years ago

zeroSteiner commented 4 years ago

I noticed a bug that was causing the SMB::AlwaysEncrypt setting in Metasploit to not be honored. It turns out that with the dialect is SMB 3.1.1 and the encryption algorithms are passed as part of the negotiation capabilities, the flag is not set. This is at least the behavior I observed while testing Windows 10 1809 x64. Per MS-SMB2 the SMB2_GLOBAL_CAP_ENCRYPTION is only valid for the 3.0 and 3.0.2 dialects. This caused session encryption to always be disabled for 3.1.1 servers, there by ignoring the SMB::AlwaysEncrypt setting.

My proposed solution for this is to move the @session_encrypt_data assignment until after the SMB3 capabilities have been set (and by extension the @encryption_algorithm attribute). We can then check if this attribute is not nil to identify that the server supports encryption and we can enable session encryption if it was requested by the user.

While investigating this issue I also noticed that the parse_smb3_encryption_data function was parsing and processing the encryption, compression and preauth integrity capabilities so I renamed it to more accurately represent what it does.

Testing

For testing:

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.03%) to 97.634% when pulling 5dbb050f6991c06d1a2a06cac96e57074f117192 on zeroSteiner:fix/session-signing into 1fca4e42fa757971e3f69961fa564e925767ba7b on rapid7:master.

zeroSteiner commented 4 years ago

image

Yeah so the change I added broke the unit tests, so I did quite a bit of refactoring work in 38a0c1f to fix them up and get them back running smoothly.

One major thing I did was move the attribute assignments into parse_negotiate_response. This mean that parse_smb3_capabilities had to be moved into there and because the request wasn't available, that was moved higher up into negotiate which has access to both the request and response. This then meant I had a bunch of reworking to do in the specs. I hope this looks good, I wouldn't be opposed to an alternative approach.

cdelafuente-r7 commented 4 years ago

Oh! I added this line as an improvement and turned out I introduced a bug by misreading the specs. Thanks for catching this!

zeroSteiner commented 4 years ago

Thanks for the review @cdelafuente-r7! I updated the specs per your suggestions.

cdelafuente-r7 commented 4 years ago

Thanks @zeroSteiner! It is merged now.