Re: updated ksmbd (cifsd)

[Sergey Senozhatsky <sergey.senozhatsky@gmail.com>]

On (20/12/16 13:21), Sergey Senozhatsky wrote:
> So SMB_SERVER_CHECK_CAP_NET_ADMIN enforces the "user-space must
> be a privileged process" policy. Even CAP_NET_ADMIN is too huge,
> not to mention that _probably_ this CAP requirement means that
> people will just "sudo cifsd". One way or another a malformed
> RPC request can do quite a bit of damage to the system, because
> user-space runs with the CAPs it doesn't really need.
> 
> It would be better to enforce a different policy, IMHO.
> Something like:
> 
> 	groupadd ... CIFSD_GROUP
> 	useradd -g CIFSD_GID -p CIFSD_PASSWORD CIFSD_LOGIN
> 	chmod 0700 smb.conf and password db
> 	chown CIFSD_LOGIN:CIFSD_GROUP smb.conf and password db

Just a sketch. Completely untested (wasn't even compile tested).
Merely an idea.

This removes the requirement for user-space to run as a privileged
process. Because "let's grant some random user-space daemon
administrative privileges" most likely doesn't improve security
of the system.

So the idea is to provide CIFSD administrator UID and GID during
kcifsd modprobe and then in IPC handlers check if the app issuing
a netlink syscall has the same UID and GID as the CIFSD administrator
or not.

This untested, sorry. Just checking the idea.

---

diff --git a/fs/cifsd/Kconfig b/fs/cifsd/Kconfig
index e12616cf8477..9f221a37223d 100644
--- a/fs/cifsd/Kconfig
+++ b/fs/cifsd/Kconfig
@@ -50,14 +50,6 @@ config SMB_SERVER_SMBDIRECT
 	  SMB Direct allows transferring SMB packets over RDMA. If unsure,
 	  say N.
 
-config SMB_SERVER_CHECK_CAP_NET_ADMIN
-	bool "Enable check network administration capability"
-	depends on SMB_SERVER
-	default n
-
-	help
-	  Prevent unprivileged processes to start the cifsd kernel server.
-
 config SMB_SERVER_KERBEROS5
 	bool "Support for Kerberos 5"
 	depends on SMB_SERVER
diff --git a/fs/cifsd/server.c b/fs/cifsd/server.c
index b9e114f8a5d2..dbbdb3503b0d 100644
--- a/fs/cifsd/server.c
+++ b/fs/cifsd/server.c
@@ -25,6 +25,7 @@
 
 int ksmbd_debug_types;
 
+static int admin_uid = -1, admin_gid = -1;
 struct ksmbd_server_config server_conf;
 
 enum SERVER_CTRL_TYPE {
@@ -335,6 +336,26 @@ static void server_conf_free(void)
 	}
 }
 
+static int server_admin_cred_init(void)
+{
+	if (admin_uid == -1 || admin_gid == -1)
+		return 0;
+
+	server_conf.admin_cred = kmalloc(sizeof(struct admin_cred), GFP_KERNEL);
+	if (!server_conf.admin_cred)
+		return -ENOMEM;
+
+	server_conf.admin_cred.uid = make_kuid(&init_user_ns, admin_uid);
+	server_conf.admin_cred.gid = make_kgid(&init_user_ns, admin_gid);
+	if (!(uid_validserver_conf.admin_cred.uid() &&
+	      gid_valid(server_conf.admin_cred.gid))) {
+		kfree(server_conf.admin_cred);
+		server_conf.admin_cred = NULL;
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int server_conf_init(void)
 {
 	WRITE_ONCE(server_conf.state, SERVER_STATE_STARTING_UP);
@@ -343,10 +364,9 @@ static int server_conf_init(void)
 	server_conf.max_protocol = ksmbd_max_protocol();
 	server_conf.auth_mechs = KSMBD_AUTH_NTLMSSP;
 #ifdef CONFIG_SMB_SERVER_KERBEROS5
-	server_conf.auth_mechs |= KSMBD_AUTH_KRB5 |
-				KSMBD_AUTH_MSKRB5;
+	server_conf.auth_mechs |= KSMBD_AUTH_KRB5 | KSMBD_AUTH_MSKRB5;
 #endif
-	return 0;
+	return server_admin_cred_init();
 }
 
 static void server_ctrl_handle_init(struct server_ctrl_struct *ctrl)
@@ -418,6 +438,21 @@ int server_queue_ctrl_reset_work(void)
 	return __queue_ctrl_work(SERVER_CTRL_TYPE_RESET);
 }
 
+int server_validate_admin_cred(void)
+{
+	if (admin_uid == -1 || admin_gid == -1)
+		return 0;
+
+	/* We couldn't init server admin UID/GID */
+	if (!server_conf.admin_cred)
+		return -EINVAL;
+
+	if (uid_eq(task_uid(current), server_conf.admin_cred.uid) &&
+	    gid_eq(task_gid(current), server_conf.admin_cred.gid))
+		return 0;
+	return -EINVAL;
+}
+
 static ssize_t stats_show(struct class *class,
 			  struct class_attribute *attr,
 			  char *buf)
@@ -614,6 +649,11 @@ static void __exit ksmbd_server_exit(void)
 	ksmbd_release_inode_hash();
 }
 
+module_param(admin_uid, int, 0);
+MODULE_PARM_DESC(admin_uid, "ksmb administrator user id");
+module_param(admin_gid, int, 0);
+MODULE_PARM_DESC(admin_gid, "ksmb administrator group id");
+
 MODULE_AUTHOR("Namjae Jeon <linkinjeon@kernel.org>");
 MODULE_VERSION(KSMBD_VERSION);
 MODULE_DESCRIPTION("Linux kernel CIFS/SMB SERVER");
diff --git a/fs/cifsd/server.h b/fs/cifsd/server.h
index 7b2f6318fcff..ed0249061470 100644
--- a/fs/cifsd/server.h
+++ b/fs/cifsd/server.h
@@ -19,6 +19,11 @@
 
 extern int ksmbd_debugging;
 
+struct admin_cred {
+	kuid_t			uid;
+	kgid_t			gid;
+};
+
 struct ksmbd_server_config {
 	unsigned int		flags;
 	unsigned int		state;
@@ -34,6 +39,7 @@ struct ksmbd_server_config {
 	struct smb_sid		domain_sid;
 	unsigned int		auth_mechs;
 
+	struct admin_cred	*admin_cred;
 	char			*conf[SERVER_CONF_WORK_GROUP + 1];
 };
 
@@ -57,6 +63,7 @@ static inline int ksmbd_server_configurable(void)
 	return READ_ONCE(server_conf.state) < SERVER_STATE_RESETTING;
 }
 
+int server_verify_admin_cred(void);
 int server_queue_ctrl_init_work(void);
 int server_queue_ctrl_reset_work(void);
 #endif /* __SERVER_H__ */
diff --git a/fs/cifsd/transport_ipc.c b/fs/cifsd/transport_ipc.c
index 5f24a1ed5c34..b2a42f6ce6e3 100644
--- a/fs/cifsd/transport_ipc.c
+++ b/fs/cifsd/transport_ipc.c
@@ -345,10 +345,8 @@ static int handle_startup_event(struct sk_buff *skb, struct genl_info *info)
 {
 	int ret = 0;
 
-#ifdef CONFIG_SMB_SERVER_CHECK_CAP_NET_ADMIN
-	if (!netlink_capable(skb, CAP_NET_ADMIN))
+	if (server_validate_admin_cred())
 		return -EPERM;
-#endif
 
 	if (!ksmbd_ipc_validate_version(info))
 		return -EINVAL;
@@ -401,10 +399,8 @@ static int handle_generic_event(struct sk_buff *skb, struct genl_info *info)
 	int sz;
 	int type = info->genlhdr->cmd;
 
-#ifdef CONFIG_SMB_SERVER_CHECK_CAP_NET_ADMIN
-	if (!netlink_capable(skb, CAP_NET_ADMIN))
+	if (server_validate_admin_cred())
 		return -EPERM;
-#endif
 
 	if (type >= KSMBD_EVENT_MAX) {
 		WARN_ON(1);