[PATCH] smb: client: fail SMB2 responses with a bad signature

[PATCH] smb: client: fail SMB2 responses with a bad signature

[Suyuan Wang via B4 Relay]

From: Suyuan Wang <stevewong@hust.edu.cn>

smb2_check_receive() checks the response signature when signing is on,
but a failed check is only logged:

    rc = smb2_verify_signature(&rqst, server);
    if (rc)
            cifs_server_dbg(VFS, "SMB signature verification returned ...",
                            rc);
    ...
    return map_smb2_to_linux_error(mid->resp_buf, log_error);

The status from the response is returned as usual, and the caller goes
on to use a response whose signature is wrong. This is the path for
synchronous requests and for smb2_writev_callback(). On a signed but
unencrypted connection, anyone who can modify the traffic can change
e.g. a directory listing, and the client accepts it.

Return the verification error instead.

Unlike the SMB1 fix in commit dc96f01d54cc ("smb: client: terminate
session upon failed client required signing"), don't reconnect: this
runs in the thread that sent the request, and only cifsd should call
cifs_reconnect(). And fail the request whichever side required signing,
since a response with a bad signature can't be trusted either way.

Found with a static analysis tool and checked by hand.

Reported-by: Daniel Birtwhistle <daniel.birtwhistle@gehealthcare.com>
Closes: https://lore.kernel.org/linux-cifs/SN4PR22MB28883DAF57E0B4D4BFB1C22AEEC42@SN4PR22MB2888.namprd22.prod.outlook.com/
Fixes: 3c1bf7e48e9e ("CIFS: Enable signing in SMB2")
Cc: stable@vger.kernel.org
Co-developed-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
Signed-off-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
Signed-off-by: Suyuan Wang <stevewong@hust.edu.cn>
---
We found this on our own and only saw Daniel's report afterwards.

For SMB1 Steve wanted to keep just logging when the server, but not the
client, required signing:
https://lore.kernel.org/all/CAH2r5msfYRdaGAMuTbni-0T20tQi=N=fT4qxiaQ3siQ_WVm=Qg@mail.gmail.com/
This patch doesn't make that exception. If you want it here too, I can
send a v2 that remembers whether the client required signing.

Tested in QEMU with Samba 4.15 (server signing = mandatory, mounted with
vers=3.1.1,sign) and a proxy in between that renames a file in
QUERY_DIRECTORY responses and leaves the signature as is. Before the
patch the renamed file shows up in getdents64(), after it getdents64()
fails with EACCES. Mount, read and write on the same connection still
work, and lockdep stayed quiet.
---
 fs/smb/client/smb2transport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c
index 080864b24a0c..62121d7a44a8 100644
--- a/fs/smb/client/smb2transport.c
+++ b/fs/smb/client/smb2transport.c
@@ -711,9 +711,11 @@ smb2_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server,
 		int rc;
 
 		rc = smb2_verify_signature(&rqst, server);
-		if (rc)
+		if (rc) {
 			cifs_server_dbg(VFS, "SMB signature verification returned error = %d\n",
 				 rc);
+			return rc;
+		}
 	}
 
 	return map_smb2_to_linux_error(mid->resp_buf, log_error);

---
base-commit: 630059a5a632377b9b7b487baccde92b7ae1dd47
change-id: 20261113-smb2-sign-2f316c91c873

Best regards,
--  
Suyuan Wang <shyckless@gmail.com>

Re: [PATCH] smb: client: fail SMB2 responses with a bad signature

[Enzo Matsumiya]

On 11/13, Suyuan Wang via B4 Relay wrote:
>From: Suyuan Wang <stevewong@hust.edu.cn>
>
>smb2_check_receive() checks the response signature when signing is on,
>but a failed check is only logged:
>
>    rc = smb2_verify_signature(&rqst, server);
>    if (rc)
>            cifs_server_dbg(VFS, "SMB signature verification returned ...",
>                            rc);
>    ...
>    return map_smb2_to_linux_error(mid->resp_buf, log_error);
>
>The status from the response is returned as usual, and the caller goes
>on to use a response whose signature is wrong. This is the path for
>synchronous requests and for smb2_writev_callback(). On a signed but
>unencrypted connection, anyone who can modify the traffic can change
>e.g. a directory listing, and the client accepts it.
>
>Return the verification error instead.
>
>Unlike the SMB1 fix in commit dc96f01d54cc ("smb: client: terminate
>session upon failed client required signing"), don't reconnect: this
>runs in the thread that sent the request, and only cifsd should call
>cifs_reconnect(). And fail the request whichever side required signing,
>since a response with a bad signature can't be trusted either way.
>
>Found with a static analysis tool and checked by hand.
>
>Reported-by: Daniel Birtwhistle <daniel.birtwhistle@gehealthcare.com>
>Closes: https://lore.kernel.org/linux-cifs/SN4PR22MB28883DAF57E0B4D4BFB1C22AEEC42@SN4PR22MB2888.namprd22.prod.outlook.com/
>Fixes: 3c1bf7e48e9e ("CIFS: Enable signing in SMB2")
>Cc: stable@vger.kernel.org
>Co-developed-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
>Signed-off-by: Kaixuan Li <kaixuan.li@ntu.edu.sg>
>Signed-off-by: Suyuan Wang <stevewong@hust.edu.cn>
>---
>We found this on our own and only saw Daniel's report afterwards.
>
>For SMB1 Steve wanted to keep just logging when the server, but not the
>client, required signing:
>https://lore.kernel.org/all/CAH2r5msfYRdaGAMuTbni-0T20tQi=N=fT4qxiaQ3siQ_WVm=Qg@mail.gmail.com/
>This patch doesn't make that exception. If you want it here too, I can
>send a v2 that remembers whether the client required signing.
>
>Tested in QEMU with Samba 4.15 (server signing = mandatory, mounted with
>vers=3.1.1,sign) and a proxy in between that renames a file in
>QUERY_DIRECTORY responses and leaves the signature as is. Before the
>patch the renamed file shows up in getdents64(), after it getdents64()
>fails with EACCES. Mount, read and write on the same connection still
>work, and lockdep stayed quiet.
>---
> fs/smb/client/smb2transport.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c
>index 080864b24a0c..62121d7a44a8 100644
>--- a/fs/smb/client/smb2transport.c
>+++ b/fs/smb/client/smb2transport.c
>@@ -711,9 +711,11 @@ smb2_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server,
> 		int rc;
>
> 		rc = smb2_verify_signature(&rqst, server);
>-		if (rc)
>+		if (rc) {
> 			cifs_server_dbg(VFS, "SMB signature verification returned error = %d\n",
> 				 rc);
>+			return rc;
>+		}

Reading my reply to that other patch again https://lore.kernel.org/all/s4kmfu25glkgu44wl46e3q3bjvyhlbcvnlaiuqkuey4qlg4d5o@s7ispothcved/
I now think that, indeed, reconnecting on mismatching signatures might
be too much.

However, I still think the error should only be returned if signing is
required (*) AND rc == -EACCES.

Any other error should always be returned unconditionally.

(*) Note that "signing required" at that point means that either/both
client and server required signing, and the other side agreed -- it
doesn't really matter who asked for it.

> 	}
>
> 	return map_smb2_to_linux_error(mid->resp_buf, log_error);
>
>---

You might want to strip the lines below.

>base-commit: 630059a5a632377b9b7b487baccde92b7ae1dd47
>change-id: 20261113-smb2-sign-2f316c91c873
>
>Best regards,
>--
>Suyuan Wang <shyckless@gmail.com>


Cheers,

Enzo

Re: [PATCH] smb: client: fail SMB2 responses with a bad signature

[Enzo Matsumiya]

On 11/13, Wong Steve wrote:
>On 11/13, Enzo Matsumiya wrote:
>> However, I still think the error should only be returned if signing is
>> required (*) AND rc == -EACCES.
>>
>> Any other error should always be returned unconditionally.
>>
>> (*) Note that "signing required" at that point means that either/both
>> client and server required signing, and the other side agreed -- it
>> doesn't really matter who asked for it.
>
>Thanks for looking at it.
>
>I think the existing server->sign check already covers that.
>cifs_enable_signing() only sets server->sign when the server requires
>signing and the client allows it, or when the client requires it (mount
>option or SecurityFlags) and the server supports it. Both sides merely
>supporting signing doesn't turn it on.
>
>smb2_verify_signature() is only called under that check, so the new
>return passes on -EACCES for a mismatch, and any other error from it
>(e.g. from smb3_calc_signature()) as well.
>
>Is there a case I'm missing where server->sign is set but signing isn't
>required in your sense?

Not really, you're 100% correct, sorry.

I just remembered that there's a very specific exception to that, and it's
currently not handled;  SMB 3.1.1 tcon requests (!anonymous && !guest) are
signed unconditionally, but the responses are never verified because
they're gated by server->sign.

While your patch fix this indirectly, I couldn't find in the spec
whether such tcon responses should also fail if signing isn't required.

Anyway, I think this can be discussed later in a follow up patch.

@Paulo, @Namjae:
Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>


Cheers,

Enzo