Re: [PATCH] cifs: allow guest mounts to work for smb3.11

[Steve French <smfrench@gmail.com>]

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



-- 
Thanks,

Steve