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

[Enzo Matsumiya <ematsumiya@suse.de>]

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;