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

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

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

In the negotiate protocol preauth context, the server is not required
to populate the salt (although it is recommended, and done by 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.

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);
  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");
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
+ * 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.
+ */
+#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];
 } __packed;

 /* Encryption Algorithms Ciphers */


-- 
Thanks,

Steve

[-- Attachment #2: 0001-SMB3.1.1-do-not-log-warning-message-if-server-doesn-.patch --]
[-- Type: text/x-patch, Size: 3208 bytes --]

From 67a86f8d20a0bdb8a3832aff79137cbd29f398e7 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 9 Dec 2020 22:19:00 -0600
Subject: [PATCH] SMB3.1.1: do not log warning message if server doesn't
 populate salt

In the negotiate protocol preauth context, the server is not required
to populate the salt (although it is recommended, and done by 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.

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);
 	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");
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
+ * 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.
+ */
+#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];
 } __packed;
 
 /* Encryption Algorithms Ciphers */
-- 
2.27.0

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

[Tom Talpey]

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

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

[Steve French]

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

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

[Steve French]

If negotiate fails, due to server bug or packet corruption it would be
quite confusing if we silently fail with no indication of a server
problem.  ... And "warn_once" is used to avoid flooding the logs.   It
is quite common that these "warn_once" errors can be useful in problem
determination.  Typically when there are problems with the client
accessing a server we ask for the contents of dmesg and this kind of
thing (indications of unexpected behavior) can be helpful in problem
determination.   Currently if the preauth context is broken we can
guess that they wanted SHA512 but there is a risk if we guessed wrong
(eg server adds new algorithms in the future and breaks us due to
client or server bug) and don't note it in some way because the admin
would have nothing to report.   Logging to dmesg is something the user
generally would  not see on mount, but is useful for problem
determination and low risk.

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

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

[Steve French]

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

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

[Pavel Shilovsky]

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