Re: [PATCH] cifs: fix broken lanman (lm) auth code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 16 Feb 2011 09:06:08 -0600
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:

> On Wed, Feb 16, 2011 at 9:01 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Wed, 16 Feb 2011 08:46:03 -0600
> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> >
> >> On Wed, Feb 16, 2011 at 6:53 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >> > On Tue, 15 Feb 2011 17:10:43 -0600
> >> > shirishpargaonkar@xxxxxxxxx wrote:
> >> >
> >> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> >>
> >> >>
> >> >> Fix lanman (lm) authentication code.
> >> >>
> >> >> Change lm response length back to 24 from 16.
> >> >> Parse lanmani mount option.
> >> >> Add code to add odd parity bit to each of the eight bytes of a DES key.
> >> >>
> >> >>
> >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> >> ---
> >> >>  fs/cifs/cifsglob.h |    3 ++-
> >> >>  fs/cifs/connect.c  |    3 +++
> >> >>  fs/cifs/sess.c     |    8 ++++----
> >> >>  fs/cifs/smbdes.c   |   19 ++++++++++++++++++-
> >> >>  4 files changed, 27 insertions(+), 6 deletions(-)
> >> >>
> >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> >> index 17afb0f..0b5c950 100644
> >> >> --- a/fs/cifs/cifsglob.h
> >> >> +++ b/fs/cifs/cifsglob.h
> >> >> @@ -710,7 +710,8 @@ require use of the stronger protocol */
> >> >>  #define   CIFSSEC_MUST_SEAL  0x40040 /* not supported yet */
> >> >>  #define   CIFSSEC_MUST_NTLMSSP       0x80080 /* raw ntlmssp with ntlmv2 */
> >> >>
> >> >> -#define   CIFSSEC_DEF (CIFSSEC_MAY_SIGN | CIFSSEC_MAY_NTLM | CIFSSEC_MAY_NTLMV2)
> >> >> +#define   CIFSSEC_DEF (CIFSSEC_MAY_LANMAN | CIFSSEC_MAY_SIGN | \
> >> >> +                     CIFSSEC_MAY_NTLM | CIFSSEC_MAY_NTLMV2)
> >> >                        ^^^^^^^^^^^
> >> >        Won't this change enable lanman auth by default? Is that
> >> >        intended?
> >>
> >> I think it depends on what server sends in negprot response, the
> >> minimun dialect it supports.
> >> If the dialect is greater than Lanman2.1, the default auth mech for
> >> cifs client is NTLM (ntlmv1)
> >> if the dialect is less than or upto Lanman2.1, default auth mech for
> >> cifs client is LANMAN (lm).
> >>
> >
> > Historically we've required that the admin
> > set /proc/fs/cifs/SecurityFlags to allow LANMAN auth before the client
> > will allow it to be used. I'm not opposed to changing that, but the
> > description doesn't even mention anything about that.
> >
> > I think this ought to be a separate patch with a clearly described
> > reason, and probably doesn't belong in 2.6.38. It doesn't seem like
> > it's necessary to fix this bug.
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>
> >
> 
> I think that is still the case.  If we do not have the code,  we will get error
>  CIFS VFS: mount failed weak security disabled in /proc/fs/cifs/SecurityFlags
> and auth will fail even if admin did necessary to allow LANMAN.

For the record, I'm of the opinion that this secFlags stuff is far too
convoluted for anyone's good. It's horribly confusing. I don't think
what you're saying is the case however:

...global_secflags is initialized to CIFSSEC_DEF:

	unsigned int global_secflags = CIFSSEC_DEF;

...then, if ses->overrideSecFlg is not set (which is the case if no
sec= option is set):

        /* if any of auth flags (ie not sign or seal) are overriden use them */
        if (ses->overrideSecFlg & (~(CIFSSEC_MUST_SIGN | CIFSSEC_MUST_SEAL)))
                secFlags = ses->overrideSecFlg;  /* BB FIXME fix sign flags? */
        else /* if override flags set only sign/seal OR them with global auth */
                secFlags = global_secflags | ses->overrideSecFlg;


...now, secFlags ends up being global_secflags plus some optional
sign/seal flags. Then later:

                if ((secFlags & CIFSSEC_MAY_LANMAN) ||
                        (secFlags & CIFSSEC_MAY_PLNTXT))
                        server->secType = LANMAN;
                else {
                        cERROR(1, "mount failed weak security disabled"
                                   " in /proc/fs/cifs/SecurityFlags");
                        rc = -EOPNOTSUPP;
                        goto neg_err_exit;
                }

...so, because CIFSSEC_MAY_LANMAN is now set by default, we'll not fall
into the else condition above and the mount will work by default when
it would have failed before, right? So the default behavior has
changed...or am I missing something?

-- 
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux