diff mbox series

Add support for GCM256 encryption

Message ID CAH2r5mtAOxF=PCndMTXxj_dZVLc-NQJfoawOvMeS3FbxiCU6xw@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series Add support for GCM256 encryption | expand

Commit Message

Steve French Oct. 15, 2020, 6:21 a.m. UTC 
Patch series attached that adds support for GCM256 encryption.  It
also clarifies errors returned and warnings on mounts where gcm256 is
required but not supported.  To control this two global parms for the
cifs.ko module are introduced
(/sys/module/cifs/parameters/enable_gcm_256 and
/sys/module/cifs/parameters/require_gcm_256  which are both disabled
by default to reduce the risk of any regressions to servers which do
not support gcm256  (a mount option e.g. "seal=gcm256" also can be
introduced after we have had a chance to test against a wider variety
of servers)

When /sys/module/cifs/parameters/enable_gcm_256 is set then we add
gcm256 to the list of ciphers we request during protocol negotiation
(gcm128 preferred, then gcm256, then lowest in the list is ccm128
since it is slower).

When  /sys/module/cifs/parameters/require_gcm_256 is set then we only
request gcm256 and fail if the server does not support it during
protocol negotiation.

One additional change is going to be needed (to set the session key to
the correct size).

Comments

Aurélien Aptel Oct. 15, 2020, 8:49 a.m. UTC | #1 
Hi Steve,

Patch 2:

> From 3897b440fd14dfc7b2ad2b0a922302ea7705b5d9 Mon Sep 17 00:00:00 2001
> From: Steve French <stfrench@microsoft.com>
> Date: Wed, 14 Oct 2020 20:24:09 -0500
> Subject: [PATCH 2/5] smb3.1.1: add new module load parm enable_gcm_256
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -361,8 +361,9 @@ struct smb2_encryption_neg_context {
>  	__le16	ContextType; /* 2 */
>  	__le16	DataLength;
>  	__le32	Reserved;
> -	__le16	CipherCount; /* AES-128-GCM and AES-128-CCM */
> -	__le16	Ciphers[2];
> +	/* CipherCount usally 2, but can be 3 when AES256-GCM enabled */
> +	__le16	CipherCount; /* AES128-GCM and AES128-CCM by defalt */

Typo defalt => default

> +	__le16	Ciphers[3];
>  } __packed;
>  
>  /* See MS-SMB2 2.2.3.1.3 */
> -- 
> 2.25.1
>

Patch 5:

> From 314d7476e404c37acb77c3f9ecc142122e7afbfd Mon Sep 17 00:00:00 2001
> From: Steve French <stfrench@microsoft.com>
> Date: Fri, 11 Sep 2020 16:47:09 -0500
> Subject: [PATCH 5/5] smb3.1.1: set gcm256 when requested
>
> update code to set 32 byte key length and to set gcm256 when requested
> on mount.
>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/cifs/smb2glob.h      |  1 +
>  fs/cifs/smb2ops.c       | 20 ++++++++++++--------
>  fs/cifs/smb2transport.c | 16 ++++++++--------
>  3 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index dd1edabec328..d8e74954d101 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3954,7 +3954,12 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
>  
>  	tfm = enc ? server->secmech.ccmaesencrypt :
>  						server->secmech.ccmaesdecrypt;
> -	rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> +
> +	if (require_gcm_256)
> +		rc = crypto_aead_setkey(tfm, key, SMB3_GCM256_CRYPTKEY_SIZE);

Shouldn't the check be on server->cipher_type?

> +	else
> +		rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> +
>  	if (rc) {
>  		cifs_server_dbg(VFS, "%s: Failed to set aead key %d\n", __func__, rc);
>  		return rc;
Steve French Oct. 15, 2020, 4:33 p.m. UTC | #2 
Good point.  Updated patches attached.  Also added a one line comment
to smb2pdu.h mentioning why we don't request AES_256_CCM


On Thu, Oct 15, 2020 at 3:49 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Hi Steve,
>
> Patch 2:
>
> > From 3897b440fd14dfc7b2ad2b0a922302ea7705b5d9 Mon Sep 17 00:00:00 2001
> > From: Steve French <stfrench@microsoft.com>
> > Date: Wed, 14 Oct 2020 20:24:09 -0500
> > Subject: [PATCH 2/5] smb3.1.1: add new module load parm enable_gcm_256
> > --- a/fs/cifs/smb2pdu.h
> > +++ b/fs/cifs/smb2pdu.h
> > @@ -361,8 +361,9 @@ struct smb2_encryption_neg_context {
> >       __le16  ContextType; /* 2 */
> >       __le16  DataLength;
> >       __le32  Reserved;
> > -     __le16  CipherCount; /* AES-128-GCM and AES-128-CCM */
> > -     __le16  Ciphers[2];
> > +     /* CipherCount usally 2, but can be 3 when AES256-GCM enabled */
> > +     __le16  CipherCount; /* AES128-GCM and AES128-CCM by defalt */
>
> Typo defalt => default
>
> > +     __le16  Ciphers[3];
> >  } __packed;
> >
> >  /* See MS-SMB2 2.2.3.1.3 */
> > --
> > 2.25.1
> >
>
> Patch 5:
>
> > From 314d7476e404c37acb77c3f9ecc142122e7afbfd Mon Sep 17 00:00:00 2001
> > From: Steve French <stfrench@microsoft.com>
> > Date: Fri, 11 Sep 2020 16:47:09 -0500
> > Subject: [PATCH 5/5] smb3.1.1: set gcm256 when requested
> >
> > update code to set 32 byte key length and to set gcm256 when requested
> > on mount.
> >
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/cifs/smb2glob.h      |  1 +
> >  fs/cifs/smb2ops.c       | 20 ++++++++++++--------
> >  fs/cifs/smb2transport.c | 16 ++++++++--------
> >  3 files changed, 21 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index dd1edabec328..d8e74954d101 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -3954,7 +3954,12 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> >
> >       tfm = enc ? server->secmech.ccmaesencrypt :
> >                                               server->secmech.ccmaesdecrypt;
> > -     rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> > +
> > +     if (require_gcm_256)
> > +             rc = crypto_aead_setkey(tfm, key, SMB3_GCM256_CRYPTKEY_SIZE);
>
> Shouldn't the check be on server->cipher_type?
>
> > +     else
> > +             rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> > +
> >       if (rc) {
> >               cifs_server_dbg(VFS, "%s: Failed to set aead key %d\n", __func__, rc);
> >               return rc;
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Steve French Oct. 15, 2020, 5:41 p.m. UTC | #3 
found another typo in patch 5 ccm instead of gcm - fixing it now

On Thu, Oct 15, 2020 at 11:33 AM Steve French <smfrench@gmail.com> wrote:
>
> Good point.  Updated patches attached.  Also added a one line comment
> to smb2pdu.h mentioning why we don't request AES_256_CCM
>
>
> On Thu, Oct 15, 2020 at 3:49 AM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > Hi Steve,
> >
> > Patch 2:
> >
> > > From 3897b440fd14dfc7b2ad2b0a922302ea7705b5d9 Mon Sep 17 00:00:00 2001
> > > From: Steve French <stfrench@microsoft.com>
> > > Date: Wed, 14 Oct 2020 20:24:09 -0500
> > > Subject: [PATCH 2/5] smb3.1.1: add new module load parm enable_gcm_256
> > > --- a/fs/cifs/smb2pdu.h
> > > +++ b/fs/cifs/smb2pdu.h
> > > @@ -361,8 +361,9 @@ struct smb2_encryption_neg_context {
> > >       __le16  ContextType; /* 2 */
> > >       __le16  DataLength;
> > >       __le32  Reserved;
> > > -     __le16  CipherCount; /* AES-128-GCM and AES-128-CCM */
> > > -     __le16  Ciphers[2];
> > > +     /* CipherCount usally 2, but can be 3 when AES256-GCM enabled */
> > > +     __le16  CipherCount; /* AES128-GCM and AES128-CCM by defalt */
> >
> > Typo defalt => default
> >
> > > +     __le16  Ciphers[3];
> > >  } __packed;
> > >
> > >  /* See MS-SMB2 2.2.3.1.3 */
> > > --
> > > 2.25.1
> > >
> >
> > Patch 5:
> >
> > > From 314d7476e404c37acb77c3f9ecc142122e7afbfd Mon Sep 17 00:00:00 2001
> > > From: Steve French <stfrench@microsoft.com>
> > > Date: Fri, 11 Sep 2020 16:47:09 -0500
> > > Subject: [PATCH 5/5] smb3.1.1: set gcm256 when requested
> > >
> > > update code to set 32 byte key length and to set gcm256 when requested
> > > on mount.
> > >
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > ---
> > >  fs/cifs/smb2glob.h      |  1 +
> > >  fs/cifs/smb2ops.c       | 20 ++++++++++++--------
> > >  fs/cifs/smb2transport.c | 16 ++++++++--------
> > >  3 files changed, 21 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > index dd1edabec328..d8e74954d101 100644
> > > --- a/fs/cifs/smb2ops.c
> > > +++ b/fs/cifs/smb2ops.c
> > > @@ -3954,7 +3954,12 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> > >
> > >       tfm = enc ? server->secmech.ccmaesencrypt :
> > >                                               server->secmech.ccmaesdecrypt;
> > > -     rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> > > +
> > > +     if (require_gcm_256)
> > > +             rc = crypto_aead_setkey(tfm, key, SMB3_GCM256_CRYPTKEY_SIZE);
> >
> > Shouldn't the check be on server->cipher_type?
> >
> > > +     else
> > > +             rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> > > +
> > >       if (rc) {
> > >               cifs_server_dbg(VFS, "%s: Failed to set aead key %d\n", __func__, rc);
> > >               return rc;
> >
> > --
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
>
>
> --
> Thanks,
>
> Steve
Steve French Oct. 16, 2020, 4:45 a.m. UTC | #4 
Redid patch 5 (includes Aurelien's suggestion, fixes a typo and fixes
a problem with vers=3.0 mounts) - attached.

On Thu, Oct 15, 2020 at 12:41 PM Steve French <smfrench@gmail.com> wrote:
>
> found another typo in patch 5 ccm instead of gcm - fixing it now
>
> On Thu, Oct 15, 2020 at 11:33 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Good point.  Updated patches attached.  Also added a one line comment
> > to smb2pdu.h mentioning why we don't request AES_256_CCM
> >
> >
> > On Thu, Oct 15, 2020 at 3:49 AM Aurélien Aptel <aaptel@suse.com> wrote:
> > >
> > > Hi Steve,
> > >
> > > Patch 2:
> > >
> > > > From 3897b440fd14dfc7b2ad2b0a922302ea7705b5d9 Mon Sep 17 00:00:00 2001
> > > > From: Steve French <stfrench@microsoft.com>
> > > > Date: Wed, 14 Oct 2020 20:24:09 -0500
> > > > Subject: [PATCH 2/5] smb3.1.1: add new module load parm enable_gcm_256
> > > > --- a/fs/cifs/smb2pdu.h
> > > > +++ b/fs/cifs/smb2pdu.h
> > > > @@ -361,8 +361,9 @@ struct smb2_encryption_neg_context {
> > > >       __le16  ContextType; /* 2 */
> > > >       __le16  DataLength;
> > > >       __le32  Reserved;
> > > > -     __le16  CipherCount; /* AES-128-GCM and AES-128-CCM */
> > > > -     __le16  Ciphers[2];
> > > > +     /* CipherCount usally 2, but can be 3 when AES256-GCM enabled */
> > > > +     __le16  CipherCount; /* AES128-GCM and AES128-CCM by defalt */
> > >
> > > Typo defalt => default
> > >
> > > > +     __le16  Ciphers[3];
> > > >  } __packed;
> > > >
> > > >  /* See MS-SMB2 2.2.3.1.3 */
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > Patch 5:
> > >
> > > > From 314d7476e404c37acb77c3f9ecc142122e7afbfd Mon Sep 17 00:00:00 2001
> > > > From: Steve French <stfrench@microsoft.com>
> > > > Date: Fri, 11 Sep 2020 16:47:09 -0500
> > > > Subject: [PATCH 5/5] smb3.1.1: set gcm256 when requested
> > > >
> > > > update code to set 32 byte key length and to set gcm256 when requested
> > > > on mount.
> > > >
> > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > > ---
> > > >  fs/cifs/smb2glob.h      |  1 +
> > > >  fs/cifs/smb2ops.c       | 20 ++++++++++++--------
> > > >  fs/cifs/smb2transport.c | 16 ++++++++--------
> > > >  3 files changed, 21 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > index dd1edabec328..d8e74954d101 100644
> > > > --- a/fs/cifs/smb2ops.c
> > > > +++ b/fs/cifs/smb2ops.c
> > > > @@ -3954,7 +3954,12 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> > > >
> > > >       tfm = enc ? server->secmech.ccmaesencrypt :
> > > >                                               server->secmech.ccmaesdecrypt;
> > > > -     rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> > > > +
> > > > +     if (require_gcm_256)
> > > > +             rc = crypto_aead_setkey(tfm, key, SMB3_GCM256_CRYPTKEY_SIZE);
> > >
> > > Shouldn't the check be on server->cipher_type?
> > >
> > > > +     else
> > > > +             rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> > > > +
> > > >       if (rc) {
> > > >               cifs_server_dbg(VFS, "%s: Failed to set aead key %d\n", __func__, rc);
> > > >               return rc;
> > >
> > > --
> > > Aurélien Aptel / SUSE Labs Samba Team
> > > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve
Stefan Metzmacher Oct. 22, 2020, 12:59 p.m. UTC | #5 
Am 16.10.20 um 06:45 schrieb Steve French:
> Redid patch 5 (includes Aurelien's suggestion, fixes a typo and fixes
> a problem with vers=3.0 mounts) - attached.
> 
> On Thu, Oct 15, 2020 at 12:41 PM Steve French <smfrench@gmail.com> wrote:
>>
>> found another typo in patch 5 ccm instead of gcm - fixing it now
>>
>> On Thu, Oct 15, 2020 at 11:33 AM Steve French <smfrench@gmail.com> wrote:
>>>
>>> Good point.  Updated patches attached.  Also added a one line comment
>>> to smb2pdu.h mentioning why we don't request AES_256_CCM
>>>
>>>
>>> On Thu, Oct 15, 2020 at 3:49 AM Aurélien Aptel <aaptel@suse.com> wrote:
>>>>
>>>> Hi Steve,
>>>>
>>>> Patch 2:
>>>>
>>>>> From 3897b440fd14dfc7b2ad2b0a922302ea7705b5d9 Mon Sep 17 00:00:00 2001
>>>>> From: Steve French <stfrench@microsoft.com>
>>>>> Date: Wed, 14 Oct 2020 20:24:09 -0500
>>>>> Subject: [PATCH 2/5] smb3.1.1: add new module load parm enable_gcm_256
>>>>> --- a/fs/cifs/smb2pdu.h
>>>>> +++ b/fs/cifs/smb2pdu.h
>>>>> @@ -361,8 +361,9 @@ struct smb2_encryption_neg_context {
>>>>>       __le16  ContextType; /* 2 */
>>>>>       __le16  DataLength;
>>>>>       __le32  Reserved;
>>>>> -     __le16  CipherCount; /* AES-128-GCM and AES-128-CCM */
>>>>> -     __le16  Ciphers[2];
>>>>> +     /* CipherCount usally 2, but can be 3 when AES256-GCM enabled */
>>>>> +     __le16  CipherCount; /* AES128-GCM and AES128-CCM by defalt */
>>>>
>>>> Typo defalt => default
>>>>
>>>>> +     __le16  Ciphers[3];
>>>>>  } __packed;
>>>>>
>>>>>  /* See MS-SMB2 2.2.3.1.3 */
>>>>> --
>>>>> 2.25.1
>>>>>
>>>>
>>>> Patch 5:
>>>>
>>>>> From 314d7476e404c37acb77c3f9ecc142122e7afbfd Mon Sep 17 00:00:00 2001
>>>>> From: Steve French <stfrench@microsoft.com>
>>>>> Date: Fri, 11 Sep 2020 16:47:09 -0500
>>>>> Subject: [PATCH 5/5] smb3.1.1: set gcm256 when requested
>>>>>
>>>>> update code to set 32 byte key length and to set gcm256 when requested
>>>>> on mount.
>>>>>
>>>>> Signed-off-by: Steve French <stfrench@microsoft.com>
>>>>> ---
>>>>>  fs/cifs/smb2glob.h      |  1 +
>>>>>  fs/cifs/smb2ops.c       | 20 ++++++++++++--------
>>>>>  fs/cifs/smb2transport.c | 16 ++++++++--------
>>>>>  3 files changed, 21 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>>>>> index dd1edabec328..d8e74954d101 100644
>>>>> --- a/fs/cifs/smb2ops.c
>>>>> +++ b/fs/cifs/smb2ops.c
>>>>> @@ -3954,7 +3954,12 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
>>>>>
>>>>>       tfm = enc ? server->secmech.ccmaesencrypt :
>>>>>                                               server->secmech.ccmaesdecrypt;
>>>>> -     rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
>>>>> +
>>>>> +     if (require_gcm_256)
>>>>> +             rc = crypto_aead_setkey(tfm, key, SMB3_GCM256_CRYPTKEY_SIZE);
>>>>
>>>> Shouldn't the check be on server->cipher_type?
>>>>
>>>>> +     else
>>>>> +             rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
>>>>> +

You still just use u8 key[SMB3_SIGN_KEY_SIZE];

Shouldn't smb2_get_enc_key() get the buffer size and return the size actually used?

I also don't see where you setup the 32 byte encryption/decryption keys from
the authentication session key?

[MS-SMB2] 3.3.5.5.3 Handling GSS-API Authentication point 11.)
specifies that the full authentication session key should be used as key derivation key
for AES256 (NTLMSSP always returns 16 bytes, kerberos can return 16 or 32 bytes).
3.3.1.8 Per Session says the resulting keys should be 256-bit (32 bytes) for AES-256.

I don't see any of this in your patchset.

Did you actually tested this successful against a Windows Server?

Can you use 'git format-patch --stdout > patches.txt' and attach patches.txt
as inline text attachment? Or use git send-email ...
Individual randomly sorted non text/plain attachments are very hard to comment on
(at least for me).

metze
Steve French Oct. 22, 2020, 5:24 p.m. UTC | #6 
The patch series is missing the final piece (the small change to
encryption related parms), because I was waiting to test that part
with Pavel.  I did verify the negotiation works fine to the newest
Windows download mentioned earlier but also wanted to try it to Azure
and expect the final change needed to be small.  I had also been
trying to focus on the larger changes needed during the merge window
and finish the final piece off next week of this one.


On Thu, Oct 22, 2020 at 7:59 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> Am 16.10.20 um 06:45 schrieb Steve French:
> > Redid patch 5 (includes Aurelien's suggestion, fixes a typo and fixes
> > a problem with vers=3.0 mounts) - attached.
> >
> > On Thu, Oct 15, 2020 at 12:41 PM Steve French <smfrench@gmail.com> wrote:
> >>
> >> found another typo in patch 5 ccm instead of gcm - fixing it now
> >>
> >> On Thu, Oct 15, 2020 at 11:33 AM Steve French <smfrench@gmail.com> wrote:
> >>>
> >>> Good point.  Updated patches attached.  Also added a one line comment
> >>> to smb2pdu.h mentioning why we don't request AES_256_CCM
> >>>
> >>>
> >>> On Thu, Oct 15, 2020 at 3:49 AM Aurélien Aptel <aaptel@suse.com> wrote:
> >>>>
> >>>> Hi Steve,
> >>>>
> >>>> Patch 2:
> >>>>
> >>>>> From 3897b440fd14dfc7b2ad2b0a922302ea7705b5d9 Mon Sep 17 00:00:00 2001
> >>>>> From: Steve French <stfrench@microsoft.com>
> >>>>> Date: Wed, 14 Oct 2020 20:24:09 -0500
> >>>>> Subject: [PATCH 2/5] smb3.1.1: add new module load parm enable_gcm_256
> >>>>> --- a/fs/cifs/smb2pdu.h
> >>>>> +++ b/fs/cifs/smb2pdu.h
> >>>>> @@ -361,8 +361,9 @@ struct smb2_encryption_neg_context {
> >>>>>       __le16  ContextType; /* 2 */
> >>>>>       __le16  DataLength;
> >>>>>       __le32  Reserved;
> >>>>> -     __le16  CipherCount; /* AES-128-GCM and AES-128-CCM */
> >>>>> -     __le16  Ciphers[2];
> >>>>> +     /* CipherCount usally 2, but can be 3 when AES256-GCM enabled */
> >>>>> +     __le16  CipherCount; /* AES128-GCM and AES128-CCM by defalt */
> >>>>
> >>>> Typo defalt => default
> >>>>
> >>>>> +     __le16  Ciphers[3];
> >>>>>  } __packed;
> >>>>>
> >>>>>  /* See MS-SMB2 2.2.3.1.3 */
> >>>>> --
> >>>>> 2.25.1
> >>>>>
> >>>>
> >>>> Patch 5:
> >>>>
> >>>>> From 314d7476e404c37acb77c3f9ecc142122e7afbfd Mon Sep 17 00:00:00 2001
> >>>>> From: Steve French <stfrench@microsoft.com>
> >>>>> Date: Fri, 11 Sep 2020 16:47:09 -0500
> >>>>> Subject: [PATCH 5/5] smb3.1.1: set gcm256 when requested
> >>>>>
> >>>>> update code to set 32 byte key length and to set gcm256 when requested
> >>>>> on mount.
> >>>>>
> >>>>> Signed-off-by: Steve French <stfrench@microsoft.com>
> >>>>> ---
> >>>>>  fs/cifs/smb2glob.h      |  1 +
> >>>>>  fs/cifs/smb2ops.c       | 20 ++++++++++++--------
> >>>>>  fs/cifs/smb2transport.c | 16 ++++++++--------
> >>>>>  3 files changed, 21 insertions(+), 16 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> >>>>> index dd1edabec328..d8e74954d101 100644
> >>>>> --- a/fs/cifs/smb2ops.c
> >>>>> +++ b/fs/cifs/smb2ops.c
> >>>>> @@ -3954,7 +3954,12 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> >>>>>
> >>>>>       tfm = enc ? server->secmech.ccmaesencrypt :
> >>>>>                                               server->secmech.ccmaesdecrypt;
> >>>>> -     rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> >>>>> +
> >>>>> +     if (require_gcm_256)
> >>>>> +             rc = crypto_aead_setkey(tfm, key, SMB3_GCM256_CRYPTKEY_SIZE);
> >>>>
> >>>> Shouldn't the check be on server->cipher_type?
> >>>>
> >>>>> +     else
> >>>>> +             rc = crypto_aead_setkey(tfm, key, SMB3_SIGN_KEY_SIZE);
> >>>>> +
>
> You still just use u8 key[SMB3_SIGN_KEY_SIZE];
>
> Shouldn't smb2_get_enc_key() get the buffer size and return the size actually used?
>
> I also don't see where you setup the 32 byte encryption/decryption keys from
> the authentication session key?
>
> [MS-SMB2] 3.3.5.5.3 Handling GSS-API Authentication point 11.)
> specifies that the full authentication session key should be used as key derivation key
> for AES256 (NTLMSSP always returns 16 bytes, kerberos can return 16 or 32 bytes).
> 3.3.1.8 Per Session says the resulting keys should be 256-bit (32 bytes) for AES-256.
>
> I don't see any of this in your patchset.
>
> Did you actually tested this successful against a Windows Server?
>
> Can you use 'git format-patch --stdout > patches.txt' and attach patches.txt
> as inline text attachment? Or use git send-email ...
> Individual randomly sorted non text/plain attachments are very hard to comment on
> (at least for me).
>
> metze
>


--
Thanks,

Steve
diff mbox series

Patch

From 671d6f325f958b6123ee467a3e00fa134cf9195f Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 11 Sep 2020 16:19:28 -0500
Subject: [PATCH 1/5] smb3.1.1: add new module load parm require_gcm_256

Add new module load parameter require_gcm_256. If set, then only
request AES-256-GCM (strongest encryption type).

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

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 0fb99d25e8a8..462dbbd17c5f 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -71,6 +71,7 @@  bool enable_oplocks = true;
 bool linuxExtEnabled = true;
 bool lookupCacheEnabled = true;
 bool disable_legacy_dialects; /* false by default */
+bool require_gcm_256; /* 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(slow_rsp_threshold, "Amount of time (in seconds) to wait "
 module_param(enable_oplocks, bool, 0644);
 MODULE_PARM_DESC(enable_oplocks, "Enable or disable oplocks. Default: y/Y/1");
 
+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(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 5a491afafacc..ec21af833749 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1956,6 +1956,7 @@  extern bool lookupCacheEnabled;
 extern unsigned int global_secflags;	/* if on, session setup sent
 				with more secure ntlmssp2 challenge/resp */
 extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
+extern bool require_gcm_256; /* require use of strongest signing (aes-gcm-256) */
 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 96c172d94fba..fcae1e3dfcc5 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -449,10 +449,16 @@  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); /* Cipher Count + two ciphers */
-	pneg_ctxt->CipherCount = cpu_to_le16(2);
-	pneg_ctxt->Ciphers[0] = SMB2_ENCRYPTION_AES128_GCM;
-	pneg_ctxt->Ciphers[1] = SMB2_ENCRYPTION_AES128_CCM;
+	if (require_gcm_256) {
+		pneg_ctxt->DataLength = cpu_to_le16(4); /* Cipher Count + 1 cipher */
+		pneg_ctxt->CipherCount = cpu_to_le16(1);
+		pneg_ctxt->Ciphers[0] = SMB2_ENCRYPTION_AES256_GCM;
+	} else {
+		pneg_ctxt->DataLength = cpu_to_le16(6); /* Cipher Count + 2 ciphers */
+		pneg_ctxt->CipherCount = cpu_to_le16(2);
+		pneg_ctxt->Ciphers[0] = SMB2_ENCRYPTION_AES128_GCM;
+		pneg_ctxt->Ciphers[1] = SMB2_ENCRYPTION_AES128_CCM;
+	}
 }
 
 static unsigned int
-- 
2.25.1