Signature check for LOGOFF response

Signature check for LOGOFF response

[Enzo Matsumiya]

Hi,

The LOGOFF command response is not signed (=> signature is 0x0), but we
check it anyway, displaying "sign fail" errors in ring buffer.

As far as I checked, an explicit LOGOUT is only sent when tlink pruning
happens (i.e. TLINK_IDLE_EXPIRE expires), but we have a case of this
causing issues on production env.

I didn't find LOGOFF being a signature check exception in MS-SMB2 rev64.
Relevant sections:

2.2.7 SMB2 LOGOFF Request
2.2.8 SMB2 LOGOFF Response
3.2.5.4 Receiving an SMB2 LOGOFF Response
3.3.5.6 Receiving an SMB2 LOGOFF Request

If this is implementation defined, maybe something like this could work?
(100% untested)

--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -667,6 +667,7 @@ smb2_verify_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server)
         if ((shdr->Command == SMB2_NEGOTIATE) ||
             (shdr->Command == SMB2_SESSION_SETUP) ||
             (shdr->Command == SMB2_OPLOCK_BREAK) ||
+           (shdr->Command == SMB2_LOGOFF) ||
             server->ignore_signature ||
             (!server->session_estab))
                 return 0;

Thoughts?


Enzo

Re: Signature check for LOGOFF response

[Tom Talpey]

On 3/18/2022 11:20 PM, Enzo Matsumiya wrote:
> Hi,
> 
> The LOGOFF command response is not signed (=> signature is 0x0), but we
> check it anyway, displaying "sign fail" errors in ring buffer.

What server is returning this unsigned response? Assuming it's Windows,
that is either a doc bug or (arguably) a server bug, and should be
reported before deciding how to address it here.

Tom.

> As far as I checked, an explicit LOGOUT is only sent when tlink pruning
> happens (i.e. TLINK_IDLE_EXPIRE expires), but we have a case of this
> causing issues on production env.
> 
> I didn't find LOGOFF being a signature check exception in MS-SMB2 rev64.
> Relevant sections:
> 
> 2.2.7 SMB2 LOGOFF Request
> 2.2.8 SMB2 LOGOFF Response
> 3.2.5.4 Receiving an SMB2 LOGOFF Response
> 3.3.5.6 Receiving an SMB2 LOGOFF Request
> 
> If this is implementation defined, maybe something like this could work?
> (100% untested)
> 
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -667,6 +667,7 @@ smb2_verify_signature(struct smb_rqst *rqst, struct 
> TCP_Server_Info *server)
>          if ((shdr->Command == SMB2_NEGOTIATE) ||
>              (shdr->Command == SMB2_SESSION_SETUP) ||
>              (shdr->Command == SMB2_OPLOCK_BREAK) ||
> +           (shdr->Command == SMB2_LOGOFF) ||
>              server->ignore_signature ||
>              (!server->session_estab))
>                  return 0;
> 
> Thoughts?
> 
> 
> Enzo
> 

Re: Signature check for LOGOFF response

[Enzo Matsumiya]

Hi Tom,

On 03/19, Tom Talpey wrote:
>What server is returning this unsigned response? Assuming it's Windows,
>that is either a doc bug or (arguably) a server bug, and should be
>reported before deciding how to address it here.

It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong
signatures on READ responses with STATUS_END_OF_FILE.

Our tests against Windows Server 2019 showed it to behave ok, so it
looks like something on NetApp side.

Thanks anyway.


Enzo

Re: Signature check for LOGOFF response

[Tom Talpey]

On 3/23/2022 1:29 PM, Enzo Matsumiya wrote:
> Hi Tom,
> 
> On 03/19, Tom Talpey wrote:
>> What server is returning this unsigned response? Assuming it's Windows,
>> that is either a doc bug or (arguably) a server bug, and should be
>> reported before deciding how to address it here.
> 
> It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong
> signatures on READ responses with STATUS_END_OF_FILE.
> 
> Our tests against Windows Server 2019 showed it to behave ok, so it
> looks like something on NetApp side.

In this case I don't think it is appropriate to apply the suggested
patch. Allowing unsigned or invalidly signed responses will greatly
reduce security. I'll be interested if NetApp provides any information.

Tom.

Re: Signature check for LOGOFF response

[Jeremy Allison]

On Thu, Mar 24, 2022 at 11:04:30AM -0400, Tom Talpey wrote:
>On 3/23/2022 1:29 PM, Enzo Matsumiya wrote:
>>Hi Tom,
>>
>>On 03/19, Tom Talpey wrote:
>>>What server is returning this unsigned response? Assuming it's Windows,
>>>that is either a doc bug or (arguably) a server bug, and should be
>>>reported before deciding how to address it here.
>>
>>It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong
>>signatures on READ responses with STATUS_END_OF_FILE.
>>
>>Our tests against Windows Server 2019 showed it to behave ok, so it
>>looks like something on NetApp side.
>
>In this case I don't think it is appropriate to apply the suggested
>patch. Allowing unsigned or invalidly signed responses will greatly
>reduce security. I'll be interested if NetApp provides any information.

Welcome to our world :-). Doing:

git log|grep -i NetApp|wc -l

shows 32 instances (some are commit messages with NetApp in
them two or more times so the number is probably a little
smaller than 32) of commits in Samba especially to
deal with NetApp bugs :-).

That's a lot of client bugfixes :-).

Re: Signature check for LOGOFF response

[Tom Talpey]

On 3/24/2022 12:23 PM, Jeremy Allison wrote:
> On Thu, Mar 24, 2022 at 11:04:30AM -0400, Tom Talpey wrote:
>> On 3/23/2022 1:29 PM, Enzo Matsumiya wrote:
>>> Hi Tom,
>>>
>>> On 03/19, Tom Talpey wrote:
>>>> What server is returning this unsigned response? Assuming it's Windows,
>>>> that is either a doc bug or (arguably) a server bug, and should be
>>>> reported before deciding how to address it here.
>>>
>>> It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong
>>> signatures on READ responses with STATUS_END_OF_FILE.
>>>
>>> Our tests against Windows Server 2019 showed it to behave ok, so it
>>> looks like something on NetApp side.
>>
>> In this case I don't think it is appropriate to apply the suggested
>> patch. Allowing unsigned or invalidly signed responses will greatly
>> reduce security. I'll be interested if NetApp provides any information.
> 
> Welcome to our world :-). Doing:
> 
> git log|grep -i NetApp|wc -l
> 
> shows 32 instances (some are commit messages with NetApp in
> them two or more times so the number is probably a little
> smaller than 32) of commits in Samba especially to
> deal with NetApp bugs :-).
> 
> That's a lot of client bugfixes :-).

Well, it could be argued that this is prolonging the bad behavior.
The NFS client maintainer's approach is the opposite - if the server
is violating the protocol, he holds the line and will not change
the client. I know, I know, it's all in how each person sees it. :)

Tom.

Re: Signature check for LOGOFF response

[Jeremy Allison]

On Thu, Mar 24, 2022 at 02:48:22PM -0400, Tom Talpey wrote:
>On 3/24/2022 12:23 PM, Jeremy Allison wrote:
>>On Thu, Mar 24, 2022 at 11:04:30AM -0400, Tom Talpey wrote:
>>>On 3/23/2022 1:29 PM, Enzo Matsumiya wrote:
>>>>Hi Tom,
>>>>
>>>>On 03/19, Tom Talpey wrote:
>>>>>What server is returning this unsigned response? Assuming it's Windows,
>>>>>that is either a doc bug or (arguably) a server bug, and should be
>>>>>reported before deciding how to address it here.
>>>>
>>>>It's a NetApp ONTAP 9.5P13. We've identified it's also setting wrong
>>>>signatures on READ responses with STATUS_END_OF_FILE.
>>>>
>>>>Our tests against Windows Server 2019 showed it to behave ok, so it
>>>>looks like something on NetApp side.
>>>
>>>In this case I don't think it is appropriate to apply the suggested
>>>patch. Allowing unsigned or invalidly signed responses will greatly
>>>reduce security. I'll be interested if NetApp provides any information.
>>
>>Welcome to our world :-). Doing:
>>
>>git log|grep -i NetApp|wc -l
>>
>>shows 32 instances (some are commit messages with NetApp in
>>them two or more times so the number is probably a little
>>smaller than 32) of commits in Samba especially to
>>deal with NetApp bugs :-).
>>
>>That's a lot of client bugfixes :-).
>
>Well, it could be argued that this is prolonging the bad behavior.
>The NFS client maintainer's approach is the opposite - if the server
>is violating the protocol, he holds the line and will not change
>the client. I know, I know, it's all in how each person sees it. :)

It's all a question of leverage. A Microsoft behavior defines
the protocol, even if it's an inadvertant bug.

For NetApp, people using cifsfs, libsmbclient or our client tools just
want the damn thing to work. NetApp only (as far as I know)
test against a Microsoft client, so we have zero leverage here.

Sucks, but as I said, "Welcome to our world" :-).