[PATCHv2,CIFS] Validate Negotiate Info

Commit Message

Steve French Nov. 19, 2013, 7:20 p.m. UTC

From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
From: Steve French <[e-mail]>
Date: Tue, 19 Nov 2013 12:58:08 -0600
Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks

When we are running SMB3 or SMB3.02 connections which are signed
we need to validate the protocol negotiation information,
to ensure that thenegotiate protocol response was not tampered with.

Add the missing FSCTL which is sent at mount time (immediately after
the SMB3 Tree Connect) to validate that the capabilities match
what we think the server sent.

"Secure dialect negotiation is introduced in SMB3 to protect against
man-in-the-middle attempt to downgrade dialect negotiation.
The idea is to prevent an eavesdropper from downgrading the initially
negotiated dialect and capabilities between the client and the server."

For more explanation see 2.2.31.4 of MS-SMB2 or
http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx

Signed-off-by: Steve French <[e-mail]>
---
 fs/cifs/cifsglob.h  |  1 +
 fs/cifs/smb2ops.c   |  1 +
 fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/smb2pdu.h   | 12 ++++++---
 fs/cifs/smb2proto.h |  1 +
 fs/cifs/smbfsctl.h  |  2 +-
 6 files changed, 89 insertions(+), 4 deletions(-)

Comments

Pavel Shilovsky Nov. 20, 2013, 5:33 a.m. UTC | #1

2013/11/19 Steve French <[e-mail]>:
> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
> From: Steve French <[e-mail]>
> Date: Tue, 19 Nov 2013 12:58:08 -0600
> Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks
>
> When we are running SMB3 or SMB3.02 connections which are signed
> we need to validate the protocol negotiation information,
> to ensure that thenegotiate protocol response was not tampered with.
>
> Add the missing FSCTL which is sent at mount time (immediately after
> the SMB3 Tree Connect) to validate that the capabilities match
> what we think the server sent.
>
> "Secure dialect negotiation is introduced in SMB3 to protect against
> man-in-the-middle attempt to downgrade dialect negotiation.
> The idea is to prevent an eavesdropper from downgrading the initially
> negotiated dialect and capabilities between the client and the server."
>
> For more explanation see 2.2.31.4 of MS-SMB2 or
> http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx
>
> Signed-off-by: Steve French <[e-mail]>
> ---
>  fs/cifs/cifsglob.h  |  1 +
>  fs/cifs/smb2ops.c   |  1 +
>  fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/smb2pdu.h   | 12 ++++++---
>  fs/cifs/smb2proto.h |  1 +
>  fs/cifs/smbfsctl.h  |  2 +-
>  6 files changed, 89 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index d9ea7ad..f918a99 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -384,6 +384,7 @@ struct smb_version_operations {
>   int (*clone_range)(const unsigned int, struct cifsFileInfo *src_file,
>   struct cifsFileInfo *target_file, u64 src_off, u64 len,
>   u64 dest_off);
> + int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
>  };
>
>  struct smb_version_values {
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index a3968ee..757da3e 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1319,6 +1319,7 @@ struct smb_version_operations smb30_operations = {
>   .create_lease_buf = smb3_create_lease_buf,
>   .parse_lease_buf = smb3_parse_lease_buf,
>   .clone_range = smb2_clone_range,
> + .validate_negotiate = smb3_validate_negotiate,
>  };
>
>  struct smb_version_values smb20_values = {
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 1e136ee..7756f9b 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -454,6 +454,80 @@ neg_exit:
>   return rc;
>  }
>
> +int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> +{
> + int rc = 0;
> + struct validate_negotiate_info_req vneg_inbuf;
> + struct validate_negotiate_info_rsp *pneg_rsp;
> + u32 rsplen;
> +
> + cifs_dbg(VFS, "validate negotiate ");
> + vneg_inbuf.Capabilities =
> + cpu_to_le32(tcon->ses->server->vals->req_capabilities);
> + memcpy(vneg_inbuf.Guid, cifs_client_guid, SMB2_CLIENT_GUID_SIZE);
> +
> + /*
> + * validation ioctl must be signed, so no point sending this if we
> + * can not sign it.  We could eventually change this to selectively
> + * sign just this, the first and only signed request on a connection.
> + * This is good enough for now since a user who wants better security
> + * would also enable signing on the mount. Having validation of
> + * negotiate info for signed connections helps reduce attack vectors
> + */
> + if (tcon->ses->server->sign == false)
> + return 0; /* validation requires signing */

Shouldn't we move the above check and comments before we set
Capabilities and Guid of vneg_inbuf?

> +
> + if (tcon->ses->sign)
> + vneg_inbuf.SecurityMode =
> + cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
> + else if (global_secflags & CIFSSEC_MAY_SIGN)
> + vneg_inbuf.SecurityMode =
> + cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
> + else
> + vneg_inbuf.SecurityMode = 0;
> +
> + vneg_inbuf.DialectCount = cpu_to_le16(1);
> + vneg_inbuf.Dialects[0] =
> + cpu_to_le16(tcon->ses->server->vals->protocol_id);
> +
> + rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> + FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
> + (char *)&vneg_inbuf, sizeof(struct validate_negotiate_info_req),
> + (char **)&pneg_rsp, &rsplen);
> +
> + if (rc != 0) {
> + cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
> + return -EIO;
> + }
> +
> + if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
> + cifs_dbg(VFS, "invalid size of protocol negotiate response\n");
> + return -EIO;
> + }
> +
> + /* check validate negotiate info response matches what we got earlier */
> + if (pneg_rsp->Dialect !=
> + cpu_to_le16(tcon->ses->server->vals->protocol_id))
> + goto vneg_out;
> +
> + if (pneg_rsp->SecurityMode != cpu_to_le16(tcon->ses->server->sec_mode))
> + goto vneg_out;
> +
> + /* do not validate server guid because not saved at negprot time yet */
> +
> + if ((le32_to_cpu(pneg_rsp->Capabilities) | SMB2_NT_FIND |
> +      SMB2_LARGE_FILES) != tcon->ses->server->capabilities)
> + goto vneg_out;
> +
> + /* validate negotiate successful */
> + cifs_dbg(FYI, "validate negotiate info successful\n");
> + return 0;
> +
> +vneg_out:
> + cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
> + return -EIO;
> +}
> +
>  int
>  SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
>   const struct nls_table *nls_cp)
> @@ -829,6 +903,8 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses
> *ses, const char *tree,
>      ((tcon->share_flags & SHI1005_FLAGS_DFS) == 0))
>   cifs_dbg(VFS, "DFS capability contradicts DFS flag\n");
>   init_copy_chunk_defaults(tcon);
> + if (tcon->ses->server->ops->validate_negotiate)
> + rc = tcon->ses->server->ops->validate_negotiate(xid, tcon);
>  tcon_exit:
>   free_rsp_buf(resp_buftype, rsp);
>   kfree(unc_path);
> diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> index f88320b..2022c54 100644
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -577,13 +577,19 @@ struct copychunk_ioctl_rsp {
>   __le32 TotalBytesWritten;
>  } __packed;
>
> -/* Response and Request are the same format */
> -struct validate_negotiate_info {
> +struct validate_negotiate_info_req {
>   __le32 Capabilities;
>   __u8   Guid[SMB2_CLIENT_GUID_SIZE];
>   __le16 SecurityMode;
>   __le16 DialectCount;
> - __le16 Dialect[1];
> + __le16 Dialects[1]; /* dialect (someday maybe list) client asked for */
> +} __packed;
> +
> +struct validate_negotiate_info_rsp {
> + __le32 Capabilities;
> + __u8   Guid[SMB2_CLIENT_GUID_SIZE];
> + __le16 SecurityMode;
> + __le16 Dialect; /* Dialect in use for the connection */
>  } __packed;
>
>  #define RSS_CAPABLE 0x00000001
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index b4eea10..93adc64 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -162,5 +162,6 @@ extern int smb2_lockv(const unsigned int xid,
> struct cifs_tcon *tcon,
>        struct smb2_lock_element *buf);
>  extern int SMB2_lease_break(const unsigned int xid, struct cifs_tcon *tcon,
>      __u8 *lease_key, const __le32 lease_state);
> +extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
>
>  #endif /* _SMB2PROTO_H */
> diff --git a/fs/cifs/smbfsctl.h b/fs/cifs/smbfsctl.h
> index a4b2391f..0e538b5 100644
> --- a/fs/cifs/smbfsctl.h
> +++ b/fs/cifs/smbfsctl.h
> @@ -90,7 +90,7 @@
>  #define FSCTL_LMR_REQUEST_RESILIENCY 0x001401D4 /* BB add struct */
>  #define FSCTL_LMR_GET_LINK_TRACK_INF 0x001400E8 /* BB add struct */
>  #define FSCTL_LMR_SET_LINK_TRACK_INF 0x001400EC /* BB add struct */
> -#define FSCTL_VALIDATE_NEGOTIATE_INFO 0x00140204 /* BB add struct */
> +#define FSCTL_VALIDATE_NEGOTIATE_INFO 0x00140204
>  /* Perform server-side data movement */
>  #define FSCTL_SRV_COPYCHUNK 0x001440F2
>  #define FSCTL_SRV_COPYCHUNK_WRITE 0x001480F2
> --
> 1.8.3.1

Except the note above, the patch looks good.

Reviewed-by: Pavel Shilovsky <[e-mail]>

Steve French Nov. 20, 2013, 5:34 a.m. UTC | #2

On Tue, Nov 19, 2013 at 11:33 PM, Pavel Shilovsky <[e-mail]> wrote:
> 2013/11/19 Steve French <[e-mail]>:
>> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
>> From: Steve French <[e-mail]>
>> Date: Tue, 19 Nov 2013 12:58:08 -0600
>> Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks
>>
>> When we are running SMB3 or SMB3.02 connections which are signed
>> we need to validate the protocol negotiation information,
>> to ensure that thenegotiate protocol response was not tampered with.
>>
>> Add the missing FSCTL which is sent at mount time (immediately after
>> the SMB3 Tree Connect) to validate that the capabilities match
>> what we think the server sent.
>>
>> "Secure dialect negotiation is introduced in SMB3 to protect against
>> man-in-the-middle attempt to downgrade dialect negotiation.
>> The idea is to prevent an eavesdropper from downgrading the initially
>> negotiated dialect and capabilities between the client and the server."
>>
>> For more explanation see 2.2.31.4 of MS-SMB2 or
>> http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx
>>
>> Signed-off-by: Steve French <[e-mail]>
>> ---
>>  fs/cifs/cifsglob.h  |  1 +
>>  fs/cifs/smb2ops.c   |  1 +
>>  fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/cifs/smb2pdu.h   | 12 ++++++---
>>  fs/cifs/smb2proto.h |  1 +
>>  fs/cifs/smbfsctl.h  |  2 +-
>>  6 files changed, 89 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index d9ea7ad..f918a99 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -384,6 +384,7 @@ struct smb_version_operations {
>>   int (*clone_range)(const unsigned int, struct cifsFileInfo *src_file,
>>   struct cifsFileInfo *target_file, u64 src_off, u64 len,
>>   u64 dest_off);
>> + int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
>>  };
>>
>>  struct smb_version_values {
>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>> index a3968ee..757da3e 100644
>> --- a/fs/cifs/smb2ops.c
>> +++ b/fs/cifs/smb2ops.c
>> @@ -1319,6 +1319,7 @@ struct smb_version_operations smb30_operations = {
>>   .create_lease_buf = smb3_create_lease_buf,
>>   .parse_lease_buf = smb3_parse_lease_buf,
>>   .clone_range = smb2_clone_range,
>> + .validate_negotiate = smb3_validate_negotiate,
>>  };
>>
>>  struct smb_version_values smb20_values = {
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 1e136ee..7756f9b 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -454,6 +454,80 @@ neg_exit:
>>   return rc;
>>  }
>>
>> +int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>> +{
>> + int rc = 0;
>> + struct validate_negotiate_info_req vneg_inbuf;
>> + struct validate_negotiate_info_rsp *pneg_rsp;
>> + u32 rsplen;
>> +
>> + cifs_dbg(VFS, "validate negotiate ");
>> + vneg_inbuf.Capabilities =
>> + cpu_to_le32(tcon->ses->server->vals->req_capabilities);
>> + memcpy(vneg_inbuf.Guid, cifs_client_guid, SMB2_CLIENT_GUID_SIZE);
>> +
>> + /*
>> + * validation ioctl must be signed, so no point sending this if we
>> + * can not sign it.  We could eventually change this to selectively
>> + * sign just this, the first and only signed request on a connection.
>> + * This is good enough for now since a user who wants better security
>> + * would also enable signing on the mount. Having validation of
>> + * negotiate info for signed connections helps reduce attack vectors
>> + */
>> + if (tcon->ses->server->sign == false)
>> + return 0; /* validation requires signing */
>
> Shouldn't we move the above check and comments before we set
> Capabilities and Guid of vneg_inbuf?

Yes - will do

Steve French Nov. 20, 2013, 5:50 a.m. UTC | #3

Updated patch with the minor change you suggested is now in
cifs-2.6.git for-next

http://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=6f3219c1bc3b0f33a513ba63cea8278f1cd89ab3

On Tue, Nov 19, 2013 at 11:34 PM, Steve French <[e-mail]> wrote:
> On Tue, Nov 19, 2013 at 11:33 PM, Pavel Shilovsky <[e-mail]> wrote:
>> 2013/11/19 Steve French <[e-mail]>:
>>> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
>>> From: Steve French <[e-mail]>
>>> Date: Tue, 19 Nov 2013 12:58:08 -0600
>>> Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks
>>>
>>> When we are running SMB3 or SMB3.02 connections which are signed
>>> we need to validate the protocol negotiation information,
>>> to ensure that thenegotiate protocol response was not tampered with.
>>>
>>> Add the missing FSCTL which is sent at mount time (immediately after
>>> the SMB3 Tree Connect) to validate that the capabilities match
>>> what we think the server sent.
>>>
>>> "Secure dialect negotiation is introduced in SMB3 to protect against
>>> man-in-the-middle attempt to downgrade dialect negotiation.
>>> The idea is to prevent an eavesdropper from downgrading the initially
>>> negotiated dialect and capabilities between the client and the server."
>>>
>>> For more explanation see 2.2.31.4 of MS-SMB2 or
>>> http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx
>>>
>>> Signed-off-by: Steve French <[e-mail]>
>>> ---
>>>  fs/cifs/cifsglob.h  |  1 +
>>>  fs/cifs/smb2ops.c   |  1 +
>>>  fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  fs/cifs/smb2pdu.h   | 12 ++++++---
>>>  fs/cifs/smb2proto.h |  1 +
>>>  fs/cifs/smbfsctl.h  |  2 +-
>>>  6 files changed, 89 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>>> index d9ea7ad..f918a99 100644
>>> --- a/fs/cifs/cifsglob.h
>>> +++ b/fs/cifs/cifsglob.h
>>> @@ -384,6 +384,7 @@ struct smb_version_operations {
>>>   int (*clone_range)(const unsigned int, struct cifsFileInfo *src_file,
>>>   struct cifsFileInfo *target_file, u64 src_off, u64 len,
>>>   u64 dest_off);
>>> + int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
>>>  };
>>>
>>>  struct smb_version_values {
>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>>> index a3968ee..757da3e 100644
>>> --- a/fs/cifs/smb2ops.c
>>> +++ b/fs/cifs/smb2ops.c
>>> @@ -1319,6 +1319,7 @@ struct smb_version_operations smb30_operations = {
>>>   .create_lease_buf = smb3_create_lease_buf,
>>>   .parse_lease_buf = smb3_parse_lease_buf,
>>>   .clone_range = smb2_clone_range,
>>> + .validate_negotiate = smb3_validate_negotiate,
>>>  };
>>>
>>>  struct smb_version_values smb20_values = {
>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>>> index 1e136ee..7756f9b 100644
>>> --- a/fs/cifs/smb2pdu.c
>>> +++ b/fs/cifs/smb2pdu.c
>>> @@ -454,6 +454,80 @@ neg_exit:
>>>   return rc;
>>>  }
>>>
>>> +int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>>> +{
>>> + int rc = 0;
>>> + struct validate_negotiate_info_req vneg_inbuf;
>>> + struct validate_negotiate_info_rsp *pneg_rsp;
>>> + u32 rsplen;
>>> +
>>> + cifs_dbg(VFS, "validate negotiate ");
>>> + vneg_inbuf.Capabilities =
>>> + cpu_to_le32(tcon->ses->server->vals->req_capabilities);
>>> + memcpy(vneg_inbuf.Guid, cifs_client_guid, SMB2_CLIENT_GUID_SIZE);
>>> +
>>> + /*
>>> + * validation ioctl must be signed, so no point sending this if we
>>> + * can not sign it.  We could eventually change this to selectively
>>> + * sign just this, the first and only signed request on a connection.
>>> + * This is good enough for now since a user who wants better security
>>> + * would also enable signing on the mount. Having validation of
>>> + * negotiate info for signed connections helps reduce attack vectors
>>> + */
>>> + if (tcon->ses->server->sign == false)
>>> + return 0; /* validation requires signing */
>>
>> Shouldn't we move the above check and comments before we set
>> Capabilities and Guid of vneg_inbuf?
>
> Yes - will do
>
>
> --
> Thanks,
>
> Steve

Steve French Nov. 20, 2013, 5:55 a.m. UTC | #4

Minor change to not log error message

http://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=ff1c038addc4f205d5f1ede449426c7d316c0eed

On Tue, Nov 19, 2013 at 11:50 PM, Steve French <[e-mail]> wrote:
> Updated patch with the minor change you suggested is now in
> cifs-2.6.git for-next
>
> http://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=6f3219c1bc3b0f33a513ba63cea8278f1cd89ab3
>
> On Tue, Nov 19, 2013 at 11:34 PM, Steve French <[e-mail]> wrote:
>> On Tue, Nov 19, 2013 at 11:33 PM, Pavel Shilovsky <[e-mail]> wrote:
>>> 2013/11/19 Steve French <[e-mail]>:
>>>> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
>>>> From: Steve French <[e-mail]>
>>>> Date: Tue, 19 Nov 2013 12:58:08 -0600
>>>> Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks
>>>>
>>>> When we are running SMB3 or SMB3.02 connections which are signed
>>>> we need to validate the protocol negotiation information,
>>>> to ensure that thenegotiate protocol response was not tampered with.
>>>>
>>>> Add the missing FSCTL which is sent at mount time (immediately after
>>>> the SMB3 Tree Connect) to validate that the capabilities match
>>>> what we think the server sent.
>>>>
>>>> "Secure dialect negotiation is introduced in SMB3 to protect against
>>>> man-in-the-middle attempt to downgrade dialect negotiation.
>>>> The idea is to prevent an eavesdropper from downgrading the initially
>>>> negotiated dialect and capabilities between the client and the server."
>>>>
>>>> For more explanation see 2.2.31.4 of MS-SMB2 or
>>>> http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx
>>>>
>>>> Signed-off-by: Steve French <[e-mail]>
>>>> ---
>>>>  fs/cifs/cifsglob.h  |  1 +
>>>>  fs/cifs/smb2ops.c   |  1 +
>>>>  fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  fs/cifs/smb2pdu.h   | 12 ++++++---
>>>>  fs/cifs/smb2proto.h |  1 +
>>>>  fs/cifs/smbfsctl.h  |  2 +-
>>>>  6 files changed, 89 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>>>> index d9ea7ad..f918a99 100644
>>>> --- a/fs/cifs/cifsglob.h
>>>> +++ b/fs/cifs/cifsglob.h
>>>> @@ -384,6 +384,7 @@ struct smb_version_operations {
>>>>   int (*clone_range)(const unsigned int, struct cifsFileInfo *src_file,
>>>>   struct cifsFileInfo *target_file, u64 src_off, u64 len,
>>>>   u64 dest_off);
>>>> + int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
>>>>  };
>>>>
>>>>  struct smb_version_values {
>>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>>>> index a3968ee..757da3e 100644
>>>> --- a/fs/cifs/smb2ops.c
>>>> +++ b/fs/cifs/smb2ops.c
>>>> @@ -1319,6 +1319,7 @@ struct smb_version_operations smb30_operations = {
>>>>   .create_lease_buf = smb3_create_lease_buf,
>>>>   .parse_lease_buf = smb3_parse_lease_buf,
>>>>   .clone_range = smb2_clone_range,
>>>> + .validate_negotiate = smb3_validate_negotiate,
>>>>  };
>>>>
>>>>  struct smb_version_values smb20_values = {
>>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>>>> index 1e136ee..7756f9b 100644
>>>> --- a/fs/cifs/smb2pdu.c
>>>> +++ b/fs/cifs/smb2pdu.c
>>>> @@ -454,6 +454,80 @@ neg_exit:
>>>>   return rc;
>>>>  }
>>>>
>>>> +int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>>>> +{
>>>> + int rc = 0;
>>>> + struct validate_negotiate_info_req vneg_inbuf;
>>>> + struct validate_negotiate_info_rsp *pneg_rsp;
>>>> + u32 rsplen;
>>>> +
>>>> + cifs_dbg(VFS, "validate negotiate ");
>>>> + vneg_inbuf.Capabilities =
>>>> + cpu_to_le32(tcon->ses->server->vals->req_capabilities);
>>>> + memcpy(vneg_inbuf.Guid, cifs_client_guid, SMB2_CLIENT_GUID_SIZE);
>>>> +
>>>> + /*
>>>> + * validation ioctl must be signed, so no point sending this if we
>>>> + * can not sign it.  We could eventually change this to selectively
>>>> + * sign just this, the first and only signed request on a connection.
>>>> + * This is good enough for now since a user who wants better security
>>>> + * would also enable signing on the mount. Having validation of
>>>> + * negotiate info for signed connections helps reduce attack vectors
>>>> + */
>>>> + if (tcon->ses->server->sign == false)
>>>> + return 0; /* validation requires signing */
>>>
>>> Shouldn't we move the above check and comments before we set
>>> Capabilities and Guid of vneg_inbuf?
>>
>> Yes - will do
>>
>>
>> --
>> Thanks,
>>
>> Steve
>
>
>
> --
> Thanks,
>
> Steve

Pavel Shilovsky Nov. 20, 2013, 6:29 a.m. UTC | #5

2013/11/20 Steve French <[e-mail]>:
> Minor change to not log error message
>
> http://git.samba.org/?p=sfrench/cifs-2.6.git;a=commit;h=ff1c038addc4f205d5f1ede449426c7d316c0eed

Looks ok.

Shirish Pargaonkar Nov. 20, 2013, 2:34 p.m. UTC | #6

On Tue, Nov 19, 2013 at 1:20 PM, Steve French <[e-mail]> wrote:
> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
> From: Steve French <[e-mail]>
> Date: Tue, 19 Nov 2013 12:58:08 -0600
> Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks
>
> When we are running SMB3 or SMB3.02 connections which are signed
> we need to validate the protocol negotiation information,
> to ensure that thenegotiate protocol response was not tampered with.
>
> Add the missing FSCTL which is sent at mount time (immediately after
> the SMB3 Tree Connect) to validate that the capabilities match
> what we think the server sent.
>
> "Secure dialect negotiation is introduced in SMB3 to protect against
> man-in-the-middle attempt to downgrade dialect negotiation.
> The idea is to prevent an eavesdropper from downgrading the initially
> negotiated dialect and capabilities between the client and the server."
>
> For more explanation see 2.2.31.4 of MS-SMB2 or
> http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx
>
> Signed-off-by: Steve French <[e-mail]>
> ---
>  fs/cifs/cifsglob.h  |  1 +
>  fs/cifs/smb2ops.c   |  1 +
>  fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/smb2pdu.h   | 12 ++++++---
>  fs/cifs/smb2proto.h |  1 +
>  fs/cifs/smbfsctl.h  |  2 +-
>  6 files changed, 89 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index d9ea7ad..f918a99 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -384,6 +384,7 @@ struct smb_version_operations {
>   int (*clone_range)(const unsigned int, struct cifsFileInfo *src_file,
>   struct cifsFileInfo *target_file, u64 src_off, u64 len,
>   u64 dest_off);
> + int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
>  };
>
>  struct smb_version_values {
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index a3968ee..757da3e 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1319,6 +1319,7 @@ struct smb_version_operations smb30_operations = {
>   .create_lease_buf = smb3_create_lease_buf,
>   .parse_lease_buf = smb3_parse_lease_buf,
>   .clone_range = smb2_clone_range,
> + .validate_negotiate = smb3_validate_negotiate,
>  };
>
>  struct smb_version_values smb20_values = {
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 1e136ee..7756f9b 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -454,6 +454,80 @@ neg_exit:
>   return rc;
>  }
>
> +int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> +{
> + int rc = 0;
> + struct validate_negotiate_info_req vneg_inbuf;
> + struct validate_negotiate_info_rsp *pneg_rsp;
> + u32 rsplen;
> +
> + cifs_dbg(VFS, "validate negotiate ");
> + vneg_inbuf.Capabilities =
> + cpu_to_le32(tcon->ses->server->vals->req_capabilities);
> + memcpy(vneg_inbuf.Guid, cifs_client_guid, SMB2_CLIENT_GUID_SIZE);
> +
> + /*
> + * validation ioctl must be signed, so no point sending this if we
> + * can not sign it.  We could eventually change this to selectively
> + * sign just this, the first and only signed request on a connection.
> + * This is good enough for now since a user who wants better security
> + * would also enable signing on the mount. Having validation of
> + * negotiate info for signed connections helps reduce attack vectors
> + */
> + if (tcon->ses->server->sign == false)
> + return 0; /* validation requires signing */

Perhaps we should move this check even before invoking
validate_negotiate?

Also, not sure -EIO is the right error to return, may be -EACCESS or
-EINVAL might be better?

> +
> + if (tcon->ses->sign)
> + vneg_inbuf.SecurityMode =
> + cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
> + else if (global_secflags & CIFSSEC_MAY_SIGN)
> + vneg_inbuf.SecurityMode =
> + cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
> + else
> + vneg_inbuf.SecurityMode = 0;
> +
> + vneg_inbuf.DialectCount = cpu_to_le16(1);
> + vneg_inbuf.Dialects[0] =
> + cpu_to_le16(tcon->ses->server->vals->protocol_id);
> +
> + rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> + FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
> + (char *)&vneg_inbuf, sizeof(struct validate_negotiate_info_req),
> + (char **)&pneg_rsp, &rsplen);
> +
> + if (rc != 0) {
> + cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
> + return -EIO;
> + }
> +
> + if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
> + cifs_dbg(VFS, "invalid size of protocol negotiate response\n");
> + return -EIO;
> + }
> +
> + /* check validate negotiate info response matches what we got earlier */
> + if (pneg_rsp->Dialect !=
> + cpu_to_le16(tcon->ses->server->vals->protocol_id))
> + goto vneg_out;
> +
> + if (pneg_rsp->SecurityMode != cpu_to_le16(tcon->ses->server->sec_mode))
> + goto vneg_out;
> +
> + /* do not validate server guid because not saved at negprot time yet */
> +
> + if ((le32_to_cpu(pneg_rsp->Capabilities) | SMB2_NT_FIND |
> +      SMB2_LARGE_FILES) != tcon->ses->server->capabilities)
> + goto vneg_out;
> +
> + /* validate negotiate successful */
> + cifs_dbg(FYI, "validate negotiate info successful\n");
> + return 0;
> +
> +vneg_out:
> + cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
> + return -EIO;
> +}
> +
>  int
>  SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
>   const struct nls_table *nls_cp)
> @@ -829,6 +903,8 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses
> *ses, const char *tree,
>      ((tcon->share_flags & SHI1005_FLAGS_DFS) == 0))
>   cifs_dbg(VFS, "DFS capability contradicts DFS flag\n");
>   init_copy_chunk_defaults(tcon);
> + if (tcon->ses->server->ops->validate_negotiate)
> + rc = tcon->ses->server->ops->validate_negotiate(xid, tcon);
>  tcon_exit:
>   free_rsp_buf(resp_buftype, rsp);
>   kfree(unc_path);
> diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> index f88320b..2022c54 100644
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -577,13 +577,19 @@ struct copychunk_ioctl_rsp {
>   __le32 TotalBytesWritten;
>  } __packed;
>
> -/* Response and Request are the same format */
> -struct validate_negotiate_info {
> +struct validate_negotiate_info_req {
>   __le32 Capabilities;
>   __u8   Guid[SMB2_CLIENT_GUID_SIZE];
>   __le16 SecurityMode;
>   __le16 DialectCount;
> - __le16 Dialect[1];
> + __le16 Dialects[1]; /* dialect (someday maybe list) client asked for */
> +} __packed;
> +
> +struct validate_negotiate_info_rsp {
> + __le32 Capabilities;
> + __u8   Guid[SMB2_CLIENT_GUID_SIZE];
> + __le16 SecurityMode;
> + __le16 Dialect; /* Dialect in use for the connection */
>  } __packed;
>
>  #define RSS_CAPABLE 0x00000001
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index b4eea10..93adc64 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -162,5 +162,6 @@ extern int smb2_lockv(const unsigned int xid,
> struct cifs_tcon *tcon,
>        struct smb2_lock_element *buf);
>  extern int SMB2_lease_break(const unsigned int xid, struct cifs_tcon *tcon,
>      __u8 *lease_key, const __le32 lease_state);
> +extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
>
>  #endif /* _SMB2PROTO_H */
> diff --git a/fs/cifs/smbfsctl.h b/fs/cifs/smbfsctl.h
> index a4b2391f..0e538b5 100644
> --- a/fs/cifs/smbfsctl.h
> +++ b/fs/cifs/smbfsctl.h
> @@ -90,7 +90,7 @@
>  #define FSCTL_LMR_REQUEST_RESILIENCY 0x001401D4 /* BB add struct */
>  #define FSCTL_LMR_GET_LINK_TRACK_INF 0x001400E8 /* BB add struct */
>  #define FSCTL_LMR_SET_LINK_TRACK_INF 0x001400EC /* BB add struct */
> -#define FSCTL_VALIDATE_NEGOTIATE_INFO 0x00140204 /* BB add struct */
> +#define FSCTL_VALIDATE_NEGOTIATE_INFO 0x00140204
>  /* Perform server-side data movement */
>  #define FSCTL_SRV_COPYCHUNK 0x001440F2
>  #define FSCTL_SRV_COPYCHUNK_WRITE 0x001480F2
> --
> 1.8.3.1
>
>
> --
> Thanks,
>
> Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve French Nov. 20, 2013, 3:22 p.m. UTC | #7

On Wed, Nov 20, 2013 at 8:34 AM, Shirish Pargaonkar
<[e-mail]> wrote:
> On Tue, Nov 19, 2013 at 1:20 PM, Steve French <[e-mail]> wrote:
>> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
>> From: Steve French <[e-mail]>
>> Date: Tue, 19 Nov 2013 12:58:08 -0600
>> Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks
>>
>> When we are running SMB3 or SMB3.02 connections which are signed
>> we need to validate the protocol negotiation information,
>> to ensure that thenegotiate protocol response was not tampered with.
>>
>> Add the missing FSCTL which is sent at mount time (immediately after
>> the SMB3 Tree Connect) to validate that the capabilities match
>> what we think the server sent.
>>
>> "Secure dialect negotiation is introduced in SMB3 to protect against
>> man-in-the-middle attempt to downgrade dialect negotiation.
>> The idea is to prevent an eavesdropper from downgrading the initially
>> negotiated dialect and capabilities between the client and the server."
>>
>> For more explanation see 2.2.31.4 of MS-SMB2 or
>> http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx
>>
>> Signed-off-by: Steve French <[e-mail]>
>> ---
>>  fs/cifs/cifsglob.h  |  1 +
>>  fs/cifs/smb2ops.c   |  1 +
>>  fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/cifs/smb2pdu.h   | 12 ++++++---
>>  fs/cifs/smb2proto.h |  1 +
>>  fs/cifs/smbfsctl.h  |  2 +-
>>  6 files changed, 89 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index d9ea7ad..f918a99 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -384,6 +384,7 @@ struct smb_version_operations {
>>   int (*clone_range)(const unsigned int, struct cifsFileInfo *src_file,
>>   struct cifsFileInfo *target_file, u64 src_off, u64 len,
>>   u64 dest_off);
>> + int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
>>  };
>>
>>  struct smb_version_values {
>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>> index a3968ee..757da3e 100644
>> --- a/fs/cifs/smb2ops.c
>> +++ b/fs/cifs/smb2ops.c
>> @@ -1319,6 +1319,7 @@ struct smb_version_operations smb30_operations = {
>>   .create_lease_buf = smb3_create_lease_buf,
>>   .parse_lease_buf = smb3_parse_lease_buf,
>>   .clone_range = smb2_clone_range,
>> + .validate_negotiate = smb3_validate_negotiate,
>>  };
>>
>>  struct smb_version_values smb20_values = {
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 1e136ee..7756f9b 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -454,6 +454,80 @@ neg_exit:
>>   return rc;
>>  }
>>
>> +int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>> +{
>> + int rc = 0;
>> + struct validate_negotiate_info_req vneg_inbuf;
>> + struct validate_negotiate_info_rsp *pneg_rsp;
>> + u32 rsplen;
>> +
>> + cifs_dbg(VFS, "validate negotiate ");
>> + vneg_inbuf.Capabilities =
>> + cpu_to_le32(tcon->ses->server->vals->req_capabilities);
>> + memcpy(vneg_inbuf.Guid, cifs_client_guid, SMB2_CLIENT_GUID_SIZE);
>> +
>> + /*
>> + * validation ioctl must be signed, so no point sending this if we
>> + * can not sign it.  We could eventually change this to selectively
>> + * sign just this, the first and only signed request on a connection.
>> + * This is good enough for now since a user who wants better security
>> + * would also enable signing on the mount. Having validation of
>> + * negotiate info for signed connections helps reduce attack vectors
>> + */
>> + if (tcon->ses->server->sign == false)
>> + return 0; /* validation requires signing */
>
> Perhaps we should move this check even before invoking
> validate_negotiate?
>
> Also, not sure -EIO is the right error to return, may be -EACCESS or
> -EINVAL might be better?


-EINVAL may be ok, but was worried that the user might think that they
did something wrong on mount (bad mount option etc.) rather than
unexpected i/o error since the server acted weird.

I don't think it makes sense to move the check before
validate_negotiate because then it hits all code paths not just
SMB3/SMB3.02 - this is a version dependent operation

Patch

From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
From: Steve French <[e-mail]>
Date: Tue, 19 Nov 2013 12:58:08 -0600
Subject: [PATCH] [CIFS] Check SMB3 dialects against downgrade attacks

When we are running SMB3 or SMB3.02 connections which are signed
we need to validate the protocol negotiation information,
to ensure that thenegotiate protocol response was not tampered with.

Add the missing FSCTL which is sent at mount time (immediately after
the SMB3 Tree Connect) to validate that the capabilities match
what we think the server sent.

"Secure dialect negotiation is introduced in SMB3 to protect against
man-in-the-middle attempt to downgrade dialect negotiation.
The idea is to prevent an eavesdropper from downgrading the initially
negotiated dialect and capabilities between the client and the server."

For more explanation see 2.2.31.4 of MS-SMB2 or
http://blogs.msdn.com/b/openspecification/archive/2012/06/28/smb3-secure-dialect-negotiation.aspx

Signed-off-by: Steve French <[e-mail]>
---
 fs/cifs/cifsglob.h  |  1 +
 fs/cifs/smb2ops.c   |  1 +
 fs/cifs/smb2pdu.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/smb2pdu.h   | 12 ++++++---
 fs/cifs/smb2proto.h |  1 +
 fs/cifs/smbfsctl.h  |  2 +-
 6 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index d9ea7ad..f918a99 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -384,6 +384,7 @@  struct smb_version_operations {
 	int (*clone_range)(const unsigned int, struct cifsFileInfo *src_file,
 			struct cifsFileInfo *target_file, u64 src_off, u64 len,
 			u64 dest_off);
+	int (*validate_negotiate)(const unsigned int, struct cifs_tcon *);
 };
 
 struct smb_version_values {
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index a3968ee..757da3e 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1319,6 +1319,7 @@  struct smb_version_operations smb30_operations = {
 	.create_lease_buf = smb3_create_lease_buf,
 	.parse_lease_buf = smb3_parse_lease_buf,
 	.clone_range = smb2_clone_range,
+	.validate_negotiate = smb3_validate_negotiate,
 };
 
 struct smb_version_values smb20_values = {
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 1e136ee..7756f9b 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -454,6 +454,80 @@  neg_exit:
 	return rc;
 }
 
+int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
+{
+	int rc = 0;
+	struct validate_negotiate_info_req vneg_inbuf;
+	struct validate_negotiate_info_rsp *pneg_rsp;
+	u32 rsplen;
+
+	cifs_dbg(VFS, "validate negotiate ");
+	vneg_inbuf.Capabilities =
+			cpu_to_le32(tcon->ses->server->vals->req_capabilities);
+	memcpy(vneg_inbuf.Guid, cifs_client_guid, SMB2_CLIENT_GUID_SIZE);
+
+	/*
+	 * validation ioctl must be signed, so no point sending this if we
+	 * can not sign it.  We could eventually change this to selectively
+	 * sign just this, the first and only signed request on a connection.
+	 * This is good enough for now since a user who wants better security
+	 * would also enable signing on the mount. Having validation of
+	 * negotiate info for signed connections helps reduce attack vectors
+	 */
+	if (tcon->ses->server->sign == false)
+		return 0; /* validation requires signing */
+
+	if (tcon->ses->sign)
+		vneg_inbuf.SecurityMode =
+			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
+	else if (global_secflags & CIFSSEC_MAY_SIGN)
+		vneg_inbuf.SecurityMode =
+			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
+	else
+		vneg_inbuf.SecurityMode = 0;
+
+	vneg_inbuf.DialectCount = cpu_to_le16(1);
+	vneg_inbuf.Dialects[0] =
+		cpu_to_le16(tcon->ses->server->vals->protocol_id);
+
+	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
+		(char *)&vneg_inbuf, sizeof(struct validate_negotiate_info_req),
+		(char **)&pneg_rsp, &rsplen);
+
+	if (rc != 0) {
+		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
+		return -EIO;
+	}
+
+	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
+		cifs_dbg(VFS, "invalid size of protocol negotiate response\n");
+		return -EIO;
+	}
+
+	/* check validate negotiate info response matches what we got earlier */
+	if (pneg_rsp->Dialect !=
+			cpu_to_le16(tcon->ses->server->vals->protocol_id))
+		goto vneg_out;
+
+	if (pneg_rsp->SecurityMode != cpu_to_le16(tcon->ses->server->sec_mode))
+		goto vneg_out;
+
+	/* do not validate server guid because not saved at negprot time yet */
+
+	if ((le32_to_cpu(pneg_rsp->Capabilities) | SMB2_NT_FIND |
+	      SMB2_LARGE_FILES) != tcon->ses->server->capabilities)
+		goto vneg_out;
+
+	/* validate negotiate successful */
+	cifs_dbg(FYI, "validate negotiate info successful\n");
+	return 0;
+
+vneg_out:
+	cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
+	return -EIO;
+}
+
 int
 SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
 		const struct nls_table *nls_cp)
@@ -829,6 +903,8 @@  SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
 	    ((tcon->share_flags & SHI1005_FLAGS_DFS) == 0))
 		cifs_dbg(VFS, "DFS capability contradicts DFS flag\n");
 	init_copy_chunk_defaults(tcon);
+	if (tcon->ses->server->ops->validate_negotiate)
+		rc = tcon->ses->server->ops->validate_negotiate(xid, tcon);
 tcon_exit:
 	free_rsp_buf(resp_buftype, rsp);
 	kfree(unc_path);
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index f88320b..2022c54 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -577,13 +577,19 @@  struct copychunk_ioctl_rsp {
 	__le32 TotalBytesWritten;
 } __packed;
 
-/* Response and Request are the same format */
-struct validate_negotiate_info {
+struct validate_negotiate_info_req {
 	__le32 Capabilities;
 	__u8   Guid[SMB2_CLIENT_GUID_SIZE];
 	__le16 SecurityMode;
 	__le16 DialectCount;
-	__le16 Dialect[1];
+	__le16 Dialects[1]; /* dialect (someday maybe list) client asked for */
+} __packed;
+
+struct validate_negotiate_info_rsp {
+	__le32 Capabilities;
+	__u8   Guid[SMB2_CLIENT_GUID_SIZE];
+	__le16 SecurityMode;
+	__le16 Dialect; /* Dialect in use for the connection */
 } __packed;
 
 #define RSS_CAPABLE	0x00000001
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index b4eea10..93adc64 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -162,5 +162,6 @@  extern int smb2_lockv(const unsigned int xid, struct cifs_tcon *tcon,
 		      struct smb2_lock_element *buf);
 extern int SMB2_lease_break(const unsigned int xid, struct cifs_tcon *tcon,
 			    __u8 *lease_key, const __le32 lease_state);
+extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *);
 
 #endif			/* _SMB2PROTO_H */
diff --git a/fs/cifs/smbfsctl.h b/fs/cifs/smbfsctl.h
index a4b2391f..0e538b5 100644
--- a/fs/cifs/smbfsctl.h
+++ b/fs/cifs/smbfsctl.h
@@ -90,7 +90,7 @@ 
 #define FSCTL_LMR_REQUEST_RESILIENCY 0x001401D4 /* BB add struct */
 #define FSCTL_LMR_GET_LINK_TRACK_INF 0x001400E8 /* BB add struct */
 #define FSCTL_LMR_SET_LINK_TRACK_INF 0x001400EC /* BB add struct */
-#define FSCTL_VALIDATE_NEGOTIATE_INFO 0x00140204 /* BB add struct */
+#define FSCTL_VALIDATE_NEGOTIATE_INFO 0x00140204
 /* Perform server-side data movement */
 #define FSCTL_SRV_COPYCHUNK 0x001440F2
 #define FSCTL_SRV_COPYCHUNK_WRITE 0x001480F2
-- 
1.8.3.1