[PATCHv2][CIFS] Validate Negotiate Info
Steve French
smfrench at gmail.com
Tue Nov 19 22:34:58 MST 2013
On Tue, Nov 19, 2013 at 11:33 PM, Pavel Shilovsky <piastryyy at gmail.com> wrote:
> 2013/11/19 Steve French <smfrench at gmail.com>:
>> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
>> From: Steve French <smfrench at gmail.com>
>> 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 <smfrench at gmail.com>
>> ---
>> 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
More information about the samba-technical
mailing list