linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][SMB3.1.1] add ability to send signing negotiate context
@ 2021-07-08  2:44 Steve French
  2021-07-08  4:27 ` Steve French
  0 siblings, 1 reply; 9+ messages in thread
From: Steve French @ 2021-07-08  2:44 UTC (permalink / raw)
  To: CIFS; +Cc: ronnie sahlberg

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

Support for faster packet signing (using GMAC instead of CMAC) can
now be negotiated to some newer servers, including Windows.
See MS-SMB2 section 2.2.3.17.

This patch adds support for sending the new negotiate context
with the first of three supported signing algorithms (AES-CMAC)
and decoding the response.  A followon patch will add support
for sending the other two (including AES-GMAC, which is fastest)
and changing the signing algorithm used based on what was
negotiated.

To allow the client to request GMAC signing set module parameter
"enable_negotiate_signing" to 1.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c   |  4 +++
 fs/cifs/cifsglob.h |  3 ++
 fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
 fs/cifs/smb2pdu.h  |  5 ++-
 4 files changed, 84 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9fb874dd8d24..476b07213fcd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
 bool disable_legacy_dialects; /* false by default */
 bool enable_gcm_256 = true;
 bool require_gcm_256; /* false by default */
+bool enable_negotiate_signing; /* false by default */
 unsigned int global_secflags = CIFSSEC_DEF;
 /* unsigned int ntlmv2_support = 0; */
 unsigned int sign_CIFS_PDUs = 1;
@@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
requesting strongest (256 bit) GCM encr
 module_param(require_gcm_256, bool, 0644);
 MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
encryption. Default: n/N/0");

+module_param(enable_negotiate_signing, bool, 0644);
+MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
(GMAC) packet signing. Default: n/N/0");
+
 module_param(disable_legacy_dialects, bool, 0644);
 MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
    "helpful to restrict the ability to "
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 921680fb7931..3c2e117bb926 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -667,9 +667,11 @@ struct TCP_Server_Info {
  unsigned int max_write;
  unsigned int min_offload;
  __le16 compress_algorithm;
+ __u16 signing_algorithm;
  __le16 cipher_type;
  /* save initital negprot hash */
  __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
+ bool signing_negotiated; /* true if valid signing context rcvd from server */
  bool posix_ext_supported;
  struct delayed_work reconnect; /* reconnect workqueue job */
  struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
@@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
session setup sent
 extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
 extern bool enable_gcm_256; /* allow optional negotiate of strongest
signing (aes-gcm-256) */
 extern bool require_gcm_256; /* require use of strongest signing
(aes-gcm-256) */
+extern bool enable_negotiate_signing; /* request use of faster (GMAC)
signing if available */
 extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
 extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
 extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 962826dc3316..757f145e70e5 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -433,6 +433,23 @@ build_compression_ctxt(struct
smb2_compression_capabilities_context *pneg_ctxt)
  pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
 }

+static void
+build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
+{
+ pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
+ /*
+ * Data length must be increased here if more than 3 signing algorithms
+ * sent but currently only 3 are defined. And context Data length must
+ * be rounded to multiple of 8 for some servers.
+ */
+ pneg_ctxt->DataLength =
+ cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
+ sizeof(struct smb2_neg_context), 8) * 8);
+ pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
+ pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
+ /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
+}
+
 static void
 build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
 {
@@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
        struct TCP_Server_Info *server, unsigned int *total_len)
 {
  char *pneg_ctxt;
- unsigned int ctxt_len;
+ unsigned int ctxt_len, neg_context_count;

  if (*total_len > 200) {
  /* In case length corrupted don't want to overrun smb buffer */
@@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
  *total_len += ctxt_len;
  pneg_ctxt += ctxt_len;

+ ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
+ server->hostname);
+ *total_len += ctxt_len;
+ pneg_ctxt += ctxt_len;
+
+ build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
+ *total_len += sizeof(struct smb2_posix_neg_context);
+ pneg_ctxt += sizeof(struct smb2_posix_neg_context);
+
+ neg_context_count = 4;
+
  if (server->compress_algorithm) {
  build_compression_ctxt((struct smb2_compression_capabilities_context *)
  pneg_ctxt);
@@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
  8) * 8;
  *total_len += ctxt_len;
  pneg_ctxt += ctxt_len;
- req->NegotiateContextCount = cpu_to_le16(5);
- } else
- req->NegotiateContextCount = cpu_to_le16(4);
+ neg_context_count++;
+ }

- ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
- server->hostname);
- *total_len += ctxt_len;
- pneg_ctxt += ctxt_len;
+ if (enable_negotiate_signing) {
+ pr_warn_once("requesting GMAC signing is experimental\n");
+ build_signing_ctxt((struct smb2_signing_capabilities *)
+ pneg_ctxt);
+ ctxt_len = DIV_ROUND_UP(
+ sizeof(struct smb2_signing_capabilities),
+ 8) * 8;
+ *total_len += ctxt_len;
+ pneg_ctxt += ctxt_len;
+ neg_context_count++;
+ }
+
+ /* check for and add transport_capabilities and signing capabilities */
+ req->NegotiateContextCount = cpu_to_le16(neg_context_count);

- build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
- *total_len += sizeof(struct smb2_posix_neg_context);
 }

 static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
@@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
TCP_Server_Info *server,
  return 0;
 }

+static void decode_signing_ctx(struct TCP_Server_Info *server,
+        struct smb2_signing_capabilities *pctxt)
+{
+ unsigned int len = le16_to_cpu(pctxt->DataLength);
+
+ if ((len < 4) || (len > 16)) {
+ pr_warn_once("server sent bad signing negcontext\n");
+ return;
+ }
+ if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
+ pr_warn_once("Invalid signing algorithm count\n");
+ return;
+ }
+ if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
+ pr_warn_once("unknown signing algorithm\n");
+ return;
+ }
+
+ server->signing_negotiated = true;
+ server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
+ cifs_dbg(FYI, "signing algorithm %d chosen\n",
+      server->signing_algorithm);
+}
+
+
 static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
       struct TCP_Server_Info *server,
       unsigned int len_of_smb)
@@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
smb2_negotiate_rsp *rsp,
  (struct smb2_compression_capabilities_context *)pctx);
  else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
  server->posix_ext_supported = true;
+ else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
+ decode_signing_ctx(server,
+ (struct smb2_signing_capabilities *)pctx);
  else
  cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
  le16_to_cpu(pctx->ContextType));
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index ba75e65924ac..4b27cb9105fd 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -329,7 +329,7 @@ struct smb2_neg_context {
  __le16 ContextType;
  __le16 DataLength;
  __le32 Reserved;
- /* Followed by array of data */
+ /* Followed by array of data. NOTE: some servers require padding to
8 byte boundary */
 } __packed;

 #define SMB311_LINUX_CLIENT_SALT_SIZE 32
@@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
  __u16 Padding;
  __u32 Flags;
  __le16 CompressionAlgorithms[3];
+ /* Check if pad needed */
 } __packed;

 /*
@@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
  __le16  DataLength;
  __u32 Reserved;
  __le32 Flags;
+ __u32 Pad;
 } __packed;

 /*
@@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
  __u32 Reserved;
  __le16 SigningAlgorithmCount;
  __le16 SigningAlgorithms[];
+ /*  Followed by padding to 8 byte boundary (required by some servers) */
 } __packed;

 #define POSIX_CTXT_DATA_LEN 16

-- 
Thanks,

Steve

[-- Attachment #2: 0001-SMB3.1.1-Add-support-for-negotiating-signing-algorit.patch --]
[-- Type: text/x-patch, Size: 9121 bytes --]

From 8b55a00636ba7e1be8c1659df939b6e3cb53abe2 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Mon, 5 Jul 2021 15:05:39 -0500
Subject: [PATCH] SMB3.1.1: Add support for negotiating signing algorithm

Support for faster packet signing (using GMAC instead of CMAC) can
now be negotiated to some newer servers, including Windows.
See MS-SMB2 section 2.2.3.17.

This patch adds support for sending the new negotiate context
with the first of three supported signing algorithms (AES-CMAC)
and decoding the response.  A followon patch will add support
for sending the other two (including AES-GMAC, which is fastest)
and changing the signing algorithm used based on what was
negotiated.

To allow the client to request GMAC signing set module parameter
"enable_negotiate_signing" to 1.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c   |  4 +++
 fs/cifs/cifsglob.h |  3 ++
 fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
 fs/cifs/smb2pdu.h  |  5 ++-
 4 files changed, 84 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9fb874dd8d24..476b07213fcd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
 bool disable_legacy_dialects; /* false by default */
 bool enable_gcm_256 = true;
 bool require_gcm_256; /* false by default */
+bool enable_negotiate_signing; /* false by default */
 unsigned int global_secflags = CIFSSEC_DEF;
 /* unsigned int ntlmv2_support = 0; */
 unsigned int sign_CIFS_PDUs = 1;
@@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable requesting strongest (256 bit) GCM encr
 module_param(require_gcm_256, bool, 0644);
 MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM encryption. Default: n/N/0");
 
+module_param(enable_negotiate_signing, bool, 0644);
+MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster (GMAC) packet signing. Default: n/N/0");
+
 module_param(disable_legacy_dialects, bool, 0644);
 MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
 				  "helpful to restrict the ability to "
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 921680fb7931..3c2e117bb926 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -667,9 +667,11 @@ struct TCP_Server_Info {
 	unsigned int	max_write;
 	unsigned int	min_offload;
 	__le16	compress_algorithm;
+	__u16	signing_algorithm;
 	__le16	cipher_type;
 	 /* save initital negprot hash */
 	__u8	preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
+	bool	signing_negotiated; /* true if valid signing context rcvd from server */
 	bool	posix_ext_supported;
 	struct delayed_work reconnect; /* reconnect workqueue job */
 	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
@@ -1869,6 +1871,7 @@ extern unsigned int global_secflags;	/* if on, session setup sent
 extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
 extern bool enable_gcm_256; /* allow optional negotiate of strongest signing (aes-gcm-256) */
 extern bool require_gcm_256; /* require use of strongest signing (aes-gcm-256) */
+extern bool enable_negotiate_signing; /* request use of faster (GMAC) signing if available */
 extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
 extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
 extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 962826dc3316..757f145e70e5 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -433,6 +433,23 @@ build_compression_ctxt(struct smb2_compression_capabilities_context *pneg_ctxt)
 	pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
 }
 
+static void
+build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
+{
+	pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
+	/*
+	 * Data length must be increased here if more than 3 signing algorithms
+	 * sent but currently only 3 are defined. And context Data length must
+	 * be rounded to multiple of 8 for some servers.
+	 */
+	pneg_ctxt->DataLength =
+		cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
+					sizeof(struct smb2_neg_context), 8) * 8);
+	pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
+	pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
+	/* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
+}
+
 static void
 build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
 {
@@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 		      struct TCP_Server_Info *server, unsigned int *total_len)
 {
 	char *pneg_ctxt;
-	unsigned int ctxt_len;
+	unsigned int ctxt_len, neg_context_count;
 
 	if (*total_len > 200) {
 		/* In case length corrupted don't want to overrun smb buffer */
@@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 	*total_len += ctxt_len;
 	pneg_ctxt += ctxt_len;
 
+	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
+					server->hostname);
+	*total_len += ctxt_len;
+	pneg_ctxt += ctxt_len;
+
+	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
+	*total_len += sizeof(struct smb2_posix_neg_context);
+	pneg_ctxt += sizeof(struct smb2_posix_neg_context);
+
+	neg_context_count = 4;
+
 	if (server->compress_algorithm) {
 		build_compression_ctxt((struct smb2_compression_capabilities_context *)
 				pneg_ctxt);
@@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 				8) * 8;
 		*total_len += ctxt_len;
 		pneg_ctxt += ctxt_len;
-		req->NegotiateContextCount = cpu_to_le16(5);
-	} else
-		req->NegotiateContextCount = cpu_to_le16(4);
+		neg_context_count++;
+	}
 
-	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
-					server->hostname);
-	*total_len += ctxt_len;
-	pneg_ctxt += ctxt_len;
+	if (enable_negotiate_signing) {
+		pr_warn_once("requesting GMAC signing is experimental\n");
+		build_signing_ctxt((struct smb2_signing_capabilities *)
+				pneg_ctxt);
+		ctxt_len = DIV_ROUND_UP(
+			sizeof(struct smb2_signing_capabilities),
+				8) * 8;
+		*total_len += ctxt_len;
+		pneg_ctxt += ctxt_len;
+		neg_context_count++;
+	}
+
+	/* check for and add transport_capabilities and signing capabilities */
+	req->NegotiateContextCount = cpu_to_le16(neg_context_count);
 
-	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
-	*total_len += sizeof(struct smb2_posix_neg_context);
 }
 
 static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
@@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct TCP_Server_Info *server,
 	return 0;
 }
 
+static void decode_signing_ctx(struct TCP_Server_Info *server,
+			       struct smb2_signing_capabilities *pctxt)
+{
+	unsigned int len = le16_to_cpu(pctxt->DataLength);
+
+	if ((len < 4) || (len > 16)) {
+		pr_warn_once("server sent bad signing negcontext\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
+		pr_warn_once("Invalid signing algorithm count\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
+		pr_warn_once("unknown signing algorithm\n");
+		return;
+	}
+
+	server->signing_negotiated = true;
+	server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
+	cifs_dbg(FYI, "signing algorithm %d chosen\n",
+		     server->signing_algorithm);
+}
+
+
 static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				     struct TCP_Server_Info *server,
 				     unsigned int len_of_smb)
@@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				(struct smb2_compression_capabilities_context *)pctx);
 		else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
 			server->posix_ext_supported = true;
+		else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
+			decode_signing_ctx(server,
+				(struct smb2_signing_capabilities *)pctx);
 		else
 			cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
 				le16_to_cpu(pctx->ContextType));
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index ba75e65924ac..4b27cb9105fd 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -329,7 +329,7 @@ struct smb2_neg_context {
 	__le16	ContextType;
 	__le16	DataLength;
 	__le32	Reserved;
-	/* Followed by array of data */
+	/* Followed by array of data. NOTE: some servers require padding to 8 byte boundary */
 } __packed;
 
 #define SMB311_LINUX_CLIENT_SALT_SIZE			32
@@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
 	__u16	Padding;
 	__u32	Flags;
 	__le16	CompressionAlgorithms[3];
+	/* Check if pad needed */
 } __packed;
 
 /*
@@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
 	__le16  DataLength;
 	__u32	Reserved;
 	__le32	Flags;
+	__u32	Pad;
 } __packed;
 
 /*
@@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
 	__u32	Reserved;
 	__le16	SigningAlgorithmCount;
 	__le16	SigningAlgorithms[];
+	/*  Followed by padding to 8 byte boundary (required by some servers) */
 } __packed;
 
 #define POSIX_CTXT_DATA_LEN	16
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08  2:44 [PATCH][SMB3.1.1] add ability to send signing negotiate context Steve French
@ 2021-07-08  4:27 ` Steve French
  2021-07-08  4:49   ` Steve French
  0 siblings, 1 reply; 9+ messages in thread
From: Steve French @ 2021-07-08  4:27 UTC (permalink / raw)
  To: CIFS; +Cc: ronnie sahlberg

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

v3 of patch.  Updated with additional feedback from Ronnie (to make it
more context len and datalength clearer)


On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
>
> Support for faster packet signing (using GMAC instead of CMAC) can
> now be negotiated to some newer servers, including Windows.
> See MS-SMB2 section 2.2.3.17.
>
> This patch adds support for sending the new negotiate context
> with the first of three supported signing algorithms (AES-CMAC)
> and decoding the response.  A followon patch will add support
> for sending the other two (including AES-GMAC, which is fastest)
> and changing the signing algorithm used based on what was
> negotiated.
>
> To allow the client to request GMAC signing set module parameter
> "enable_negotiate_signing" to 1.
>
> Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/cifs/cifsfs.c   |  4 +++
>  fs/cifs/cifsglob.h |  3 ++
>  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
>  fs/cifs/smb2pdu.h  |  5 ++-
>  4 files changed, 84 insertions(+), 11 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 9fb874dd8d24..476b07213fcd 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
>  bool disable_legacy_dialects; /* false by default */
>  bool enable_gcm_256 = true;
>  bool require_gcm_256; /* false by default */
> +bool enable_negotiate_signing; /* false by default */
>  unsigned int global_secflags = CIFSSEC_DEF;
>  /* unsigned int ntlmv2_support = 0; */
>  unsigned int sign_CIFS_PDUs = 1;
> @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> requesting strongest (256 bit) GCM encr
>  module_param(require_gcm_256, bool, 0644);
>  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> encryption. Default: n/N/0");
>
> +module_param(enable_negotiate_signing, bool, 0644);
> +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> (GMAC) packet signing. Default: n/N/0");
> +
>  module_param(disable_legacy_dialects, bool, 0644);
>  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
>     "helpful to restrict the ability to "
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 921680fb7931..3c2e117bb926 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -667,9 +667,11 @@ struct TCP_Server_Info {
>   unsigned int max_write;
>   unsigned int min_offload;
>   __le16 compress_algorithm;
> + __u16 signing_algorithm;
>   __le16 cipher_type;
>   /* save initital negprot hash */
>   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> + bool signing_negotiated; /* true if valid signing context rcvd from server */
>   bool posix_ext_supported;
>   struct delayed_work reconnect; /* reconnect workqueue job */
>   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> session setup sent
>  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
>  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> signing (aes-gcm-256) */
>  extern bool require_gcm_256; /* require use of strongest signing
> (aes-gcm-256) */
> +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> signing if available */
>  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
>  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
>  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 962826dc3316..757f145e70e5 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> smb2_compression_capabilities_context *pneg_ctxt)
>   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
>  }
>
> +static void
> +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> +{
> + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> + /*
> + * Data length must be increased here if more than 3 signing algorithms
> + * sent but currently only 3 are defined. And context Data length must
> + * be rounded to multiple of 8 for some servers.
> + */
> + pneg_ctxt->DataLength =
> + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> + sizeof(struct smb2_neg_context), 8) * 8);
> + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> +}
> +
>  static void
>  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
>  {
> @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
>         struct TCP_Server_Info *server, unsigned int *total_len)
>  {
>   char *pneg_ctxt;
> - unsigned int ctxt_len;
> + unsigned int ctxt_len, neg_context_count;
>
>   if (*total_len > 200) {
>   /* In case length corrupted don't want to overrun smb buffer */
> @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
>   *total_len += ctxt_len;
>   pneg_ctxt += ctxt_len;
>
> + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> + server->hostname);
> + *total_len += ctxt_len;
> + pneg_ctxt += ctxt_len;
> +
> + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> + *total_len += sizeof(struct smb2_posix_neg_context);
> + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> +
> + neg_context_count = 4;
> +
>   if (server->compress_algorithm) {
>   build_compression_ctxt((struct smb2_compression_capabilities_context *)
>   pneg_ctxt);
> @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
>   8) * 8;
>   *total_len += ctxt_len;
>   pneg_ctxt += ctxt_len;
> - req->NegotiateContextCount = cpu_to_le16(5);
> - } else
> - req->NegotiateContextCount = cpu_to_le16(4);
> + neg_context_count++;
> + }
>
> - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> - server->hostname);
> - *total_len += ctxt_len;
> - pneg_ctxt += ctxt_len;
> + if (enable_negotiate_signing) {
> + pr_warn_once("requesting GMAC signing is experimental\n");
> + build_signing_ctxt((struct smb2_signing_capabilities *)
> + pneg_ctxt);
> + ctxt_len = DIV_ROUND_UP(
> + sizeof(struct smb2_signing_capabilities),
> + 8) * 8;
> + *total_len += ctxt_len;
> + pneg_ctxt += ctxt_len;
> + neg_context_count++;
> + }
> +
> + /* check for and add transport_capabilities and signing capabilities */
> + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
>
> - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> - *total_len += sizeof(struct smb2_posix_neg_context);
>  }
>
>  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> TCP_Server_Info *server,
>   return 0;
>  }
>
> +static void decode_signing_ctx(struct TCP_Server_Info *server,
> +        struct smb2_signing_capabilities *pctxt)
> +{
> + unsigned int len = le16_to_cpu(pctxt->DataLength);
> +
> + if ((len < 4) || (len > 16)) {
> + pr_warn_once("server sent bad signing negcontext\n");
> + return;
> + }
> + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> + pr_warn_once("Invalid signing algorithm count\n");
> + return;
> + }
> + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> + pr_warn_once("unknown signing algorithm\n");
> + return;
> + }
> +
> + server->signing_negotiated = true;
> + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> +      server->signing_algorithm);
> +}
> +
> +
>  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
>        struct TCP_Server_Info *server,
>        unsigned int len_of_smb)
> @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> smb2_negotiate_rsp *rsp,
>   (struct smb2_compression_capabilities_context *)pctx);
>   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
>   server->posix_ext_supported = true;
> + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> + decode_signing_ctx(server,
> + (struct smb2_signing_capabilities *)pctx);
>   else
>   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
>   le16_to_cpu(pctx->ContextType));
> diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> index ba75e65924ac..4b27cb9105fd 100644
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -329,7 +329,7 @@ struct smb2_neg_context {
>   __le16 ContextType;
>   __le16 DataLength;
>   __le32 Reserved;
> - /* Followed by array of data */
> + /* Followed by array of data. NOTE: some servers require padding to
> 8 byte boundary */
>  } __packed;
>
>  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
>   __u16 Padding;
>   __u32 Flags;
>   __le16 CompressionAlgorithms[3];
> + /* Check if pad needed */
>  } __packed;
>
>  /*
> @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
>   __le16  DataLength;
>   __u32 Reserved;
>   __le32 Flags;
> + __u32 Pad;
>  } __packed;
>
>  /*
> @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
>   __u32 Reserved;
>   __le16 SigningAlgorithmCount;
>   __le16 SigningAlgorithms[];
> + /*  Followed by padding to 8 byte boundary (required by some servers) */
>  } __packed;
>
>  #define POSIX_CTXT_DATA_LEN 16
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-SMB3.1.1-Add-support-for-negotiating-signing-algorit.patch --]
[-- Type: text/x-patch, Size: 9193 bytes --]

From 387c856bf8b8098c2d8750376dcde289f5952ee7 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Mon, 5 Jul 2021 15:05:39 -0500
Subject: [PATCH] SMB3.1.1: Add support for negotiating signing algorithm

Support for faster packet signing (using GMAC instead of CMAC) can
now be negotiated to some newer servers, including Windows.
See MS-SMB2 section 2.2.3.17.

This patch adds support for sending the new negotiate context
with the first of three supported signing algorithms (AES-CMAC)
and decoding the response.  A followon patch will add support
for sending the other two (including AES-GMAC, which is fastest)
and changing the signing algorithm used based on what was
negotiated.

To allow the client to request GMAC signing set module parameter
"enable_negotiate_signing" to 1.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c   |  4 +++
 fs/cifs/cifsglob.h |  3 ++
 fs/cifs/smb2pdu.c  | 85 ++++++++++++++++++++++++++++++++++++++++------
 fs/cifs/smb2pdu.h  |  5 ++-
 4 files changed, 86 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9fb874dd8d24..476b07213fcd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
 bool disable_legacy_dialects; /* false by default */
 bool enable_gcm_256 = true;
 bool require_gcm_256; /* false by default */
+bool enable_negotiate_signing; /* false by default */
 unsigned int global_secflags = CIFSSEC_DEF;
 /* unsigned int ntlmv2_support = 0; */
 unsigned int sign_CIFS_PDUs = 1;
@@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable requesting strongest (256 bit) GCM encr
 module_param(require_gcm_256, bool, 0644);
 MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM encryption. Default: n/N/0");
 
+module_param(enable_negotiate_signing, bool, 0644);
+MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster (GMAC) packet signing. Default: n/N/0");
+
 module_param(disable_legacy_dialects, bool, 0644);
 MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
 				  "helpful to restrict the ability to "
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 921680fb7931..3c2e117bb926 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -667,9 +667,11 @@ struct TCP_Server_Info {
 	unsigned int	max_write;
 	unsigned int	min_offload;
 	__le16	compress_algorithm;
+	__u16	signing_algorithm;
 	__le16	cipher_type;
 	 /* save initital negprot hash */
 	__u8	preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
+	bool	signing_negotiated; /* true if valid signing context rcvd from server */
 	bool	posix_ext_supported;
 	struct delayed_work reconnect; /* reconnect workqueue job */
 	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
@@ -1869,6 +1871,7 @@ extern unsigned int global_secflags;	/* if on, session setup sent
 extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
 extern bool enable_gcm_256; /* allow optional negotiate of strongest signing (aes-gcm-256) */
 extern bool require_gcm_256; /* require use of strongest signing (aes-gcm-256) */
+extern bool enable_negotiate_signing; /* request use of faster (GMAC) signing if available */
 extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
 extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
 extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 962826dc3316..2258ff413204 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -433,6 +433,28 @@ build_compression_ctxt(struct smb2_compression_capabilities_context *pneg_ctxt)
 	pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
 }
 
+static unsigned int
+build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
+{
+	unsigned int ctxt_len = sizeof(struct smb2_signing_capabilities);
+
+	pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
+	/*
+	 * Context Data length must be rounded to multiple of 8 for some servers
+	 */
+	pneg_ctxt->DataLength = cpu_to_le16(DIV_ROUND_UP(
+				sizeof(struct smb2_signing_capabilities) -
+				sizeof(struct smb2_neg_context) +
+				(2 /* sizeof u16 */ * 1 /* SigningAlgorithmCount */), 8) * 8);
+	pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
+	pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
+
+	ctxt_len += 2 /* sizeof u16 */ * 1 /* SigningAlgorithmCount */;
+	ctxt_len = DIV_ROUND_UP(ctxt_len, 8) * 8;
+	return ctxt_len;
+	/* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
+}
+
 static void
 build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
 {
@@ -498,7 +520,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 		      struct TCP_Server_Info *server, unsigned int *total_len)
 {
 	char *pneg_ctxt;
-	unsigned int ctxt_len;
+	unsigned int ctxt_len, neg_context_count;
 
 	if (*total_len > 200) {
 		/* In case length corrupted don't want to overrun smb buffer */
@@ -525,6 +547,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 	*total_len += ctxt_len;
 	pneg_ctxt += ctxt_len;
 
+	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
+					server->hostname);
+	*total_len += ctxt_len;
+	pneg_ctxt += ctxt_len;
+
+	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
+	*total_len += sizeof(struct smb2_posix_neg_context);
+	pneg_ctxt += sizeof(struct smb2_posix_neg_context);
+
+	neg_context_count = 4;
+
 	if (server->compress_algorithm) {
 		build_compression_ctxt((struct smb2_compression_capabilities_context *)
 				pneg_ctxt);
@@ -533,17 +566,21 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 				8) * 8;
 		*total_len += ctxt_len;
 		pneg_ctxt += ctxt_len;
-		req->NegotiateContextCount = cpu_to_le16(5);
-	} else
-		req->NegotiateContextCount = cpu_to_le16(4);
+		neg_context_count++;
+	}
 
-	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
-					server->hostname);
-	*total_len += ctxt_len;
-	pneg_ctxt += ctxt_len;
+	if (enable_negotiate_signing) {
+		pr_warn_once("requesting GMAC signing is experimental\n");
+		ctxt_len = build_signing_ctxt((struct smb2_signing_capabilities *)
+				pneg_ctxt);
+		*total_len += ctxt_len;
+		pneg_ctxt += ctxt_len;
+		neg_context_count++;
+	}
+
+	/* check for and add transport_capabilities and signing capabilities */
+	req->NegotiateContextCount = cpu_to_le16(neg_context_count);
 
-	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
-	*total_len += sizeof(struct smb2_posix_neg_context);
 }
 
 static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
@@ -632,6 +669,31 @@ static int decode_encrypt_ctx(struct TCP_Server_Info *server,
 	return 0;
 }
 
+static void decode_signing_ctx(struct TCP_Server_Info *server,
+			       struct smb2_signing_capabilities *pctxt)
+{
+	unsigned int len = le16_to_cpu(pctxt->DataLength);
+
+	if ((len < 4) || (len > 16)) {
+		pr_warn_once("server sent bad signing negcontext\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
+		pr_warn_once("Invalid signing algorithm count\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
+		pr_warn_once("unknown signing algorithm\n");
+		return;
+	}
+
+	server->signing_negotiated = true;
+	server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
+	cifs_dbg(FYI, "signing algorithm %d chosen\n",
+		     server->signing_algorithm);
+}
+
+
 static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				     struct TCP_Server_Info *server,
 				     unsigned int len_of_smb)
@@ -675,6 +737,9 @@ static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				(struct smb2_compression_capabilities_context *)pctx);
 		else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
 			server->posix_ext_supported = true;
+		else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
+			decode_signing_ctx(server,
+				(struct smb2_signing_capabilities *)pctx);
 		else
 			cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
 				le16_to_cpu(pctx->ContextType));
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index ba75e65924ac..4b27cb9105fd 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -329,7 +329,7 @@ struct smb2_neg_context {
 	__le16	ContextType;
 	__le16	DataLength;
 	__le32	Reserved;
-	/* Followed by array of data */
+	/* Followed by array of data. NOTE: some servers require padding to 8 byte boundary */
 } __packed;
 
 #define SMB311_LINUX_CLIENT_SALT_SIZE			32
@@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
 	__u16	Padding;
 	__u32	Flags;
 	__le16	CompressionAlgorithms[3];
+	/* Check if pad needed */
 } __packed;
 
 /*
@@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
 	__le16  DataLength;
 	__u32	Reserved;
 	__le32	Flags;
+	__u32	Pad;
 } __packed;
 
 /*
@@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
 	__u32	Reserved;
 	__le16	SigningAlgorithmCount;
 	__le16	SigningAlgorithms[];
+	/*  Followed by padding to 8 byte boundary (required by some servers) */
 } __packed;
 
 #define POSIX_CTXT_DATA_LEN	16
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08  4:27 ` Steve French
@ 2021-07-08  4:49   ` Steve French
  2021-07-08  4:51     ` ronnie sahlberg
  0 siblings, 1 reply; 9+ messages in thread
From: Steve French @ 2021-07-08  4:49 UTC (permalink / raw)
  To: CIFS; +Cc: ronnie sahlberg

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

v4 of patch (includes minor change to set local variable "num_algs"
once to number of algorithms and use that throughout
build_signing_context to make code a little clearer)


On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
>
> v3 of patch.  Updated with additional feedback from Ronnie (to make it
> more context len and datalength clearer)
>
>
> On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> >
> > Support for faster packet signing (using GMAC instead of CMAC) can
> > now be negotiated to some newer servers, including Windows.
> > See MS-SMB2 section 2.2.3.17.
> >
> > This patch adds support for sending the new negotiate context
> > with the first of three supported signing algorithms (AES-CMAC)
> > and decoding the response.  A followon patch will add support
> > for sending the other two (including AES-GMAC, which is fastest)
> > and changing the signing algorithm used based on what was
> > negotiated.
> >
> > To allow the client to request GMAC signing set module parameter
> > "enable_negotiate_signing" to 1.
> >
> > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/cifs/cifsfs.c   |  4 +++
> >  fs/cifs/cifsglob.h |  3 ++
> >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> >  fs/cifs/smb2pdu.h  |  5 ++-
> >  4 files changed, 84 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 9fb874dd8d24..476b07213fcd 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> >  bool disable_legacy_dialects; /* false by default */
> >  bool enable_gcm_256 = true;
> >  bool require_gcm_256; /* false by default */
> > +bool enable_negotiate_signing; /* false by default */
> >  unsigned int global_secflags = CIFSSEC_DEF;
> >  /* unsigned int ntlmv2_support = 0; */
> >  unsigned int sign_CIFS_PDUs = 1;
> > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > requesting strongest (256 bit) GCM encr
> >  module_param(require_gcm_256, bool, 0644);
> >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > encryption. Default: n/N/0");
> >
> > +module_param(enable_negotiate_signing, bool, 0644);
> > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > (GMAC) packet signing. Default: n/N/0");
> > +
> >  module_param(disable_legacy_dialects, bool, 0644);
> >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> >     "helpful to restrict the ability to "
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 921680fb7931..3c2e117bb926 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> >   unsigned int max_write;
> >   unsigned int min_offload;
> >   __le16 compress_algorithm;
> > + __u16 signing_algorithm;
> >   __le16 cipher_type;
> >   /* save initital negprot hash */
> >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> >   bool posix_ext_supported;
> >   struct delayed_work reconnect; /* reconnect workqueue job */
> >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > session setup sent
> >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > signing (aes-gcm-256) */
> >  extern bool require_gcm_256; /* require use of strongest signing
> > (aes-gcm-256) */
> > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > signing if available */
> >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > index 962826dc3316..757f145e70e5 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > smb2_compression_capabilities_context *pneg_ctxt)
> >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> >  }
> >
> > +static void
> > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > +{
> > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > + /*
> > + * Data length must be increased here if more than 3 signing algorithms
> > + * sent but currently only 3 are defined. And context Data length must
> > + * be rounded to multiple of 8 for some servers.
> > + */
> > + pneg_ctxt->DataLength =
> > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > + sizeof(struct smb2_neg_context), 8) * 8);
> > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > +}
> > +
> >  static void
> >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> >  {
> > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> >         struct TCP_Server_Info *server, unsigned int *total_len)
> >  {
> >   char *pneg_ctxt;
> > - unsigned int ctxt_len;
> > + unsigned int ctxt_len, neg_context_count;
> >
> >   if (*total_len > 200) {
> >   /* In case length corrupted don't want to overrun smb buffer */
> > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> >   *total_len += ctxt_len;
> >   pneg_ctxt += ctxt_len;
> >
> > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > + server->hostname);
> > + *total_len += ctxt_len;
> > + pneg_ctxt += ctxt_len;
> > +
> > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > + *total_len += sizeof(struct smb2_posix_neg_context);
> > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > +
> > + neg_context_count = 4;
> > +
> >   if (server->compress_algorithm) {
> >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> >   pneg_ctxt);
> > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> >   8) * 8;
> >   *total_len += ctxt_len;
> >   pneg_ctxt += ctxt_len;
> > - req->NegotiateContextCount = cpu_to_le16(5);
> > - } else
> > - req->NegotiateContextCount = cpu_to_le16(4);
> > + neg_context_count++;
> > + }
> >
> > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > - server->hostname);
> > - *total_len += ctxt_len;
> > - pneg_ctxt += ctxt_len;
> > + if (enable_negotiate_signing) {
> > + pr_warn_once("requesting GMAC signing is experimental\n");
> > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > + pneg_ctxt);
> > + ctxt_len = DIV_ROUND_UP(
> > + sizeof(struct smb2_signing_capabilities),
> > + 8) * 8;
> > + *total_len += ctxt_len;
> > + pneg_ctxt += ctxt_len;
> > + neg_context_count++;
> > + }
> > +
> > + /* check for and add transport_capabilities and signing capabilities */
> > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> >
> > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > - *total_len += sizeof(struct smb2_posix_neg_context);
> >  }
> >
> >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > TCP_Server_Info *server,
> >   return 0;
> >  }
> >
> > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > +        struct smb2_signing_capabilities *pctxt)
> > +{
> > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > +
> > + if ((len < 4) || (len > 16)) {
> > + pr_warn_once("server sent bad signing negcontext\n");
> > + return;
> > + }
> > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > + pr_warn_once("Invalid signing algorithm count\n");
> > + return;
> > + }
> > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > + pr_warn_once("unknown signing algorithm\n");
> > + return;
> > + }
> > +
> > + server->signing_negotiated = true;
> > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > +      server->signing_algorithm);
> > +}
> > +
> > +
> >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> >        struct TCP_Server_Info *server,
> >        unsigned int len_of_smb)
> > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > smb2_negotiate_rsp *rsp,
> >   (struct smb2_compression_capabilities_context *)pctx);
> >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> >   server->posix_ext_supported = true;
> > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > + decode_signing_ctx(server,
> > + (struct smb2_signing_capabilities *)pctx);
> >   else
> >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> >   le16_to_cpu(pctx->ContextType));
> > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > index ba75e65924ac..4b27cb9105fd 100644
> > --- a/fs/cifs/smb2pdu.h
> > +++ b/fs/cifs/smb2pdu.h
> > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> >   __le16 ContextType;
> >   __le16 DataLength;
> >   __le32 Reserved;
> > - /* Followed by array of data */
> > + /* Followed by array of data. NOTE: some servers require padding to
> > 8 byte boundary */
> >  } __packed;
> >
> >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> >   __u16 Padding;
> >   __u32 Flags;
> >   __le16 CompressionAlgorithms[3];
> > + /* Check if pad needed */
> >  } __packed;
> >
> >  /*
> > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> >   __le16  DataLength;
> >   __u32 Reserved;
> >   __le32 Flags;
> > + __u32 Pad;
> >  } __packed;
> >
> >  /*
> > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> >   __u32 Reserved;
> >   __le16 SigningAlgorithmCount;
> >   __le16 SigningAlgorithms[];
> > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> >  } __packed;
> >
> >  #define POSIX_CTXT_DATA_LEN 16
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-SMB3.1.1-Add-support-for-negotiating-signing-algorit.patch --]
[-- Type: text/x-patch, Size: 9230 bytes --]

From de96853dacc7cd319d5fddab3cb5ada8f106c52c Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Mon, 5 Jul 2021 15:05:39 -0500
Subject: [PATCH] SMB3.1.1: Add support for negotiating signing algorithm

Support for faster packet signing (using GMAC instead of CMAC) can
now be negotiated to some newer servers, including Windows.
See MS-SMB2 section 2.2.3.17.

This patch adds support for sending the new negotiate context
with the first of three supported signing algorithms (AES-CMAC)
and decoding the response.  A followon patch will add support
for sending the other two (including AES-GMAC, which is fastest)
and changing the signing algorithm used based on what was
negotiated.

To allow the client to request GMAC signing set module parameter
"enable_negotiate_signing" to 1.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c   |  4 +++
 fs/cifs/cifsglob.h |  3 ++
 fs/cifs/smb2pdu.c  | 86 ++++++++++++++++++++++++++++++++++++++++------
 fs/cifs/smb2pdu.h  |  5 ++-
 4 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9fb874dd8d24..476b07213fcd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
 bool disable_legacy_dialects; /* false by default */
 bool enable_gcm_256 = true;
 bool require_gcm_256; /* false by default */
+bool enable_negotiate_signing; /* false by default */
 unsigned int global_secflags = CIFSSEC_DEF;
 /* unsigned int ntlmv2_support = 0; */
 unsigned int sign_CIFS_PDUs = 1;
@@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable requesting strongest (256 bit) GCM encr
 module_param(require_gcm_256, bool, 0644);
 MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM encryption. Default: n/N/0");
 
+module_param(enable_negotiate_signing, bool, 0644);
+MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster (GMAC) packet signing. Default: n/N/0");
+
 module_param(disable_legacy_dialects, bool, 0644);
 MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
 				  "helpful to restrict the ability to "
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 921680fb7931..3c2e117bb926 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -667,9 +667,11 @@ struct TCP_Server_Info {
 	unsigned int	max_write;
 	unsigned int	min_offload;
 	__le16	compress_algorithm;
+	__u16	signing_algorithm;
 	__le16	cipher_type;
 	 /* save initital negprot hash */
 	__u8	preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
+	bool	signing_negotiated; /* true if valid signing context rcvd from server */
 	bool	posix_ext_supported;
 	struct delayed_work reconnect; /* reconnect workqueue job */
 	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
@@ -1869,6 +1871,7 @@ extern unsigned int global_secflags;	/* if on, session setup sent
 extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
 extern bool enable_gcm_256; /* allow optional negotiate of strongest signing (aes-gcm-256) */
 extern bool require_gcm_256; /* require use of strongest signing (aes-gcm-256) */
+extern bool enable_negotiate_signing; /* request use of faster (GMAC) signing if available */
 extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
 extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
 extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 962826dc3316..c1e76eb12109 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -433,6 +433,29 @@ build_compression_ctxt(struct smb2_compression_capabilities_context *pneg_ctxt)
 	pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
 }
 
+static unsigned int
+build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
+{
+	unsigned int ctxt_len = sizeof(struct smb2_signing_capabilities);
+	unsigned short num_algs = 1; /* number of signing algorithms sent */
+
+	pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
+	/*
+	 * Context Data length must be rounded to multiple of 8 for some servers
+	 */
+	pneg_ctxt->DataLength = cpu_to_le16(DIV_ROUND_UP(
+				sizeof(struct smb2_signing_capabilities) -
+				sizeof(struct smb2_neg_context) +
+				(num_algs * 2 /* sizeof u16 */), 8) * 8);
+	pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(num_algs);
+	pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
+
+	ctxt_len += 2 /* sizeof le16 */ * num_algs;
+	ctxt_len = DIV_ROUND_UP(ctxt_len, 8) * 8;
+	return ctxt_len;
+	/* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
+}
+
 static void
 build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
 {
@@ -498,7 +521,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 		      struct TCP_Server_Info *server, unsigned int *total_len)
 {
 	char *pneg_ctxt;
-	unsigned int ctxt_len;
+	unsigned int ctxt_len, neg_context_count;
 
 	if (*total_len > 200) {
 		/* In case length corrupted don't want to overrun smb buffer */
@@ -525,6 +548,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 	*total_len += ctxt_len;
 	pneg_ctxt += ctxt_len;
 
+	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
+					server->hostname);
+	*total_len += ctxt_len;
+	pneg_ctxt += ctxt_len;
+
+	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
+	*total_len += sizeof(struct smb2_posix_neg_context);
+	pneg_ctxt += sizeof(struct smb2_posix_neg_context);
+
+	neg_context_count = 4;
+
 	if (server->compress_algorithm) {
 		build_compression_ctxt((struct smb2_compression_capabilities_context *)
 				pneg_ctxt);
@@ -533,17 +567,21 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 				8) * 8;
 		*total_len += ctxt_len;
 		pneg_ctxt += ctxt_len;
-		req->NegotiateContextCount = cpu_to_le16(5);
-	} else
-		req->NegotiateContextCount = cpu_to_le16(4);
+		neg_context_count++;
+	}
 
-	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
-					server->hostname);
-	*total_len += ctxt_len;
-	pneg_ctxt += ctxt_len;
+	if (enable_negotiate_signing) {
+		pr_warn_once("requesting GMAC signing is experimental\n");
+		ctxt_len = build_signing_ctxt((struct smb2_signing_capabilities *)
+				pneg_ctxt);
+		*total_len += ctxt_len;
+		pneg_ctxt += ctxt_len;
+		neg_context_count++;
+	}
+
+	/* check for and add transport_capabilities and signing capabilities */
+	req->NegotiateContextCount = cpu_to_le16(neg_context_count);
 
-	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
-	*total_len += sizeof(struct smb2_posix_neg_context);
 }
 
 static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
@@ -632,6 +670,31 @@ static int decode_encrypt_ctx(struct TCP_Server_Info *server,
 	return 0;
 }
 
+static void decode_signing_ctx(struct TCP_Server_Info *server,
+			       struct smb2_signing_capabilities *pctxt)
+{
+	unsigned int len = le16_to_cpu(pctxt->DataLength);
+
+	if ((len < 4) || (len > 16)) {
+		pr_warn_once("server sent bad signing negcontext\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
+		pr_warn_once("Invalid signing algorithm count\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
+		pr_warn_once("unknown signing algorithm\n");
+		return;
+	}
+
+	server->signing_negotiated = true;
+	server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
+	cifs_dbg(FYI, "signing algorithm %d chosen\n",
+		     server->signing_algorithm);
+}
+
+
 static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				     struct TCP_Server_Info *server,
 				     unsigned int len_of_smb)
@@ -675,6 +738,9 @@ static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				(struct smb2_compression_capabilities_context *)pctx);
 		else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
 			server->posix_ext_supported = true;
+		else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
+			decode_signing_ctx(server,
+				(struct smb2_signing_capabilities *)pctx);
 		else
 			cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
 				le16_to_cpu(pctx->ContextType));
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index ba75e65924ac..4b27cb9105fd 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -329,7 +329,7 @@ struct smb2_neg_context {
 	__le16	ContextType;
 	__le16	DataLength;
 	__le32	Reserved;
-	/* Followed by array of data */
+	/* Followed by array of data. NOTE: some servers require padding to 8 byte boundary */
 } __packed;
 
 #define SMB311_LINUX_CLIENT_SALT_SIZE			32
@@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
 	__u16	Padding;
 	__u32	Flags;
 	__le16	CompressionAlgorithms[3];
+	/* Check if pad needed */
 } __packed;
 
 /*
@@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
 	__le16  DataLength;
 	__u32	Reserved;
 	__le32	Flags;
+	__u32	Pad;
 } __packed;
 
 /*
@@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
 	__u32	Reserved;
 	__le16	SigningAlgorithmCount;
 	__le16	SigningAlgorithms[];
+	/*  Followed by padding to 8 byte boundary (required by some servers) */
 } __packed;
 
 #define POSIX_CTXT_DATA_LEN	16
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08  4:49   ` Steve French
@ 2021-07-08  4:51     ` ronnie sahlberg
  2021-07-08 22:05       ` Pavel Shilovsky
  0 siblings, 1 reply; 9+ messages in thread
From: ronnie sahlberg @ 2021-07-08  4:51 UTC (permalink / raw)
  To: Steve French; +Cc: CIFS

lgtm

On Thu, Jul 8, 2021 at 2:49 PM Steve French <smfrench@gmail.com> wrote:
>
> v4 of patch (includes minor change to set local variable "num_algs"
> once to number of algorithms and use that throughout
> build_signing_context to make code a little clearer)
>
>
> On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
> >
> > v3 of patch.  Updated with additional feedback from Ronnie (to make it
> > more context len and datalength clearer)
> >
> >
> > On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > Support for faster packet signing (using GMAC instead of CMAC) can
> > > now be negotiated to some newer servers, including Windows.
> > > See MS-SMB2 section 2.2.3.17.
> > >
> > > This patch adds support for sending the new negotiate context
> > > with the first of three supported signing algorithms (AES-CMAC)
> > > and decoding the response.  A followon patch will add support
> > > for sending the other two (including AES-GMAC, which is fastest)
> > > and changing the signing algorithm used based on what was
> > > negotiated.
> > >
> > > To allow the client to request GMAC signing set module parameter
> > > "enable_negotiate_signing" to 1.
> > >
> > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > ---
> > >  fs/cifs/cifsfs.c   |  4 +++
> > >  fs/cifs/cifsglob.h |  3 ++
> > >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> > >  fs/cifs/smb2pdu.h  |  5 ++-
> > >  4 files changed, 84 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index 9fb874dd8d24..476b07213fcd 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> > >  bool disable_legacy_dialects; /* false by default */
> > >  bool enable_gcm_256 = true;
> > >  bool require_gcm_256; /* false by default */
> > > +bool enable_negotiate_signing; /* false by default */
> > >  unsigned int global_secflags = CIFSSEC_DEF;
> > >  /* unsigned int ntlmv2_support = 0; */
> > >  unsigned int sign_CIFS_PDUs = 1;
> > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > requesting strongest (256 bit) GCM encr
> > >  module_param(require_gcm_256, bool, 0644);
> > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > encryption. Default: n/N/0");
> > >
> > > +module_param(enable_negotiate_signing, bool, 0644);
> > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > (GMAC) packet signing. Default: n/N/0");
> > > +
> > >  module_param(disable_legacy_dialects, bool, 0644);
> > >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> > >     "helpful to restrict the ability to "
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index 921680fb7931..3c2e117bb926 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> > >   unsigned int max_write;
> > >   unsigned int min_offload;
> > >   __le16 compress_algorithm;
> > > + __u16 signing_algorithm;
> > >   __le16 cipher_type;
> > >   /* save initital negprot hash */
> > >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> > >   bool posix_ext_supported;
> > >   struct delayed_work reconnect; /* reconnect workqueue job */
> > >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > > session setup sent
> > >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> > >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > > signing (aes-gcm-256) */
> > >  extern bool require_gcm_256; /* require use of strongest signing
> > > (aes-gcm-256) */
> > > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > > signing if available */
> > >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> > >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> > >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > index 962826dc3316..757f145e70e5 100644
> > > --- a/fs/cifs/smb2pdu.c
> > > +++ b/fs/cifs/smb2pdu.c
> > > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > > smb2_compression_capabilities_context *pneg_ctxt)
> > >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> > >  }
> > >
> > > +static void
> > > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > > +{
> > > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > > + /*
> > > + * Data length must be increased here if more than 3 signing algorithms
> > > + * sent but currently only 3 are defined. And context Data length must
> > > + * be rounded to multiple of 8 for some servers.
> > > + */
> > > + pneg_ctxt->DataLength =
> > > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > > + sizeof(struct smb2_neg_context), 8) * 8);
> > > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > > +}
> > > +
> > >  static void
> > >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> > >  {
> > > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > >         struct TCP_Server_Info *server, unsigned int *total_len)
> > >  {
> > >   char *pneg_ctxt;
> > > - unsigned int ctxt_len;
> > > + unsigned int ctxt_len, neg_context_count;
> > >
> > >   if (*total_len > 200) {
> > >   /* In case length corrupted don't want to overrun smb buffer */
> > > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > >   *total_len += ctxt_len;
> > >   pneg_ctxt += ctxt_len;
> > >
> > > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > + server->hostname);
> > > + *total_len += ctxt_len;
> > > + pneg_ctxt += ctxt_len;
> > > +
> > > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > + *total_len += sizeof(struct smb2_posix_neg_context);
> > > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > > +
> > > + neg_context_count = 4;
> > > +
> > >   if (server->compress_algorithm) {
> > >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> > >   pneg_ctxt);
> > > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > >   8) * 8;
> > >   *total_len += ctxt_len;
> > >   pneg_ctxt += ctxt_len;
> > > - req->NegotiateContextCount = cpu_to_le16(5);
> > > - } else
> > > - req->NegotiateContextCount = cpu_to_le16(4);
> > > + neg_context_count++;
> > > + }
> > >
> > > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > - server->hostname);
> > > - *total_len += ctxt_len;
> > > - pneg_ctxt += ctxt_len;
> > > + if (enable_negotiate_signing) {
> > > + pr_warn_once("requesting GMAC signing is experimental\n");
> > > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > > + pneg_ctxt);
> > > + ctxt_len = DIV_ROUND_UP(
> > > + sizeof(struct smb2_signing_capabilities),
> > > + 8) * 8;
> > > + *total_len += ctxt_len;
> > > + pneg_ctxt += ctxt_len;
> > > + neg_context_count++;
> > > + }
> > > +
> > > + /* check for and add transport_capabilities and signing capabilities */
> > > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> > >
> > > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > - *total_len += sizeof(struct smb2_posix_neg_context);
> > >  }
> > >
> > >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > > TCP_Server_Info *server,
> > >   return 0;
> > >  }
> > >
> > > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > > +        struct smb2_signing_capabilities *pctxt)
> > > +{
> > > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > > +
> > > + if ((len < 4) || (len > 16)) {
> > > + pr_warn_once("server sent bad signing negcontext\n");
> > > + return;
> > > + }
> > > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > > + pr_warn_once("Invalid signing algorithm count\n");
> > > + return;
> > > + }
> > > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > > + pr_warn_once("unknown signing algorithm\n");
> > > + return;
> > > + }
> > > +
> > > + server->signing_negotiated = true;
> > > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > > +      server->signing_algorithm);
> > > +}
> > > +
> > > +
> > >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> > >        struct TCP_Server_Info *server,
> > >        unsigned int len_of_smb)
> > > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > > smb2_negotiate_rsp *rsp,
> > >   (struct smb2_compression_capabilities_context *)pctx);
> > >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> > >   server->posix_ext_supported = true;
> > > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > > + decode_signing_ctx(server,
> > > + (struct smb2_signing_capabilities *)pctx);
> > >   else
> > >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> > >   le16_to_cpu(pctx->ContextType));
> > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > > index ba75e65924ac..4b27cb9105fd 100644
> > > --- a/fs/cifs/smb2pdu.h
> > > +++ b/fs/cifs/smb2pdu.h
> > > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> > >   __le16 ContextType;
> > >   __le16 DataLength;
> > >   __le32 Reserved;
> > > - /* Followed by array of data */
> > > + /* Followed by array of data. NOTE: some servers require padding to
> > > 8 byte boundary */
> > >  } __packed;
> > >
> > >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> > >   __u16 Padding;
> > >   __u32 Flags;
> > >   __le16 CompressionAlgorithms[3];
> > > + /* Check if pad needed */
> > >  } __packed;
> > >
> > >  /*
> > > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> > >   __le16  DataLength;
> > >   __u32 Reserved;
> > >   __le32 Flags;
> > > + __u32 Pad;
> > >  } __packed;
> > >
> > >  /*
> > > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> > >   __u32 Reserved;
> > >   __le16 SigningAlgorithmCount;
> > >   __le16 SigningAlgorithms[];
> > > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> > >  } __packed;
> > >
> > >  #define POSIX_CTXT_DATA_LEN 16
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08  4:51     ` ronnie sahlberg
@ 2021-07-08 22:05       ` Pavel Shilovsky
  2021-07-08 23:19         ` Steve French
  0 siblings, 1 reply; 9+ messages in thread
From: Pavel Shilovsky @ 2021-07-08 22:05 UTC (permalink / raw)
  To: ronnie sahlberg; +Cc: Steve French, CIFS

@@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
requesting strongest (256 bit) GCM encr
 module_param(require_gcm_256, bool, 0644);
 MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
encryption. Default: n/N/0");

+module_param(enable_negotiate_signing, bool, 0644);
+MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
(GMAC) packet signing. Default: n/N/0");
+

s/enable_GMAC_signing/enable_negotiate_signing/ ?

Also the description here is misleading: this param enables sending
the signing context not requesting GMAC which is not supported yet.


+    if (enable_negotiate_signing) {
+        pr_warn_once("requesting GMAC signing is experimental\n");

Here as well - we are not requesting GMAC here, we are sending the
signing context with CMAC. I don't think this should be marked as
experimental.

--
Best regards,
Pavel Shilovsky

ср, 7 июл. 2021 г. в 21:52, ronnie sahlberg <ronniesahlberg@gmail.com>:
>
> lgtm
>
> On Thu, Jul 8, 2021 at 2:49 PM Steve French <smfrench@gmail.com> wrote:
> >
> > v4 of patch (includes minor change to set local variable "num_algs"
> > once to number of algorithms and use that throughout
> > build_signing_context to make code a little clearer)
> >
> >
> > On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > v3 of patch.  Updated with additional feedback from Ronnie (to make it
> > > more context len and datalength clearer)
> > >
> > >
> > > On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > Support for faster packet signing (using GMAC instead of CMAC) can
> > > > now be negotiated to some newer servers, including Windows.
> > > > See MS-SMB2 section 2.2.3.17.
> > > >
> > > > This patch adds support for sending the new negotiate context
> > > > with the first of three supported signing algorithms (AES-CMAC)
> > > > and decoding the response.  A followon patch will add support
> > > > for sending the other two (including AES-GMAC, which is fastest)
> > > > and changing the signing algorithm used based on what was
> > > > negotiated.
> > > >
> > > > To allow the client to request GMAC signing set module parameter
> > > > "enable_negotiate_signing" to 1.
> > > >
> > > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > > ---
> > > >  fs/cifs/cifsfs.c   |  4 +++
> > > >  fs/cifs/cifsglob.h |  3 ++
> > > >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> > > >  fs/cifs/smb2pdu.h  |  5 ++-
> > > >  4 files changed, 84 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > > index 9fb874dd8d24..476b07213fcd 100644
> > > > --- a/fs/cifs/cifsfs.c
> > > > +++ b/fs/cifs/cifsfs.c
> > > > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> > > >  bool disable_legacy_dialects; /* false by default */
> > > >  bool enable_gcm_256 = true;
> > > >  bool require_gcm_256; /* false by default */
> > > > +bool enable_negotiate_signing; /* false by default */
> > > >  unsigned int global_secflags = CIFSSEC_DEF;
> > > >  /* unsigned int ntlmv2_support = 0; */
> > > >  unsigned int sign_CIFS_PDUs = 1;
> > > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > > requesting strongest (256 bit) GCM encr
> > > >  module_param(require_gcm_256, bool, 0644);
> > > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > > encryption. Default: n/N/0");
> > > >
> > > > +module_param(enable_negotiate_signing, bool, 0644);
> > > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > > (GMAC) packet signing. Default: n/N/0");
> > > > +
> > > >  module_param(disable_legacy_dialects, bool, 0644);
> > > >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> > > >     "helpful to restrict the ability to "
> > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > > index 921680fb7931..3c2e117bb926 100644
> > > > --- a/fs/cifs/cifsglob.h
> > > > +++ b/fs/cifs/cifsglob.h
> > > > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> > > >   unsigned int max_write;
> > > >   unsigned int min_offload;
> > > >   __le16 compress_algorithm;
> > > > + __u16 signing_algorithm;
> > > >   __le16 cipher_type;
> > > >   /* save initital negprot hash */
> > > >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > > > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> > > >   bool posix_ext_supported;
> > > >   struct delayed_work reconnect; /* reconnect workqueue job */
> > > >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > > > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > > > session setup sent
> > > >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> > > >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > > > signing (aes-gcm-256) */
> > > >  extern bool require_gcm_256; /* require use of strongest signing
> > > > (aes-gcm-256) */
> > > > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > > > signing if available */
> > > >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> > > >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> > > >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > index 962826dc3316..757f145e70e5 100644
> > > > --- a/fs/cifs/smb2pdu.c
> > > > +++ b/fs/cifs/smb2pdu.c
> > > > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > > > smb2_compression_capabilities_context *pneg_ctxt)
> > > >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> > > >  }
> > > >
> > > > +static void
> > > > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > > > +{
> > > > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > > > + /*
> > > > + * Data length must be increased here if more than 3 signing algorithms
> > > > + * sent but currently only 3 are defined. And context Data length must
> > > > + * be rounded to multiple of 8 for some servers.
> > > > + */
> > > > + pneg_ctxt->DataLength =
> > > > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > > > + sizeof(struct smb2_neg_context), 8) * 8);
> > > > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > > > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > > > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > > > +}
> > > > +
> > > >  static void
> > > >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> > > >  {
> > > > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > >         struct TCP_Server_Info *server, unsigned int *total_len)
> > > >  {
> > > >   char *pneg_ctxt;
> > > > - unsigned int ctxt_len;
> > > > + unsigned int ctxt_len, neg_context_count;
> > > >
> > > >   if (*total_len > 200) {
> > > >   /* In case length corrupted don't want to overrun smb buffer */
> > > > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > >   *total_len += ctxt_len;
> > > >   pneg_ctxt += ctxt_len;
> > > >
> > > > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > + server->hostname);
> > > > + *total_len += ctxt_len;
> > > > + pneg_ctxt += ctxt_len;
> > > > +
> > > > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > + *total_len += sizeof(struct smb2_posix_neg_context);
> > > > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > > > +
> > > > + neg_context_count = 4;
> > > > +
> > > >   if (server->compress_algorithm) {
> > > >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> > > >   pneg_ctxt);
> > > > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > >   8) * 8;
> > > >   *total_len += ctxt_len;
> > > >   pneg_ctxt += ctxt_len;
> > > > - req->NegotiateContextCount = cpu_to_le16(5);
> > > > - } else
> > > > - req->NegotiateContextCount = cpu_to_le16(4);
> > > > + neg_context_count++;
> > > > + }
> > > >
> > > > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > - server->hostname);
> > > > - *total_len += ctxt_len;
> > > > - pneg_ctxt += ctxt_len;
> > > > + if (enable_negotiate_signing) {
> > > > + pr_warn_once("requesting GMAC signing is experimental\n");
> > > > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > > > + pneg_ctxt);
> > > > + ctxt_len = DIV_ROUND_UP(
> > > > + sizeof(struct smb2_signing_capabilities),
> > > > + 8) * 8;
> > > > + *total_len += ctxt_len;
> > > > + pneg_ctxt += ctxt_len;
> > > > + neg_context_count++;
> > > > + }
> > > > +
> > > > + /* check for and add transport_capabilities and signing capabilities */
> > > > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> > > >
> > > > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > - *total_len += sizeof(struct smb2_posix_neg_context);
> > > >  }
> > > >
> > > >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > > > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > > > TCP_Server_Info *server,
> > > >   return 0;
> > > >  }
> > > >
> > > > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > > > +        struct smb2_signing_capabilities *pctxt)
> > > > +{
> > > > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > > > +
> > > > + if ((len < 4) || (len > 16)) {
> > > > + pr_warn_once("server sent bad signing negcontext\n");
> > > > + return;
> > > > + }
> > > > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > > > + pr_warn_once("Invalid signing algorithm count\n");
> > > > + return;
> > > > + }
> > > > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > > > + pr_warn_once("unknown signing algorithm\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + server->signing_negotiated = true;
> > > > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > > > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > > > +      server->signing_algorithm);
> > > > +}
> > > > +
> > > > +
> > > >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> > > >        struct TCP_Server_Info *server,
> > > >        unsigned int len_of_smb)
> > > > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > > > smb2_negotiate_rsp *rsp,
> > > >   (struct smb2_compression_capabilities_context *)pctx);
> > > >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> > > >   server->posix_ext_supported = true;
> > > > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > > > + decode_signing_ctx(server,
> > > > + (struct smb2_signing_capabilities *)pctx);
> > > >   else
> > > >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> > > >   le16_to_cpu(pctx->ContextType));
> > > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > > > index ba75e65924ac..4b27cb9105fd 100644
> > > > --- a/fs/cifs/smb2pdu.h
> > > > +++ b/fs/cifs/smb2pdu.h
> > > > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> > > >   __le16 ContextType;
> > > >   __le16 DataLength;
> > > >   __le32 Reserved;
> > > > - /* Followed by array of data */
> > > > + /* Followed by array of data. NOTE: some servers require padding to
> > > > 8 byte boundary */
> > > >  } __packed;
> > > >
> > > >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > > > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> > > >   __u16 Padding;
> > > >   __u32 Flags;
> > > >   __le16 CompressionAlgorithms[3];
> > > > + /* Check if pad needed */
> > > >  } __packed;
> > > >
> > > >  /*
> > > > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> > > >   __le16  DataLength;
> > > >   __u32 Reserved;
> > > >   __le32 Flags;
> > > > + __u32 Pad;
> > > >  } __packed;
> > > >
> > > >  /*
> > > > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> > > >   __u32 Reserved;
> > > >   __le16 SigningAlgorithmCount;
> > > >   __le16 SigningAlgorithms[];
> > > > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> > > >  } __packed;
> > > >
> > > >  #define POSIX_CTXT_DATA_LEN 16
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08 22:05       ` Pavel Shilovsky
@ 2021-07-08 23:19         ` Steve French
  2021-07-08 23:29           ` Steve French
  0 siblings, 1 reply; 9+ messages in thread
From: Steve French @ 2021-07-08 23:19 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: ronnie sahlberg, CIFS

It is a little tricky to figure out good wording here.

"enable_negotiate_signing" (eventually) could end up requesting all 3
... which means that the server could choose SHA256 over GMAC over
CMAC.  Maybe I should avoid all mention of GMAC.

My reason for noting "GMAC is experimental" is that if GMAC is
negotiated it is less tested than the other two (since fewer servers
currently support it compared to the other two which are broadly
supported and thus already tested with cifs.ko) - but the effect of
the patch is to let the server choose which algorithm it prefers ...
so perhaps I should avoid mentioning GMAC here.

On Thu, Jul 8, 2021 at 5:06 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> requesting strongest (256 bit) GCM encr
>  module_param(require_gcm_256, bool, 0644);
>  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> encryption. Default: n/N/0");
>
> +module_param(enable_negotiate_signing, bool, 0644);
> +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> (GMAC) packet signing. Default: n/N/0");
> +
>
> s/enable_GMAC_signing/enable_negotiate_signing/ ?
>
> Also the description here is misleading: this param enables sending
> the signing context not requesting GMAC which is not supported yet.
>
>
> +    if (enable_negotiate_signing) {
> +        pr_warn_once("requesting GMAC signing is experimental\n");
>
> Here as well - we are not requesting GMAC here, we are sending the
> signing context with CMAC. I don't think this should be marked as
> experimental.
>
> --
> Best regards,
> Pavel Shilovsky
>
> ср, 7 июл. 2021 г. в 21:52, ronnie sahlberg <ronniesahlberg@gmail.com>:
> >
> > lgtm
> >
> > On Thu, Jul 8, 2021 at 2:49 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > v4 of patch (includes minor change to set local variable "num_algs"
> > > once to number of algorithms and use that throughout
> > > build_signing_context to make code a little clearer)
> > >
> > >
> > > On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > v3 of patch.  Updated with additional feedback from Ronnie (to make it
> > > > more context len and datalength clearer)
> > > >
> > > >
> > > > On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> > > > >
> > > > > Support for faster packet signing (using GMAC instead of CMAC) can
> > > > > now be negotiated to some newer servers, including Windows.
> > > > > See MS-SMB2 section 2.2.3.17.
> > > > >
> > > > > This patch adds support for sending the new negotiate context
> > > > > with the first of three supported signing algorithms (AES-CMAC)
> > > > > and decoding the response.  A followon patch will add support
> > > > > for sending the other two (including AES-GMAC, which is fastest)
> > > > > and changing the signing algorithm used based on what was
> > > > > negotiated.
> > > > >
> > > > > To allow the client to request GMAC signing set module parameter
> > > > > "enable_negotiate_signing" to 1.
> > > > >
> > > > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > > > ---
> > > > >  fs/cifs/cifsfs.c   |  4 +++
> > > > >  fs/cifs/cifsglob.h |  3 ++
> > > > >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> > > > >  fs/cifs/smb2pdu.h  |  5 ++-
> > > > >  4 files changed, 84 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > > > index 9fb874dd8d24..476b07213fcd 100644
> > > > > --- a/fs/cifs/cifsfs.c
> > > > > +++ b/fs/cifs/cifsfs.c
> > > > > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> > > > >  bool disable_legacy_dialects; /* false by default */
> > > > >  bool enable_gcm_256 = true;
> > > > >  bool require_gcm_256; /* false by default */
> > > > > +bool enable_negotiate_signing; /* false by default */
> > > > >  unsigned int global_secflags = CIFSSEC_DEF;
> > > > >  /* unsigned int ntlmv2_support = 0; */
> > > > >  unsigned int sign_CIFS_PDUs = 1;
> > > > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > > > requesting strongest (256 bit) GCM encr
> > > > >  module_param(require_gcm_256, bool, 0644);
> > > > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > > > encryption. Default: n/N/0");
> > > > >
> > > > > +module_param(enable_negotiate_signing, bool, 0644);
> > > > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > > > (GMAC) packet signing. Default: n/N/0");
> > > > > +
> > > > >  module_param(disable_legacy_dialects, bool, 0644);
> > > > >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> > > > >     "helpful to restrict the ability to "
> > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > > > index 921680fb7931..3c2e117bb926 100644
> > > > > --- a/fs/cifs/cifsglob.h
> > > > > +++ b/fs/cifs/cifsglob.h
> > > > > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> > > > >   unsigned int max_write;
> > > > >   unsigned int min_offload;
> > > > >   __le16 compress_algorithm;
> > > > > + __u16 signing_algorithm;
> > > > >   __le16 cipher_type;
> > > > >   /* save initital negprot hash */
> > > > >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > > > > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> > > > >   bool posix_ext_supported;
> > > > >   struct delayed_work reconnect; /* reconnect workqueue job */
> > > > >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > > > > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > > > > session setup sent
> > > > >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> > > > >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > > > > signing (aes-gcm-256) */
> > > > >  extern bool require_gcm_256; /* require use of strongest signing
> > > > > (aes-gcm-256) */
> > > > > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > > > > signing if available */
> > > > >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> > > > >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> > > > >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > > index 962826dc3316..757f145e70e5 100644
> > > > > --- a/fs/cifs/smb2pdu.c
> > > > > +++ b/fs/cifs/smb2pdu.c
> > > > > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > > > > smb2_compression_capabilities_context *pneg_ctxt)
> > > > >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> > > > >  }
> > > > >
> > > > > +static void
> > > > > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > > > > +{
> > > > > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > > > > + /*
> > > > > + * Data length must be increased here if more than 3 signing algorithms
> > > > > + * sent but currently only 3 are defined. And context Data length must
> > > > > + * be rounded to multiple of 8 for some servers.
> > > > > + */
> > > > > + pneg_ctxt->DataLength =
> > > > > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > > > > + sizeof(struct smb2_neg_context), 8) * 8);
> > > > > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > > > > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > > > > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> > > > >  {
> > > > > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > >         struct TCP_Server_Info *server, unsigned int *total_len)
> > > > >  {
> > > > >   char *pneg_ctxt;
> > > > > - unsigned int ctxt_len;
> > > > > + unsigned int ctxt_len, neg_context_count;
> > > > >
> > > > >   if (*total_len > 200) {
> > > > >   /* In case length corrupted don't want to overrun smb buffer */
> > > > > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > >   *total_len += ctxt_len;
> > > > >   pneg_ctxt += ctxt_len;
> > > > >
> > > > > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > + server->hostname);
> > > > > + *total_len += ctxt_len;
> > > > > + pneg_ctxt += ctxt_len;
> > > > > +
> > > > > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > + *total_len += sizeof(struct smb2_posix_neg_context);
> > > > > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > > > > +
> > > > > + neg_context_count = 4;
> > > > > +
> > > > >   if (server->compress_algorithm) {
> > > > >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> > > > >   pneg_ctxt);
> > > > > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > >   8) * 8;
> > > > >   *total_len += ctxt_len;
> > > > >   pneg_ctxt += ctxt_len;
> > > > > - req->NegotiateContextCount = cpu_to_le16(5);
> > > > > - } else
> > > > > - req->NegotiateContextCount = cpu_to_le16(4);
> > > > > + neg_context_count++;
> > > > > + }
> > > > >
> > > > > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > - server->hostname);
> > > > > - *total_len += ctxt_len;
> > > > > - pneg_ctxt += ctxt_len;
> > > > > + if (enable_negotiate_signing) {
> > > > > + pr_warn_once("requesting GMAC signing is experimental\n");
> > > > > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > > > > + pneg_ctxt);
> > > > > + ctxt_len = DIV_ROUND_UP(
> > > > > + sizeof(struct smb2_signing_capabilities),
> > > > > + 8) * 8;
> > > > > + *total_len += ctxt_len;
> > > > > + pneg_ctxt += ctxt_len;
> > > > > + neg_context_count++;
> > > > > + }
> > > > > +
> > > > > + /* check for and add transport_capabilities and signing capabilities */
> > > > > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> > > > >
> > > > > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > - *total_len += sizeof(struct smb2_posix_neg_context);
> > > > >  }
> > > > >
> > > > >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > > > > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > > > > TCP_Server_Info *server,
> > > > >   return 0;
> > > > >  }
> > > > >
> > > > > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > > > > +        struct smb2_signing_capabilities *pctxt)
> > > > > +{
> > > > > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > > > > +
> > > > > + if ((len < 4) || (len > 16)) {
> > > > > + pr_warn_once("server sent bad signing negcontext\n");
> > > > > + return;
> > > > > + }
> > > > > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > > > > + pr_warn_once("Invalid signing algorithm count\n");
> > > > > + return;
> > > > > + }
> > > > > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > > > > + pr_warn_once("unknown signing algorithm\n");
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + server->signing_negotiated = true;
> > > > > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > > > > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > > > > +      server->signing_algorithm);
> > > > > +}
> > > > > +
> > > > > +
> > > > >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> > > > >        struct TCP_Server_Info *server,
> > > > >        unsigned int len_of_smb)
> > > > > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > > > > smb2_negotiate_rsp *rsp,
> > > > >   (struct smb2_compression_capabilities_context *)pctx);
> > > > >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> > > > >   server->posix_ext_supported = true;
> > > > > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > > > > + decode_signing_ctx(server,
> > > > > + (struct smb2_signing_capabilities *)pctx);
> > > > >   else
> > > > >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> > > > >   le16_to_cpu(pctx->ContextType));
> > > > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > > > > index ba75e65924ac..4b27cb9105fd 100644
> > > > > --- a/fs/cifs/smb2pdu.h
> > > > > +++ b/fs/cifs/smb2pdu.h
> > > > > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> > > > >   __le16 ContextType;
> > > > >   __le16 DataLength;
> > > > >   __le32 Reserved;
> > > > > - /* Followed by array of data */
> > > > > + /* Followed by array of data. NOTE: some servers require padding to
> > > > > 8 byte boundary */
> > > > >  } __packed;
> > > > >
> > > > >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > > > > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> > > > >   __u16 Padding;
> > > > >   __u32 Flags;
> > > > >   __le16 CompressionAlgorithms[3];
> > > > > + /* Check if pad needed */
> > > > >  } __packed;
> > > > >
> > > > >  /*
> > > > > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> > > > >   __le16  DataLength;
> > > > >   __u32 Reserved;
> > > > >   __le32 Flags;
> > > > > + __u32 Pad;
> > > > >  } __packed;
> > > > >
> > > > >  /*
> > > > > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> > > > >   __u32 Reserved;
> > > > >   __le16 SigningAlgorithmCount;
> > > > >   __le16 SigningAlgorithms[];
> > > > > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> > > > >  } __packed;
> > > > >
> > > > >  #define POSIX_CTXT_DATA_LEN 16
> > > > >
> > > > > --
> > > > > Thanks,
> > > > >
> > > > > Steve
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08 23:19         ` Steve French
@ 2021-07-08 23:29           ` Steve French
  2021-07-08 23:34             ` Steve French
  0 siblings, 1 reply; 9+ messages in thread
From: Steve French @ 2021-07-08 23:29 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: ronnie sahlberg, CIFS

Tentatively I made the change to clarify:

+MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
(GMAC) packet signing. Default: n/N/0");

to

+MODULE_PARM_DESC(enable_negotiate_signing, "Enable negotiating packet
signing algorithm with server. Default: n/N/0");

I think it is ok to leave the GMAC comment to minimize the change in
the next patch (enabling GMAC) ...

On Thu, Jul 8, 2021 at 6:19 PM Steve French <smfrench@gmail.com> wrote:
>
> It is a little tricky to figure out good wording here.
>
> "enable_negotiate_signing" (eventually) could end up requesting all 3
> ... which means that the server could choose SHA256 over GMAC over
> CMAC.  Maybe I should avoid all mention of GMAC.
>
> My reason for noting "GMAC is experimental" is that if GMAC is
> negotiated it is less tested than the other two (since fewer servers
> currently support it compared to the other two which are broadly
> supported and thus already tested with cifs.ko) - but the effect of
> the patch is to let the server choose which algorithm it prefers ...
> so perhaps I should avoid mentioning GMAC here.
>
> On Thu, Jul 8, 2021 at 5:06 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > requesting strongest (256 bit) GCM encr
> >  module_param(require_gcm_256, bool, 0644);
> >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > encryption. Default: n/N/0");
> >
> > +module_param(enable_negotiate_signing, bool, 0644);
> > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > (GMAC) packet signing. Default: n/N/0");
> > +
> >
> > s/enable_GMAC_signing/enable_negotiate_signing/ ?
> >
> > Also the description here is misleading: this param enables sending
> > the signing context not requesting GMAC which is not supported yet.
> >
> >
> > +    if (enable_negotiate_signing) {
> > +        pr_warn_once("requesting GMAC signing is experimental\n");
> >
> > Here as well - we are not requesting GMAC here, we are sending the
> > signing context with CMAC. I don't think this should be marked as
> > experimental.
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > ср, 7 июл. 2021 г. в 21:52, ronnie sahlberg <ronniesahlberg@gmail.com>:
> > >
> > > lgtm
> > >
> > > On Thu, Jul 8, 2021 at 2:49 PM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > v4 of patch (includes minor change to set local variable "num_algs"
> > > > once to number of algorithms and use that throughout
> > > > build_signing_context to make code a little clearer)
> > > >
> > > >
> > > > On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
> > > > >
> > > > > v3 of patch.  Updated with additional feedback from Ronnie (to make it
> > > > > more context len and datalength clearer)
> > > > >
> > > > >
> > > > > On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > Support for faster packet signing (using GMAC instead of CMAC) can
> > > > > > now be negotiated to some newer servers, including Windows.
> > > > > > See MS-SMB2 section 2.2.3.17.
> > > > > >
> > > > > > This patch adds support for sending the new negotiate context
> > > > > > with the first of three supported signing algorithms (AES-CMAC)
> > > > > > and decoding the response.  A followon patch will add support
> > > > > > for sending the other two (including AES-GMAC, which is fastest)
> > > > > > and changing the signing algorithm used based on what was
> > > > > > negotiated.
> > > > > >
> > > > > > To allow the client to request GMAC signing set module parameter
> > > > > > "enable_negotiate_signing" to 1.
> > > > > >
> > > > > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > > > > ---
> > > > > >  fs/cifs/cifsfs.c   |  4 +++
> > > > > >  fs/cifs/cifsglob.h |  3 ++
> > > > > >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> > > > > >  fs/cifs/smb2pdu.h  |  5 ++-
> > > > > >  4 files changed, 84 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > > > > index 9fb874dd8d24..476b07213fcd 100644
> > > > > > --- a/fs/cifs/cifsfs.c
> > > > > > +++ b/fs/cifs/cifsfs.c
> > > > > > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> > > > > >  bool disable_legacy_dialects; /* false by default */
> > > > > >  bool enable_gcm_256 = true;
> > > > > >  bool require_gcm_256; /* false by default */
> > > > > > +bool enable_negotiate_signing; /* false by default */
> > > > > >  unsigned int global_secflags = CIFSSEC_DEF;
> > > > > >  /* unsigned int ntlmv2_support = 0; */
> > > > > >  unsigned int sign_CIFS_PDUs = 1;
> > > > > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > > > > requesting strongest (256 bit) GCM encr
> > > > > >  module_param(require_gcm_256, bool, 0644);
> > > > > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > > > > encryption. Default: n/N/0");
> > > > > >
> > > > > > +module_param(enable_negotiate_signing, bool, 0644);
> > > > > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > > > > (GMAC) packet signing. Default: n/N/0");
> > > > > > +
> > > > > >  module_param(disable_legacy_dialects, bool, 0644);
> > > > > >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> > > > > >     "helpful to restrict the ability to "
> > > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > > > > index 921680fb7931..3c2e117bb926 100644
> > > > > > --- a/fs/cifs/cifsglob.h
> > > > > > +++ b/fs/cifs/cifsglob.h
> > > > > > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> > > > > >   unsigned int max_write;
> > > > > >   unsigned int min_offload;
> > > > > >   __le16 compress_algorithm;
> > > > > > + __u16 signing_algorithm;
> > > > > >   __le16 cipher_type;
> > > > > >   /* save initital negprot hash */
> > > > > >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > > > > > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> > > > > >   bool posix_ext_supported;
> > > > > >   struct delayed_work reconnect; /* reconnect workqueue job */
> > > > > >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > > > > > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > > > > > session setup sent
> > > > > >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> > > > > >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > > > > > signing (aes-gcm-256) */
> > > > > >  extern bool require_gcm_256; /* require use of strongest signing
> > > > > > (aes-gcm-256) */
> > > > > > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > > > > > signing if available */
> > > > > >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> > > > > >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> > > > > >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > > > index 962826dc3316..757f145e70e5 100644
> > > > > > --- a/fs/cifs/smb2pdu.c
> > > > > > +++ b/fs/cifs/smb2pdu.c
> > > > > > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > > > > > smb2_compression_capabilities_context *pneg_ctxt)
> > > > > >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> > > > > >  }
> > > > > >
> > > > > > +static void
> > > > > > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > > > > > +{
> > > > > > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > > > > > + /*
> > > > > > + * Data length must be increased here if more than 3 signing algorithms
> > > > > > + * sent but currently only 3 are defined. And context Data length must
> > > > > > + * be rounded to multiple of 8 for some servers.
> > > > > > + */
> > > > > > + pneg_ctxt->DataLength =
> > > > > > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > > > > > + sizeof(struct smb2_neg_context), 8) * 8);
> > > > > > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > > > > > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > > > > > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > > > > > +}
> > > > > > +
> > > > > >  static void
> > > > > >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> > > > > >  {
> > > > > > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > >         struct TCP_Server_Info *server, unsigned int *total_len)
> > > > > >  {
> > > > > >   char *pneg_ctxt;
> > > > > > - unsigned int ctxt_len;
> > > > > > + unsigned int ctxt_len, neg_context_count;
> > > > > >
> > > > > >   if (*total_len > 200) {
> > > > > >   /* In case length corrupted don't want to overrun smb buffer */
> > > > > > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > >   *total_len += ctxt_len;
> > > > > >   pneg_ctxt += ctxt_len;
> > > > > >
> > > > > > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > > + server->hostname);
> > > > > > + *total_len += ctxt_len;
> > > > > > + pneg_ctxt += ctxt_len;
> > > > > > +
> > > > > > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > > + *total_len += sizeof(struct smb2_posix_neg_context);
> > > > > > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > > > > > +
> > > > > > + neg_context_count = 4;
> > > > > > +
> > > > > >   if (server->compress_algorithm) {
> > > > > >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> > > > > >   pneg_ctxt);
> > > > > > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > >   8) * 8;
> > > > > >   *total_len += ctxt_len;
> > > > > >   pneg_ctxt += ctxt_len;
> > > > > > - req->NegotiateContextCount = cpu_to_le16(5);
> > > > > > - } else
> > > > > > - req->NegotiateContextCount = cpu_to_le16(4);
> > > > > > + neg_context_count++;
> > > > > > + }
> > > > > >
> > > > > > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > > - server->hostname);
> > > > > > - *total_len += ctxt_len;
> > > > > > - pneg_ctxt += ctxt_len;
> > > > > > + if (enable_negotiate_signing) {
> > > > > > + pr_warn_once("requesting GMAC signing is experimental\n");
> > > > > > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > > > > > + pneg_ctxt);
> > > > > > + ctxt_len = DIV_ROUND_UP(
> > > > > > + sizeof(struct smb2_signing_capabilities),
> > > > > > + 8) * 8;
> > > > > > + *total_len += ctxt_len;
> > > > > > + pneg_ctxt += ctxt_len;
> > > > > > + neg_context_count++;
> > > > > > + }
> > > > > > +
> > > > > > + /* check for and add transport_capabilities and signing capabilities */
> > > > > > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> > > > > >
> > > > > > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > > - *total_len += sizeof(struct smb2_posix_neg_context);
> > > > > >  }
> > > > > >
> > > > > >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > > > > > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > > > > > TCP_Server_Info *server,
> > > > > >   return 0;
> > > > > >  }
> > > > > >
> > > > > > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > > > > > +        struct smb2_signing_capabilities *pctxt)
> > > > > > +{
> > > > > > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > > > > > +
> > > > > > + if ((len < 4) || (len > 16)) {
> > > > > > + pr_warn_once("server sent bad signing negcontext\n");
> > > > > > + return;
> > > > > > + }
> > > > > > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > > > > > + pr_warn_once("Invalid signing algorithm count\n");
> > > > > > + return;
> > > > > > + }
> > > > > > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > > > > > + pr_warn_once("unknown signing algorithm\n");
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + server->signing_negotiated = true;
> > > > > > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > > > > > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > > > > > +      server->signing_algorithm);
> > > > > > +}
> > > > > > +
> > > > > > +
> > > > > >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> > > > > >        struct TCP_Server_Info *server,
> > > > > >        unsigned int len_of_smb)
> > > > > > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > > > > > smb2_negotiate_rsp *rsp,
> > > > > >   (struct smb2_compression_capabilities_context *)pctx);
> > > > > >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> > > > > >   server->posix_ext_supported = true;
> > > > > > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > > > > > + decode_signing_ctx(server,
> > > > > > + (struct smb2_signing_capabilities *)pctx);
> > > > > >   else
> > > > > >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> > > > > >   le16_to_cpu(pctx->ContextType));
> > > > > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > > > > > index ba75e65924ac..4b27cb9105fd 100644
> > > > > > --- a/fs/cifs/smb2pdu.h
> > > > > > +++ b/fs/cifs/smb2pdu.h
> > > > > > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> > > > > >   __le16 ContextType;
> > > > > >   __le16 DataLength;
> > > > > >   __le32 Reserved;
> > > > > > - /* Followed by array of data */
> > > > > > + /* Followed by array of data. NOTE: some servers require padding to
> > > > > > 8 byte boundary */
> > > > > >  } __packed;
> > > > > >
> > > > > >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > > > > > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> > > > > >   __u16 Padding;
> > > > > >   __u32 Flags;
> > > > > >   __le16 CompressionAlgorithms[3];
> > > > > > + /* Check if pad needed */
> > > > > >  } __packed;
> > > > > >
> > > > > >  /*
> > > > > > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> > > > > >   __le16  DataLength;
> > > > > >   __u32 Reserved;
> > > > > >   __le32 Flags;
> > > > > > + __u32 Pad;
> > > > > >  } __packed;
> > > > > >
> > > > > >  /*
> > > > > > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> > > > > >   __u32 Reserved;
> > > > > >   __le16 SigningAlgorithmCount;
> > > > > >   __le16 SigningAlgorithms[];
> > > > > > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> > > > > >  } __packed;
> > > > > >
> > > > > >  #define POSIX_CTXT_DATA_LEN 16
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > >
> > > > > Steve
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08 23:29           ` Steve French
@ 2021-07-08 23:34             ` Steve French
  2021-07-09 17:36               ` Pavel Shilovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Steve French @ 2021-07-08 23:34 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: ronnie sahlberg, CIFS

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

I also removed the line you noted:
          pr_warn_once("requesting GMAC signing is experimental\n");
No point to print that unless GMAC was actually negotiated (ie until
the followon patch which adds GMAC to the list ...)



On Thu, Jul 8, 2021 at 6:29 PM Steve French <smfrench@gmail.com> wrote:
>
> Tentatively I made the change to clarify:
>
> +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> (GMAC) packet signing. Default: n/N/0");
>
> to
>
> +MODULE_PARM_DESC(enable_negotiate_signing, "Enable negotiating packet
> signing algorithm with server. Default: n/N/0");
>
> I think it is ok to leave the GMAC comment to minimize the change in
> the next patch (enabling GMAC) ...
>
> On Thu, Jul 8, 2021 at 6:19 PM Steve French <smfrench@gmail.com> wrote:
> >
> > It is a little tricky to figure out good wording here.
> >
> > "enable_negotiate_signing" (eventually) could end up requesting all 3
> > ... which means that the server could choose SHA256 over GMAC over
> > CMAC.  Maybe I should avoid all mention of GMAC.
> >
> > My reason for noting "GMAC is experimental" is that if GMAC is
> > negotiated it is less tested than the other two (since fewer servers
> > currently support it compared to the other two which are broadly
> > supported and thus already tested with cifs.ko) - but the effect of
> > the patch is to let the server choose which algorithm it prefers ...
> > so perhaps I should avoid mentioning GMAC here.
> >
> > On Thu, Jul 8, 2021 at 5:06 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > requesting strongest (256 bit) GCM encr
> > >  module_param(require_gcm_256, bool, 0644);
> > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > encryption. Default: n/N/0");
> > >
> > > +module_param(enable_negotiate_signing, bool, 0644);
> > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > (GMAC) packet signing. Default: n/N/0");
> > > +
> > >
> > > s/enable_GMAC_signing/enable_negotiate_signing/ ?
> > >
> > > Also the description here is misleading: this param enables sending
> > > the signing context not requesting GMAC which is not supported yet.
> > >
> > >
> > > +    if (enable_negotiate_signing) {
> > > +        pr_warn_once("requesting GMAC signing is experimental\n");
> > >
> > > Here as well - we are not requesting GMAC here, we are sending the
> > > signing context with CMAC. I don't think this should be marked as
> > > experimental.
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> > >
> > > ср, 7 июл. 2021 г. в 21:52, ronnie sahlberg <ronniesahlberg@gmail.com>:
> > > >
> > > > lgtm
> > > >
> > > > On Thu, Jul 8, 2021 at 2:49 PM Steve French <smfrench@gmail.com> wrote:
> > > > >
> > > > > v4 of patch (includes minor change to set local variable "num_algs"
> > > > > once to number of algorithms and use that throughout
> > > > > build_signing_context to make code a little clearer)
> > > > >
> > > > >
> > > > > On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > v3 of patch.  Updated with additional feedback from Ronnie (to make it
> > > > > > more context len and datalength clearer)
> > > > > >
> > > > > >
> > > > > > On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > >
> > > > > > > Support for faster packet signing (using GMAC instead of CMAC) can
> > > > > > > now be negotiated to some newer servers, including Windows.
> > > > > > > See MS-SMB2 section 2.2.3.17.
> > > > > > >
> > > > > > > This patch adds support for sending the new negotiate context
> > > > > > > with the first of three supported signing algorithms (AES-CMAC)
> > > > > > > and decoding the response.  A followon patch will add support
> > > > > > > for sending the other two (including AES-GMAC, which is fastest)
> > > > > > > and changing the signing algorithm used based on what was
> > > > > > > negotiated.
> > > > > > >
> > > > > > > To allow the client to request GMAC signing set module parameter
> > > > > > > "enable_negotiate_signing" to 1.
> > > > > > >
> > > > > > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > > > > > ---
> > > > > > >  fs/cifs/cifsfs.c   |  4 +++
> > > > > > >  fs/cifs/cifsglob.h |  3 ++
> > > > > > >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> > > > > > >  fs/cifs/smb2pdu.h  |  5 ++-
> > > > > > >  4 files changed, 84 insertions(+), 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > > > > > index 9fb874dd8d24..476b07213fcd 100644
> > > > > > > --- a/fs/cifs/cifsfs.c
> > > > > > > +++ b/fs/cifs/cifsfs.c
> > > > > > > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> > > > > > >  bool disable_legacy_dialects; /* false by default */
> > > > > > >  bool enable_gcm_256 = true;
> > > > > > >  bool require_gcm_256; /* false by default */
> > > > > > > +bool enable_negotiate_signing; /* false by default */
> > > > > > >  unsigned int global_secflags = CIFSSEC_DEF;
> > > > > > >  /* unsigned int ntlmv2_support = 0; */
> > > > > > >  unsigned int sign_CIFS_PDUs = 1;
> > > > > > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > > > > > requesting strongest (256 bit) GCM encr
> > > > > > >  module_param(require_gcm_256, bool, 0644);
> > > > > > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > > > > > encryption. Default: n/N/0");
> > > > > > >
> > > > > > > +module_param(enable_negotiate_signing, bool, 0644);
> > > > > > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > > > > > (GMAC) packet signing. Default: n/N/0");
> > > > > > > +
> > > > > > >  module_param(disable_legacy_dialects, bool, 0644);
> > > > > > >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> > > > > > >     "helpful to restrict the ability to "
> > > > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > > > > > index 921680fb7931..3c2e117bb926 100644
> > > > > > > --- a/fs/cifs/cifsglob.h
> > > > > > > +++ b/fs/cifs/cifsglob.h
> > > > > > > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> > > > > > >   unsigned int max_write;
> > > > > > >   unsigned int min_offload;
> > > > > > >   __le16 compress_algorithm;
> > > > > > > + __u16 signing_algorithm;
> > > > > > >   __le16 cipher_type;
> > > > > > >   /* save initital negprot hash */
> > > > > > >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > > > > > > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> > > > > > >   bool posix_ext_supported;
> > > > > > >   struct delayed_work reconnect; /* reconnect workqueue job */
> > > > > > >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > > > > > > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > > > > > > session setup sent
> > > > > > >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> > > > > > >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > > > > > > signing (aes-gcm-256) */
> > > > > > >  extern bool require_gcm_256; /* require use of strongest signing
> > > > > > > (aes-gcm-256) */
> > > > > > > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > > > > > > signing if available */
> > > > > > >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> > > > > > >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> > > > > > >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > > > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > > > > index 962826dc3316..757f145e70e5 100644
> > > > > > > --- a/fs/cifs/smb2pdu.c
> > > > > > > +++ b/fs/cifs/smb2pdu.c
> > > > > > > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > > > > > > smb2_compression_capabilities_context *pneg_ctxt)
> > > > > > >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> > > > > > >  }
> > > > > > >
> > > > > > > +static void
> > > > > > > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > > > > > > +{
> > > > > > > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > > > > > > + /*
> > > > > > > + * Data length must be increased here if more than 3 signing algorithms
> > > > > > > + * sent but currently only 3 are defined. And context Data length must
> > > > > > > + * be rounded to multiple of 8 for some servers.
> > > > > > > + */
> > > > > > > + pneg_ctxt->DataLength =
> > > > > > > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > > > > > > + sizeof(struct smb2_neg_context), 8) * 8);
> > > > > > > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > > > > > > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > > > > > > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void
> > > > > > >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> > > > > > >  {
> > > > > > > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > > >         struct TCP_Server_Info *server, unsigned int *total_len)
> > > > > > >  {
> > > > > > >   char *pneg_ctxt;
> > > > > > > - unsigned int ctxt_len;
> > > > > > > + unsigned int ctxt_len, neg_context_count;
> > > > > > >
> > > > > > >   if (*total_len > 200) {
> > > > > > >   /* In case length corrupted don't want to overrun smb buffer */
> > > > > > > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > > >   *total_len += ctxt_len;
> > > > > > >   pneg_ctxt += ctxt_len;
> > > > > > >
> > > > > > > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > > > + server->hostname);
> > > > > > > + *total_len += ctxt_len;
> > > > > > > + pneg_ctxt += ctxt_len;
> > > > > > > +
> > > > > > > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > > > + *total_len += sizeof(struct smb2_posix_neg_context);
> > > > > > > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > > > > > > +
> > > > > > > + neg_context_count = 4;
> > > > > > > +
> > > > > > >   if (server->compress_algorithm) {
> > > > > > >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> > > > > > >   pneg_ctxt);
> > > > > > > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > > >   8) * 8;
> > > > > > >   *total_len += ctxt_len;
> > > > > > >   pneg_ctxt += ctxt_len;
> > > > > > > - req->NegotiateContextCount = cpu_to_le16(5);
> > > > > > > - } else
> > > > > > > - req->NegotiateContextCount = cpu_to_le16(4);
> > > > > > > + neg_context_count++;
> > > > > > > + }
> > > > > > >
> > > > > > > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > > > - server->hostname);
> > > > > > > - *total_len += ctxt_len;
> > > > > > > - pneg_ctxt += ctxt_len;
> > > > > > > + if (enable_negotiate_signing) {
> > > > > > > + pr_warn_once("requesting GMAC signing is experimental\n");
> > > > > > > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > > > > > > + pneg_ctxt);
> > > > > > > + ctxt_len = DIV_ROUND_UP(
> > > > > > > + sizeof(struct smb2_signing_capabilities),
> > > > > > > + 8) * 8;
> > > > > > > + *total_len += ctxt_len;
> > > > > > > + pneg_ctxt += ctxt_len;
> > > > > > > + neg_context_count++;
> > > > > > > + }
> > > > > > > +
> > > > > > > + /* check for and add transport_capabilities and signing capabilities */
> > > > > > > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> > > > > > >
> > > > > > > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > > > - *total_len += sizeof(struct smb2_posix_neg_context);
> > > > > > >  }
> > > > > > >
> > > > > > >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > > > > > > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > > > > > > TCP_Server_Info *server,
> > > > > > >   return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > > > > > > +        struct smb2_signing_capabilities *pctxt)
> > > > > > > +{
> > > > > > > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > > > > > > +
> > > > > > > + if ((len < 4) || (len > 16)) {
> > > > > > > + pr_warn_once("server sent bad signing negcontext\n");
> > > > > > > + return;
> > > > > > > + }
> > > > > > > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > > > > > > + pr_warn_once("Invalid signing algorithm count\n");
> > > > > > > + return;
> > > > > > > + }
> > > > > > > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > > > > > > + pr_warn_once("unknown signing algorithm\n");
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + server->signing_negotiated = true;
> > > > > > > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > > > > > > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > > > > > > +      server->signing_algorithm);
> > > > > > > +}
> > > > > > > +
> > > > > > > +
> > > > > > >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> > > > > > >        struct TCP_Server_Info *server,
> > > > > > >        unsigned int len_of_smb)
> > > > > > > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > > > > > > smb2_negotiate_rsp *rsp,
> > > > > > >   (struct smb2_compression_capabilities_context *)pctx);
> > > > > > >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> > > > > > >   server->posix_ext_supported = true;
> > > > > > > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > > > > > > + decode_signing_ctx(server,
> > > > > > > + (struct smb2_signing_capabilities *)pctx);
> > > > > > >   else
> > > > > > >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> > > > > > >   le16_to_cpu(pctx->ContextType));
> > > > > > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > > > > > > index ba75e65924ac..4b27cb9105fd 100644
> > > > > > > --- a/fs/cifs/smb2pdu.h
> > > > > > > +++ b/fs/cifs/smb2pdu.h
> > > > > > > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> > > > > > >   __le16 ContextType;
> > > > > > >   __le16 DataLength;
> > > > > > >   __le32 Reserved;
> > > > > > > - /* Followed by array of data */
> > > > > > > + /* Followed by array of data. NOTE: some servers require padding to
> > > > > > > 8 byte boundary */
> > > > > > >  } __packed;
> > > > > > >
> > > > > > >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > > > > > > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> > > > > > >   __u16 Padding;
> > > > > > >   __u32 Flags;
> > > > > > >   __le16 CompressionAlgorithms[3];
> > > > > > > + /* Check if pad needed */
> > > > > > >  } __packed;
> > > > > > >
> > > > > > >  /*
> > > > > > > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> > > > > > >   __le16  DataLength;
> > > > > > >   __u32 Reserved;
> > > > > > >   __le32 Flags;
> > > > > > > + __u32 Pad;
> > > > > > >  } __packed;
> > > > > > >
> > > > > > >  /*
> > > > > > > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> > > > > > >   __u32 Reserved;
> > > > > > >   __le16 SigningAlgorithmCount;
> > > > > > >   __le16 SigningAlgorithms[];
> > > > > > > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> > > > > > >  } __packed;
> > > > > > >
> > > > > > >  #define POSIX_CTXT_DATA_LEN 16
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Steve
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > >
> > > > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0002-SMB3.1.1-Add-support-for-negotiating-signing-algorit.patch --]
[-- Type: text/x-patch, Size: 9248 bytes --]

From de96853dacc7cd319d5fddab3cb5ada8f106c52c Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Mon, 5 Jul 2021 15:05:39 -0500
Subject: [PATCH 2/2] SMB3.1.1: Add support for negotiating signing algorithm

Support for faster packet signing (using GMAC instead of CMAC) can
now be negotiated to some newer servers, including Windows.
See MS-SMB2 section 2.2.3.17.

This patch adds support for sending the new negotiate context
with the first of three supported signing algorithms (AES-CMAC)
and decoding the response.  A followon patch will add support
for sending the other two (including AES-GMAC, which is fastest)
and changing the signing algorithm used based on what was
negotiated.

To allow the client to request GMAC signing set module parameter
"enable_negotiate_signing" to 1.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c   |  4 +++
 fs/cifs/cifsglob.h |  3 ++
 fs/cifs/smb2pdu.c  | 86 ++++++++++++++++++++++++++++++++++++++++------
 fs/cifs/smb2pdu.h  |  5 ++-
 4 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9fb874dd8d24..476b07213fcd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
 bool disable_legacy_dialects; /* false by default */
 bool enable_gcm_256 = true;
 bool require_gcm_256; /* false by default */
+bool enable_negotiate_signing; /* false by default */
 unsigned int global_secflags = CIFSSEC_DEF;
 /* unsigned int ntlmv2_support = 0; */
 unsigned int sign_CIFS_PDUs = 1;
@@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable requesting strongest (256 bit) GCM encr
 module_param(require_gcm_256, bool, 0644);
 MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM encryption. Default: n/N/0");
 
+module_param(enable_negotiate_signing, bool, 0644);
+MODULE_PARM_DESC(enable_negotiate_signing, "Enable negotiating packet signing algorithm with server. Default: n/N/0");
+
 module_param(disable_legacy_dialects, bool, 0644);
 MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
 				  "helpful to restrict the ability to "
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 921680fb7931..3c2e117bb926 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -667,9 +667,11 @@ struct TCP_Server_Info {
 	unsigned int	max_write;
 	unsigned int	min_offload;
 	__le16	compress_algorithm;
+	__u16	signing_algorithm;
 	__le16	cipher_type;
 	 /* save initital negprot hash */
 	__u8	preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
+	bool	signing_negotiated; /* true if valid signing context rcvd from server */
 	bool	posix_ext_supported;
 	struct delayed_work reconnect; /* reconnect workqueue job */
 	struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
@@ -1869,6 +1871,7 @@ extern unsigned int global_secflags;	/* if on, session setup sent
 extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
 extern bool enable_gcm_256; /* allow optional negotiate of strongest signing (aes-gcm-256) */
 extern bool require_gcm_256; /* require use of strongest signing (aes-gcm-256) */
+extern bool enable_negotiate_signing; /* request use of faster (GMAC) signing if available */
 extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
 extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
 extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 962826dc3316..c1e76eb12109 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -433,6 +433,29 @@ build_compression_ctxt(struct smb2_compression_capabilities_context *pneg_ctxt)
 	pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
 }
 
+static unsigned int
+build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
+{
+	unsigned int ctxt_len = sizeof(struct smb2_signing_capabilities);
+	unsigned short num_algs = 1; /* number of signing algorithms sent */
+
+	pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
+	/*
+	 * Context Data length must be rounded to multiple of 8 for some servers
+	 */
+	pneg_ctxt->DataLength = cpu_to_le16(DIV_ROUND_UP(
+				sizeof(struct smb2_signing_capabilities) -
+				sizeof(struct smb2_neg_context) +
+				(num_algs * 2 /* sizeof u16 */), 8) * 8);
+	pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(num_algs);
+	pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
+
+	ctxt_len += 2 /* sizeof le16 */ * num_algs;
+	ctxt_len = DIV_ROUND_UP(ctxt_len, 8) * 8;
+	return ctxt_len;
+	/* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
+}
+
 static void
 build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
 {
@@ -498,7 +521,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 		      struct TCP_Server_Info *server, unsigned int *total_len)
 {
 	char *pneg_ctxt;
-	unsigned int ctxt_len;
+	unsigned int ctxt_len, neg_context_count;
 
 	if (*total_len > 200) {
 		/* In case length corrupted don't want to overrun smb buffer */
@@ -525,6 +548,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 	*total_len += ctxt_len;
 	pneg_ctxt += ctxt_len;
 
+	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
+					server->hostname);
+	*total_len += ctxt_len;
+	pneg_ctxt += ctxt_len;
+
+	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
+	*total_len += sizeof(struct smb2_posix_neg_context);
+	pneg_ctxt += sizeof(struct smb2_posix_neg_context);
+
+	neg_context_count = 4;
+
 	if (server->compress_algorithm) {
 		build_compression_ctxt((struct smb2_compression_capabilities_context *)
 				pneg_ctxt);
@@ -533,17 +567,21 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
 				8) * 8;
 		*total_len += ctxt_len;
 		pneg_ctxt += ctxt_len;
-		req->NegotiateContextCount = cpu_to_le16(5);
-	} else
-		req->NegotiateContextCount = cpu_to_le16(4);
+		neg_context_count++;
+	}
 
-	ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
-					server->hostname);
-	*total_len += ctxt_len;
-	pneg_ctxt += ctxt_len;
+	if (enable_negotiate_signing) {
+		pr_warn_once("requesting GMAC signing is experimental\n");
+		ctxt_len = build_signing_ctxt((struct smb2_signing_capabilities *)
+				pneg_ctxt);
+		*total_len += ctxt_len;
+		pneg_ctxt += ctxt_len;
+		neg_context_count++;
+	}
+
+	/* check for and add transport_capabilities and signing capabilities */
+	req->NegotiateContextCount = cpu_to_le16(neg_context_count);
 
-	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
-	*total_len += sizeof(struct smb2_posix_neg_context);
 }
 
 static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
@@ -632,6 +670,31 @@ static int decode_encrypt_ctx(struct TCP_Server_Info *server,
 	return 0;
 }
 
+static void decode_signing_ctx(struct TCP_Server_Info *server,
+			       struct smb2_signing_capabilities *pctxt)
+{
+	unsigned int len = le16_to_cpu(pctxt->DataLength);
+
+	if ((len < 4) || (len > 16)) {
+		pr_warn_once("server sent bad signing negcontext\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
+		pr_warn_once("Invalid signing algorithm count\n");
+		return;
+	}
+	if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
+		pr_warn_once("unknown signing algorithm\n");
+		return;
+	}
+
+	server->signing_negotiated = true;
+	server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
+	cifs_dbg(FYI, "signing algorithm %d chosen\n",
+		     server->signing_algorithm);
+}
+
+
 static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				     struct TCP_Server_Info *server,
 				     unsigned int len_of_smb)
@@ -675,6 +738,9 @@ static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
 				(struct smb2_compression_capabilities_context *)pctx);
 		else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
 			server->posix_ext_supported = true;
+		else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
+			decode_signing_ctx(server,
+				(struct smb2_signing_capabilities *)pctx);
 		else
 			cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
 				le16_to_cpu(pctx->ContextType));
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index ba75e65924ac..4b27cb9105fd 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -329,7 +329,7 @@ struct smb2_neg_context {
 	__le16	ContextType;
 	__le16	DataLength;
 	__le32	Reserved;
-	/* Followed by array of data */
+	/* Followed by array of data. NOTE: some servers require padding to 8 byte boundary */
 } __packed;
 
 #define SMB311_LINUX_CLIENT_SALT_SIZE			32
@@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
 	__u16	Padding;
 	__u32	Flags;
 	__le16	CompressionAlgorithms[3];
+	/* Check if pad needed */
 } __packed;
 
 /*
@@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
 	__le16  DataLength;
 	__u32	Reserved;
 	__le32	Flags;
+	__u32	Pad;
 } __packed;
 
 /*
@@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
 	__u32	Reserved;
 	__le16	SigningAlgorithmCount;
 	__le16	SigningAlgorithms[];
+	/*  Followed by padding to 8 byte boundary (required by some servers) */
 } __packed;
 
 #define POSIX_CTXT_DATA_LEN	16
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
  2021-07-08 23:34             ` Steve French
@ 2021-07-09 17:36               ` Pavel Shilovsky
  0 siblings, 0 replies; 9+ messages in thread
From: Pavel Shilovsky @ 2021-07-09 17:36 UTC (permalink / raw)
  To: Steve French; +Cc: ronnie sahlberg, CIFS

Thanks for making the change! The patch in your for-next branch looks good:

https://git.samba.org/sfrench/?p=sfrench/cifs-2.6.git;a=commitdiff;h=c99696885096f71e59023996f7453485a20ba2c8

Reviewed-by: Pavel Shilovsky <pshilovsky@samba.org>
--
Best regards,
Pavel Shilovsky

чт, 8 июл. 2021 г. в 16:34, Steve French <smfrench@gmail.com>:
>
> I also removed the line you noted:
>           pr_warn_once("requesting GMAC signing is experimental\n");
> No point to print that unless GMAC was actually negotiated (ie until
> the followon patch which adds GMAC to the list ...)
>
>
>
> On Thu, Jul 8, 2021 at 6:29 PM Steve French <smfrench@gmail.com> wrote:
> >
> > Tentatively I made the change to clarify:
> >
> > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > (GMAC) packet signing. Default: n/N/0");
> >
> > to
> >
> > +MODULE_PARM_DESC(enable_negotiate_signing, "Enable negotiating packet
> > signing algorithm with server. Default: n/N/0");
> >
> > I think it is ok to leave the GMAC comment to minimize the change in
> > the next patch (enabling GMAC) ...
> >
> > On Thu, Jul 8, 2021 at 6:19 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > It is a little tricky to figure out good wording here.
> > >
> > > "enable_negotiate_signing" (eventually) could end up requesting all 3
> > > ... which means that the server could choose SHA256 over GMAC over
> > > CMAC.  Maybe I should avoid all mention of GMAC.
> > >
> > > My reason for noting "GMAC is experimental" is that if GMAC is
> > > negotiated it is less tested than the other two (since fewer servers
> > > currently support it compared to the other two which are broadly
> > > supported and thus already tested with cifs.ko) - but the effect of
> > > the patch is to let the server choose which algorithm it prefers ...
> > > so perhaps I should avoid mentioning GMAC here.
> > >
> > > On Thu, Jul 8, 2021 at 5:06 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > >
> > > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > > requesting strongest (256 bit) GCM encr
> > > >  module_param(require_gcm_256, bool, 0644);
> > > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > > encryption. Default: n/N/0");
> > > >
> > > > +module_param(enable_negotiate_signing, bool, 0644);
> > > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > > (GMAC) packet signing. Default: n/N/0");
> > > > +
> > > >
> > > > s/enable_GMAC_signing/enable_negotiate_signing/ ?
> > > >
> > > > Also the description here is misleading: this param enables sending
> > > > the signing context not requesting GMAC which is not supported yet.
> > > >
> > > >
> > > > +    if (enable_negotiate_signing) {
> > > > +        pr_warn_once("requesting GMAC signing is experimental\n");
> > > >
> > > > Here as well - we are not requesting GMAC here, we are sending the
> > > > signing context with CMAC. I don't think this should be marked as
> > > > experimental.
> > > >
> > > > --
> > > > Best regards,
> > > > Pavel Shilovsky
> > > >
> > > > ср, 7 июл. 2021 г. в 21:52, ronnie sahlberg <ronniesahlberg@gmail.com>:
> > > > >
> > > > > lgtm
> > > > >
> > > > > On Thu, Jul 8, 2021 at 2:49 PM Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > v4 of patch (includes minor change to set local variable "num_algs"
> > > > > > once to number of algorithms and use that throughout
> > > > > > build_signing_context to make code a little clearer)
> > > > > >
> > > > > >
> > > > > > On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > >
> > > > > > > v3 of patch.  Updated with additional feedback from Ronnie (to make it
> > > > > > > more context len and datalength clearer)
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Support for faster packet signing (using GMAC instead of CMAC) can
> > > > > > > > now be negotiated to some newer servers, including Windows.
> > > > > > > > See MS-SMB2 section 2.2.3.17.
> > > > > > > >
> > > > > > > > This patch adds support for sending the new negotiate context
> > > > > > > > with the first of three supported signing algorithms (AES-CMAC)
> > > > > > > > and decoding the response.  A followon patch will add support
> > > > > > > > for sending the other two (including AES-GMAC, which is fastest)
> > > > > > > > and changing the signing algorithm used based on what was
> > > > > > > > negotiated.
> > > > > > > >
> > > > > > > > To allow the client to request GMAC signing set module parameter
> > > > > > > > "enable_negotiate_signing" to 1.
> > > > > > > >
> > > > > > > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > > > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > > > > > > ---
> > > > > > > >  fs/cifs/cifsfs.c   |  4 +++
> > > > > > > >  fs/cifs/cifsglob.h |  3 ++
> > > > > > > >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> > > > > > > >  fs/cifs/smb2pdu.h  |  5 ++-
> > > > > > > >  4 files changed, 84 insertions(+), 11 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > > > > > > index 9fb874dd8d24..476b07213fcd 100644
> > > > > > > > --- a/fs/cifs/cifsfs.c
> > > > > > > > +++ b/fs/cifs/cifsfs.c
> > > > > > > > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> > > > > > > >  bool disable_legacy_dialects; /* false by default */
> > > > > > > >  bool enable_gcm_256 = true;
> > > > > > > >  bool require_gcm_256; /* false by default */
> > > > > > > > +bool enable_negotiate_signing; /* false by default */
> > > > > > > >  unsigned int global_secflags = CIFSSEC_DEF;
> > > > > > > >  /* unsigned int ntlmv2_support = 0; */
> > > > > > > >  unsigned int sign_CIFS_PDUs = 1;
> > > > > > > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > > > > > > requesting strongest (256 bit) GCM encr
> > > > > > > >  module_param(require_gcm_256, bool, 0644);
> > > > > > > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > > > > > > encryption. Default: n/N/0");
> > > > > > > >
> > > > > > > > +module_param(enable_negotiate_signing, bool, 0644);
> > > > > > > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > > > > > > (GMAC) packet signing. Default: n/N/0");
> > > > > > > > +
> > > > > > > >  module_param(disable_legacy_dialects, bool, 0644);
> > > > > > > >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> > > > > > > >     "helpful to restrict the ability to "
> > > > > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > > > > > > index 921680fb7931..3c2e117bb926 100644
> > > > > > > > --- a/fs/cifs/cifsglob.h
> > > > > > > > +++ b/fs/cifs/cifsglob.h
> > > > > > > > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> > > > > > > >   unsigned int max_write;
> > > > > > > >   unsigned int min_offload;
> > > > > > > >   __le16 compress_algorithm;
> > > > > > > > + __u16 signing_algorithm;
> > > > > > > >   __le16 cipher_type;
> > > > > > > >   /* save initital negprot hash */
> > > > > > > >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > > > > > > > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> > > > > > > >   bool posix_ext_supported;
> > > > > > > >   struct delayed_work reconnect; /* reconnect workqueue job */
> > > > > > > >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > > > > > > > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > > > > > > > session setup sent
> > > > > > > >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> > > > > > > >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > > > > > > > signing (aes-gcm-256) */
> > > > > > > >  extern bool require_gcm_256; /* require use of strongest signing
> > > > > > > > (aes-gcm-256) */
> > > > > > > > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > > > > > > > signing if available */
> > > > > > > >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> > > > > > > >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> > > > > > > >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > > > > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > > > > > > index 962826dc3316..757f145e70e5 100644
> > > > > > > > --- a/fs/cifs/smb2pdu.c
> > > > > > > > +++ b/fs/cifs/smb2pdu.c
> > > > > > > > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > > > > > > > smb2_compression_capabilities_context *pneg_ctxt)
> > > > > > > >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static void
> > > > > > > > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > > > > > > > +{
> > > > > > > > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > > > > > > > + /*
> > > > > > > > + * Data length must be increased here if more than 3 signing algorithms
> > > > > > > > + * sent but currently only 3 are defined. And context Data length must
> > > > > > > > + * be rounded to multiple of 8 for some servers.
> > > > > > > > + */
> > > > > > > > + pneg_ctxt->DataLength =
> > > > > > > > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > > > > > > > + sizeof(struct smb2_neg_context), 8) * 8);
> > > > > > > > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > > > > > > > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > > > > > > > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void
> > > > > > > >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> > > > > > > >  {
> > > > > > > > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > > > >         struct TCP_Server_Info *server, unsigned int *total_len)
> > > > > > > >  {
> > > > > > > >   char *pneg_ctxt;
> > > > > > > > - unsigned int ctxt_len;
> > > > > > > > + unsigned int ctxt_len, neg_context_count;
> > > > > > > >
> > > > > > > >   if (*total_len > 200) {
> > > > > > > >   /* In case length corrupted don't want to overrun smb buffer */
> > > > > > > > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > > > >   *total_len += ctxt_len;
> > > > > > > >   pneg_ctxt += ctxt_len;
> > > > > > > >
> > > > > > > > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > > > > + server->hostname);
> > > > > > > > + *total_len += ctxt_len;
> > > > > > > > + pneg_ctxt += ctxt_len;
> > > > > > > > +
> > > > > > > > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > > > > + *total_len += sizeof(struct smb2_posix_neg_context);
> > > > > > > > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > > > > > > > +
> > > > > > > > + neg_context_count = 4;
> > > > > > > > +
> > > > > > > >   if (server->compress_algorithm) {
> > > > > > > >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> > > > > > > >   pneg_ctxt);
> > > > > > > > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > > > > > > >   8) * 8;
> > > > > > > >   *total_len += ctxt_len;
> > > > > > > >   pneg_ctxt += ctxt_len;
> > > > > > > > - req->NegotiateContextCount = cpu_to_le16(5);
> > > > > > > > - } else
> > > > > > > > - req->NegotiateContextCount = cpu_to_le16(4);
> > > > > > > > + neg_context_count++;
> > > > > > > > + }
> > > > > > > >
> > > > > > > > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > > > > > > - server->hostname);
> > > > > > > > - *total_len += ctxt_len;
> > > > > > > > - pneg_ctxt += ctxt_len;
> > > > > > > > + if (enable_negotiate_signing) {
> > > > > > > > + pr_warn_once("requesting GMAC signing is experimental\n");
> > > > > > > > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > > > > > > > + pneg_ctxt);
> > > > > > > > + ctxt_len = DIV_ROUND_UP(
> > > > > > > > + sizeof(struct smb2_signing_capabilities),
> > > > > > > > + 8) * 8;
> > > > > > > > + *total_len += ctxt_len;
> > > > > > > > + pneg_ctxt += ctxt_len;
> > > > > > > > + neg_context_count++;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + /* check for and add transport_capabilities and signing capabilities */
> > > > > > > > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> > > > > > > >
> > > > > > > > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > > > > > > - *total_len += sizeof(struct smb2_posix_neg_context);
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > > > > > > > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > > > > > > > TCP_Server_Info *server,
> > > > > > > >   return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > > > > > > > +        struct smb2_signing_capabilities *pctxt)
> > > > > > > > +{
> > > > > > > > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > > > > > > > +
> > > > > > > > + if ((len < 4) || (len > 16)) {
> > > > > > > > + pr_warn_once("server sent bad signing negcontext\n");
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > > > > > > > + pr_warn_once("Invalid signing algorithm count\n");
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > > > > > > > + pr_warn_once("unknown signing algorithm\n");
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + server->signing_negotiated = true;
> > > > > > > > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > > > > > > > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > > > > > > > +      server->signing_algorithm);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +
> > > > > > > >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> > > > > > > >        struct TCP_Server_Info *server,
> > > > > > > >        unsigned int len_of_smb)
> > > > > > > > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > > > > > > > smb2_negotiate_rsp *rsp,
> > > > > > > >   (struct smb2_compression_capabilities_context *)pctx);
> > > > > > > >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> > > > > > > >   server->posix_ext_supported = true;
> > > > > > > > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > > > > > > > + decode_signing_ctx(server,
> > > > > > > > + (struct smb2_signing_capabilities *)pctx);
> > > > > > > >   else
> > > > > > > >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> > > > > > > >   le16_to_cpu(pctx->ContextType));
> > > > > > > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > > > > > > > index ba75e65924ac..4b27cb9105fd 100644
> > > > > > > > --- a/fs/cifs/smb2pdu.h
> > > > > > > > +++ b/fs/cifs/smb2pdu.h
> > > > > > > > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> > > > > > > >   __le16 ContextType;
> > > > > > > >   __le16 DataLength;
> > > > > > > >   __le32 Reserved;
> > > > > > > > - /* Followed by array of data */
> > > > > > > > + /* Followed by array of data. NOTE: some servers require padding to
> > > > > > > > 8 byte boundary */
> > > > > > > >  } __packed;
> > > > > > > >
> > > > > > > >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > > > > > > > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> > > > > > > >   __u16 Padding;
> > > > > > > >   __u32 Flags;
> > > > > > > >   __le16 CompressionAlgorithms[3];
> > > > > > > > + /* Check if pad needed */
> > > > > > > >  } __packed;
> > > > > > > >
> > > > > > > >  /*
> > > > > > > > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> > > > > > > >   __le16  DataLength;
> > > > > > > >   __u32 Reserved;
> > > > > > > >   __le32 Flags;
> > > > > > > > + __u32 Pad;
> > > > > > > >  } __packed;
> > > > > > > >
> > > > > > > >  /*
> > > > > > > > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> > > > > > > >   __u32 Reserved;
> > > > > > > >   __le16 SigningAlgorithmCount;
> > > > > > > >   __le16 SigningAlgorithms[];
> > > > > > > > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> > > > > > > >  } __packed;
> > > > > > > >
> > > > > > > >  #define POSIX_CTXT_DATA_LEN 16
> > > > > > > >
> > > > > > > > --
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > Steve
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Steve
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-09 17:37 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-08  2:44 [PATCH][SMB3.1.1] add ability to send signing negotiate context Steve French
2021-07-08  4:27 ` Steve French
2021-07-08  4:49   ` Steve French
2021-07-08  4:51     ` ronnie sahlberg
2021-07-08 22:05       ` Pavel Shilovsky
2021-07-08 23:19         ` Steve French
2021-07-08 23:29           ` Steve French
2021-07-08 23:34             ` Steve French
2021-07-09 17:36               ` Pavel Shilovsky

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox