Re: [PATCH] SMB3.1.1: do not log warning message if server doesn't populate salt

[Steve French <smfrench@gmail.com>]

The intent with "SMB311_CLIENT_SALT_SIZE 32" (Pavel's suggestion in
rewording it) was to make it clear it wasn't the protocol's salt size,
rather what the Linux client chose as the salt size.   I think it is
important to keep the lowest risk salt size and since every
implementation had used 32 for the SHA salt size with SMB3.1.1 - 32
seemed safer.    Changing the #define makes sense to make it more
clear that it is our Linux client choice.


On Thu, Dec 10, 2020 at 8:32 AM Tom Talpey <tom@talpey.com> wrote:
>
> tl;dr - the issue here goes deeper than Salt length
>
> On 12/9/2020 11:24 PM, Steve French wrote:
> > In the negotiate protocol preauth context, the server is not required
> > to populate the salt (although it is recommended, and done by most
>
> I don't think "recommended" is correct. The salt is optional, and that's
> because not all hashes use salt. Of course, the protocol currently
> only defines one hash algorithm, which does. But that could change.
> So delete "it is recommended,", and "most".
>
> > servers) so do not warn on mount if the salt is not 32 bytes, but
> > instead simply check that the preauth context is the minimum size
> > and that the salt would not overflow the buffer length.
>
> Suggest ending at "so do not warn.", see following.
>
> > CC: Stable <stable@vger.kernel.org>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >   fs/cifs/smb2pdu.c |  7 +++++--
> >   fs/cifs/smb2pdu.h | 14 +++++++++++---
> >   2 files changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index acb72705062d..8d572dcf330a 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -427,8 +427,8 @@ build_preauth_ctxt(struct smb2_preauth_neg_context
> > *pneg_ctxt)
> >    pneg_ctxt->ContextType = SMB2_PREAUTH_INTEGRITY_CAPABILITIES;
> >    pneg_ctxt->DataLength = cpu_to_le16(38);
> >    pneg_ctxt->HashAlgorithmCount = cpu_to_le16(1);
> > - pneg_ctxt->SaltLength = cpu_to_le16(SMB311_SALT_SIZE);
> > - get_random_bytes(pneg_ctxt->Salt, SMB311_SALT_SIZE);
> > + pneg_ctxt->SaltLength = cpu_to_le16(SMB311_CLIENT_SALT_SIZE);
> > + get_random_bytes(pneg_ctxt->Salt, SMB311_CLIENT_SALT_SIZE);
>
> Changing the salt size define is ok, but it's important to keep clear
> that "32" is not specified by the protocol, it just happens to be
> what Windows chose, for SHA512. I think it's a fine idea to do the
> same, since we're all using the same hash algorithm.
>
> Why not simply code a constant 32? Or, make the define something
> like LINUX_SMB3_SHA512_SALT_LENGTH_CHOICE?
>
> >    pneg_ctxt->HashAlgorithms = SMB2_PREAUTH_INTEGRITY_SHA512;
> >   }
> >
> > @@ -566,6 +566,9 @@ static void decode_preauth_context(struct
> > smb2_preauth_neg_context *ctxt)
> >    if (len < MIN_PREAUTH_CTXT_DATA_LEN) {
> >    pr_warn_once("server sent bad preauth context\n");
> >    return;
> > + } else if (len < MIN_PREAUTH_CTXT_DATA_LEN + le16_to_cpu(ctxt->SaltLength)) {
> > + pr_warn_once("server sent invalid SaltLength\n");
> > + return;
> >    }
> >    if (le16_to_cpu(ctxt->HashAlgorithmCount) != 1)
> >    pr_warn_once("Invalid SMB3 hash algorithm count\n");
>
> This comment applies to all three pr_warn's.
>
> Why are these here? The server is busted, sure, but the client is being
> a protocol validation test suite. And the information is really not very
> useful. How is a sysadmin supposed to respond? Finally, why are they
> pr_warn_once? If this server is broken, what about all the other ones,
> for which it suppresses the next warning?
>
> I say, if the negotiate response is invalid, abort the negotiate and
> forget throwing the book at it. No pr_warn's, or a simple generic one.
>
> > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > index fa57b03ca98c..de3127a6fc34 100644
> > --- a/fs/cifs/smb2pdu.h
> > +++ b/fs/cifs/smb2pdu.h
> > @@ -333,12 +333,20 @@ struct smb2_neg_context {
> >    /* Followed by array of data */
> >   } __packed;
> >
> > -#define SMB311_SALT_SIZE 32
> > +#define SMB311_CLIENT_SALT_SIZE 32
> >   /* Hash Algorithm Types */
> >   #define SMB2_PREAUTH_INTEGRITY_SHA512 cpu_to_le16(0x0001)
> >   #define SMB2_PREAUTH_HASH_SIZE 64
> >
> > -#define MIN_PREAUTH_CTXT_DATA_LEN (SMB311_SALT_SIZE + 6)
> > +/*
> > + * SaltLength that the server send can be zero, so the only three required
>
> It can be "any value", including zero.
>
> > + * fields (all __le16) end up six bytes total, so the minimum context data len
> > + * in the response is six.
> > + * The three required are: HashAlgorithmCount, SaltLength, and 1 HashAlgorithm
> > + * Although most servers send a SaltLength of 32 bytes, technically it is
> > + * optional.
>
> "Required" is ambiguous. All three of these fields are in the header,
> so they're all required. It's their value that's important. Obviously
> HashAlgorithmCount must be >0. SaltLength can be any value. Suggest
> removing this last sentence entirely.
>
> > + */
> > +#define MIN_PREAUTH_CTXT_DATA_LEN 6
> >   struct smb2_preauth_neg_context {
> >    __le16 ContextType; /* 1 */
> >    __le16 DataLength;
> > @@ -346,7 +354,7 @@ struct smb2_preauth_neg_context {
> >    __le16 HashAlgorithmCount; /* 1 */
> >    __le16 SaltLength;
> >    __le16 HashAlgorithms; /* HashAlgorithms[0] since only one defined */
> > - __u8 Salt[SMB311_SALT_SIZE];
> > + __u8 Salt[SMB311_CLIENT_SALT_SIZE];
>
> Incorrect! The protocol does not define this value. 32 is only an
> implementation behavior. This field is not constant, and may be 0.
>
> Tom.
>
> >   } __packed;
> >
> >   /* Encryption Algorithms Ciphers */
> >
> >



--
Thanks,

Steve