ksmbd: do not sign response to session request for guest login

If ksmbd.mountd is configured to assign unknown users to the guest account
("map to guest = bad user" in the config), ksmbd signs the response.

This is wrong according to MS-SMB2 3.3.5.5.3:
   12. If the SMB2_SESSION_FLAG_IS_GUEST bit is not set in the SessionFlags
   field, and Session.IsAnonymous is FALSE, the server MUST sign the
   final session setup response before sending it to the client, as
   follows:
    [...]

This fixes libsmb2 based applications failing to establish a session
("Wrong signature in received").

Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
---
 fs/ksmbd/smb2pdu.c | 2 ++
 1 file changed, 2 insertions(+)

On 1/11/2023 11:39 AM, Marios Makassikis wrote:
> If ksmbd.mountd is configured to assign unknown users to the guest account
> ("map to guest = bad user" in the config), ksmbd signs the response.
> 
> This is wrong according to MS-SMB2 3.3.5.5.3:
>     12. If the SMB2_SESSION_FLAG_IS_GUEST bit is not set in the SessionFlags
>     field, and Session.IsAnonymous is FALSE, the server MUST sign the
>     final session setup response before sending it to the client, as
>     follows:
>      [...]
> 
> This fixes libsmb2 based applications failing to establish a session
> ("Wrong signature in received").

I can definitely see why! The reason for requiring the SESSION_IS_GUEST
flag to be false is because with guest there's no secret, and therefore
no key to use for signing!

But, I'd expect the code in smb3_set_sign_rsp to determine this. Before
signing it checks the following:

	if (conn->binding == false &&
	    le16_to_cpu(hdr->Command) == SMB2_SESSION_SETUP_HE) {
		signing_key = work->sess->smb3signingkey;
	} else {
		read_lock(&work->sess->chann_lock);
		chann = lookup_chann_list(work->sess, work->conn);
		if (!chann) {
			read_unlock(&work->sess->chann_lock);
			return;
		}
		signing_key = chann->smb3signingkey;
		read_unlock(&work->sess->chann_lock);
	}

	if (!signing_key)
		return;

So, the work->sess->smb3signingkey is obviously non-null.

What value is being set? Addressing that seems to be the more
general and appropriate fix here.

Tom.

> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> ---
>   fs/ksmbd/smb2pdu.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 38fbda52e06f..d681f91947d9 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -8663,6 +8663,7 @@ int smb3_decrypt_req(struct ksmbd_work *work)
>   bool smb3_11_final_sess_setup_resp(struct ksmbd_work *work)
>   {
>   	struct ksmbd_conn *conn = work->conn;
> +	struct ksmbd_session *sess = work->sess;
>   	struct smb2_hdr *rsp = smb2_get_msg(work->response_buf);
>   
>   	if (conn->dialect < SMB30_PROT_ID)
> @@ -8672,6 +8673,7 @@ bool smb3_11_final_sess_setup_resp(struct ksmbd_work *work)
>   		rsp = ksmbd_resp_buf_next(work);
>   
>   	if (le16_to_cpu(rsp->Command) == SMB2_SESSION_SETUP_HE &&
> +	    sess->user && !user_guest(sess->user) &&
>   	    rsp->Status == STATUS_SUCCESS)
>   		return true;
>   	return false;

On Thu, Jan 12, 2023 at 3:36 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 1/11/2023 11:39 AM, Marios Makassikis wrote:
> > If ksmbd.mountd is configured to assign unknown users to the guest account
> > ("map to guest = bad user" in the config), ksmbd signs the response.
> >
> > This is wrong according to MS-SMB2 3.3.5.5.3:
> >     12. If the SMB2_SESSION_FLAG_IS_GUEST bit is not set in the SessionFlags
> >     field, and Session.IsAnonymous is FALSE, the server MUST sign the
> >     final session setup response before sending it to the client, as
> >     follows:
> >      [...]
> >
> > This fixes libsmb2 based applications failing to establish a session
> > ("Wrong signature in received").
>
> I can definitely see why! The reason for requiring the SESSION_IS_GUEST
> flag to be false is because with guest there's no secret, and therefore
> no key to use for signing!
>
> But, I'd expect the code in smb3_set_sign_rsp to determine this. Before
> signing it checks the following:
>
>         if (conn->binding == false &&
>             le16_to_cpu(hdr->Command) == SMB2_SESSION_SETUP_HE) {
>                 signing_key = work->sess->smb3signingkey;
>         } else {
>                 read_lock(&work->sess->chann_lock);
>                 chann = lookup_chann_list(work->sess, work->conn);
>                 if (!chann) {
>                         read_unlock(&work->sess->chann_lock);
>                         return;
>                 }
>                 signing_key = chann->smb3signingkey;
>                 read_unlock(&work->sess->chann_lock);
>         }
>
>         if (!signing_key)
>                 return;
>
> So, the work->sess->smb3signingkey is obviously non-null.

smb3signingkey is a u8 array rather than a pointer, so the condition
is never true. Additionally, a signing key is always generated even
if signing is disabled.

>
> What value is being set? Addressing that seems to be the more
> general and appropriate fix here.
>
> Tom.

How about something like this ?

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index ebad5008ec05..d650292739ef 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -1540,7 +1540,7 @@ static int ntlm_authenticate(struct ksmbd_work *work)
         }
     }

-    if (conn->ops->generate_signingkey) {
+    if (sess->sign && conn->ops->generate_signingkey) {
         rc = conn->ops->generate_signingkey(sess, conn);
         if (rc) {
             ksmbd_debug(SMB, "SMB3 signing key generation failed\n");
@@ -1626,7 +1626,7 @@ static int krb5_authenticate(struct ksmbd_work *work)
         }
     }

-    if (conn->ops->generate_signingkey) {
+    if (sess->sign && conn->ops->generate_signingkey) {
         retval = conn->ops->generate_signingkey(sess, conn);
         if (retval) {
             ksmbd_debug(SMB, "SMB3 signing key generation failed\n");
@@ -8423,6 +8423,7 @@ int smb3_check_sign_req(struct ksmbd_work *work)
  */
 void smb3_set_sign_rsp(struct ksmbd_work *work)
 {
+    struct ksmbd_session *sess = work->sess;
     struct ksmbd_conn *conn = work->conn;
     struct smb2_hdr *req_hdr, *hdr;
     struct channel *chann;
@@ -8432,6 +8433,9 @@ void smb3_set_sign_rsp(struct ksmbd_work *work)
     size_t len;
     char *signing_key;

+    if (!sess->sign)
+        return;
+
     hdr = smb2_get_msg(work->response_buf);
     if (work->next_smb2_rsp_hdr_off)
         hdr = ksmbd_resp_buf_next(work);
@@ -8462,9 +8466,6 @@ void smb3_set_sign_rsp(struct ksmbd_work *work)
         read_unlock(&work->sess->chann_lock);
     }

-    if (!signing_key)
-        return;
-
     if (req_hdr->NextCommand)
         hdr->NextCommand = cpu_to_le32(len);

2023-01-12 1:39 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>:
> If ksmbd.mountd is configured to assign unknown users to the guest account
> ("map to guest = bad user" in the config), ksmbd signs the response.
>
> This is wrong according to MS-SMB2 3.3.5.5.3:
>    12. If the SMB2_SESSION_FLAG_IS_GUEST bit is not set in the SessionFlags
>    field, and Session.IsAnonymous is FALSE, the server MUST sign the
>    final session setup response before sending it to the client, as
>    follows:
>     [...]
>
> This fixes libsmb2 based applications failing to establish a session
> ("Wrong signature in received").
>
> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks!