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

[Tom Talpey <tom@talpey.com>]

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;