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

[Tom Talpey <ttalpey@microsoft.com>]

> -----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