diff mbox series

encrypt the tcon itself if seal requested on mount and set encryption support for 3.11 properly

Message ID CAH2r5msm6-1Caf6ztd-31H1c_Xy=fu8cWhDcqssSTBGXykr0JA@mail.gmail.com
State New
Headers show
Series encrypt the tcon itself if seal requested on mount and set encryption support for 3.11 properly | expand

Commit Message

Steve French April 20, 2018, 10:11 p.m. UTC
This patch doesn't fix all the problems (mount with 3.11 and "seal"
fails presumably because the validate negotiate like hash for the
signature is not attached to the tcon the right way - signing is
usually disabled when encryption is enabled).

Should the signature be also included in the frame even if the tcon is
encryption in SMB3.11?

Comments

Pavel Shilovsky April 21, 2018, 12:14 a.m. UTC | #1
2018-04-20 15:11 GMT-07:00 Steve French via samba-technical
<samba-technical@lists.samba.org>:
> This patch doesn't fix all the problems (mount with 3.11 and "seal"
> fails presumably because the validate negotiate like hash for the
> signature is not attached to the tcon the right way - signing is
> usually disabled when encryption is enabled).
>
> Should the signature be also included in the frame even if the tcon is
> encryption in SMB3.11?
>
>
> --
> Thanks,
>
> Steve


Looks good. Please also fix the encryption negotiate context:

static void
build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
{
pneg_ctxt->ContextType = SMB2_ENCRYPTION_CAPABILITIES;
pneg_ctxt->DataLength = cpu_to_le16(6);
pneg_ctxt->CipherCount = cpu_to_le16(2);
pneg_ctxt->Ciphers[0] = SMB2_ENCRYPTION_AES128_GCM;
pneg_ctxt->Ciphers[1] = SMB2_ENCRYPTION_AES128_CCM;
}

as we currently do not support AES128_GCM encryption. This is probably
why mount fails.

The SMB3 encryption includes signing, so, I think SMB2 header
signature may be omitted in validate negotiate if encryption is used.

We need to check if the current code works with CCM encryption for SMB
3.0 because there was a stable patch "Validate negotiate request mush
always be signed" that changed the behavior of validate negotiate
request.

Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve French April 21, 2018, 4:55 a.m. UTC | #2
On Fri, Apr 20, 2018 at 7:14 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> Looks good. Please also fix the encryption negotiate context:

 Fixed. Disabled AES-128GCM.  See attached.

Seems to work ok to Windows 3.11 now, and SMB3 tconx is also now
encrypted if "seal" chosen on mount - tried it to Windows 2016 and to
Samba 4.7

Main remaining problem that I see is smb3.11 reconnect (it looks like
we are clearing the hash - but must be missing something)
Steve French April 21, 2018, 5:04 p.m. UTC | #3
Any extra testing would be appreciated of this - I tried to Windows
2016 with and without encrypted share and also to Samba 4.7

On Fri, Apr 20, 2018 at 11:55 PM, Steve French <smfrench@gmail.com> wrote:
> On Fri, Apr 20, 2018 at 7:14 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
>> Looks good. Please also fix the encryption negotiate context:
>
>  Fixed. Disabled AES-128GCM.  See attached.
>
> Seems to work ok to Windows 3.11 now, and SMB3 tconx is also now
> encrypted if "seal" chosen on mount - tried it to Windows 2016 and to
> Samba 4.7
>
> Main remaining problem that I see is smb3.11 reconnect (it looks like
> we are clearing the hash - but must be missing something)
> --
> Thanks,
>
> Steve
Steve French April 22, 2018, 3:44 p.m. UTC | #4
Needed to add one additional minor change for Samba (samba server
doesn't allow the two byte pad at the end of the negotiate context
that was the result of removing one of the ciphers and returned an
error on SMB311 negprot

I need to add:

diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index 6093e5142b2b..d28f358022c5 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -297,7 +297,7 @@ struct smb2_encryption_neg_context {
        __le16  DataLength;
        __le32  Reserved;
        __le16  CipherCount; /* AES-128-GCM and AES-128-CCM */
-       __le16  Ciphers[2]; /* Ciphers[0] since only one used now */
+       __le16  Ciphers[1]; /* Ciphers[0] since only one used now */
 } __packed;

 struct smb2_negotiate_rsp {
sfrench@Ubuntu-17-Virtual-Ma

On Sat, Apr 21, 2018 at 12:04 PM, Steve French <smfrench@gmail.com> wrote:
> Any extra testing would be appreciated of this - I tried to Windows
> 2016 with and without encrypted share and also to Samba 4.7
>
> On Fri, Apr 20, 2018 at 11:55 PM, Steve French <smfrench@gmail.com> wrote:
>> On Fri, Apr 20, 2018 at 7:14 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
>>> Looks good. Please also fix the encryption negotiate context:
>>
>>  Fixed. Disabled AES-128GCM.  See attached.
>>
>> Seems to work ok to Windows 3.11 now, and SMB3 tconx is also now
>> encrypted if "seal" chosen on mount - tried it to Windows 2016 and to
>> Samba 4.7
>>
>> Main remaining problem that I see is smb3.11 reconnect (it looks like
>> we are clearing the hash - but must be missing something)
>> --
>> Thanks,
>>
>> Steve
>
>
>
> --
> Thanks,
>
> Steve
Steve French April 22, 2018, 11:21 p.m. UTC | #5
Version 3 of patch attached (works to Samba and Windows with 3.11)



On Sun, Apr 22, 2018 at 10:44 AM, Steve French <smfrench@gmail.com> wrote:
> Needed to add one additional minor change for Samba (samba server
> doesn't allow the two byte pad at the end of the negotiate context
> that was the result of removing one of the ciphers and returned an
> error on SMB311 negprot
>
> I need to add:
>
> diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> index 6093e5142b2b..d28f358022c5 100644
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -297,7 +297,7 @@ struct smb2_encryption_neg_context {
>         __le16  DataLength;
>         __le32  Reserved;
>         __le16  CipherCount; /* AES-128-GCM and AES-128-CCM */
> -       __le16  Ciphers[2]; /* Ciphers[0] since only one used now */
> +       __le16  Ciphers[1]; /* Ciphers[0] since only one used now */
>  } __packed;
>
>  struct smb2_negotiate_rsp {
> sfrench@Ubuntu-17-Virtual-Ma
>
> On Sat, Apr 21, 2018 at 12:04 PM, Steve French <smfrench@gmail.com> wrote:
>> Any extra testing would be appreciated of this - I tried to Windows
>> 2016 with and without encrypted share and also to Samba 4.7
>>
>> On Fri, Apr 20, 2018 at 11:55 PM, Steve French <smfrench@gmail.com> wrote:
>>> On Fri, Apr 20, 2018 at 7:14 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
>>>> Looks good. Please also fix the encryption negotiate context:
>>>
>>>  Fixed. Disabled AES-128GCM.  See attached.
>>>
>>> Seems to work ok to Windows 3.11 now, and SMB3 tconx is also now
>>> encrypted if "seal" chosen on mount - tried it to Windows 2016 and to
>>> Samba 4.7
>>>
>>> Main remaining problem that I see is smb3.11 reconnect (it looks like
>>> we are clearing the hash - but must be missing something)
>>> --
>>> Thanks,
>>>
>>> Steve
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>
>
>
> --
> Thanks,
>
> Steve
diff mbox series

Patch

From f1f488e9af87dc07f96769e4d834ccbea478e746 Mon Sep 17 00:00:00 2001
From: Steve French <smfrench@gmail.com>
Date: Fri, 20 Apr 2018 17:04:14 -0500
Subject: [PATCH] SMB3: Make sure encryption set for 3.11 and handle encrypted
 smb3 tcon

The tree connect request itself should be encrypted if the client
requests encryption ("seal" on mount), in addition we should be
enabling encryption in 3.11 based on whether we got any valid
encryption ciphers back in negprot (the corresponding session flag is
not set as it is in 3.0 and 3.02)

Signed-off-by: Steve French <smfrench@gmail.com>
CC: Stable <stable@vger.kernel.org>
---
 fs/cifs/connect.c | 32 ++++++++++++++++----------------
 fs/cifs/smb2pdu.c |  1 +
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index e8830f076a7f..a5aa158d535a 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2959,6 +2959,22 @@  cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
 		}
 	}
 
+	if (volume_info->seal) {
+		if (ses->server->vals->protocol_id == 0) {
+			cifs_dbg(VFS,
+				 "SMB3 or later required for encryption\n");
+			rc = -EOPNOTSUPP;
+			goto out_fail;
+		} else if (tcon->ses->server->capabilities &
+					SMB2_GLOBAL_CAP_ENCRYPTION)
+			tcon->seal = true;
+		else {
+			cifs_dbg(VFS, "Encryption is not supported on share\n");
+			rc = -EOPNOTSUPP;
+			goto out_fail;
+		}
+	}
+
 	/*
 	 * BB Do we need to wrap session_mutex around this TCon call and Unix
 	 * SetFS as we do on SessSetup and reconnect?
@@ -3007,22 +3023,6 @@  cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
 		tcon->use_resilient = true;
 	}
 
-	if (volume_info->seal) {
-		if (ses->server->vals->protocol_id == 0) {
-			cifs_dbg(VFS,
-				 "SMB3 or later required for encryption\n");
-			rc = -EOPNOTSUPP;
-			goto out_fail;
-		} else if (tcon->ses->server->capabilities &
-					SMB2_GLOBAL_CAP_ENCRYPTION)
-			tcon->seal = true;
-		else {
-			cifs_dbg(VFS, "Encryption is not supported on share\n");
-			rc = -EOPNOTSUPP;
-			goto out_fail;
-		}
-	}
-
 	/*
 	 * We can have only one retry value for a connection to a share so for
 	 * resources mounted more than once to the same server share the last
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 0f044c4a2dc9..b8928e389147 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -444,6 +444,7 @@  static int decode_encrypt_ctx(struct TCP_Server_Info *server,
 		return -EINVAL;
 	}
 	server->cipher_type = ctxt->Ciphers[0];
+	server->capabilities |= SMB2_GLOBAL_CAP_ENCRYPTION;
 	return 0;
 }
 
-- 
2.14.1