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

[Pavel Shilovsky <piastryyy@gmail.com>]

Looks good to me. Thanks!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky

чт, 10 дек. 2020 г. в 18:47, Steve French <smfrench@gmail.com>:
>
> updated patch
>
> https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=ced4cd5388614b39b6ef1e6d6e70c9b6044e3b43
>
> 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