[PATCH] cifs: verify signature only for valid responses

[PATCH] cifs: verify signature only for valid responses

[Enzo Matsumiya]

The signature check will always fail for a response with SMB2
Status == STATUS_END_OF_FILE, so skip the verification of those.

Also, in async IO, it doesn't make sense to verify the signature
of an unsuccessful read (rdata->result != 0), as the data is
probably corrupt/inconsistent/incomplete. Verify only the responses
of successful reads.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 fs/cifs/smb2pdu.c       | 4 ++--
 fs/cifs/smb2transport.c | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 6352ab32c7e7..9ae25ba909f5 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -4144,8 +4144,8 @@ smb2_readv_callback(struct mid_q_entry *mid)
 	case MID_RESPONSE_RECEIVED:
 		credits.value = le16_to_cpu(shdr->CreditRequest);
 		credits.instance = server->reconnect_instance;
-		/* result already set, check signature */
-		if (server->sign && !mid->decrypted) {
+		/* check signature only if read was successful */
+		if (server->sign && !mid->decrypted && rdata->result == 0) {
 			int rc;
 
 			rc = smb2_verify_signature(&rqst, server);
diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
index 1a5fc3314dbf..37c7ed2f1984 100644
--- a/fs/cifs/smb2transport.c
+++ b/fs/cifs/smb2transport.c
@@ -668,6 +668,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->Status == STATUS_END_OF_FILE) ||
 	    server->ignore_signature ||
 	    (!server->session_estab))
 		return 0;
-- 
2.35.3

Re: [PATCH] cifs: verify signature only for valid responses

[Tom Talpey]

On 9/16/2022 10:07 PM, Enzo Matsumiya wrote:
> The signature check will always fail for a response with SMB2
> Status == STATUS_END_OF_FILE, so skip the verification of those.

Can you elaborate on this assertion? I don't see this as a protocol
requirement:

   3.2.5.1.3 Verifying the Signature
     The client MUST skip the processing in this section if any of the
     following is TRUE:
     - Client implements the SMB 3.x dialect family and decryption in
       section 3.2.5.1.1.1 succeeds
     - MessageId is 0xFFFFFFFFFFFFFFFF
     - Status in the SMB2 header is STATUS_PENDING
     [goes on to discuss action if session not found, etc]

> Also, in async IO, it doesn't make sense to verify the signature
> of an unsuccessful read (rdata->result != 0), as the data is
> probably corrupt/inconsistent/incomplete. Verify only the responses
> of successful reads.

Same question. Why would we ever want to selectively skip signing
verification? Signing protects against corrupted SMB headers, MITM,
etc etc.

Tom.

> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
>   fs/cifs/smb2pdu.c       | 4 ++--
>   fs/cifs/smb2transport.c | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 6352ab32c7e7..9ae25ba909f5 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -4144,8 +4144,8 @@ smb2_readv_callback(struct mid_q_entry *mid)
>   	case MID_RESPONSE_RECEIVED:
>   		credits.value = le16_to_cpu(shdr->CreditRequest);
>   		credits.instance = server->reconnect_instance;
> -		/* result already set, check signature */
> -		if (server->sign && !mid->decrypted) {
> +		/* check signature only if read was successful */
> +		if (server->sign && !mid->decrypted && rdata->result == 0) {
>   			int rc;
>   
>   			rc = smb2_verify_signature(&rqst, server);
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index 1a5fc3314dbf..37c7ed2f1984 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -668,6 +668,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->Status == STATUS_END_OF_FILE) ||
>   	    server->ignore_signature ||
>   	    (!server->session_estab))
>   		return 0;

Re: [PATCH] cifs: verify signature only for valid responses

[Enzo Matsumiya]

On 09/17, Tom Talpey wrote:
>On 9/16/2022 10:07 PM, Enzo Matsumiya wrote:
>>The signature check will always fail for a response with SMB2
>>Status == STATUS_END_OF_FILE, so skip the verification of those.
>
>Can you elaborate on this assertion? I don't see this as a protocol
>requirement:
>
>  3.2.5.1.3 Verifying the Signature
>    The client MUST skip the processing in this section if any of the
>    following is TRUE:
>    - Client implements the SMB 3.x dialect family and decryption in
>      section 3.2.5.1.1.1 succeeds
>    - MessageId is 0xFFFFFFFFFFFFFFFF
>    - Status in the SMB2 header is STATUS_PENDING
>    [goes on to discuss action if session not found, etc]

Yeah I didn't find anything in the spec either. I woke up this morning
thinking about this actually, and it might actually be a miscalculation
on our side. My initial assumption, and debugging target now, is the
1-byte cropping done on some odd-sized structs, but I haven't deepened
on that so far.

I'll reply back with my findings later.

>>Also, in async IO, it doesn't make sense to verify the signature
>>of an unsuccessful read (rdata->result != 0), as the data is
>>probably corrupt/inconsistent/incomplete. Verify only the responses
>>of successful reads.
>
>Same question. Why would we ever want to selectively skip signing
>verification? Signing protects against corrupted SMB headers, MITM,
>etc etc.

The problem here is actually different because rdata->result can
contain an internal (kernel) error code when an underlying problem
occurred (think EIO, EINTR, ECONNABORTED (not sure if possible this one),
ENOMEM maybe?). But in between "mid set with MID_RESPONSE_RECEIVED state"
and "verify the signature", the SMB2 header/message itself might be
correct/valid, but our internal processing failed somewhere, so, the
way I see it, computing the signature for such cases adds overhead and
could (*) cover up the original internal error.

(*) This actually brings to another inconsistency I'm currently looking at:

          switch (mid->mid_state) {
          case MID_RESPONSE_RECEIVED:
                  credits.value = le16_to_cpu(shdr->CreditRequest);
                  credits.instance = server->reconnect_instance;
                  /* check signature only if read was successful */
                  if (server->sign && !mid->decrypted && rdata->result == 0) {
   rc is local >>>>       int rc;

                          rc = smb2_verify_signature(&rqst, server);
                          if (rc)
                                  cifs_tcon_dbg(VFS, "SMB signature verification returned error = %d\n",
                                           rc);
   and never acted upon >>>
                  }
                  /* FIXME: should this be counted toward the initiating task? */
                  task_io_account_read(rdata->got_bytes);
                  cifs_stats_bytes_read(tcon, rdata->got_bytes);
                  break;
	}

See, the return value of smb2_verify_signature() is never (aside from
printing the error message) checked, used, or acted upon. So, even if 
server->ignore_signature is false (default), an invalid signature is
never accounted for (regardless if the message is integral or a
miscalculation on our side). Where, again IMO, the correct action would
be to discard the mid and cancel the operation, as the data could be,
intentionally or not, corrupted.

Thoughts?

>Tom.

Cheers,

Enzo

>>Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
>>---
>>  fs/cifs/smb2pdu.c       | 4 ++--
>>  fs/cifs/smb2transport.c | 1 +
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>>diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>>index 6352ab32c7e7..9ae25ba909f5 100644
>>--- a/fs/cifs/smb2pdu.c
>>+++ b/fs/cifs/smb2pdu.c
>>@@ -4144,8 +4144,8 @@ smb2_readv_callback(struct mid_q_entry *mid)
>>  	case MID_RESPONSE_RECEIVED:
>>  		credits.value = le16_to_cpu(shdr->CreditRequest);
>>  		credits.instance = server->reconnect_instance;
>>-		/* result already set, check signature */
>>-		if (server->sign && !mid->decrypted) {
>>+		/* check signature only if read was successful */
>>+		if (server->sign && !mid->decrypted && rdata->result == 0) {
>>  			int rc;
>>  			rc = smb2_verify_signature(&rqst, server);
>>diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
>>index 1a5fc3314dbf..37c7ed2f1984 100644
>>--- a/fs/cifs/smb2transport.c
>>+++ b/fs/cifs/smb2transport.c
>>@@ -668,6 +668,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->Status == STATUS_END_OF_FILE) ||
>>  	    server->ignore_signature ||
>>  	    (!server->session_estab))
>>  		return 0;

Re: [PATCH] cifs: verify signature only for valid responses

[Enzo Matsumiya]

On 09/17, Enzo Matsumiya wrote:
> So, even if 
>server->ignore_signature is false (default),

Erm server->ignore_signature is checked in smb2_verify_signature()
(obviously I was aware) and rc would be 0. I need coffee...

But I stand by the rest of the argument.


Enzo

Re: [PATCH] cifs: verify signature only for valid responses

[Tom Talpey]

On 9/17/2022 12:28 PM, Enzo Matsumiya wrote:
> On 09/17, Tom Talpey wrote:
>> On 9/16/2022 10:07 PM, Enzo Matsumiya wrote:
>>> The signature check will always fail for a response with SMB2
>>> Status == STATUS_END_OF_FILE, so skip the verification of those.
>>
>> Can you elaborate on this assertion? I don't see this as a protocol
>> requirement:
>>
>>  3.2.5.1.3 Verifying the Signature
>>    The client MUST skip the processing in this section if any of the
>>    following is TRUE:
>>    - Client implements the SMB 3.x dialect family and decryption in
>>      section 3.2.5.1.1.1 succeeds
>>    - MessageId is 0xFFFFFFFFFFFFFFFF
>>    - Status in the SMB2 header is STATUS_PENDING
>>    [goes on to discuss action if session not found, etc]
> 
> Yeah I didn't find anything in the spec either. I woke up this morning
> thinking about this actually, and it might actually be a miscalculation
> on our side. My initial assumption, and debugging target now, is the
> 1-byte cropping done on some odd-sized structs, but I haven't deepened
> on that so far.
> 
> I'll reply back with my findings later.

Good, because there are definitely some tricky rules regarding what
parts of the payload are included in the signing. Padding, especially,
is easy to get wrong.

>>> Also, in async IO, it doesn't make sense to verify the signature
>>> of an unsuccessful read (rdata->result != 0), as the data is
>>> probably corrupt/inconsistent/incomplete. Verify only the responses
>>> of successful reads.
>>
>> Same question. Why would we ever want to selectively skip signing
>> verification? Signing protects against corrupted SMB headers, MITM,
>> etc etc.
> 
> The problem here is actually different because rdata->result can
> contain an internal (kernel) error code when an underlying problem
> occurred (think EIO, EINTR, ECONNABORTED (not sure if possible this one),
> ENOMEM maybe?). But in between "mid set with MID_RESPONSE_RECEIVED state"
> and "verify the signature", the SMB2 header/message itself might be
> correct/valid, but our internal processing failed somewhere, so, the

Wait, we process the message *before* we check the signature??? Apart
from inspecting the MID and verifying it's a response to a request we
made, there isn't a lot to cause such an error. See 3.2.5.1.3.

Also, if we process a bogus response, or drop a valid one, that's a
seriously important issue. It's not a server/protocol/network error
but we trashed the operation!

Not quoting the ENOCOFFEE part of the rest of your message. :)

Tom.

Re: [PATCH] cifs: verify signature only for valid responses

[Enzo Matsumiya]

Hi Tom,

On 09/17, Tom Talpey wrote:
<snip>
>Wait, we process the message *before* we check the signature??? Apart
>from inspecting the MID and verifying it's a response to a request we
>made, there isn't a lot to cause such an error. See 3.2.5.1.3.

You're right. By processing I actually meant "parsing" done right after
receive, but even that doesn't have many failure spots.

I found that the mids with STATUS_END_OF_FILE are being discarded,
apparently as per 3.2.5.11:

   If the Status field of the SMB2 header of the response indicates an
   error, the client MUST return the received status code to the calling
   application.

What I found is that mid->callback() (smb2_readv_callback()) was being
called from another thread, so even though the mid had been dequeued by
mid->receive() earlier, smb2_readv_callback() was treating it as a
valid (non-NULL), existing (mid_state == MID_RESPONSE_RECEIVED) mid.

 From this perspective, it makes sense to me to skip the signature
verification when the mid wasn't supposed to be there in the first
place, but if we consider that other messages with status !=
STATUS_SUCCESS have their signatures correctly computed (apparently),
then I'd guess there's something wrong with computing signatures for
STATUS_END_OF_FILE responses.

Sent this just now:
https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@suse.de/T/#u

I'd appreciate your, and Cc'd folks', feedback.


Cheers,

Enzo

Re: [PATCH] cifs: verify signature only for valid responses

[Enzo Matsumiya]

On 09/18, Enzo Matsumiya wrote:
> but if we consider that other messages with status !=
>STATUS_SUCCESS have their signatures correctly computed (apparently),
>then I'd guess there's something wrong with computing signatures for
>STATUS_END_OF_FILE responses.

Correcting myself here: messages with status != STATUS_SUCCESS and !=
STATUS_END_OF_FILE take different path that never reaches ->calc_signature().

>Sent this just now:
>https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@suse.de/T/#u

So I guess this patch is sufficient for now.


Cheers,

Enzo

Re: [PATCH] cifs: verify signature only for valid responses

[Tom Talpey]

On 9/18/2022 8:21 PM, Enzo Matsumiya wrote:
> Hi Tom,
> 
> On 09/17, Tom Talpey wrote:
> <snip>
>> Wait, we process the message *before* we check the signature??? Apart
>> from inspecting the MID and verifying it's a response to a request we
>> made, there isn't a lot to cause such an error. See 3.2.5.1.3.
> 
> You're right. By processing I actually meant "parsing" done right after
> receive, but even that doesn't have many failure spots.
> 
> I found that the mids with STATUS_END_OF_FILE are being discarded,
> apparently as per 3.2.5.11:
> 
>    If the Status field of the SMB2 header of the response indicates an
>    error, the client MUST return the received status code to the calling
>    application.

I don't think it follows that the signature MUST NOT be validated.
That processing is fundamental, and is independent of returning
results. 3.2.5.1.3 has only three exceptions, and STATUS_END_OF_FILE
isn't one of them.

> What I found is that mid->callback() (smb2_readv_callback()) was being
> called from another thread, so even though the mid had been dequeued by
> mid->receive() earlier, smb2_readv_callback() was treating it as a
> valid (non-NULL), existing (mid_state == MID_RESPONSE_RECEIVED) mid.

That certainly sounds like another bug. Why are two threads processing
the MID? Is this an async response?

>  From this perspective, it makes sense to me to skip the signature
> verification when the mid wasn't supposed to be there in the first

That's not what is documented. Only if the MID is 0xFFFFFFFFFFFFFFFF.

> place, but if we consider that other messages with status !=
> STATUS_SUCCESS have their signatures correctly computed (apparently),
> then I'd guess there's something wrong with computing signatures for
> STATUS_END_OF_FILE responses.

I agree.

Tom.

> Sent this just now:
> https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@suse.de/T/#u
> 
> I'd appreciate your, and Cc'd folks', feedback.
> 
> 
> Cheers,
> 
> Enzo
>