11665 SMB2 NEGOTIATE Security Mode handling is wrong

Review Request #2287 - Created Sept. 9, 2019 and updated

Information
Andrew Stormont
illumos-gate
11659, 11665
Reviewers
general
gdamore, gwr

11665 SMB2 NEGOTIATE Security Mode handling is wrong

The Security Mode is used by both the client and server to inform the other side if it requires signing to be enabled. It should not be, at least as far as I can tell, used to deny access to the client during NEGOTIATE.

Using WireShark I can see the Windows Protocol Test Suite will send a NEGOIATE request with a Security Mode of zero. The illumos smb server then responds with NT_STATUS_INVALID_PARAMETER and drops the connection.

With this patch applied the Test Suite will now use the Security Mode from the server's NEGOTIATE response in it's follow-up SESSION SETUP request - which I believe is the correct behaviour.

I have also verified that Samba does not deny service based on the client's Security Mode.

11659 SMB2 protocol version negotiation needs work

No response is emitted when a NEGOTIATE request is received with dialects that are unsupported. This appears to be due to the fact that the response header is not properly initialised. The code also attempts to send NT_STATUS_INVALID_PARAMETER whereas the correct response is NT_STATUS_NOT_SUPPORTED. We also do not properly handle the cases where no dialect is given.

Before this change, running the Windows Protocol Test Suite with only the SMB2/3 Negotiate tests enable yields a result of:

59 / 107 Passed, 41 Failed, 7 Inconclusive

After this fix the result is:

100 / 107 Passed, 0 Failed, 7 Inconclusive

The inconclusive tests are inconclusive because the require SMB 3.1 support.

Issues

  • 0
  • 2
  • 2
  • 4
Description From Last Updated
Andrew Stormont
Garrett D'Amore
Gordon Ross
Andrew Stormont
Gordon Ross
Andrew Stormont
Andrew Stormont
Andrew Stormont
Gordon Ross
Gordon Ross
Andrew Stormont
Andrew Stormont
Andrew Stormont
Gordon Ross
Andrew Stormont
Andrew Stormont
Review request changed

Change Summary:

Take care not to mark the session as NEGOTIATED when smb2_negotiate_common fails.

Description:

-  

11659 SMB2 protocol version negotiation needs work

-  
-  

No response is emitted when a NEGOTIATE request is received with dialects that are unsupported. This appears to be due to the fact that the response header is not properly initialised. The code also attempts to send NT_STATUS_INVALID_PARAMETER whereas the correct response is NT_STATUS_NOT_SUPPORTED. We also do not properly handle the cases where no dialect is given.

-  
   

11665 SMB2 NEGOTIATE Security Mode handling is wrong

   
   

The Security Mode is used by both the client and server to inform the other side if it requires signing to be enabled. It should not be, at least as far as I can tell, used to deny access to the client during NEGOTIATE.

   
   

Using WireShark I can see the Windows Protocol Test Suite will send a NEGOIATE request with a Security Mode of zero. The illumos smb server then responds with NT_STATUS_INVALID_PARAMETER and drops the connection.

   
   

With this patch applied the Test Suite will now use the Security Mode from the server's NEGOTIATE response in it's follow-up SESSION SETUP request - which I believe is the correct behaviour.

   
   

I have also verified that Samba does not deny service based on the client's Security Mode.

   
~  

11670 SMB2_FLAGS_SIGNED is not valid during NEGOTIATE

  ~

11659 SMB2 protocol version negotiation needs work

   
~  

The Test Suite sends a negotiate request with SMB2_FLAGS_SIGNED and expects to get NT_STATUS_INVALID_PARAMETER back but the server actually returns NT_STATUS_SUCCESS.

  ~

No response is emitted when a NEGOTIATE request is received with dialects that are unsupported. This appears to be due to the fact that the response header is not properly initialised. The code also attempts to send NT_STATUS_INVALID_PARAMETER whereas the correct response is NT_STATUS_NOT_SUPPORTED. We also do not properly handle the cases where no dialect is given.

Bugs:

-11670

Diff:

Revision 7 (+65 -50)

Show changes

Loading...