Re: [PATCHv2][CIFS] Validate Negotiate Info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 20, 2013 at 8:34 AM, Shirish Pargaonkar
<shirishpargaonkar@xxxxxxxxx> wrote:
> On Tue, Nov 19, 2013 at 1:20 PM, Steve French <smfrench@xxxxxxxxx> wrote:
>> From e4608aa57f9948a5e160ecb8590c885f76796e34 Mon Sep 17 00:00:00 2001
>> From: Steve French <smfrench@xxxxxxxxx>
>> 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@xxxxxxxxx>
>> ---
>>  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
-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux