diff mbox

smb3: do not allow insecure cifs (vers=1.0) mounts if mounting smb3

Message ID CAH2r5mtmSoZ1OyG22qXkj6jofXLJnTf9LLKN7-y3fEFC1HQURw@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve French June 6, 2018, 11:02 p.m. UTC

Comments

ronnie sahlberg June 6, 2018, 11:55 p.m. UTC | #1 
Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>

Only suggestion would be to change the word "forbidden" to something else.
"... downgrade to insecure xyz is not allowed with smb3 ..." or similar?


On Thu, Jun 7, 2018 at 9:02 AM, Steve French <smfrench@gmail.com> wrote:
> --
> Thanks,
>
> Steve
--
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
Tom Talpey June 7, 2018, 5:27 a.m. UTC | #2 
> -----Original Message-----

> From: ronnie sahlberg <ronniesahlberg@gmail.com>

> Sent: Thursday, June 7, 2018 1:55 AM

> To: Steve French <smfrench@gmail.com>

> Cc: CIFS <linux-cifs@vger.kernel.org>; Pavel Shilovskiy

> <pshilov@microsoft.com>; Tom Talpey <ttalpey@microsoft.com>; Aurélien

> Aptel <aaptel@suse.com>

> Subject: Re: [PATCH] smb3: do not allow insecure cifs (vers=1.0) mounts if

> mounting smb3

> 

> Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>

> 

> Only suggestion would be to change the word "forbidden" to something else.

> "... downgrade to insecure xyz is not allowed with smb3 ..." or similar?


Agreed, it's a more meaningful statement that way.

But one nit - it could be argued that SMB2.1 is less secure than 3.x, yet the patch allows it. I think the point is that with a 3.x-style negotiation, the client can ensure that it is not being attacked by a MITM attempting to downgrade, and that 2.1 is simply a minimum bar. So I'd suggest not using the rather vague word "insecure". Just say downgrade to <name of offered dialect> isn't allowed, and done.

Tom.

> On Thu, Jun 7, 2018 at 9:02 AM, Steve French <smfrench@gmail.com> wrote:

> > --

> > Thanks,

> >

> > Steve
diff mbox

Patch

From 8ea0364df15e9ca80c0e586d558319b45431812b Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 6 Jun 2018 17:59:29 -0500
Subject: [PATCH] smb3: do not allow insecure cifs mounts when using smb3

if mounting as smb3 do not allow cifs (vers=1.0) or insecure vers=2.0
mounts.

For example:
root@smf-Thinkpad-P51:~/cifs-2.6# mount -t smb3 //127.0.0.1/scratch /mnt -o username=testuser,password=Testpass1
root@smf-Thinkpad-P51:~/cifs-2.6# umount /mnt
root@smf-Thinkpad-P51:~/cifs-2.6# mount -t smb3 //127.0.0.1/scratch /mnt -o username=testuser,password=Testpass1,vers=1.0
mount: /mnt: wrong fs type, bad option, bad superblock on //127.0.0.1/scratch ...
root@smf-Thinkpad-P51:~/cifs-2.6# dmesg | grep smb3
[ 4302.200122] CIFS VFS: vers=1.0 (cifs) forbidden with smb3
root@smf-Thinkpad-P51:~/cifs-2.6# mount -t smb3 //127.0.0.1/scratch /mnt -o username=testuser,password=Testpass1,vers=3.11

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsfs.c    | 22 ++++++++++++++++++----
 fs/cifs/cifsproto.h |  2 +-
 fs/cifs/connect.c   | 26 +++++++++++++++++---------
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index eb7b6573f322..d5aa7ae917bf 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -698,8 +698,8 @@  static int cifs_set_super(struct super_block *sb, void *data)
 }
 
 static struct dentry *
-cifs_do_mount(struct file_system_type *fs_type,
-	      int flags, const char *dev_name, void *data)
+cifs_smb3_do_mount(struct file_system_type *fs_type,
+	      int flags, const char *dev_name, void *data, bool is_smb3)
 {
 	int rc;
 	struct super_block *sb;
@@ -710,7 +710,7 @@  cifs_do_mount(struct file_system_type *fs_type,
 
 	cifs_dbg(FYI, "Devname: %s flags: %d\n", dev_name, flags);
 
-	volume_info = cifs_get_volume_info((char *)data, dev_name);
+	volume_info = cifs_get_volume_info((char *)data, dev_name, is_smb3);
 	if (IS_ERR(volume_info))
 		return ERR_CAST(volume_info);
 
@@ -790,6 +790,20 @@  cifs_do_mount(struct file_system_type *fs_type,
 	goto out;
 }
 
+static struct dentry *
+smb3_do_mount(struct file_system_type *fs_type,
+	      int flags, const char *dev_name, void *data)
+{
+	return cifs_smb3_do_mount(fs_type, flags, dev_name, data, true);
+}
+
+static struct dentry *
+cifs_do_mount(struct file_system_type *fs_type,
+	      int flags, const char *dev_name, void *data)
+{
+	return cifs_smb3_do_mount(fs_type, flags, dev_name, data, false);
+}
+
 static ssize_t
 cifs_loose_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
@@ -925,7 +939,7 @@  MODULE_ALIAS_FS("cifs");
 static struct file_system_type smb3_fs_type = {
 	.owner = THIS_MODULE,
 	.name = "smb3",
-	.mount = cifs_do_mount,
+	.mount = smb3_do_mount,
 	.kill_sb = cifs_kill_sb,
 	/*  .fs_flags */
 };
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 84765c8764bf..4e0d183c3d10 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -211,7 +211,7 @@  extern int cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
 extern int cifs_match_super(struct super_block *, void *);
 extern void cifs_cleanup_volume_info(struct smb_vol *pvolume_info);
 extern struct smb_vol *cifs_get_volume_info(char *mount_data,
-					    const char *devname);
+					    const char *devname, bool is_smb3);
 extern int cifs_mount(struct cifs_sb_info *, struct smb_vol *);
 extern void cifs_umount(struct cifs_sb_info *);
 extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index e5a2fe7f0dd4..88c9d6fc539b 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -320,7 +320,7 @@  static int generic_ip_connect(struct TCP_Server_Info *server);
 static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
 static void cifs_prune_tlinks(struct work_struct *work);
 static int cifs_setup_volume_info(struct smb_vol *volume_info, char *mount_data,
-					const char *devname);
+					const char *devname, bool is_smb3);
 
 /*
  * cifs tcp session reconnection
@@ -1166,7 +1166,7 @@  cifs_parse_cache_flavor(char *value, struct smb_vol *vol)
 }
 
 static int
-cifs_parse_smb_version(char *value, struct smb_vol *vol)
+cifs_parse_smb_version(char *value, struct smb_vol *vol, bool is_smb3)
 {
 	substring_t args[MAX_OPT_ARGS];
 
@@ -1176,6 +1176,10 @@  cifs_parse_smb_version(char *value, struct smb_vol *vol)
 			cifs_dbg(VFS, "mount with legacy dialect disabled\n");
 			return 1;
 		}
+		if (is_smb3) {
+			cifs_dbg(VFS, "vers=1.0 (cifs) forbidden with smb3\n");
+			return 1;
+		}
 		vol->ops = &smb1_operations;
 		vol->vals = &smb1_values;
 		break;
@@ -1184,6 +1188,10 @@  cifs_parse_smb_version(char *value, struct smb_vol *vol)
 			cifs_dbg(VFS, "mount with legacy dialect disabled\n");
 			return 1;
 		}
+		if (is_smb3) {
+			cifs_dbg(VFS, "vers=2.0 forbidden with smb3\n");
+			return 1;
+		}
 		vol->ops = &smb20_operations;
 		vol->vals = &smb20_values;
 		break;
@@ -1272,7 +1280,7 @@  cifs_parse_devname(const char *devname, struct smb_vol *vol)
 
 static int
 cifs_parse_mount_options(const char *mountdata, const char *devname,
-			 struct smb_vol *vol)
+			 struct smb_vol *vol, bool is_smb3)
 {
 	char *data, *end;
 	char *mountdata_copy = NULL, *options;
@@ -1985,7 +1993,7 @@  cifs_parse_mount_options(const char *mountdata, const char *devname,
 			if (string == NULL)
 				goto out_nomem;
 
-			if (cifs_parse_smb_version(string, vol) != 0)
+			if (cifs_parse_smb_version(string, vol, is_smb3) != 0)
 				goto cifs_parse_mount_err;
 			got_version = true;
 			break;
@@ -3803,7 +3811,7 @@  expand_dfs_referral(const unsigned int xid, struct cifs_ses *ses,
 		} else {
 			cleanup_volume_info_contents(volume_info);
 			rc = cifs_setup_volume_info(volume_info, mdata,
-							fake_devname);
+							fake_devname, false);
 		}
 		kfree(fake_devname);
 		kfree(cifs_sb->mountdata);
@@ -3816,11 +3824,11 @@  expand_dfs_referral(const unsigned int xid, struct cifs_ses *ses,
 
 static int
 cifs_setup_volume_info(struct smb_vol *volume_info, char *mount_data,
-			const char *devname)
+			const char *devname, bool is_smb3)
 {
 	int rc = 0;
 
-	if (cifs_parse_mount_options(mount_data, devname, volume_info))
+	if (cifs_parse_mount_options(mount_data, devname, volume_info, is_smb3))
 		return -EINVAL;
 
 	if (volume_info->nullauth) {
@@ -3854,7 +3862,7 @@  cifs_setup_volume_info(struct smb_vol *volume_info, char *mount_data,
 }
 
 struct smb_vol *
-cifs_get_volume_info(char *mount_data, const char *devname)
+cifs_get_volume_info(char *mount_data, const char *devname, bool is_smb3)
 {
 	int rc;
 	struct smb_vol *volume_info;
@@ -3863,7 +3871,7 @@  cifs_get_volume_info(char *mount_data, const char *devname)
 	if (!volume_info)
 		return ERR_PTR(-ENOMEM);
 
-	rc = cifs_setup_volume_info(volume_info, mount_data, devname);
+	rc = cifs_setup_volume_info(volume_info, mount_data, devname, is_smb3);
 	if (rc) {
 		cifs_cleanup_volume_info(volume_info);
 		volume_info = ERR_PTR(rc);
-- 
2.17.1