Re: [PATCH] ksmbd: do not sign response to session request for guest login

[Marios Makassikis <mmakassikis@xxxxxxxxxx>]

On Thu, Jan 12, 2023 at 3:36 AM Tom Talpey <tom@xxxxxxxxxx> 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);