diff mbox series

cifs: allow guest mounts to work for smb3.11

Message ID 20190321045902.14326-1-lsahlber@redhat.com
State New
Headers show
Series cifs: allow guest mounts to work for smb3.11 | expand

Commit Message

Ronnie Sahlberg March 21, 2019, 4:59 a.m. UTC 
Fix Guest/Anonymous sessions so that they work with SMB 3.11.

In git commit 6188f28 tightened the conditions and forced signing for
the SMB2-TreeConnect commands as per MS-SMB2.
However, this should only apply to normal user sessions and not for
Guest/Anonumous sessions.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
CC: Stable <stable@vger.kernel.org>
---
 fs/cifs/smb2pdu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Aurélien Aptel March 21, 2019, 11:46 a.m. UTC | #1 
Ronnie Sahlberg <lsahlber@redhat.com> writes:

> Fix Guest/Anonymous sessions so that they work with SMB 3.11.
>
> In git commit 6188f28 tightened the conditions and forced signing for
> the SMB2-TreeConnect commands as per MS-SMB2.

We could add a

    Fixes: 6188f28bf608 ("Tree connect for SMB3.1.1 must be signed for non-encrypted shares")

tag in the commit message.
>  
> -	/* 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1 */
> +	/*
> +	 * 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1
> +	 * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> +	 */
>  	if ((ses->server->dialect == SMB311_PROT_ID) &&
> -	    !smb3_encryption_required(tcon))
> +	    !smb3_encryption_required(tcon) &&
> +	    !(ses->session_flags & (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
>  		req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
>  
>  	memset(&rqst, 0, sizeof(struct smb_rqst));

I suspect there might more (and/or better) places to check. If you grep for
SMB2_FLAGS_SIGNED there are a couple of other places that should be
considered:

smb2_hdr_assemble() { ...
	if (tcon->ses && tcon->ses->server && tcon->ses->server->sign &&
	    !smb3_encryption_required(tcon))
		shdr->Flags |= SMB2_FLAGS_SIGNED;
...
}

cifs_get_smb_ses() { ...
	ses->sectype = volume_info->sectype;
	ses->sign = volume_info->sign;

	mutex_lock(&ses->session_mutex);
	rc = cifs_negotiate_protocol(xid, ses);
	if (!rc)
		rc = cifs_setup_session(xid, ses, volume_info->local_nls);
	mutex_unlock(&ses->session_mutex);
	if (rc)
		goto get_ses_fail;
...
}

After negprot and before sess setup we already know the protocol version so I guess there
could be some patch there as well.

See also SMB2_logoff(), SMB2_ioctl_init(), cifs_enable_signing()

Cheers,
Steve French March 21, 2019, 3:36 p.m. UTC | #2 
I added the commit tag and cleaned up the related checkpatch warning.

I don't know of any other locations where we have to make a similar
change as this is the only place where we (on the client) forced
signing to be on for a request when the user (or server) didn't
request it (due to the requirement in MS-SMB2 that tree connect be
signed).  In the other places you noted, the client or server would
have forced signing on ("sign" mount option or "server signing =
mandatory" for example).

On Thu, Mar 21, 2019 at 6:46 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Ronnie Sahlberg <lsahlber@redhat.com> writes:
>
> > Fix Guest/Anonymous sessions so that they work with SMB 3.11.
> >
> > In git commit 6188f28 tightened the conditions and forced signing for
> > the SMB2-TreeConnect commands as per MS-SMB2.
>
> We could add a
>
>     Fixes: 6188f28bf608 ("Tree connect for SMB3.1.1 must be signed for non-encrypted shares")
>
> tag in the commit message.
> >
> > -     /* 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1 */
> > +     /*
> > +      * 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1
> > +      * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> > +      */
> >       if ((ses->server->dialect == SMB311_PROT_ID) &&
> > -         !smb3_encryption_required(tcon))
> > +         !smb3_encryption_required(tcon) &&
> > +         !(ses->session_flags & (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
> >               req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
> >
> >       memset(&rqst, 0, sizeof(struct smb_rqst));
>
> I suspect there might more (and/or better) places to check. If you grep for
> SMB2_FLAGS_SIGNED there are a couple of other places that should be
> considered:
>
> smb2_hdr_assemble() { ...
>         if (tcon->ses && tcon->ses->server && tcon->ses->server->sign &&
>             !smb3_encryption_required(tcon))
>                 shdr->Flags |= SMB2_FLAGS_SIGNED;
> ...
> }
>
> cifs_get_smb_ses() { ...
>         ses->sectype = volume_info->sectype;
>         ses->sign = volume_info->sign;
>
>         mutex_lock(&ses->session_mutex);
>         rc = cifs_negotiate_protocol(xid, ses);
>         if (!rc)
>                 rc = cifs_setup_session(xid, ses, volume_info->local_nls);
>         mutex_unlock(&ses->session_mutex);
>         if (rc)
>                 goto get_ses_fail;
> ...
> }
>
> After negprot and before sess setup we already know the protocol version so I guess there
> could be some patch there as well.
>
> See also SMB2_logoff(), SMB2_ioctl_init(), cifs_enable_signing()
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
Andreas Hasenack March 21, 2019, 5:13 p.m. UTC | #3 
Hello Ronnie,

On Thu, Mar 21, 2019 at 1:59 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> Fix Guest/Anonymous sessions so that they work with SMB 3.11.
>
> In git commit 6188f28 tightened the conditions and forced signing for
> the SMB2-TreeConnect commands as per MS-SMB2.
> However, this should only apply to normal user sessions and not for
> Guest/Anonumous sessions.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> CC: Stable <stable@vger.kernel.org>
> ---
>  fs/cifs/smb2pdu.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index c399e09b76e6..8e4a1da95418 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1628,9 +1628,13 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
>         iov[1].iov_base = unc_path;
>         iov[1].iov_len = unc_path_len;
>
> -       /* 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1 */
> +       /*
> +        * 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1
> +        * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> +        */
>         if ((ses->server->dialect == SMB311_PROT_ID) &&
> -           !smb3_encryption_required(tcon))
> +           !smb3_encryption_required(tcon) &&
> +           !(ses->session_flags & (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
>                 req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
>
>         memset(&rqst, 0, sizeof(struct smb_rqst));
> --
> 2.13.6
>


I tried this patch with an ubuntu kernel
(https://people.canonical.com/~tyhicks/disco-cifs.2/ specifically) but
it didn't work, I'm still getting failures with smb3.11 and a guest
mount.

Maybe I'm missing some other fix, or a more up-to-date kernel? Shall I
try with a self-compiled upstream one?

dmesg: http://paste.ubuntu.com/p/JGhCsgBVcb/

server logs (debug level 5, samba 4.10.0):
log.: http://paste.ubuntu.com/p/jMDJ8DBfRM/
log.smbd: http://paste.ubuntu.com/p/Z9W5z28BP9/
smb.conf: http://paste.ubuntu.com/p/9HpSyFq8n8/
ronnie sahlberg March 21, 2019, 7:54 p.m. UTC | #4 
On Fri, Mar 22, 2019 at 3:13 AM Andreas Hasenack <andreas@canonical.com> wrote:
>
> Hello Ronnie,
>
> On Thu, Mar 21, 2019 at 1:59 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> >
> > Fix Guest/Anonymous sessions so that they work with SMB 3.11.
> >
> > In git commit 6188f28 tightened the conditions and forced signing for
> > the SMB2-TreeConnect commands as per MS-SMB2.
> > However, this should only apply to normal user sessions and not for
> > Guest/Anonumous sessions.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > CC: Stable <stable@vger.kernel.org>
> > ---
> >  fs/cifs/smb2pdu.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index c399e09b76e6..8e4a1da95418 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -1628,9 +1628,13 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
> >         iov[1].iov_base = unc_path;
> >         iov[1].iov_len = unc_path_len;
> >
> > -       /* 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1 */
> > +       /*
> > +        * 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1
> > +        * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> > +        */
> >         if ((ses->server->dialect == SMB311_PROT_ID) &&
> > -           !smb3_encryption_required(tcon))
> > +           !smb3_encryption_required(tcon) &&
> > +           !(ses->session_flags & (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
> >                 req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
> >
> >         memset(&rqst, 0, sizeof(struct smb_rqst));
> > --
> > 2.13.6
> >
>
>
> I tried this patch with an ubuntu kernel
> (https://people.canonical.com/~tyhicks/disco-cifs.2/ specifically) but
> it didn't work, I'm still getting failures with smb3.11 and a guest
> mount.
>
> Maybe I'm missing some other fix, or a more up-to-date kernel? Shall I
> try with a self-compiled upstream one?

Try with the current version of Steve's for-next branch plus this patch.
I could reproduce the failure with 3.11 on for-next
but when I added this patch then the mount was successful.

At least that would verify that the current for-net works for you (or not).
There may be other things missing in older kernels that broke 3.11 guest mounts,
but lets check if for-next works first.

Regards
Ronnie Sahlberg

>
> dmesg: http://paste.ubuntu.com/p/JGhCsgBVcb/
>
> server logs (debug level 5, samba 4.10.0):
> log.: http://paste.ubuntu.com/p/jMDJ8DBfRM/
> log.smbd: http://paste.ubuntu.com/p/Z9W5z28BP9/
> smb.conf: http://paste.ubuntu.com/p/9HpSyFq8n8/
Steve French March 22, 2019, 11:30 p.m. UTC | #5 
I tried to Samba with guest mount and noted that Samba server did not
set the guest or null flags in the session setup response so we still
marked tcon as signed even with guest mount in my quick test (to Samba
4.10)

On Thu, Mar 21, 2019 at 2:54 PM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> On Fri, Mar 22, 2019 at 3:13 AM Andreas Hasenack <andreas@canonical.com> wrote:
> >
> > Hello Ronnie,
> >
> > On Thu, Mar 21, 2019 at 1:59 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > >
> > > Fix Guest/Anonymous sessions so that they work with SMB 3.11.
> > >
> > > In git commit 6188f28 tightened the conditions and forced signing for
> > > the SMB2-TreeConnect commands as per MS-SMB2.
> > > However, this should only apply to normal user sessions and not for
> > > Guest/Anonumous sessions.
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > CC: Stable <stable@vger.kernel.org>
> > > ---
> > >  fs/cifs/smb2pdu.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > index c399e09b76e6..8e4a1da95418 100644
> > > --- a/fs/cifs/smb2pdu.c
> > > +++ b/fs/cifs/smb2pdu.c
> > > @@ -1628,9 +1628,13 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
> > >         iov[1].iov_base = unc_path;
> > >         iov[1].iov_len = unc_path_len;
> > >
> > > -       /* 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1 */
> > > +       /*
> > > +        * 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1
> > > +        * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> > > +        */
> > >         if ((ses->server->dialect == SMB311_PROT_ID) &&
> > > -           !smb3_encryption_required(tcon))
> > > +           !smb3_encryption_required(tcon) &&
> > > +           !(ses->session_flags & (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
> > >                 req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
> > >
> > >         memset(&rqst, 0, sizeof(struct smb_rqst));
> > > --
> > > 2.13.6
> > >
> >
> >
> > I tried this patch with an ubuntu kernel
> > (https://people.canonical.com/~tyhicks/disco-cifs.2/ specifically) but
> > it didn't work, I'm still getting failures with smb3.11 and a guest
> > mount.
> >
> > Maybe I'm missing some other fix, or a more up-to-date kernel? Shall I
> > try with a self-compiled upstream one?
>
> Try with the current version of Steve's for-next branch plus this patch.
> I could reproduce the failure with 3.11 on for-next
> but when I added this patch then the mount was successful.
>
> At least that would verify that the current for-net works for you (or not).
> There may be other things missing in older kernels that broke 3.11 guest mounts,
> but lets check if for-next works first.
>
> Regards
> Ronnie Sahlberg
>
> >
> > dmesg: http://paste.ubuntu.com/p/JGhCsgBVcb/
> >
> > server logs (debug level 5, samba 4.10.0):
> > log.: http://paste.ubuntu.com/p/jMDJ8DBfRM/
> > log.smbd: http://paste.ubuntu.com/p/Z9W5z28BP9/
> > smb.conf: http://paste.ubuntu.com/p/9HpSyFq8n8/
Tom Talpey March 23, 2019, 12:20 a.m. UTC | #6 
> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org> On
> Behalf Of Steve French
> Sent: Friday, March 22, 2019 7:30 PM
> To: ronnie sahlberg <ronniesahlberg@gmail.com>
> Cc: Andreas Hasenack <andreas@canonical.com>; Ronnie Sahlberg
> <lsahlber@redhat.com>; linux-cifs <linux-cifs@vger.kernel.org>; Stable
> <stable@vger.kernel.org>
> Subject: Re: [PATCH] cifs: allow guest mounts to work for smb3.11
> 
> I tried to Samba with guest mount and noted that Samba server did not
> set the guest or null flags in the session setup response so we still
> marked tcon as signed even with guest mount in my quick test (to Samba
> 4.10)

This gets to the point of my earlier comment. The flags and dialect etc
are all fine to check, but the code is setting the SIGNED bit before it
actually signs. Shouldn't it be verifying that it has a valid signing key
before it does this?

Another point to make here is that the SESSION_IS_GUEST is entirely
the server's choice. Basically, what it means is the server arbitrarily
authenticated the user as a guest, regardless of how the client attempted
to log in. If it's set, then absolutely the client should forget that it ever
attempted to authenticate as a given principal (and not sign, encrypt, etc).
However, if it's clear, and the server made the user "guest", then:

 a) the server is arguably buggy
 b) the signing failure is by protocol design.

Tom.

> On Thu, Mar 21, 2019 at 2:54 PM ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
> >
> > On Fri, Mar 22, 2019 at 3:13 AM Andreas Hasenack
> <andreas@canonical.com> wrote:
> > >
> > > Hello Ronnie,
> > >
> > > On Thu, Mar 21, 2019 at 1:59 AM Ronnie Sahlberg <lsahlber@redhat.com>
> wrote:
> > > >
> > > > Fix Guest/Anonymous sessions so that they work with SMB 3.11.
> > > >
> > > > In git commit 6188f28 tightened the conditions and forced signing for
> > > > the SMB2-TreeConnect commands as per MS-SMB2.
> > > > However, this should only apply to normal user sessions and not for
> > > > Guest/Anonumous sessions.
> > > >
> > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > CC: Stable <stable@vger.kernel.org>
> > > > ---
> > > >  fs/cifs/smb2pdu.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > index c399e09b76e6..8e4a1da95418 100644
> > > > --- a/fs/cifs/smb2pdu.c
> > > > +++ b/fs/cifs/smb2pdu.c
> > > > @@ -1628,9 +1628,13 @@ SMB2_tcon(const unsigned int xid, struct
> cifs_ses *ses, const char *tree,
> > > >         iov[1].iov_base = unc_path;
> > > >         iov[1].iov_len = unc_path_len;
> > > >
> > > > -       /* 3.11 tcon req must be signed if not encrypted. See MS-SMB2
> 3.2.4.1.1 */
> > > > +       /*
> > > > +        * 3.11 tcon req must be signed if not encrypted. See MS-SMB2
> 3.2.4.1.1
> > > > +        * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> > > > +        */
> > > >         if ((ses->server->dialect == SMB311_PROT_ID) &&
> > > > -           !smb3_encryption_required(tcon))
> > > > +           !smb3_encryption_required(tcon) &&
> > > > +           !(ses->session_flags &
> (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
> > > >                 req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
> > > >
> > > >         memset(&rqst, 0, sizeof(struct smb_rqst));
> > > > --
> > > > 2.13.6
> > > >
> > >
> > >
> > > I tried this patch with an ubuntu kernel
> > >
> (https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fpeople.can
> onical.com%2F~tyhicks%2Fdisco-
> cifs.2%2F&amp;data=02%7C01%7Cttalpey%40microsoft.com%7C3cc97bf9482
> 747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0
> %7C636888942385347937&amp;sdata=i0kUZ4yinqjgvsDDc0N6LBTPlLt8DeNnt
> WW1PhS0xVg%3D&amp;reserved=0 specifically) but
> > > it didn't work, I'm still getting failures with smb3.11 and a guest
> > > mount.
> > >
> > > Maybe I'm missing some other fix, or a more up-to-date kernel? Shall I
> > > try with a self-compiled upstream one?
> >
> > Try with the current version of Steve's for-next branch plus this patch.
> > I could reproduce the failure with 3.11 on for-next
> > but when I added this patch then the mount was successful.
> >
> > At least that would verify that the current for-net works for you (or not).
> > There may be other things missing in older kernels that broke 3.11 guest
> mounts,
> > but lets check if for-next works first.
> >
> > Regards
> > Ronnie Sahlberg
> >
> > >
> > > dmesg:
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> untu.com%2Fp%2FJGhCsgBVcb%2F&amp;data=02%7C01%7Cttalpey%40micros
> oft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2d
> 7cd011db47%7C1%7C0%7C636888942385347937&amp;sdata=r5eMhXks%2F
> quoNzzhFahCKjMOE22fFlVCluwmRLWpmOc%3D&amp;reserved=0
> > >
> > > server logs (debug level 5, samba 4.10.0):
> > > log.:
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> untu.com%2Fp%2FjMDJ8DBfRM%2F&amp;data=02%7C01%7Cttalpey%40micr
> osoft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab
> 2d7cd011db47%7C1%7C0%7C636888942385347937&amp;sdata=4owqPDbgc
> FOjQU%2BEt2aB1P77hFHbIuzoxOkdpX5X2eE%3D&amp;reserved=0
> > > log.smbd:
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> untu.com%2Fp%2FZ9W5z28BP9%2F&amp;data=02%7C01%7Cttalpey%40micr
> osoft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab
> 2d7cd011db47%7C1%7C0%7C636888942385357938&amp;sdata=66Ng0qrdbt
> AwVirxqN3uWjEaFrRi1w6XGcX%2BZITmkOM%3D&amp;reserved=0
> > > smb.conf:
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> untu.com%2Fp%2F9HpSyFq8n8%2F&amp;data=02%7C01%7Cttalpey%40micro
> soft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2
> d7cd011db47%7C1%7C0%7C636888942385357938&amp;sdata=U4dqv23dZN
> KgaDAR2TbEmsrnkR6EnqejdVTPVLpIjKk%3D&amp;reserved=0
> 
> 
> 
> --
> Thanks,
> 
> Steve
Steve French March 23, 2019, 12:41 a.m. UTC | #7 
The reason we didn't see this before SMB3.1.1 is that in
smb3_validate_negotiate we had this check:

        if (tcon->ses->user_name == NULL) {
                cifs_dbg(FYI, "Can't validate negotiate: null user mount\n");
                return 0; /* validation requires signing */
        }

Sounds like we should add the identical check in the smb3.1.1 tcon
check that Ronnie's earlier patch fixed (but only for Windows)



On Fri, Mar 22, 2019 at 7:20 PM Tom Talpey <ttalpey@microsoft.com> wrote:
>
> > -----Original Message-----
> > From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org> On
> > Behalf Of Steve French
> > Sent: Friday, March 22, 2019 7:30 PM
> > To: ronnie sahlberg <ronniesahlberg@gmail.com>
> > Cc: Andreas Hasenack <andreas@canonical.com>; Ronnie Sahlberg
> > <lsahlber@redhat.com>; linux-cifs <linux-cifs@vger.kernel.org>; Stable
> > <stable@vger.kernel.org>
> > Subject: Re: [PATCH] cifs: allow guest mounts to work for smb3.11
> >
> > I tried to Samba with guest mount and noted that Samba server did not
> > set the guest or null flags in the session setup response so we still
> > marked tcon as signed even with guest mount in my quick test (to Samba
> > 4.10)
>
> This gets to the point of my earlier comment. The flags and dialect etc
> are all fine to check, but the code is setting the SIGNED bit before it
> actually signs. Shouldn't it be verifying that it has a valid signing key
> before it does this?
>
> Another point to make here is that the SESSION_IS_GUEST is entirely
> the server's choice. Basically, what it means is the server arbitrarily
> authenticated the user as a guest, regardless of how the client attempted
> to log in. If it's set, then absolutely the client should forget that it ever
> attempted to authenticate as a given principal (and not sign, encrypt, etc).
> However, if it's clear, and the server made the user "guest", then:
>
>  a) the server is arguably buggy
>  b) the signing failure is by protocol design.
>
> Tom.
>
> > On Thu, Mar 21, 2019 at 2:54 PM ronnie sahlberg
> > <ronniesahlberg@gmail.com> wrote:
> > >
> > > On Fri, Mar 22, 2019 at 3:13 AM Andreas Hasenack
> > <andreas@canonical.com> wrote:
> > > >
> > > > Hello Ronnie,
> > > >
> > > > On Thu, Mar 21, 2019 at 1:59 AM Ronnie Sahlberg <lsahlber@redhat.com>
> > wrote:
> > > > >
> > > > > Fix Guest/Anonymous sessions so that they work with SMB 3.11.
> > > > >
> > > > > In git commit 6188f28 tightened the conditions and forced signing for
> > > > > the SMB2-TreeConnect commands as per MS-SMB2.
> > > > > However, this should only apply to normal user sessions and not for
> > > > > Guest/Anonumous sessions.
> > > > >
> > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > > CC: Stable <stable@vger.kernel.org>
> > > > > ---
> > > > >  fs/cifs/smb2pdu.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > > index c399e09b76e6..8e4a1da95418 100644
> > > > > --- a/fs/cifs/smb2pdu.c
> > > > > +++ b/fs/cifs/smb2pdu.c
> > > > > @@ -1628,9 +1628,13 @@ SMB2_tcon(const unsigned int xid, struct
> > cifs_ses *ses, const char *tree,
> > > > >         iov[1].iov_base = unc_path;
> > > > >         iov[1].iov_len = unc_path_len;
> > > > >
> > > > > -       /* 3.11 tcon req must be signed if not encrypted. See MS-SMB2
> > 3.2.4.1.1 */
> > > > > +       /*
> > > > > +        * 3.11 tcon req must be signed if not encrypted. See MS-SMB2
> > 3.2.4.1.1
> > > > > +        * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> > > > > +        */
> > > > >         if ((ses->server->dialect == SMB311_PROT_ID) &&
> > > > > -           !smb3_encryption_required(tcon))
> > > > > +           !smb3_encryption_required(tcon) &&
> > > > > +           !(ses->session_flags &
> > (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
> > > > >                 req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
> > > > >
> > > > >         memset(&rqst, 0, sizeof(struct smb_rqst));
> > > > > --
> > > > > 2.13.6
> > > > >
> > > >
> > > >
> > > > I tried this patch with an ubuntu kernel
> > > >
> > (https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fpeople.can
> > onical.com%2F~tyhicks%2Fdisco-
> > cifs.2%2F&amp;data=02%7C01%7Cttalpey%40microsoft.com%7C3cc97bf9482
> > 747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0
> > %7C636888942385347937&amp;sdata=i0kUZ4yinqjgvsDDc0N6LBTPlLt8DeNnt
> > WW1PhS0xVg%3D&amp;reserved=0 specifically) but
> > > > it didn't work, I'm still getting failures with smb3.11 and a guest
> > > > mount.
> > > >
> > > > Maybe I'm missing some other fix, or a more up-to-date kernel? Shall I
> > > > try with a self-compiled upstream one?
> > >
> > > Try with the current version of Steve's for-next branch plus this patch.
> > > I could reproduce the failure with 3.11 on for-next
> > > but when I added this patch then the mount was successful.
> > >
> > > At least that would verify that the current for-net works for you (or not).
> > > There may be other things missing in older kernels that broke 3.11 guest
> > mounts,
> > > but lets check if for-next works first.
> > >
> > > Regards
> > > Ronnie Sahlberg
> > >
> > > >
> > > > dmesg:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > untu.com%2Fp%2FJGhCsgBVcb%2F&amp;data=02%7C01%7Cttalpey%40micros
> > oft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2d
> > 7cd011db47%7C1%7C0%7C636888942385347937&amp;sdata=r5eMhXks%2F
> > quoNzzhFahCKjMOE22fFlVCluwmRLWpmOc%3D&amp;reserved=0
> > > >
> > > > server logs (debug level 5, samba 4.10.0):
> > > > log.:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > untu.com%2Fp%2FjMDJ8DBfRM%2F&amp;data=02%7C01%7Cttalpey%40micr
> > osoft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab
> > 2d7cd011db47%7C1%7C0%7C636888942385347937&amp;sdata=4owqPDbgc
> > FOjQU%2BEt2aB1P77hFHbIuzoxOkdpX5X2eE%3D&amp;reserved=0
> > > > log.smbd:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > untu.com%2Fp%2FZ9W5z28BP9%2F&amp;data=02%7C01%7Cttalpey%40micr
> > osoft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab
> > 2d7cd011db47%7C1%7C0%7C636888942385357938&amp;sdata=66Ng0qrdbt
> > AwVirxqN3uWjEaFrRi1w6XGcX%2BZITmkOM%3D&amp;reserved=0
> > > > smb.conf:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > untu.com%2Fp%2F9HpSyFq8n8%2F&amp;data=02%7C01%7Cttalpey%40micro
> > soft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2
> > d7cd011db47%7C1%7C0%7C636888942385357938&amp;sdata=U4dqv23dZN
> > KgaDAR2TbEmsrnkR6EnqejdVTPVLpIjKk%3D&amp;reserved=0
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
Tom Talpey March 23, 2019, 1:27 a.m. UTC | #8 
> -----Original Message-----
> From: Steve French <smfrench@gmail.com>
> Sent: Friday, March 22, 2019 8:42 PM
> To: Tom Talpey <ttalpey@microsoft.com>
> Cc: ronnie sahlberg <ronniesahlberg@gmail.com>; Andreas Hasenack
> <andreas@canonical.com>; Ronnie Sahlberg <lsahlber@redhat.com>; linux-cifs
> <linux-cifs@vger.kernel.org>; Stable <stable@vger.kernel.org>
> Subject: Re: [PATCH] cifs: allow guest mounts to work for smb3.11
> 
> The reason we didn't see this before SMB3.1.1 is that in
> smb3_validate_negotiate we had this check:
> 
>         if (tcon->ses->user_name == NULL) {
>                 cifs_dbg(FYI, "Can't validate negotiate: null user mount\n");
>                 return 0; /* validation requires signing */
>         }
> 
> Sounds like we should add the identical check in the smb3.1.1 tcon
> check that Ronnie's earlier patch fixed (but only for Windows)

Technically a NULL username is a _null_ session, not _guest_. The difference
is whether it was the client or server which decided to not use a named
principal. Sending a blank user by the client leads to a null session, while
the server choosing to ignore the client-provided user leads to guest.

Either one should be avoided, of course.

Tom.

> On Fri, Mar 22, 2019 at 7:20 PM Tom Talpey <ttalpey@microsoft.com> wrote:
> >
> > > -----Original Message-----
> > > From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org>
> On
> > > Behalf Of Steve French
> > > Sent: Friday, March 22, 2019 7:30 PM
> > > To: ronnie sahlberg <ronniesahlberg@gmail.com>
> > > Cc: Andreas Hasenack <andreas@canonical.com>; Ronnie Sahlberg
> > > <lsahlber@redhat.com>; linux-cifs <linux-cifs@vger.kernel.org>; Stable
> > > <stable@vger.kernel.org>
> > > Subject: Re: [PATCH] cifs: allow guest mounts to work for smb3.11
> > >
> > > I tried to Samba with guest mount and noted that Samba server did not
> > > set the guest or null flags in the session setup response so we still
> > > marked tcon as signed even with guest mount in my quick test (to Samba
> > > 4.10)
> >
> > This gets to the point of my earlier comment. The flags and dialect etc
> > are all fine to check, but the code is setting the SIGNED bit before it
> > actually signs. Shouldn't it be verifying that it has a valid signing key
> > before it does this?
> >
> > Another point to make here is that the SESSION_IS_GUEST is entirely
> > the server's choice. Basically, what it means is the server arbitrarily
> > authenticated the user as a guest, regardless of how the client attempted
> > to log in. If it's set, then absolutely the client should forget that it ever
> > attempted to authenticate as a given principal (and not sign, encrypt, etc).
> > However, if it's clear, and the server made the user "guest", then:
> >
> >  a) the server is arguably buggy
> >  b) the signing failure is by protocol design.
> >
> > Tom.
> >
> > > On Thu, Mar 21, 2019 at 2:54 PM ronnie sahlberg
> > > <ronniesahlberg@gmail.com> wrote:
> > > >
> > > > On Fri, Mar 22, 2019 at 3:13 AM Andreas Hasenack
> > > <andreas@canonical.com> wrote:
> > > > >
> > > > > Hello Ronnie,
> > > > >
> > > > > On Thu, Mar 21, 2019 at 1:59 AM Ronnie Sahlberg
> <lsahlber@redhat.com>
> > > wrote:
> > > > > >
> > > > > > Fix Guest/Anonymous sessions so that they work with SMB 3.11.
> > > > > >
> > > > > > In git commit 6188f28 tightened the conditions and forced signing for
> > > > > > the SMB2-TreeConnect commands as per MS-SMB2.
> > > > > > However, this should only apply to normal user sessions and not for
> > > > > > Guest/Anonumous sessions.
> > > > > >
> > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > > > CC: Stable <stable@vger.kernel.org>
> > > > > > ---
> > > > > >  fs/cifs/smb2pdu.c | 8 ++++++--
> > > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > > > index c399e09b76e6..8e4a1da95418 100644
> > > > > > --- a/fs/cifs/smb2pdu.c
> > > > > > +++ b/fs/cifs/smb2pdu.c
> > > > > > @@ -1628,9 +1628,13 @@ SMB2_tcon(const unsigned int xid, struct
> > > cifs_ses *ses, const char *tree,
> > > > > >         iov[1].iov_base = unc_path;
> > > > > >         iov[1].iov_len = unc_path_len;
> > > > > >
> > > > > > -       /* 3.11 tcon req must be signed if not encrypted. See MS-SMB2
> > > 3.2.4.1.1 */
> > > > > > +       /*
> > > > > > +        * 3.11 tcon req must be signed if not encrypted. See MS-SMB2
> > > 3.2.4.1.1
> > > > > > +        * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
> > > > > > +        */
> > > > > >         if ((ses->server->dialect == SMB311_PROT_ID) &&
> > > > > > -           !smb3_encryption_required(tcon))
> > > > > > +           !smb3_encryption_required(tcon) &&
> > > > > > +           !(ses->session_flags &
> > > (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
> > > > > >                 req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
> > > > > >
> > > > > >         memset(&rqst, 0, sizeof(struct smb_rqst));
> > > > > > --
> > > > > > 2.13.6
> > > > > >
> > > > >
> > > > >
> > > > > I tried this patch with an ubuntu kernel
> > > > >
> > >
> (https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fpeople.can
> > > onical.com%2F~tyhicks%2Fdisco-
> > >
> cifs.2%2F&amp;data=02%7C01%7Cttalpey%40microsoft.com%7C3cc97bf9482
> > >
> 747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0
> > >
> %7C636888942385347937&amp;sdata=i0kUZ4yinqjgvsDDc0N6LBTPlLt8DeNnt
> > > WW1PhS0xVg%3D&amp;reserved=0 specifically) but
> > > > > it didn't work, I'm still getting failures with smb3.11 and a guest
> > > > > mount.
> > > > >
> > > > > Maybe I'm missing some other fix, or a more up-to-date kernel? Shall I
> > > > > try with a self-compiled upstream one?
> > > >
> > > > Try with the current version of Steve's for-next branch plus this patch.
> > > > I could reproduce the failure with 3.11 on for-next
> > > > but when I added this patch then the mount was successful.
> > > >
> > > > At least that would verify that the current for-net works for you (or not).
> > > > There may be other things missing in older kernels that broke 3.11 guest
> > > mounts,
> > > > but lets check if for-next works first.
> > > >
> > > > Regards
> > > > Ronnie Sahlberg
> > > >
> > > > >
> > > > > dmesg:
> > >
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > >
> untu.com%2Fp%2FJGhCsgBVcb%2F&amp;data=02%7C01%7Cttalpey%40micros
> > >
> oft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2d
> > >
> 7cd011db47%7C1%7C0%7C636888942385347937&amp;sdata=r5eMhXks%2F
> > > quoNzzhFahCKjMOE22fFlVCluwmRLWpmOc%3D&amp;reserved=0
> > > > >
> > > > > server logs (debug level 5, samba 4.10.0):
> > > > > log.:
> > >
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > >
> untu.com%2Fp%2FjMDJ8DBfRM%2F&amp;data=02%7C01%7Cttalpey%40micr
> > >
> osoft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab
> > >
> 2d7cd011db47%7C1%7C0%7C636888942385347937&amp;sdata=4owqPDbgc
> > > FOjQU%2BEt2aB1P77hFHbIuzoxOkdpX5X2eE%3D&amp;reserved=0
> > > > > log.smbd:
> > >
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > >
> untu.com%2Fp%2FZ9W5z28BP9%2F&amp;data=02%7C01%7Cttalpey%40micr
> > >
> osoft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab
> > >
> 2d7cd011db47%7C1%7C0%7C636888942385357938&amp;sdata=66Ng0qrdbt
> > > AwVirxqN3uWjEaFrRi1w6XGcX%2BZITmkOM%3D&amp;reserved=0
> > > > > smb.conf:
> > >
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpaste.ub
> > >
> untu.com%2Fp%2F9HpSyFq8n8%2F&amp;data=02%7C01%7Cttalpey%40micro
> > >
> soft.com%7C3cc97bf9482747afd16d08d6af1e62ff%7C72f988bf86f141af91ab2
> > >
> d7cd011db47%7C1%7C0%7C636888942385357938&amp;sdata=U4dqv23dZN
> > > KgaDAR2TbEmsrnkR6EnqejdVTPVLpIjKk%3D&amp;reserved=0
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> 
> 
> 
> --
> Thanks,
> 
> Steve
diff mbox series

Patch

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index c399e09b76e6..8e4a1da95418 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1628,9 +1628,13 @@  SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
 	iov[1].iov_base = unc_path;
 	iov[1].iov_len = unc_path_len;
 
-	/* 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1 */
+	/*
+	 * 3.11 tcon req must be signed if not encrypted. See MS-SMB2 3.2.4.1.1
+	 * unless it is guest or anonymous user. See MS-SMB2 3.2.5.3.1
+	 */
 	if ((ses->server->dialect == SMB311_PROT_ID) &&
-	    !smb3_encryption_required(tcon))
+	    !smb3_encryption_required(tcon) &&
+	    !(ses->session_flags & (SMB2_SESSION_FLAG_IS_GUEST|SMB2_SESSION_FLAG_IS_NULL)))
 		req->sync_hdr.Flags |= SMB2_FLAGS_SIGNED;
 
 	memset(&rqst, 0, sizeof(struct smb_rqst));