[RFC PATCH v3 0/5] kdbus LSM/SELinux hooks

[RFC PATCH v3 0/5] kdbus LSM/SELinux hooks

[Paul Moore]

No real functional improvements since the v2 patchset earlier this
week, the main update is rebasing on GregKH's current kdbus tree
which is now 4.3-rc4 based and as a result brings the LSM stacking
changes and SELinux ioctl/xperm additions.

---

Paul Moore (5):
      kdbus: add creator credentials to the endpoints
      lsm: introduce hooks for kdbus
      lsm: add support for auditing kdbus service names
      selinux: introduce kdbus names into the policy
      selinux: introduce kdbus access controls

 include/linux/lsm_audit.h           |    2 
 include/linux/lsm_hooks.h           |   63 ++++++++++++++
 include/linux/security.h            |   71 ++++++++++++++++
 ipc/kdbus/bus.c                     |   13 +--
 ipc/kdbus/connection.c              |   73 +++++++++++------
 ipc/kdbus/endpoint.c                |   14 +--
 ipc/kdbus/endpoint.h                |    3 -
 ipc/kdbus/fs.c                      |   10 ++
 ipc/kdbus/message.c                 |   19 +++-
 ipc/kdbus/metadata.c                |    6 -
 ipc/kdbus/node.c                    |   11 +--
 ipc/kdbus/node.h                    |    5 +
 security/lsm_audit.c                |    4 +
 security/security.c                 |   62 ++++++++++++++
 security/selinux/hooks.c            |  153 +++++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h |    4 +
 security/selinux/include/security.h |    5 +
 security/selinux/ss/policydb.c      |   88 ++++++++++++++++----
 security/selinux/ss/policydb.h      |    3 -
 security/selinux/ss/services.c      |   38 +++++++++
 20 files changed, 561 insertions(+), 86 deletions(-)

[RFC PATCH v3 1/5] kdbus: add creator credentials to the endpoints

[Paul Moore]

In order to effectively enforce LSM based access controls we need to
have more information about the kdbus endpoint creator than the
uid/gid currently stored in the kdbus_node_type struct.  This patch
replaces the uid/gid values with a reference to the node creator's
credential struct which serves the needs of both the kdbus DAC access
controls as well as the LSM's access controls.

Two macros have also been created, kdbus_node_[uid,gid](), which can
be used to easily extract the euid/egid information from the new
credential reference.  The effective uid/gid is used as it was used
in all areas of the previous kdbus code except for areas where the
uid/gid was never set beyond the basic initialization to zero/root;
I expect this was a bug that was never caught as the node creator in
these cases was always expect to be root.

Signed-off-by: Paul Moore <pmoore@redhat.com>

---
ChangeLog:
- v3
 * Ported to the 4.3-rc4 based kdbus tree
- v2
 * Initial draft
---
 ipc/kdbus/bus.c      |   13 +++++--------
 ipc/kdbus/endpoint.c |   14 ++++----------
 ipc/kdbus/endpoint.h |    3 +--
 ipc/kdbus/fs.c       |    4 ++--
 ipc/kdbus/node.c     |   11 ++++-------
 ipc/kdbus/node.h     |    5 +++--
 6 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/ipc/kdbus/bus.c b/ipc/kdbus/bus.c
index a67f825..0cb9501 100644
--- a/ipc/kdbus/bus.c
+++ b/ipc/kdbus/bus.c
@@ -65,8 +65,7 @@ static void kdbus_bus_release(struct kdbus_node *node, bool was_active)
 static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain,
 				       const char *name,
 				       struct kdbus_bloom_parameter *bloom,
-				       const u64 *pattach_owner,
-				       u64 flags, kuid_t uid, kgid_t gid)
+				       const u64 *pattach_owner, u64 flags)
 {
 	struct kdbus_bus *b;
 	u64 attach_owner;
@@ -81,7 +80,8 @@ static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain,
 	if (ret < 0)
 		return ERR_PTR(ret);
 
-	ret = kdbus_verify_uid_prefix(name, domain->user_namespace, uid);
+	ret = kdbus_verify_uid_prefix(name, domain->user_namespace,
+				      current_euid());
 	if (ret < 0)
 		return ERR_PTR(ret);
 
@@ -93,8 +93,6 @@ static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain,
 
 	b->node.free_cb = kdbus_bus_free;
 	b->node.release_cb = kdbus_bus_release;
-	b->node.uid = uid;
-	b->node.gid = gid;
 	b->node.mode = S_IRUSR | S_IXUSR;
 
 	if (flags & (KDBUS_MAKE_ACCESS_GROUP | KDBUS_MAKE_ACCESS_WORLD))
@@ -374,7 +372,7 @@ struct kdbus_bus *kdbus_cmd_bus_make(struct kdbus_domain *domain,
 	bus = kdbus_bus_new(domain,
 			    argv[1].item->str, &argv[2].item->bloom_parameter,
 			    argv[3].item ? argv[3].item->data64 : NULL,
-			    cmd->flags, current_euid(), current_egid());
+			    cmd->flags);
 	if (IS_ERR(bus)) {
 		ret = PTR_ERR(bus);
 		bus = NULL;
@@ -393,8 +391,7 @@ struct kdbus_bus *kdbus_cmd_bus_make(struct kdbus_domain *domain,
 		goto exit;
 	}
 
-	ep = kdbus_ep_new(bus, "bus", cmd->flags, bus->node.uid, bus->node.gid,
-			  false);
+	ep = kdbus_ep_new(bus, "bus", cmd->flags, false);
 	if (IS_ERR(ep)) {
 		ret = PTR_ERR(ep);
 		ep = NULL;
diff --git a/ipc/kdbus/endpoint.c b/ipc/kdbus/endpoint.c
index 44e7a20..1ba5d51 100644
--- a/ipc/kdbus/endpoint.c
+++ b/ipc/kdbus/endpoint.c
@@ -74,8 +74,6 @@ static void kdbus_ep_release(struct kdbus_node *node, bool was_active)
  * @bus:		The bus this endpoint will be created for
  * @name:		The name of the endpoint
  * @access:		The access flags for this node (KDBUS_MAKE_ACCESS_*)
- * @uid:		The uid of the node
- * @gid:		The gid of the node
  * @is_custom:		Whether this is a custom endpoint
  *
  * This function will create a new endpoint with the given
@@ -84,8 +82,7 @@ static void kdbus_ep_release(struct kdbus_node *node, bool was_active)
  * Return: a new kdbus_ep on success, ERR_PTR on failure.
  */
 struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
-			      unsigned int access, kuid_t uid, kgid_t gid,
-			      bool is_custom)
+			      unsigned int access, bool is_custom)
 {
 	struct kdbus_ep *e;
 	int ret;
@@ -96,7 +93,7 @@ struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
 	 */
 	if (is_custom) {
 		ret = kdbus_verify_uid_prefix(name, bus->domain->user_namespace,
-					      uid);
+					      current_euid());
 		if (ret < 0)
 			return ERR_PTR(ret);
 	}
@@ -109,8 +106,6 @@ struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
 
 	e->node.free_cb = kdbus_ep_free;
 	e->node.release_cb = kdbus_ep_release;
-	e->node.uid = uid;
-	e->node.gid = gid;
 	e->node.mode = S_IRUSR | S_IWUSR;
 	if (access & (KDBUS_MAKE_ACCESS_GROUP | KDBUS_MAKE_ACCESS_WORLD))
 		e->node.mode |= S_IRGRP | S_IWGRP;
@@ -207,7 +202,7 @@ bool kdbus_ep_is_privileged(struct kdbus_ep *ep, struct file *file)
 bool kdbus_ep_is_owner(struct kdbus_ep *ep, struct file *file)
 {
 	return !ep->user &&
-		(uid_eq(file->f_cred->euid, ep->bus->node.uid) ||
+		(uid_eq(file->f_cred->euid, kdbus_node_uid(&ep->bus->node)) ||
 		 kdbus_ep_is_privileged(ep, file));
 }
 
@@ -245,8 +240,7 @@ struct kdbus_ep *kdbus_cmd_ep_make(struct kdbus_bus *bus, void __user *argp)
 
 	item_make_name = argv[1].item->str;
 
-	ep = kdbus_ep_new(bus, item_make_name, cmd->flags,
-			  current_euid(), current_egid(), true);
+	ep = kdbus_ep_new(bus, item_make_name, cmd->flags, true);
 	if (IS_ERR(ep)) {
 		ret = PTR_ERR(ep);
 		ep = NULL;
diff --git a/ipc/kdbus/endpoint.h b/ipc/kdbus/endpoint.h
index e0da59f..c537779 100644
--- a/ipc/kdbus/endpoint.h
+++ b/ipc/kdbus/endpoint.h
@@ -56,8 +56,7 @@ struct kdbus_ep {
 	container_of((_node), struct kdbus_ep, node)
 
 struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
-			      unsigned int access, kuid_t uid, kgid_t gid,
-			      bool policy);
+			      unsigned int access, bool policy);
 struct kdbus_ep *kdbus_ep_ref(struct kdbus_ep *ep);
 struct kdbus_ep *kdbus_ep_unref(struct kdbus_ep *ep);
 
diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
index 09c4809..68818a8 100644
--- a/ipc/kdbus/fs.c
+++ b/ipc/kdbus/fs.c
@@ -204,8 +204,8 @@ static struct inode *fs_inode_get(struct super_block *sb,
 	inode->i_mapping->a_ops = &empty_aops;
 	inode->i_mode = node->mode & S_IALLUGO;
 	inode->i_atime = inode->i_ctime = inode->i_mtime = CURRENT_TIME;
-	inode->i_uid = node->uid;
-	inode->i_gid = node->gid;
+	inode->i_uid = kdbus_node_uid(node);
+	inode->i_gid = kdbus_node_gid(node);
 
 	switch (node->type) {
 	case KDBUS_NODE_DOMAIN:
diff --git a/ipc/kdbus/node.c b/ipc/kdbus/node.c
index 89f58bc..cd0c1a0 100644
--- a/ipc/kdbus/node.c
+++ b/ipc/kdbus/node.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/atomic.h>
+#include <linux/cred.h>
 #include <linux/fs.h>
 #include <linux/idr.h>
 #include <linux/kdev_t.h>
@@ -170,13 +171,7 @@
  *                         node initialization. They must remain constant. If
  *                         NULL, they're skipped.
  *
- *     * node->mode: filesystem access modes
- *       node->uid:  filesystem owner uid
- *       node->gid:  filesystem owner gid
- *                   These fields must be set by the node owner during node
- *                   initialization. They must remain constant and may be
- *                   accessed by other callers to properly initialize
- *                   filesystem nodes.
+ *     * node->creds: The credentials of the node's creator
  *
  *     * node->id: This is an unsigned 32bit integer allocated by an IDA. It is
  *                 always kept as small as possible during allocation and is
@@ -293,6 +288,7 @@ void kdbus_node_init(struct kdbus_node *node, unsigned int type)
 	mutex_init(&node->lock);
 	node->id = 0;
 	node->type = type;
+	node->creds = get_current_cred();
 	RB_CLEAR_NODE(&node->rb);
 	node->children = RB_ROOT;
 	init_waitqueue_head(&node->waitq);
@@ -433,6 +429,7 @@ struct kdbus_node *kdbus_node_unref(struct kdbus_node *node)
 		WARN_ON(atomic_read(&node->active) != KDBUS_NODE_DRAINED);
 		WARN_ON(!RB_EMPTY_NODE(&node->rb));
 
+		put_cred(node->creds);
 		if (node->free_cb)
 			node->free_cb(node);
 		if (safe.id > 0)
diff --git a/ipc/kdbus/node.h b/ipc/kdbus/node.h
index 970e02b..d52e1c2 100644
--- a/ipc/kdbus/node.h
+++ b/ipc/kdbus/node.h
@@ -41,8 +41,7 @@ struct kdbus_node {
 	kdbus_node_free_t free_cb;
 	kdbus_node_release_t release_cb;
 	umode_t mode;
-	kuid_t uid;
-	kgid_t gid;
+	const struct cred *creds;
 
 	/* valid once linked */
 	char *name;
@@ -57,6 +56,8 @@ struct kdbus_node {
 };
 
 #define kdbus_node_from_rb(_node) rb_entry((_node), struct kdbus_node, rb)
+#define kdbus_node_uid(_node)     ((_node)->creds->euid)
+#define kdbus_node_gid(_node)     ((_node)->creds->egid)
 
 extern struct ida kdbus_node_ida;
 

[RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Paul Moore]

Add LSM access control hooks to kdbus; several new hooks are added and
the existing security_file_receive() hook is reused.  The new hooks
are listed below:

 * security_kdbus_conn_new
   Check if the current task is allowed to create a new kdbus
   connection.
 * security_kdbus_own_name
   Check if a connection is allowed to own a kdbus service name.
 * security_kdbus_conn_talk
   Check if a connection is allowed to talk to a kdbus peer.
 * security_kdbus_conn_see
   Check if a connection can see a kdbus peer.
 * security_kdbus_conn_see_name
   Check if a connection can see a kdbus service name.
 * security_kdbus_conn_see_notification
   Check if a connection can receive notifications.
 * security_kdbus_proc_permission
   Check if a connection can access another task's pid namespace info.
 * security_kdbus_init_inode
   Set the security label on a kdbusfs inode

Signed-off-by: Paul Moore <pmoore@redhat.com>

---
ChangeLog:
- v3
 * Ported to the 4.3-rc4 based kdbus tree
- v2
 * Implemented suggestions by Stephen Smalley
   * call security_kdbus_conn_new() sooner
   * reworked hook inside kdbus_conn_policy_own_name()
   * fixed if-conditional in kdbus_conn_policy_talk()
   * reworked hook inside kdbus_conn_policy_see_name_unlocked()
   * reworked hook inside kdbus_conn_policy_see()
   * reworked hook inside kdbus_conn_policy_see_notification()
   * added the security_kdbus_init_inode() hook
- v1
 * Initial draft
---
 include/linux/lsm_hooks.h |   63 +++++++++++++++++++++++++++++++++++++++
 include/linux/security.h  |   71 ++++++++++++++++++++++++++++++++++++++++++++
 ipc/kdbus/connection.c    |   73 +++++++++++++++++++++++++++++----------------
 ipc/kdbus/fs.c            |    6 ++++
 ipc/kdbus/message.c       |   19 +++++++++---
 ipc/kdbus/metadata.c      |    6 +---
 security/security.c       |   62 ++++++++++++++++++++++++++++++++++++++
 7 files changed, 265 insertions(+), 35 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ec3a6ba..36d4e5d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1138,6 +1138,45 @@
  *	@file contains the struct file being transferred.
  *	@to contains the task_struct for the receiving task.
  *
+ * @kdbus_conn_new
+ *	Check if the current task is allowed to create a new kdbus connection.
+ *	@creds credentials for the new connection
+ *	@fake_creds kdbus faked credentials
+ *	@fake_pids kdbus faked pids
+ *	@fake_seclabel kdbus faked security label
+ *	@owner kdbus owner
+ *	@privileged kdbus privileged
+ *	@is_activator kdbus activator boolean
+ *	@is_monitor kdbus monitor boolean
+ *	@is_policy_holder kdbus policy holder boolean
+ * @kdbus_own_name
+ *	Check if a connection is allowed to own a kdbus service name.
+ *	@creds requestor's credentials
+ *	@name service name
+ * @kdbus_conn_talk
+ *	Check if a connection is allowed to talk to a kdbus peer.
+ *	@creds requestor's credentials
+ *	@creds_peer peer credentials
+ * @kdbus_conn_see
+ *	Check if a connection can see a kdbus peer.
+ *	@creds requestor's credentials
+ *	@creds_peer peer credentials
+ * @kdbus_conn_see_name
+ *	Check if a connection can see a kdbus service name.
+ *	@creds requestor's credentials
+ *	@name service name
+ * @kdbus_conn_see_notification
+ *	Check if a connection can receive notifications.
+ *	@creds requestor's credentials
+ * @kdbus_proc_permission
+ *	Check if a connection can access another task's pid namespace info.
+ *	@cred requestor's credentials
+ *	@pid target task's pid struct
+ * @kdbus_init_inode
+ *	Set the security label on a kdbusfs inode
+ *	@inode kdbusfs inode
+ *	@creds inode owner credentials
+ *
  * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
@@ -1310,6 +1349,22 @@ union security_list_options {
 					struct task_struct *to,
 					struct file *file);
 
+	int (*kdbus_conn_new)(const struct cred *creds,
+			      const struct kdbus_creds *fake_creds,
+			      const struct kdbus_pids *fake_pids,
+			      const char *fake_seclabel,
+			      bool owner, bool privileged, bool is_activator,
+			      bool is_monitor, bool is_policy_holder);
+	int (*kdbus_own_name)(const struct cred *creds, const char *name);
+	int (*kdbus_conn_talk)(const struct cred *creds,
+			       const struct cred *creds_peer);
+	int (*kdbus_conn_see)(const struct cred *creds,
+			      const struct cred *creds_peer);
+	int (*kdbus_conn_see_name)(const struct cred *creds, const char *name);
+	int (*kdbus_conn_see_notification)(const struct cred *creds);
+	int (*kdbus_proc_permission)(const struct cred *creds, struct pid *pid);
+	int (*kdbus_init_inode)(struct inode *inode, const struct cred *creds);
+
 	int (*ptrace_access_check)(struct task_struct *child,
 					unsigned int mode);
 	int (*ptrace_traceme)(struct task_struct *parent);
@@ -1620,6 +1675,14 @@ struct security_hook_heads {
 	struct list_head binder_transaction;
 	struct list_head binder_transfer_binder;
 	struct list_head binder_transfer_file;
+	struct list_head kdbus_conn_new;
+	struct list_head kdbus_own_name;
+	struct list_head kdbus_conn_talk;
+	struct list_head kdbus_conn_see;
+	struct list_head kdbus_conn_see_name;
+	struct list_head kdbus_conn_see_notification;
+	struct list_head kdbus_proc_permission;
+	struct list_head kdbus_init_inode;
 	struct list_head ptrace_access_check;
 	struct list_head ptrace_traceme;
 	struct list_head capget;
diff --git a/include/linux/security.h b/include/linux/security.h
index 2f4c1f7..68d83dd 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -53,6 +53,9 @@ struct msg_queue;
 struct xattr;
 struct xfrm_sec_ctx;
 struct mm_struct;
+struct kdbus_creds;
+struct kdbus_pids;
+struct pid;
 
 /* If capable should audit the security request */
 #define SECURITY_CAP_NOAUDIT 0
@@ -189,6 +192,21 @@ int security_binder_transfer_binder(struct task_struct *from,
 				    struct task_struct *to);
 int security_binder_transfer_file(struct task_struct *from,
 				  struct task_struct *to, struct file *file);
+int security_kdbus_conn_new(const struct cred *creds,
+			    const struct kdbus_creds *fake_creds,
+			    const struct kdbus_pids *fake_pids,
+			    const char *fake_seclabel,
+			    bool owner, bool privileged, bool is_activator,
+			    bool is_monitor, bool is_policy_holder);
+int security_kdbus_own_name(const struct cred *creds, const char *name);
+int security_kdbus_conn_talk(const struct cred *creds,
+			     const struct cred *creds_peer);
+int security_kdbus_conn_see(const struct cred *creds,
+			    const struct cred *creds_peer);
+int security_kdbus_conn_see_name(const struct cred *creds, const char *name);
+int security_kdbus_conn_see_notification(const struct cred *creds);
+int security_kdbus_proc_permission(const struct cred *creds, struct pid *pid);
+int security_kdbus_init_inode(struct inode *inode, const struct cred *creds);
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
@@ -402,6 +420,59 @@ static inline int security_binder_transfer_file(struct task_struct *from,
 	return 0;
 }
 
+static inline int security_kdbus_conn_new(const struct cred *creds,
+					  const struct kdbus_creds *fake_creds,
+					  const struct kdbus_pids *fake_pids,
+					  const char *fake_seclabel,
+					  bool owner, bool privileged,
+					  bool is_activator,
+					  bool is_monitor,
+					  bool is_policy_holder)
+{
+	return 0;
+}
+
+static inline int security_kdbus_own_name(const struct cred *creds,
+					  const char *name)
+{
+	return 0;
+}
+
+static inline int security_kdbus_conn_talk(const struct cred *creds,
+					   const struct cred *creds_peer)
+{
+	return 0;
+}
+
+static inline int security_kdbus_conn_see(const struct cred *creds,
+					  const struct cred *creds_peer)
+{
+	return 0;
+}
+
+static inline int security_kdbus_conn_see_name(const struct cred *creds,
+					       const char *name)
+{
+	return 0;
+}
+
+static inline int security_kdbus_conn_see_notification(const struct cred *creds)
+{
+	return 0;
+}
+
+static inline int security_kdbus_proc_permission)(const struct cred *creds,
+						  struct pid *pid)
+{
+	return 0;
+}
+
+static inline int security_kdbus_init_inode(struct inode *inode,
+					    const struct *creds)
+{
+	return 0;
+}
+
 static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index ef63d65..1cb87b3 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -26,6 +26,7 @@
 #include <linux/path.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
+#include <linux/security.h>
 #include <linux/shmem_fs.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep,
 	if (!owner && (creds || pids || seclabel))
 		return ERR_PTR(-EPERM);
 
+	ret = security_kdbus_conn_new(get_cred(file->f_cred),
+				      creds, pids, seclabel,
+				      owner, privileged,
+				      is_activator, is_monitor,
+				      is_policy_holder);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
 	ret = kdbus_sanitize_attach_flags(hello->attach_flags_send,
 					  &attach_flags_send);
 	if (ret < 0)
@@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn *conn,
 			return false;
 	}
 
-	if (conn->owner)
-		return true;
+	if (!conn->owner &&
+	    kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
+			       hash) < KDBUS_POLICY_OWN)
+		return false;
 
-	res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
-				 name, hash);
-	return res >= KDBUS_POLICY_OWN;
+	return (security_kdbus_own_name(conn_creds, name) == 0);
 }
 
 /**
@@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn *conn,
 					 to, KDBUS_POLICY_TALK))
 		return false;
 
-	if (conn->owner)
-		return true;
-	if (uid_eq(conn_creds->euid, to->cred->uid))
-		return true;
+	if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
+	    !kdbus_conn_policy_query_all(conn, conn_creds,
+					 &conn->ep->bus->policy_db, to,
+					 KDBUS_POLICY_TALK))
+		return false;
 
-	return kdbus_conn_policy_query_all(conn, conn_creds,
-					   &conn->ep->bus->policy_db, to,
-					   KDBUS_POLICY_TALK);
+	return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
 }
 
 /**
@@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct kdbus_conn *conn,
 					 const struct cred *conn_creds,
 					 const char *name)
 {
-	int res;
+	if (!conn_creds)
+		conn_creds = conn->cred;
 
 	/*
 	 * By default, all names are visible on a bus. SEE policies can only be
 	 * installed on custom endpoints, where by default no name is visible.
 	 */
-	if (!conn->ep->user)
-		return true;
+	if (conn->ep->user &&
+	    kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
+					kdbus_strhash(name)) < KDBUS_POLICY_SEE)
+		return false;
 
-	res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
-					  conn_creds ? : conn->cred,
-					  name, kdbus_strhash(name));
-	return res >= KDBUS_POLICY_SEE;
+	return (security_kdbus_conn_see_name(conn_creds, name) == 0);
 }
 
 static bool kdbus_conn_policy_see_name(struct kdbus_conn *conn,
@@ -1523,6 +1531,9 @@ static bool kdbus_conn_policy_see(struct kdbus_conn *conn,
 				  const struct cred *conn_creds,
 				  struct kdbus_conn *whom)
 {
+	if (!conn_creds)
+		conn_creds = conn->cred;
+
 	/*
 	 * By default, all names are visible on a bus, so a connection can
 	 * always see other connections. SEE policies can only be installed on
@@ -1530,10 +1541,13 @@ static bool kdbus_conn_policy_see(struct kdbus_conn *conn,
 	 * peers from each other, unless you see at least _one_ name of the
 	 * peer.
 	 */
-	return !conn->ep->user ||
-	       kdbus_conn_policy_query_all(conn, conn_creds,
-					   &conn->ep->policy_db, whom,
-					   KDBUS_POLICY_SEE);
+	if (conn->ep->user &&
+	    !kdbus_conn_policy_query_all(conn, conn_creds,
+					 &conn->ep->policy_db, whom,
+					 KDBUS_POLICY_SEE))
+		return false;
+
+	return (security_kdbus_conn_see(conn_creds, whom->cred) == 0);
 }
 
 /**
@@ -1551,6 +1565,9 @@ bool kdbus_conn_policy_see_notification(struct kdbus_conn *conn,
 					const struct cred *conn_creds,
 					const struct kdbus_msg *msg)
 {
+	if (!conn_creds)
+		conn_creds = conn->cred;
+
 	/*
 	 * Depending on the notification type, broadcasted kernel notifications
 	 * have to be filtered:
@@ -1567,18 +1584,22 @@ bool kdbus_conn_policy_see_notification(struct kdbus_conn *conn,
 	case KDBUS_ITEM_NAME_ADD:
 	case KDBUS_ITEM_NAME_REMOVE:
 	case KDBUS_ITEM_NAME_CHANGE:
-		return kdbus_conn_policy_see_name(conn, conn_creds,
-					msg->items[0].name_change.name);
+		if (!kdbus_conn_policy_see_name(conn, conn_creds,
+						msg->items[0].name_change.name))
+			return false;
 
 	case KDBUS_ITEM_ID_ADD:
 	case KDBUS_ITEM_ID_REMOVE:
-		return true;
+		/* fall through for the LSM check */
+		break;
 
 	default:
 		WARN(1, "Invalid type for notification broadcast: %llu\n",
 		     (unsigned long long)msg->items[0].type);
 		return false;
 	}
+
+	return (security_kdbus_conn_see_notification(conn_creds) == 0);
 }
 
 /**
diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
index 68818a8..4e84e89 100644
--- a/ipc/kdbus/fs.c
+++ b/ipc/kdbus/fs.c
@@ -23,6 +23,7 @@
 #include <linux/namei.h>
 #include <linux/pagemap.h>
 #include <linux/sched.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 
 #include "bus.h"
@@ -192,6 +193,7 @@ static const struct inode_operations fs_inode_iops = {
 static struct inode *fs_inode_get(struct super_block *sb,
 				  struct kdbus_node *node)
 {
+	int ret;
 	struct inode *inode;
 
 	inode = iget_locked(sb, node->id);
@@ -200,6 +202,10 @@ static struct inode *fs_inode_get(struct super_block *sb,
 	if (!(inode->i_state & I_NEW))
 		return inode;
 
+	ret = security_kdbus_init_inode(inode, node->creds);
+	if (ret)
+		return ERR_PTR(ret);
+
 	inode->i_private = kdbus_node_ref(node);
 	inode->i_mapping->a_ops = &empty_aops;
 	inode->i_mode = node->mode & S_IALLUGO;
diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index ae565cd..acbe981 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -150,12 +150,17 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 		for (i = 0; i < gaps->n_fds; ++i) {
 			int fd;
 
+			ret = security_file_receive(gaps->fd_files[i]);
+			if (ret) {
+				incomplete_fds = true;
+				fds[n_fds++] = -1;
+				continue;
+			}
+
 			fd = get_unused_fd_flags(O_CLOEXEC);
 			if (fd < 0)
 				incomplete_fds = true;
 
-			WARN_ON(!gaps->fd_files[i]);
-
 			fds[n_fds++] = fd < 0 ? -1 : fd;
 		}
 
@@ -178,6 +183,13 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 	for (i = 0; i < gaps->n_memfds; ++i) {
 		int memfd;
 
+		ret = security_file_receive(gaps->memfd_files[i]);
+		if (ret) {
+			incomplete_fds = true;
+			fds[n_fds++] = -1;
+			continue;
+		}
+
 		memfd = get_unused_fd_flags(O_CLOEXEC);
 		if (memfd < 0) {
 			incomplete_fds = true;
@@ -194,9 +206,6 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
 		 * message.
 		 */
 
-		WARN_ON(!gaps->memfd_offsets[i]);
-		WARN_ON(!gaps->memfd_files[i]);
-
 		kvec.iov_base = &memfd;
 		kvec.iov_len = sizeof(memfd);
 		ret = kdbus_pool_slice_copy_kvec(slice, gaps->memfd_offsets[i],
diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
index 71ca475..07c45d7 100644
--- a/ipc/kdbus/metadata.c
+++ b/ipc/kdbus/metadata.c
@@ -1182,11 +1182,9 @@ static unsigned int kdbus_proc_permission(const struct pid_namespace *pid_ns,
 					  const struct cred *cred,
 					  struct pid *target)
 {
-	if (pid_ns->hide_pid < 1)
-		return KDBUS_META_PROC_NORMAL;
-
 	/* XXX: we need groups_search() exported for aux-groups */
-	if (gid_eq(cred->egid, pid_ns->pid_gid))
+	if ((pid_ns->hide_pid < 1 || gid_eq(cred->egid, pid_ns->pid_gid)) &&
+	    security_kdbus_proc_permission(cred, target) == 0)
 		return KDBUS_META_PROC_NORMAL;
 
 	/*
diff --git a/security/security.c b/security/security.c
index 46f405c..1caf005 100644
--- a/security/security.c
+++ b/security/security.c
@@ -153,6 +153,55 @@ int security_binder_transfer_file(struct task_struct *from,
 	return call_int_hook(binder_transfer_file, 0, from, to, file);
 }
 
+int security_kdbus_conn_new(const struct cred *creds,
+			    const struct kdbus_creds *fake_creds,
+			    const struct kdbus_pids *fake_pids,
+			    const char *fake_seclabel,
+			    bool owner, bool privileged, bool is_activator,
+			    bool is_monitor, bool is_policy_holder)
+{
+	return call_int_hook(kdbus_conn_new, 0, creds, fake_creds, fake_pids,
+			     fake_seclabel, owner, privileged,
+			     is_activator, is_monitor, is_policy_holder);
+}
+
+int security_kdbus_own_name(const struct cred *creds, const char *name)
+{
+	return call_int_hook(kdbus_own_name, 0, creds, name);
+}
+
+int security_kdbus_conn_talk(const struct cred *creds,
+			     const struct cred *creds_peer)
+{
+	return call_int_hook(kdbus_conn_talk, 0, creds, creds_peer);
+}
+
+int security_kdbus_conn_see(const struct cred *creds,
+			    const struct cred *creds_peer)
+{
+	return call_int_hook(kdbus_conn_see, 0, creds, creds_peer);
+}
+
+int security_kdbus_conn_see_name(const struct cred *creds, const char *name)
+{
+	return call_int_hook(kdbus_conn_see_name, 0, creds, name);
+}
+
+int security_kdbus_conn_see_notification(const struct cred *creds)
+{
+	return call_int_hook(kdbus_conn_see_notification, 0, creds);
+}
+
+int security_kdbus_proc_permission(const struct cred *creds, struct pid *pid)
+{
+	return call_int_hook(kdbus_proc_permission, 0, creds, pid);
+}
+
+int security_kdbus_init_inode(struct inode *inode, const struct cred *creds)
+{
+	return call_int_hook(kdbus_init_inode, 0, inode, creds);
+}
+
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	return call_int_hook(ptrace_access_check, 0, child, mode);
@@ -1548,6 +1597,19 @@ struct security_hook_heads security_hook_heads = {
 	.binder_transfer_file =
 		LIST_HEAD_INIT(security_hook_heads.binder_transfer_file),
 
+	.kdbus_conn_new = LIST_HEAD_INIT(security_hook_heads.kdbus_conn_new),
+	.kdbus_own_name = LIST_HEAD_INIT(security_hook_heads.kdbus_own_name),
+	.kdbus_conn_talk = LIST_HEAD_INIT(security_hook_heads.kdbus_conn_talk),
+	.kdbus_conn_see = LIST_HEAD_INIT(security_hook_heads.kdbus_conn_see),
+	.kdbus_conn_see_name =
+		LIST_HEAD_INIT(security_hook_heads.kdbus_conn_see_name),
+	.kdbus_conn_see_notification =
+		LIST_HEAD_INIT(security_hook_heads.kdbus_conn_see_notification),
+	.kdbus_proc_permission =
+		LIST_HEAD_INIT(security_hook_heads.kdbus_proc_permission),
+	.kdbus_init_inode =
+		LIST_HEAD_INIT(security_hook_heads.kdbus_init_inode),
+
 	.ptrace_access_check =
 		LIST_HEAD_INIT(security_hook_heads.ptrace_access_check),
 	.ptrace_traceme =

[RFC PATCH v3 3/5] lsm: add support for auditing kdbus service names

[Paul Moore]

The kdbus service names will be recorded using 'service', similar to
the existing dbus audit records.

Signed-off-by: Paul Moore <pmoore@redhat.com>

---
ChangeLog:
- v3
 * Ported to the 4.3-rc4 based kdbus tree
- v2
 * Initial draft
---
 include/linux/lsm_audit.h |    2 ++
 security/lsm_audit.c      |    4 ++++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index ffb9c9d..d6a656f 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -59,6 +59,7 @@ struct common_audit_data {
 #define LSM_AUDIT_DATA_INODE	9
 #define LSM_AUDIT_DATA_DENTRY	10
 #define LSM_AUDIT_DATA_IOCTL_OP	11
+#define LSM_AUDIT_DATA_KDBUS	12
 	union 	{
 		struct path path;
 		struct dentry *dentry;
@@ -75,6 +76,7 @@ struct common_audit_data {
 #endif
 		char *kmod_name;
 		struct lsm_ioctlop_audit *op;
+		const char *kdbus_name;
 	} u;
 	/* this union contains LSM specific data */
 	union {
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index cccbf30..0a3dc1b 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -397,6 +397,10 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		audit_log_format(ab, " kmod=");
 		audit_log_untrustedstring(ab, a->u.kmod_name);
 		break;
+	case LSM_AUDIT_DATA_KDBUS:
+		audit_log_format(ab, " service=");
+		audit_log_untrustedstring(ab, a->u.kdbus_name);
+		break;
 	} /* switch (a->type) */
 }
 

[RFC PATCH v3 4/5] selinux: introduce kdbus names into the policy

[Paul Moore]

SELinux treats kdbus service names as objects and therefore needs a
mechanism to map service names to security labels.  This patch adds
support for loading kdbus name/label matches with the security policy.

The patch supports service name prefix matching to lessen the burden
on the policy developers and reduce the size of the resulting policy.

Signed-off-by: Paul Moore <pmoore@redhat.com>

---
ChangeLog:
- v3
 * Ported to the 4.3-rc4 based kdbus tree, v2 hacks removed
- v2
 * Porting needed to work with ioctl xperms
- v1
 * Initial draft
---
 security/selinux/include/security.h |    5 ++
 security/selinux/ss/policydb.c      |   88 +++++++++++++++++++++++++++++------
 security/selinux/ss/policydb.h      |    3 +
 security/selinux/ss/services.c      |   38 +++++++++++++++
 4 files changed, 116 insertions(+), 18 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 6a681d2..339b32b 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -36,13 +36,14 @@
 #define POLICYDB_VERSION_DEFAULT_TYPE	28
 #define POLICYDB_VERSION_CONSTRAINT_NAMES	29
 #define POLICYDB_VERSION_XPERMS_IOCTL	30
+#define POLICYDB_VERSION_KDBUS		31
 
 /* Range of policy versions we understand*/
 #define POLICYDB_VERSION_MIN   POLICYDB_VERSION_BASE
 #ifdef CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX
 #define POLICYDB_VERSION_MAX	CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX_VALUE
 #else
-#define POLICYDB_VERSION_MAX	POLICYDB_VERSION_XPERMS_IOCTL
+#define POLICYDB_VERSION_MAX	POLICYDB_VERSION_KDBUS
 #endif
 
 /* Mask for just the mount related flags */
@@ -212,6 +213,8 @@ int security_fs_use(struct super_block *sb);
 int security_genfs_sid(const char *fstype, char *name, u16 sclass,
 	u32 *sid);
 
+int security_kdbus_sid(const char *name, u32 *sid);
+
 #ifdef CONFIG_NETLABEL
 int security_netlbl_secattr_to_sid(struct netlbl_lsm_secattr *secattr,
 				   u32 *sid);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 992a315..9be2e6d 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -76,81 +76,86 @@ static struct policydb_compat_info policydb_compat[] = {
 	{
 		.version	= POLICYDB_VERSION_BASE,
 		.sym_num	= SYM_NUM - 3,
-		.ocon_num	= OCON_NUM - 1,
+		.ocon_num	= OCON_NUM - 2,
 	},
 	{
 		.version	= POLICYDB_VERSION_BOOL,
 		.sym_num	= SYM_NUM - 2,
-		.ocon_num	= OCON_NUM - 1,
+		.ocon_num	= OCON_NUM - 2,
 	},
 	{
 		.version	= POLICYDB_VERSION_IPV6,
 		.sym_num	= SYM_NUM - 2,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_NLCLASS,
 		.sym_num	= SYM_NUM - 2,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_MLS,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_AVTAB,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_RANGETRANS,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_POLCAP,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_PERMISSIVE,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_BOUNDARY,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_FILENAME_TRANS,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_ROLETRANS,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_NEW_OBJECT_DEFAULTS,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_DEFAULT_TYPE,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_CONSTRAINT_NAMES,
 		.sym_num	= SYM_NUM,
-		.ocon_num	= OCON_NUM,
+		.ocon_num	= OCON_NUM - 1,
 	},
 	{
 		.version	= POLICYDB_VERSION_XPERMS_IOCTL,
 		.sym_num	= SYM_NUM,
+		.ocon_num	= OCON_NUM - 1,
+	},
+	{
+		.version	= POLICYDB_VERSION_KDBUS,
+		.sym_num	= SYM_NUM,
 		.ocon_num	= OCON_NUM,
 	},
 };
@@ -2111,7 +2116,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
 	int i, j, rc;
 	u32 nel, len;
 	__le32 buf[3];
-	struct ocontext *l, *c;
+	struct ocontext *l, *l2, *c;
 	u32 nodebuf[8];
 
 	for (i = 0; i < info->ocon_num; i++) {
@@ -2130,6 +2135,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
 				l->next = c;
 			else
 				p->ocontexts[i] = c;
+			l2 = l;
 			l = c;
 
 			switch (i) {
@@ -2219,6 +2225,43 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
 					goto out;
 				break;
 			}
+			case OCON_KDBUS: {
+				struct ocontext *iter, *last;
+				u32 len2;
+
+				rc = next_entry(buf, fp, sizeof(u32));
+				if (rc)
+					goto out;
+				len = le32_to_cpu(buf[0]);
+				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+				if (rc)
+					goto out;
+				rc = context_read_and_validate(&c->context[0], p, fp);
+				if (rc) {
+					kfree(c->u.name);
+					goto out;
+				}
+
+				/* sort by ->u.name length, longest first */
+				last = NULL;
+				iter = p->ocontexts[OCON_KDBUS];
+				while (iter != c) {
+					len2 = strlen(iter->u.name);
+					if (len > len2) {
+						if (l2)
+							l2->next = NULL;
+						c->next = iter;
+						if (last == NULL)
+							p->ocontexts[i] = c;
+						else
+							last->next = c;
+						break;
+					}
+					last = iter;
+					iter = iter->next;
+				}
+				break;
+			}
 			}
 		}
 	}
@@ -3147,6 +3190,19 @@ static int ocontext_write(struct policydb *p, struct policydb_compat_info *info,
 				if (rc)
 					return rc;
 				break;
+			case OCON_KDBUS:
+				len = strlen(c->u.name);
+				buf[0] = cpu_to_le32(len);
+				rc = put_entry(buf, sizeof(u32), 1, fp);
+				if (rc)
+					return rc;
+				rc = put_entry(c->u.name, len, 1, fp);
+				if (rc)
+					return rc;
+				rc = context_write(p, &c->context[0], fp);
+				if (rc)
+					return rc;
+				break;
 			}
 		}
 	}
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 725d594..ee9c120 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -222,7 +222,8 @@ struct genfs {
 #define OCON_NODE  4	/* nodes */
 #define OCON_FSUSE 5	/* fs_use */
 #define OCON_NODE6 6	/* IPv6 nodes */
-#define OCON_NUM   7
+#define OCON_KDBUS 7	/* kdbus names */
+#define OCON_NUM   8
 
 /* The policy database */
 struct policydb {
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b7df12b..ada2d28 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2536,6 +2536,44 @@ int security_genfs_sid(const char *fstype,
 }
 
 /**
+ * security_kdbus_sid - Obtain a SID for a kdbus name
+ * @name: kdbus name
+ * @sid: SID for the kdbus name
+ *
+ * Obtain a SID for the given kdbus service name.  Returns zero on success,
+ * negative values on error.
+ */
+int security_kdbus_sid(const char *name, u32 *sid)
+{
+	int rc = 0;
+	struct ocontext *c;
+
+	read_lock(&policy_rwlock);
+
+	c = policydb.ocontexts[OCON_KDBUS];
+	while (c) {
+		if (strncmp(c->u.name, name, strlen(c->u.name)) == 0)
+			break;
+		c = c->next;
+	}
+
+	if (c) {
+		if (!c->sid[0]) {
+			rc = sidtab_context_to_sid(&sidtab,
+						   &c->context[0], &c->sid[0]);
+			if (rc)
+				goto out;
+		}
+		*sid = c->sid[0];
+	} else
+		*sid = SECINITSID_UNLABELED;
+
+out:
+	read_unlock(&policy_rwlock);
+	return rc;
+}
+
+/**
  * security_fs_use - Determine how to handle labeling for a filesystem.
  * @sb: superblock in question
  */

[RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Paul Moore]

Add the SELinux access control implementation for the new kdbus LSM
hooks using the new kdbus object class and the following permissions:

 [NOTE: permissions below are based on kdbus code from Aug 2015]

 * kdbus:impersonate
   Send a different security label to kdbus peers.
 * kdbus:fakecreds
   Send different DAC credentials to kdbus peers.
 * kdbus:fakepids
   Send a different PID to kdbus peers.
 * kdbus:owner
   Act as a kdbus bus owner.
 * kdbus:privileged
   Act as a privileged endpoint.
 * kdbus:activator
   Act as a kdbus activator.
 * kdbus:monitor
   Act as a kdbus monitor.
 * kdbus:policy_holder
   Act as a kdbus policy holder.
 * kdbus:connect
   Create a new kdbus connection.
 * kdbus:own
   Own a kdbus service name.
 * kdbus:talk
   Talk between two kdbus endpoints.
 * kdbus:see
   See another kdbus endpoint.
 * kdbus:see_name
   See a kdbus service name.
 * kdbus:see_notification
   See a kdbus notification.

Signed-off-by: Paul Moore <pmoore@redhat.com>

---
ChangeLog:
- v3
 * Ported to the 4.3-rc4 based kdbus tree
 * Fix the missing NULL terminator in the kdbus obj class definition
- v2
 * Add the selinux_kdbus_init_inode() hook
 * Add some very basic info on the permissions to the description
 * Add kdbus service name auditing in the AVC records
- v1
 * Initial draft
---
 security/selinux/hooks.c            |  153 +++++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h |    4 +
 2 files changed, 155 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e4369d8..5581990 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -9,8 +9,10 @@
  *	      James Morris <jmorris@redhat.com>
  *
  *  Copyright (C) 2001,2002 Networks Associates Technology, Inc.
- *  Copyright (C) 2003-2008 Red Hat, Inc., James Morris <jmorris@redhat.com>
- *					   Eric Paris <eparis@redhat.com>
+ *  Copyright (C) 2003-2008,2015 Red Hat, Inc.
+ *					James Morris <jmorris@redhat.com>
+ *					Eric Paris <eparis@redhat.com>
+ *					Paul Moore <paul@paul-moore.com>
  *  Copyright (C) 2004-2005 Trusted Computer Solutions, Inc.
  *			    <dgoeddel@trustedcs.com>
  *  Copyright (C) 2006, 2007, 2009 Hewlett-Packard Development Company, L.P.
@@ -2035,6 +2037,143 @@ static int selinux_binder_transfer_file(struct task_struct *from,
 			    &ad);
 }
 
+static int selinux_kdbus_conn_new(const struct cred *creds,
+				  const struct kdbus_creds *fake_creds,
+				  const struct kdbus_pids *fake_pids,
+				  const char *fake_seclabel,
+				  bool owner, bool privileged,
+				  bool is_activator, bool is_monitor,
+				  bool is_policy_holder)
+{
+	int rc;
+	u32 tsid = current_sid();
+	u32 av = KDBUS__CONNECT;
+
+	if (fake_creds)
+		av |= KDBUS__FAKECREDS;
+	if (fake_pids)
+		av |= KDBUS__FAKEPIDS;
+	if (owner)
+		av |= KDBUS__OWNER;
+	if (privileged)
+		av |= KDBUS__PRIVILEGED;
+	if (is_activator)
+		av |= KDBUS__ACTIVATOR;
+	if (is_monitor)
+		av |= KDBUS__MONITOR;
+	if (is_policy_holder)
+		av |= KDBUS__POLICY_HOLDER;
+
+	rc = avc_has_perm(tsid, cred_sid(creds), SECCLASS_KDBUS, av, NULL);
+	if (rc)
+		return rc;
+
+	if (fake_seclabel) {
+		u32 sid;
+		if (security_context_to_sid(fake_seclabel,
+					    strlen(fake_seclabel),
+					    &sid, GFP_KERNEL))
+			return -EINVAL;
+
+		rc = avc_has_perm(tsid, sid,
+				  SECCLASS_KDBUS, KDBUS__IMPERSONATE, NULL);
+	}
+
+	return rc;
+}
+
+static int selinux_kdbus_own_name(const struct cred *creds, const char *name)
+{
+	int rc;
+	u32 name_sid;
+	struct common_audit_data ad;
+
+	rc = security_kdbus_sid(name, &name_sid);
+	if (rc)
+		return rc;
+
+	ad.type = LSM_AUDIT_DATA_KDBUS;
+	ad.u.kdbus_name = name;
+
+	return avc_has_perm(cred_sid(creds), name_sid,
+			    SECCLASS_KDBUS, KDBUS__OWN, &ad);
+}
+
+static int selinux_kdbus_conn_talk(const struct cred *creds,
+				   const struct cred *creds_to)
+{
+	return avc_has_perm(cred_sid(creds), cred_sid(creds_to),
+			    SECCLASS_KDBUS, KDBUS__TALK, NULL);
+}
+
+static int selinux_kdbus_conn_see(const struct cred *creds,
+				  const struct cred *creds_whom)
+{
+	return avc_has_perm(cred_sid(creds), cred_sid(creds_whom),
+			    SECCLASS_KDBUS, KDBUS__SEE, NULL);
+}
+
+static int selinux_kdbus_conn_see_name(const struct cred *creds,
+				       const char *name)
+{
+	int rc;
+	u32 name_sid;
+	struct common_audit_data ad;
+
+	rc = security_kdbus_sid(name, &name_sid);
+	if (rc)
+		return rc;
+
+	ad.type = LSM_AUDIT_DATA_KDBUS;
+	ad.u.kdbus_name = name;
+
+	return avc_has_perm(cred_sid(creds), name_sid,
+			    SECCLASS_KDBUS, KDBUS__SEE_NAME, &ad);
+}
+
+static int selinux_kdbus_conn_see_notification(const struct cred *creds)
+{
+	return avc_has_perm(SECINITSID_KERNEL, cred_sid(creds),
+			    SECCLASS_KDBUS, KDBUS__SEE_NOTIFICATION, NULL);
+}
+
+static int selinux_kdbus_proc_permission(const struct cred *creds,
+					 struct pid *pid)
+{
+	int rc;
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = pid_task(pid, PIDTYPE_PID);
+	rc = avc_has_perm(cred_sid(creds), task_sid(task),
+			  SECCLASS_PROCESS, PROCESS__GETATTR, NULL);
+	rcu_read_unlock();
+
+	return rc;
+}
+
+static int selinux_kdbus_init_inode(struct inode *inode,
+				    const struct cred *creds)
+{
+	struct inode_security_struct *isec = inode->i_security;
+	u32 sid = cred_sid(creds);
+
+	/* XXX - this is very simple, e.g. no transitions, no special object
+	 *       class, etc. since this inode is basically an IPC socket ...
+	 *       however, is this too simple?  do we want transitions?  if we
+	 *       do, we should do the transition in kdbus_node_init() and not
+	 *       here so that endpoint is labeled correctly and not just this
+	 *       inode */
+
+	isec->inode = inode;
+	isec->task_sid = sid;
+	isec->sid = sid;
+	isec->sclass = SECCLASS_FILE;
+	isec->initialized = 1;
+
+	return 0;
+}
+
 static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
@@ -5862,6 +6001,16 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
 	LSM_HOOK_INIT(binder_transfer_file, selinux_binder_transfer_file),
 
+	LSM_HOOK_INIT(kdbus_conn_new, selinux_kdbus_conn_new),
+	LSM_HOOK_INIT(kdbus_own_name, selinux_kdbus_own_name),
+	LSM_HOOK_INIT(kdbus_conn_talk, selinux_kdbus_conn_talk),
+	LSM_HOOK_INIT(kdbus_conn_see_name, selinux_kdbus_conn_see_name),
+	LSM_HOOK_INIT(kdbus_conn_see, selinux_kdbus_conn_see),
+	LSM_HOOK_INIT(kdbus_conn_see_notification,
+		      selinux_kdbus_conn_see_notification),
+	LSM_HOOK_INIT(kdbus_proc_permission, selinux_kdbus_proc_permission),
+	LSM_HOOK_INIT(kdbus_init_inode, selinux_kdbus_init_inode),
+
 	LSM_HOOK_INIT(ptrace_access_check, selinux_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, selinux_ptrace_traceme),
 	LSM_HOOK_INIT(capget, selinux_capget),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 5a4eef5..95eb6c3 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -157,5 +157,9 @@ struct security_class_mapping secclass_map[] = {
 	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },
 	{ "binder", { "impersonate", "call", "set_context_mgr", "transfer",
 		      NULL } },
+	{ "kdbus", { "impersonate", "fakecreds", "fakepids", "owner",
+		     "privileged", "activator", "monitor", "policy_holder",
+		     "connect", "own", "talk", "see", "see_name",
+		     "see_notification", NULL } },
 	{ NULL }
   };

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Paul Moore]

On Wednesday, October 07, 2015 07:08:48 PM Paul Moore wrote:
> +static int selinux_kdbus_conn_see_notification(const struct cred *creds)
> +{
> +	        return avc_has_perm(SECINITSID_KERNEL, cred_sid(creds),
> +                         SECCLASS_KDBUS, KDBUS__SEE_NOTIFICATION, NULL);
> +}

I'm going to flip the subj/obj ordering here, the passed credentials should be 
the subject.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Paul Moore]

On Wednesday, October 07, 2015 07:08:48 PM Paul Moore wrote:
> +static int selinux_kdbus_conn_see_notification(const struct cred *creds)
> +{
> +	        return avc_has_perm(SECINITSID_KERNEL, cred_sid(creds),
> +                         SECCLASS_KDBUS, KDBUS__SEE_NOTIFICATION, NULL);
> +}

I'm going to flip the subj/obj ordering here, the passed credentials should be 
the subject.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 1/5] kdbus: add creator credentials to the endpoints

[Stephen Smalley]

On 10/07/2015 07:08 PM, Paul Moore wrote:
> In order to effectively enforce LSM based access controls we need to
> have more information about the kdbus endpoint creator than the
> uid/gid currently stored in the kdbus_node_type struct.  This patch
> replaces the uid/gid values with a reference to the node creator's
> credential struct which serves the needs of both the kdbus DAC access
> controls as well as the LSM's access controls.
>
> Two macros have also been created, kdbus_node_[uid,gid](), which can
> be used to easily extract the euid/egid information from the new
> credential reference.  The effective uid/gid is used as it was used
> in all areas of the previous kdbus code except for areas where the
> uid/gid was never set beyond the basic initialization to zero/root;
> I expect this was a bug that was never caught as the node creator in
> these cases was always expect to be root.
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
>
> ---
> ChangeLog:
> - v3
>   * Ported to the 4.3-rc4 based kdbus tree
> - v2
>   * Initial draft
> ---
>   ipc/kdbus/bus.c      |   13 +++++--------
>   ipc/kdbus/endpoint.c |   14 ++++----------
>   ipc/kdbus/endpoint.h |    3 +--
>   ipc/kdbus/fs.c       |    4 ++--
>   ipc/kdbus/node.c     |   11 ++++-------
>   ipc/kdbus/node.h     |    5 +++--
>   6 files changed, 19 insertions(+), 31 deletions(-)
>

> diff --git a/ipc/kdbus/node.c b/ipc/kdbus/node.c
> index 89f58bc..cd0c1a0 100644
> --- a/ipc/kdbus/node.c
> +++ b/ipc/kdbus/node.c
> @@ -12,6 +12,7 @@
>    */
>
>   #include <linux/atomic.h>
> +#include <linux/cred.h>
>   #include <linux/fs.h>
>   #include <linux/idr.h>
>   #include <linux/kdev_t.h>
> @@ -170,13 +171,7 @@
>    *                         node initialization. They must remain constant. If
>    *                         NULL, they're skipped.
>    *
> - *     * node->mode: filesystem access modes

mode still remains

Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Stephen Smalley]

On 10/07/2015 07:08 PM, Paul Moore wrote:
> Add LSM access control hooks to kdbus; several new hooks are added and
> the existing security_file_receive() hook is reused.  The new hooks
> are listed below:
>
>   * security_kdbus_conn_new
>     Check if the current task is allowed to create a new kdbus
>     connection.
>   * security_kdbus_own_name
>     Check if a connection is allowed to own a kdbus service name.
>   * security_kdbus_conn_talk
>     Check if a connection is allowed to talk to a kdbus peer.
>   * security_kdbus_conn_see
>     Check if a connection can see a kdbus peer.
>   * security_kdbus_conn_see_name
>     Check if a connection can see a kdbus service name.
>   * security_kdbus_conn_see_notification
>     Check if a connection can receive notifications.
>   * security_kdbus_proc_permission
>     Check if a connection can access another task's pid namespace info.
>   * security_kdbus_init_inode
>     Set the security label on a kdbusfs inode
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
>
> ---
> ChangeLog:
> - v3
>   * Ported to the 4.3-rc4 based kdbus tree
> - v2
>   * Implemented suggestions by Stephen Smalley
>     * call security_kdbus_conn_new() sooner
>     * reworked hook inside kdbus_conn_policy_own_name()
>     * fixed if-conditional in kdbus_conn_policy_talk()
>     * reworked hook inside kdbus_conn_policy_see_name_unlocked()
>     * reworked hook inside kdbus_conn_policy_see()
>     * reworked hook inside kdbus_conn_policy_see_notification()
>     * added the security_kdbus_init_inode() hook
> - v1
>   * Initial draft
> ---
>   include/linux/lsm_hooks.h |   63 +++++++++++++++++++++++++++++++++++++++
>   include/linux/security.h  |   71 ++++++++++++++++++++++++++++++++++++++++++++
>   ipc/kdbus/connection.c    |   73 +++++++++++++++++++++++++++++----------------
>   ipc/kdbus/fs.c            |    6 ++++
>   ipc/kdbus/message.c       |   19 +++++++++---
>   ipc/kdbus/metadata.c      |    6 +---
>   security/security.c       |   62 ++++++++++++++++++++++++++++++++++++++
>   7 files changed, 265 insertions(+), 35 deletions(-)
>
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index ef63d65..1cb87b3 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -26,6 +26,7 @@
>   #include <linux/path.h>
>   #include <linux/poll.h>
>   #include <linux/sched.h>
> +#include <linux/security.h>
>   #include <linux/shmem_fs.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
> @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep,
>   	if (!owner && (creds || pids || seclabel))
>   		return ERR_PTR(-EPERM);
>
> +	ret = security_kdbus_conn_new(get_cred(file->f_cred),

You only need to use get_cred() if saving a reference; otherwise, you'll 
leak one here.  Also, do we want file->f_cred here or 
ep->bus->node.creds (the latter is what is used by their own checks; the 
former is typically the same as current cred IIUC).  For that matter, 
what about ep->node.creds vs ep->bus->node.creds vs. 
ep->bus->domain->node.creds?  Can they differ?  Do we care?

> +				      creds, pids, seclabel,
> +				      owner, privileged,
> +				      is_activator, is_monitor,
> +				      is_policy_holder);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
>   	ret = kdbus_sanitize_attach_flags(hello->attach_flags_send,
>   					  &attach_flags_send);
>   	if (ret < 0)
> @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn *conn,
>   			return false;
>   	}
>
> -	if (conn->owner)
> -		return true;
> +	if (!conn->owner &&
> +	    kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
> +			       hash) < KDBUS_POLICY_OWN)
> +		return false;
>
> -	res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
> -				 name, hash);
> -	return res >= KDBUS_POLICY_OWN;
> +	return (security_kdbus_own_name(conn_creds, name) == 0);

Similar question here.  conn_creds is the credentials of the creator of 
the connection, typically the client/sender, right? 
conn->ep->bus->node.creds are the credentials of the bus owner, so don't 
we want to ask "Can I own this name on this bus?".  Note that their 
policy checks are based on conn->ep->policy_db, i.e. the policy 
associated with the endpoint, and conn->owner is only true if the 
connection creator has the same uid as the bus.

>   }
>
>   /**
> @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn *conn,
>   					 to, KDBUS_POLICY_TALK))
>   		return false;
>
> -	if (conn->owner)
> -		return true;
> -	if (uid_eq(conn_creds->euid, to->cred->uid))
> -		return true;
> +	if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
> +	    !kdbus_conn_policy_query_all(conn, conn_creds,
> +					 &conn->ep->bus->policy_db, to,
> +					 KDBUS_POLICY_TALK))
> +		return false;
>
> -	return kdbus_conn_policy_query_all(conn, conn_creds,
> -					   &conn->ep->bus->policy_db, to,
> -					   KDBUS_POLICY_TALK);
> +	return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);

Here at least we have a notion of client and peer.  But we still aren't 
considering conn->ep or conn->ep->bus, whereas they are querying both 
policy dbs for their decision.  The parallel would be checking access to 
the labels of both I suppose, unless we institute a control up front 
over the relationship between the label of the endpoint and the label of 
the bus.

>   }
>
>   /**
> @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct kdbus_conn *conn,
>   					 const struct cred *conn_creds,
>   					 const char *name)
>   {
> -	int res;
> +	if (!conn_creds)
> +		conn_creds = conn->cred;
>
>   	/*
>   	 * By default, all names are visible on a bus. SEE policies can only be
>   	 * installed on custom endpoints, where by default no name is visible.
>   	 */
> -	if (!conn->ep->user)
> -		return true;
> +	if (conn->ep->user &&
> +	    kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
> +					kdbus_strhash(name)) < KDBUS_POLICY_SEE)
> +		return false;
>
> -	res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
> -					  conn_creds ? : conn->cred,
> -					  name, kdbus_strhash(name));
> -	return res >= KDBUS_POLICY_SEE;
> +	return (security_kdbus_conn_see_name(conn_creds, name) == 0);

Here they only define policy based on endpoints, not bus.  Not sure what 
we want, but we need at least one of their creds.  Same for the rest.

>   }
>
>   static bool kdbus_conn_policy_see_name(struct kdbus_conn *conn,
> @@ -1523,6 +1531,9 @@ static bool kdbus_conn_policy_see(struct kdbus_conn *conn,
>   				  const struct cred *conn_creds,
>   				  struct kdbus_conn *whom)
>   {
> +	if (!conn_creds)
> +		conn_creds = conn->cred;
> +
>   	/*
>   	 * By default, all names are visible on a bus, so a connection can
>   	 * always see other connections. SEE policies can only be installed on
> @@ -1530,10 +1541,13 @@ static bool kdbus_conn_policy_see(struct kdbus_conn *conn,
>   	 * peers from each other, unless you see at least _one_ name of the
>   	 * peer.
>   	 */
> -	return !conn->ep->user ||
> -	       kdbus_conn_policy_query_all(conn, conn_creds,
> -					   &conn->ep->policy_db, whom,
> -					   KDBUS_POLICY_SEE);
> +	if (conn->ep->user &&
> +	    !kdbus_conn_policy_query_all(conn, conn_creds,
> +					 &conn->ep->policy_db, whom,
> +					 KDBUS_POLICY_SEE))
> +		return false;
> +
> +	return (security_kdbus_conn_see(conn_creds, whom->cred) == 0);
>   }
>
>   /**
> @@ -1551,6 +1565,9 @@ bool kdbus_conn_policy_see_notification(struct kdbus_conn *conn,
>   					const struct cred *conn_creds,
>   					const struct kdbus_msg *msg)
>   {
> +	if (!conn_creds)
> +		conn_creds = conn->cred;
> +
>   	/*
>   	 * Depending on the notification type, broadcasted kernel notifications
>   	 * have to be filtered:
> @@ -1567,18 +1584,22 @@ bool kdbus_conn_policy_see_notification(struct kdbus_conn *conn,
>   	case KDBUS_ITEM_NAME_ADD:
>   	case KDBUS_ITEM_NAME_REMOVE:
>   	case KDBUS_ITEM_NAME_CHANGE:
> -		return kdbus_conn_policy_see_name(conn, conn_creds,
> -					msg->items[0].name_change.name);
> +		if (!kdbus_conn_policy_see_name(conn, conn_creds,
> +						msg->items[0].name_change.name))
> +			return false;
>
>   	case KDBUS_ITEM_ID_ADD:
>   	case KDBUS_ITEM_ID_REMOVE:
> -		return true;
> +		/* fall through for the LSM check */
> +		break;
>
>   	default:
>   		WARN(1, "Invalid type for notification broadcast: %llu\n",
>   		     (unsigned long long)msg->items[0].type);
>   		return false;
>   	}
> +
> +	return (security_kdbus_conn_see_notification(conn_creds) == 0);
>   }
>
>   /**
> diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
> index 68818a8..4e84e89 100644
> --- a/ipc/kdbus/fs.c
> +++ b/ipc/kdbus/fs.c
> @@ -23,6 +23,7 @@
>   #include <linux/namei.h>
>   #include <linux/pagemap.h>
>   #include <linux/sched.h>
> +#include <linux/security.h>
>   #include <linux/slab.h>
>
>   #include "bus.h"
> @@ -192,6 +193,7 @@ static const struct inode_operations fs_inode_iops = {
>   static struct inode *fs_inode_get(struct super_block *sb,
>   				  struct kdbus_node *node)
>   {
> +	int ret;
>   	struct inode *inode;
>
>   	inode = iget_locked(sb, node->id);
> @@ -200,6 +202,10 @@ static struct inode *fs_inode_get(struct super_block *sb,
>   	if (!(inode->i_state & I_NEW))
>   		return inode;
>
> +	ret = security_kdbus_init_inode(inode, node->creds);
> +	if (ret)
> +		return ERR_PTR(ret);

Need to put the inode.

> +
>   	inode->i_private = kdbus_node_ref(node);
>   	inode->i_mapping->a_ops = &empty_aops;
>   	inode->i_mode = node->mode & S_IALLUGO;

Re: [RFC PATCH v3 1/5] kdbus: add creator credentials to the endpoints

[Paul Moore]

On Friday, October 09, 2015 10:31:07 AM Stephen Smalley wrote:
> mode still remains

Yes it does, it looks like I went a little crazy with the Ctrl-K ... thanks 
for the review, it will be fixed in the next draft.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 1/5] kdbus: add creator credentials to the endpoints

[Paul Moore]

On Friday, October 09, 2015 10:31:07 AM Stephen Smalley wrote:
> mode still remains

Yes it does, it looks like I went a little crazy with the Ctrl-K ... thanks 
for the review, it will be fixed in the next draft.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 3/5] lsm: add support for auditing kdbus service names

[Stephen Smalley]

On 10/07/2015 07:08 PM, Paul Moore wrote:
> The kdbus service names will be recorded using 'service', similar to
> the existing dbus audit records.
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
>
> ---
> ChangeLog:
> - v3
>   * Ported to the 4.3-rc4 based kdbus tree
> - v2
>   * Initial draft
> ---
>   include/linux/lsm_audit.h |    2 ++
>   security/lsm_audit.c      |    4 ++++
>   2 files changed, 6 insertions(+)
>
> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
> index ffb9c9d..d6a656f 100644
> --- a/include/linux/lsm_audit.h
> +++ b/include/linux/lsm_audit.h
> @@ -59,6 +59,7 @@ struct common_audit_data {
>   #define LSM_AUDIT_DATA_INODE	9
>   #define LSM_AUDIT_DATA_DENTRY	10
>   #define LSM_AUDIT_DATA_IOCTL_OP	11
> +#define LSM_AUDIT_DATA_KDBUS	12
>   	union 	{
>   		struct path path;
>   		struct dentry *dentry;
> @@ -75,6 +76,7 @@ struct common_audit_data {
>   #endif
>   		char *kmod_name;
>   		struct lsm_ioctlop_audit *op;
> +		const char *kdbus_name;
>   	} u;
>   	/* this union contains LSM specific data */
>   	union {
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index cccbf30..0a3dc1b 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -397,6 +397,10 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>   		audit_log_format(ab, " kmod=");
>   		audit_log_untrustedstring(ab, a->u.kmod_name);
>   		break;
> +	case LSM_AUDIT_DATA_KDBUS:
> +		audit_log_format(ab, " service=");

Not a major issue to me, but just wondering if this needs to be further 
qualified to indicate it is a kdbus service.  service= is rather generic.

> +		audit_log_untrustedstring(ab, a->u.kdbus_name);
> +		break;
>   	} /* switch (a->type) */
>   }
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Stephen Smalley]

On 10/07/2015 07:08 PM, Paul Moore wrote:
> Add the SELinux access control implementation for the new kdbus LSM
> hooks using the new kdbus object class and the following permissions:
>
>   [NOTE: permissions below are based on kdbus code from Aug 2015]
>
>   * kdbus:impersonate
>     Send a different security label to kdbus peers.
>   * kdbus:fakecreds
>     Send different DAC credentials to kdbus peers.
>   * kdbus:fakepids
>     Send a different PID to kdbus peers.
>   * kdbus:owner
>     Act as a kdbus bus owner.
>   * kdbus:privileged
>     Act as a privileged endpoint.
>   * kdbus:activator
>     Act as a kdbus activator.
>   * kdbus:monitor
>     Act as a kdbus monitor.
>   * kdbus:policy_holder
>     Act as a kdbus policy holder.
>   * kdbus:connect
>     Create a new kdbus connection.
>   * kdbus:own
>     Own a kdbus service name.
>   * kdbus:talk
>     Talk between two kdbus endpoints.
>   * kdbus:see
>     See another kdbus endpoint.
>   * kdbus:see_name
>     See a kdbus service name.
>   * kdbus:see_notification
>     See a kdbus notification.
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
>
> ---
> ChangeLog:
> - v3
>   * Ported to the 4.3-rc4 based kdbus tree
>   * Fix the missing NULL terminator in the kdbus obj class definition
> - v2
>   * Add the selinux_kdbus_init_inode() hook
>   * Add some very basic info on the permissions to the description
>   * Add kdbus service name auditing in the AVC records
> - v1
>   * Initial draft
> ---
>   security/selinux/hooks.c            |  153 +++++++++++++++++++++++++++++++++++
>   security/selinux/include/classmap.h |    4 +
>   2 files changed, 155 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e4369d8..5581990 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -9,8 +9,10 @@
>    *	      James Morris <jmorris@redhat.com>
>    *
>    *  Copyright (C) 2001,2002 Networks Associates Technology, Inc.
> - *  Copyright (C) 2003-2008 Red Hat, Inc., James Morris <jmorris@redhat.com>
> - *					   Eric Paris <eparis@redhat.com>
> + *  Copyright (C) 2003-2008,2015 Red Hat, Inc.
> + *					James Morris <jmorris@redhat.com>
> + *					Eric Paris <eparis@redhat.com>
> + *					Paul Moore <paul@paul-moore.com>
>    *  Copyright (C) 2004-2005 Trusted Computer Solutions, Inc.
>    *			    <dgoeddel@trustedcs.com>
>    *  Copyright (C) 2006, 2007, 2009 Hewlett-Packard Development Company, L.P.
> @@ -2035,6 +2037,143 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>   			    &ad);
>   }
>
> +static int selinux_kdbus_conn_new(const struct cred *creds,
> +				  const struct kdbus_creds *fake_creds,
> +				  const struct kdbus_pids *fake_pids,
> +				  const char *fake_seclabel,
> +				  bool owner, bool privileged,
> +				  bool is_activator, bool is_monitor,
> +				  bool is_policy_holder)
> +{
> +	int rc;
> +	u32 tsid = current_sid();
> +	u32 av = KDBUS__CONNECT;
> +
> +	if (fake_creds)
> +		av |= KDBUS__FAKECREDS;
> +	if (fake_pids)
> +		av |= KDBUS__FAKEPIDS;
> +	if (owner)
> +		av |= KDBUS__OWNER;
> +	if (privileged)
> +		av |= KDBUS__PRIVILEGED;
> +	if (is_activator)
> +		av |= KDBUS__ACTIVATOR;
> +	if (is_monitor)
> +		av |= KDBUS__MONITOR;
> +	if (is_policy_holder)
> +		av |= KDBUS__POLICY_HOLDER;
> +
> +	rc = avc_has_perm(tsid, cred_sid(creds), SECCLASS_KDBUS, av, NULL);
> +	if (rc)
> +		return rc;
> +
> +	if (fake_seclabel) {
> +		u32 sid;
> +		if (security_context_to_sid(fake_seclabel,
> +					    strlen(fake_seclabel),
> +					    &sid, GFP_KERNEL))
> +			return -EINVAL;
> +
> +		rc = avc_has_perm(tsid, sid,
> +				  SECCLASS_KDBUS, KDBUS__IMPERSONATE, NULL);
> +	}
> +
> +	return rc;
> +}
> +
> +static int selinux_kdbus_own_name(const struct cred *creds, const char *name)
> +{
> +	int rc;
> +	u32 name_sid;
> +	struct common_audit_data ad;
> +
> +	rc = security_kdbus_sid(name, &name_sid);
> +	if (rc)
> +		return rc;
> +
> +	ad.type = LSM_AUDIT_DATA_KDBUS;
> +	ad.u.kdbus_name = name;
> +
> +	return avc_has_perm(cred_sid(creds), name_sid,
> +			    SECCLASS_KDBUS, KDBUS__OWN, &ad);
> +}
> +
> +static int selinux_kdbus_conn_talk(const struct cred *creds,
> +				   const struct cred *creds_to)
> +{
> +	return avc_has_perm(cred_sid(creds), cred_sid(creds_to),
> +			    SECCLASS_KDBUS, KDBUS__TALK, NULL);
> +}
> +
> +static int selinux_kdbus_conn_see(const struct cred *creds,
> +				  const struct cred *creds_whom)
> +{
> +	return avc_has_perm(cred_sid(creds), cred_sid(creds_whom),
> +			    SECCLASS_KDBUS, KDBUS__SEE, NULL);
> +}
> +
> +static int selinux_kdbus_conn_see_name(const struct cred *creds,
> +				       const char *name)
> +{
> +	int rc;
> +	u32 name_sid;
> +	struct common_audit_data ad;
> +
> +	rc = security_kdbus_sid(name, &name_sid);
> +	if (rc)
> +		return rc;
> +
> +	ad.type = LSM_AUDIT_DATA_KDBUS;
> +	ad.u.kdbus_name = name;
> +
> +	return avc_has_perm(cred_sid(creds), name_sid,
> +			    SECCLASS_KDBUS, KDBUS__SEE_NAME, &ad);
> +}
> +
> +static int selinux_kdbus_conn_see_notification(const struct cred *creds)
> +{
> +	return avc_has_perm(SECINITSID_KERNEL, cred_sid(creds),
> +			    SECCLASS_KDBUS, KDBUS__SEE_NOTIFICATION, NULL);
> +}
> +
> +static int selinux_kdbus_proc_permission(const struct cred *creds,
> +					 struct pid *pid)
> +{
> +	int rc;
> +	struct task_struct *task;
> +
> +	rcu_read_lock();
> +	task = pid_task(pid, PIDTYPE_PID);
> +	rc = avc_has_perm(cred_sid(creds), task_sid(task),
> +			  SECCLASS_PROCESS, PROCESS__GETATTR, NULL);
> +	rcu_read_unlock();
> +
> +	return rc;
> +}
> +
> +static int selinux_kdbus_init_inode(struct inode *inode,
> +				    const struct cred *creds)
> +{
> +	struct inode_security_struct *isec = inode->i_security;
> +	u32 sid = cred_sid(creds);
> +
> +	/* XXX - this is very simple, e.g. no transitions, no special object
> +	 *       class, etc. since this inode is basically an IPC socket ...
> +	 *       however, is this too simple?  do we want transitions?  if we
> +	 *       do, we should do the transition in kdbus_node_init() and not
> +	 *       here so that endpoint is labeled correctly and not just this
> +	 *       inode */
> +
> +	isec->inode = inode;
> +	isec->task_sid = sid;
> +	isec->sid = sid;
> +	isec->sclass = SECCLASS_FILE;
> +	isec->initialized = 1;

These are used for files exposed in the filesystem namespace, unlike 
sockets (sockfs can't be mounted by userspace, and the socket objects 
themselves have their own class, so there is no ambiguity).  Currently 
the only such files that are labeled with the same SID as the associated 
task are /proc files.  So if we label the kdbusfs files with the same 
SID, then you can't allow read/write to kdbusfs nodes owned by another 
task without also exposing its /proc/pid files in the same manner. 
Doubt we want that.  Probably should compute a transition from the task 
SID and the kdbusfs SID.

> +
> +	return 0;
> +}
> +

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Paul Moore]

On Friday, October 09, 2015 11:05:58 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > +static int selinux_kdbus_init_inode(struct inode *inode,
> > +				    const struct cred *creds)
> > +{
> > +	struct inode_security_struct *isec = inode->i_security;
> > +	u32 sid = cred_sid(creds);
> > +
> > +	/* XXX - this is very simple, e.g. no transitions, no special object
> > +	 *       class, etc. since this inode is basically an IPC socket ...
> > +	 *       however, is this too simple?  do we want transitions?  if we
> > +	 *       do, we should do the transition in kdbus_node_init() and not
> > +	 *       here so that endpoint is labeled correctly and not just this
> > +	 *       inode */
> > +
> > +	isec->inode = inode;
> > +	isec->task_sid = sid;
> > +	isec->sid = sid;
> > +	isec->sclass = SECCLASS_FILE;
> > +	isec->initialized = 1;
> 
> These are used for files exposed in the filesystem namespace, unlike
> sockets (sockfs can't be mounted by userspace, and the socket objects
> themselves have their own class, so there is no ambiguity).  Currently
> the only such files that are labeled with the same SID as the associated
> task are /proc files.  So if we label the kdbusfs files with the same
> SID, then you can't allow read/write to kdbusfs nodes owned by another
> task without also exposing its /proc/pid files in the same manner.
> Doubt we want that.  Probably should compute a transition from the task
> SID and the kdbusfs SID.

Okay, that was one of my main concerns; your suggestion makes sense to me.

I'm also thinking that is we do a file transition using the task label and the 
kdbusfs superblock label we should limit it to just the inode label and not 
the kdbus endpoint as I suggested in the comment above (the bit about 
kdbus_node_init()), yes?

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Paul Moore]

On Friday, October 09, 2015 11:05:58 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > +static int selinux_kdbus_init_inode(struct inode *inode,
> > +				    const struct cred *creds)
> > +{
> > +	struct inode_security_struct *isec = inode->i_security;
> > +	u32 sid = cred_sid(creds);
> > +
> > +	/* XXX - this is very simple, e.g. no transitions, no special object
> > +	 *       class, etc. since this inode is basically an IPC socket ...
> > +	 *       however, is this too simple?  do we want transitions?  if we
> > +	 *       do, we should do the transition in kdbus_node_init() and not
> > +	 *       here so that endpoint is labeled correctly and not just this
> > +	 *       inode */
> > +
> > +	isec->inode = inode;
> > +	isec->task_sid = sid;
> > +	isec->sid = sid;
> > +	isec->sclass = SECCLASS_FILE;
> > +	isec->initialized = 1;
> 
> These are used for files exposed in the filesystem namespace, unlike
> sockets (sockfs can't be mounted by userspace, and the socket objects
> themselves have their own class, so there is no ambiguity).  Currently
> the only such files that are labeled with the same SID as the associated
> task are /proc files.  So if we label the kdbusfs files with the same
> SID, then you can't allow read/write to kdbusfs nodes owned by another
> task without also exposing its /proc/pid files in the same manner.
> Doubt we want that.  Probably should compute a transition from the task
> SID and the kdbusfs SID.

Okay, that was one of my main concerns; your suggestion makes sense to me.

I'm also thinking that is we do a file transition using the task label and the 
kdbusfs superblock label we should limit it to just the inode label and not 
the kdbus endpoint as I suggested in the comment above (the bit about 
kdbus_node_init()), yes?

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 3/5] lsm: add support for auditing kdbus service names

[Steve Grubb]

On Friday, October 09, 2015 10:57:44 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > The kdbus service names will be recorded using 'service', similar to
> > the existing dbus audit records.
> > 
> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> > 
> > ---
> > ChangeLog:
> > - v3
> > 
> >   * Ported to the 4.3-rc4 based kdbus tree
> > 
> > - v2
> > 
> >   * Initial draft
> > 
> > ---
> > 
> >   include/linux/lsm_audit.h |    2 ++
> >   security/lsm_audit.c      |    4 ++++
> >   2 files changed, 6 insertions(+)
> > 
> > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
> > index ffb9c9d..d6a656f 100644
> > --- a/include/linux/lsm_audit.h
> > +++ b/include/linux/lsm_audit.h
> > @@ -59,6 +59,7 @@ struct common_audit_data {
> > 
> >   #define LSM_AUDIT_DATA_INODE	9
> >   #define LSM_AUDIT_DATA_DENTRY	10
> >   #define LSM_AUDIT_DATA_IOCTL_OP	11
> > 
> > +#define LSM_AUDIT_DATA_KDBUS	12
> > 
> >   	union 	{
> >   	
> >   		struct path path;
> >   		struct dentry *dentry;
> > 
> > @@ -75,6 +76,7 @@ struct common_audit_data {
> > 
> >   #endif
> >   
> >   		char *kmod_name;
> >   		struct lsm_ioctlop_audit *op;
> > 
> > +		const char *kdbus_name;
> > 
> >   	} u;
> >   	/* this union contains LSM specific data */
> >   	union {
> > 
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index cccbf30..0a3dc1b 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -397,6 +397,10 @@ static void dump_common_audit_data(struct
> > audit_buffer *ab,> 
> >   		audit_log_format(ab, " kmod=");
> >   		audit_log_untrustedstring(ab, a->u.kmod_name);
> >   		break;
> > 
> > +	case LSM_AUDIT_DATA_KDBUS:
> > +		audit_log_format(ab, " service=");
> 
> Not a major issue to me, but just wondering if this needs to be further
> qualified to indicate it is a kdbus service.  service= is rather generic.

>From the audit perspective, its fine as service. Too many names that mean the 
same thing causes string lookup tables to get big. Service is what dbus is 
currently using. So, it makes sense to re-use the field name. If the selinux 
tooling wants to know an AVC originated from kdbus activity, then maybe 
another name=value should be added.

-Steve
 
> > +		audit_log_untrustedstring(ab, a->u.kdbus_name);
> > +		break;
> > 
> >   	} /* switch (a->type) */
> >   
> >   }
> > 
> > _______________________________________________
> > Selinux mailing list
> > Selinux@tycho.nsa.gov
> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> > To get help, send an email containing "help" to
> > Selinux-request@tycho.nsa.gov.
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH v3 3/5] lsm: add support for auditing kdbus service names

[Steve Grubb]

On Friday, October 09, 2015 10:57:44 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > The kdbus service names will be recorded using 'service', similar to
> > the existing dbus audit records.
> > 
> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> > 
> > ---
> > ChangeLog:
> > - v3
> > 
> >   * Ported to the 4.3-rc4 based kdbus tree
> > 
> > - v2
> > 
> >   * Initial draft
> > 
> > ---
> > 
> >   include/linux/lsm_audit.h |    2 ++
> >   security/lsm_audit.c      |    4 ++++
> >   2 files changed, 6 insertions(+)
> > 
> > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
> > index ffb9c9d..d6a656f 100644
> > --- a/include/linux/lsm_audit.h
> > +++ b/include/linux/lsm_audit.h
> > @@ -59,6 +59,7 @@ struct common_audit_data {
> > 
> >   #define LSM_AUDIT_DATA_INODE	9
> >   #define LSM_AUDIT_DATA_DENTRY	10
> >   #define LSM_AUDIT_DATA_IOCTL_OP	11
> > 
> > +#define LSM_AUDIT_DATA_KDBUS	12
> > 
> >   	union 	{
> >   	
> >   		struct path path;
> >   		struct dentry *dentry;
> > 
> > @@ -75,6 +76,7 @@ struct common_audit_data {
> > 
> >   #endif
> >   
> >   		char *kmod_name;
> >   		struct lsm_ioctlop_audit *op;
> > 
> > +		const char *kdbus_name;
> > 
> >   	} u;
> >   	/* this union contains LSM specific data */
> >   	union {
> > 
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index cccbf30..0a3dc1b 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -397,6 +397,10 @@ static void dump_common_audit_data(struct
> > audit_buffer *ab,> 
> >   		audit_log_format(ab, " kmod=");
> >   		audit_log_untrustedstring(ab, a->u.kmod_name);
> >   		break;
> > 
> > +	case LSM_AUDIT_DATA_KDBUS:
> > +		audit_log_format(ab, " service=");
> 
> Not a major issue to me, but just wondering if this needs to be further
> qualified to indicate it is a kdbus service.  service= is rather generic.

>From the audit perspective, its fine as service. Too many names that mean the 
same thing causes string lookup tables to get big. Service is what dbus is 
currently using. So, it makes sense to re-use the field name. If the selinux 
tooling wants to know an AVC originated from kdbus activity, then maybe 
another name=value should be added.

-Steve
 
> > +		audit_log_untrustedstring(ab, a->u.kdbus_name);
> > +		break;
> > 
> >   	} /* switch (a->type) */
> >   
> >   }
> > 
> > _______________________________________________
> > Selinux mailing list
> > Selinux@tycho.nsa.gov
> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> > To get help, send an email containing "help" to
> > Selinux-request@tycho.nsa.gov.
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH v3 4/5] selinux: introduce kdbus names into the policy

[Stephen Smalley]

On 10/07/2015 07:08 PM, Paul Moore wrote:
> SELinux treats kdbus service names as objects and therefore needs a
> mechanism to map service names to security labels.  This patch adds
> support for loading kdbus name/label matches with the security policy.
>
> The patch supports service name prefix matching to lessen the burden
> on the policy developers and reduce the size of the resulting policy.
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
>
> ---
> ChangeLog:
> - v3
>   * Ported to the 4.3-rc4 based kdbus tree, v2 hacks removed
> - v2
>   * Porting needed to work with ioctl xperms
> - v1
>   * Initial draft
> ---
>   security/selinux/include/security.h |    5 ++
>   security/selinux/ss/policydb.c      |   88 +++++++++++++++++++++++++++++------
>   security/selinux/ss/policydb.h      |    3 +
>   security/selinux/ss/services.c      |   38 +++++++++++++++
>   4 files changed, 116 insertions(+), 18 deletions(-)
>

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 992a315..9be2e6d 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2111,7 +2116,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
>   	int i, j, rc;
>   	u32 nel, len;
>   	__le32 buf[3];
> -	struct ocontext *l, *c;
> +	struct ocontext *l, *l2, *c;
>   	u32 nodebuf[8];
>
>   	for (i = 0; i < info->ocon_num; i++) {
> @@ -2130,6 +2135,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
>   				l->next = c;
>   			else
>   				p->ocontexts[i] = c;
> +			l2 = l;
>   			l = c;
>
>   			switch (i) {
> @@ -2219,6 +2225,43 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
>   					goto out;
>   				break;
>   			}
> +			case OCON_KDBUS: {
> +				struct ocontext *iter, *last;
> +				u32 len2;
> +
> +				rc = next_entry(buf, fp, sizeof(u32));
> +				if (rc)
> +					goto out;
> +				len = le32_to_cpu(buf[0]);
> +				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
> +				if (rc)
> +					goto out;
> +				rc = context_read_and_validate(&c->context[0], p, fp);
> +				if (rc) {
> +					kfree(c->u.name);
> +					goto out;
> +				}
> +
> +				/* sort by ->u.name length, longest first */
> +				last = NULL;
> +				iter = p->ocontexts[OCON_KDBUS];
> +				while (iter != c) {
> +					len2 = strlen(iter->u.name);
> +					if (len > len2) {
> +						if (l2)
> +							l2->next = NULL;
> +						c->next = iter;
> +						if (last == NULL)
> +							p->ocontexts[i] = c;
> +						else
> +							last->next = c;
> +						break;
> +					}
> +					last = iter;
> +					iter = iter->next;
> +				}
> +				break;

This seems complicated compared to genfs_read() due to the fact that 
ocontext_read() pre-inserts node into the list.  Maybe we should change 
ocontext_read() to defer insertion until after the switch statement, and 
then we don't have to un-link and re-link these entries?

> +			}
>   			}
>   		}
>   	}
> @@ -3147,6 +3190,19 @@ static int ocontext_write(struct policydb *p, struct policydb_compat_info *info,
>   				if (rc)
>   					return rc;
>   				break;
> +			case OCON_KDBUS:
> +				len = strlen(c->u.name);
> +				buf[0] = cpu_to_le32(len);
> +				rc = put_entry(buf, sizeof(u32), 1, fp);
> +				if (rc)
> +					return rc;
> +				rc = put_entry(c->u.name, len, 1, fp);
> +				if (rc)
> +					return rc;
> +				rc = context_write(p, &c->context[0], fp);
> +				if (rc)
> +					return rc;
> +				break;
>   			}
>   		}
>   	}
> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 725d594..ee9c120 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -222,7 +222,8 @@ struct genfs {
>   #define OCON_NODE  4	/* nodes */
>   #define OCON_FSUSE 5	/* fs_use */
>   #define OCON_NODE6 6	/* IPv6 nodes */
> -#define OCON_NUM   7
> +#define OCON_KDBUS 7	/* kdbus names */
> +#define OCON_NUM   8
>
>   /* The policy database */
>   struct policydb {
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index b7df12b..ada2d28 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2536,6 +2536,44 @@ int security_genfs_sid(const char *fstype,
>   }
>
>   /**
> + * security_kdbus_sid - Obtain a SID for a kdbus name
> + * @name: kdbus name
> + * @sid: SID for the kdbus name
> + *
> + * Obtain a SID for the given kdbus service name.  Returns zero on success,
> + * negative values on error.
> + */
> +int security_kdbus_sid(const char *name, u32 *sid)
> +{
> +	int rc = 0;
> +	struct ocontext *c;
> +
> +	read_lock(&policy_rwlock);
> +
> +	c = policydb.ocontexts[OCON_KDBUS];
> +	while (c) {
> +		if (strncmp(c->u.name, name, strlen(c->u.name)) == 0)
> +			break;
> +		c = c->next;
> +	}
> +
> +	if (c) {
> +		if (!c->sid[0]) {
> +			rc = sidtab_context_to_sid(&sidtab,
> +						   &c->context[0], &c->sid[0]);
> +			if (rc)
> +				goto out;
> +		}
> +		*sid = c->sid[0];
> +	} else
> +		*sid = SECINITSID_UNLABELED;
> +
> +out:
> +	read_unlock(&policy_rwlock);
> +	return rc;
> +}
> +
> +/**
>    * security_fs_use - Determine how to handle labeling for a filesystem.
>    * @sb: superblock in question
>    */
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>

Re: [RFC PATCH v3 3/5] lsm: add support for auditing kdbus service names

[Stephen Smalley]

On 10/09/2015 12:25 PM, Steve Grubb wrote:
> On Friday, October 09, 2015 10:57:44 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>>> The kdbus service names will be recorded using 'service', similar to
>>> the existing dbus audit records.
>>>
>>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>>>
>>> ---
>>> ChangeLog:
>>> - v3
>>>
>>>    * Ported to the 4.3-rc4 based kdbus tree
>>>
>>> - v2
>>>
>>>    * Initial draft
>>>
>>> ---
>>>
>>>    include/linux/lsm_audit.h |    2 ++
>>>    security/lsm_audit.c      |    4 ++++
>>>    2 files changed, 6 insertions(+)
>>>
>>> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
>>> index ffb9c9d..d6a656f 100644
>>> --- a/include/linux/lsm_audit.h
>>> +++ b/include/linux/lsm_audit.h
>>> @@ -59,6 +59,7 @@ struct common_audit_data {
>>>
>>>    #define LSM_AUDIT_DATA_INODE	9
>>>    #define LSM_AUDIT_DATA_DENTRY	10
>>>    #define LSM_AUDIT_DATA_IOCTL_OP	11
>>>
>>> +#define LSM_AUDIT_DATA_KDBUS	12
>>>
>>>    	union 	{
>>>    	
>>>    		struct path path;
>>>    		struct dentry *dentry;
>>>
>>> @@ -75,6 +76,7 @@ struct common_audit_data {
>>>
>>>    #endif
>>>
>>>    		char *kmod_name;
>>>    		struct lsm_ioctlop_audit *op;
>>>
>>> +		const char *kdbus_name;
>>>
>>>    	} u;
>>>    	/* this union contains LSM specific data */
>>>    	union {
>>>
>>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>>> index cccbf30..0a3dc1b 100644
>>> --- a/security/lsm_audit.c
>>> +++ b/security/lsm_audit.c
>>> @@ -397,6 +397,10 @@ static void dump_common_audit_data(struct
>>> audit_buffer *ab,>
>>>    		audit_log_format(ab, " kmod=");
>>>    		audit_log_untrustedstring(ab, a->u.kmod_name);
>>>    		break;
>>>
>>> +	case LSM_AUDIT_DATA_KDBUS:
>>> +		audit_log_format(ab, " service=");
>>
>> Not a major issue to me, but just wondering if this needs to be further
>> qualified to indicate it is a kdbus service.  service= is rather generic.
>
>>From the audit perspective, its fine as service. Too many names that mean the
> same thing causes string lookup tables to get big. Service is what dbus is
> currently using. So, it makes sense to re-use the field name. If the selinux
> tooling wants to know an AVC originated from kdbus activity, then maybe
> another name=value should be added.

Ok, never mind then - just leave it as is.

Re: [RFC PATCH v3 3/5] lsm: add support for auditing kdbus service names

[Stephen Smalley]

On 10/09/2015 12:25 PM, Steve Grubb wrote:
> On Friday, October 09, 2015 10:57:44 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>>> The kdbus service names will be recorded using 'service', similar to
>>> the existing dbus audit records.
>>>
>>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>>>
>>> ---
>>> ChangeLog:
>>> - v3
>>>
>>>    * Ported to the 4.3-rc4 based kdbus tree
>>>
>>> - v2
>>>
>>>    * Initial draft
>>>
>>> ---
>>>
>>>    include/linux/lsm_audit.h |    2 ++
>>>    security/lsm_audit.c      |    4 ++++
>>>    2 files changed, 6 insertions(+)
>>>
>>> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
>>> index ffb9c9d..d6a656f 100644
>>> --- a/include/linux/lsm_audit.h
>>> +++ b/include/linux/lsm_audit.h
>>> @@ -59,6 +59,7 @@ struct common_audit_data {
>>>
>>>    #define LSM_AUDIT_DATA_INODE	9
>>>    #define LSM_AUDIT_DATA_DENTRY	10
>>>    #define LSM_AUDIT_DATA_IOCTL_OP	11
>>>
>>> +#define LSM_AUDIT_DATA_KDBUS	12
>>>
>>>    	union 	{
>>>    	
>>>    		struct path path;
>>>    		struct dentry *dentry;
>>>
>>> @@ -75,6 +76,7 @@ struct common_audit_data {
>>>
>>>    #endif
>>>
>>>    		char *kmod_name;
>>>    		struct lsm_ioctlop_audit *op;
>>>
>>> +		const char *kdbus_name;
>>>
>>>    	} u;
>>>    	/* this union contains LSM specific data */
>>>    	union {
>>>
>>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>>> index cccbf30..0a3dc1b 100644
>>> --- a/security/lsm_audit.c
>>> +++ b/security/lsm_audit.c
>>> @@ -397,6 +397,10 @@ static void dump_common_audit_data(struct
>>> audit_buffer *ab,>
>>>    		audit_log_format(ab, " kmod=");
>>>    		audit_log_untrustedstring(ab, a->u.kmod_name);
>>>    		break;
>>>
>>> +	case LSM_AUDIT_DATA_KDBUS:
>>> +		audit_log_format(ab, " service=");
>>
>> Not a major issue to me, but just wondering if this needs to be further
>> qualified to indicate it is a kdbus service.  service= is rather generic.
>
>>From the audit perspective, its fine as service. Too many names that mean the
> same thing causes string lookup tables to get big. Service is what dbus is
> currently using. So, it makes sense to re-use the field name. If the selinux
> tooling wants to know an AVC originated from kdbus activity, then maybe
> another name=value should be added.

Ok, never mind then - just leave it as is.

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Stephen Smalley]

On 10/09/2015 11:39 AM, Paul Moore wrote:
> On Friday, October 09, 2015 11:05:58 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>>> +static int selinux_kdbus_init_inode(struct inode *inode,
>>> +				    const struct cred *creds)
>>> +{
>>> +	struct inode_security_struct *isec = inode->i_security;
>>> +	u32 sid = cred_sid(creds);
>>> +
>>> +	/* XXX - this is very simple, e.g. no transitions, no special object
>>> +	 *       class, etc. since this inode is basically an IPC socket ...
>>> +	 *       however, is this too simple?  do we want transitions?  if we
>>> +	 *       do, we should do the transition in kdbus_node_init() and not
>>> +	 *       here so that endpoint is labeled correctly and not just this
>>> +	 *       inode */
>>> +
>>> +	isec->inode = inode;
>>> +	isec->task_sid = sid;
>>> +	isec->sid = sid;
>>> +	isec->sclass = SECCLASS_FILE;
>>> +	isec->initialized = 1;
>>
>> These are used for files exposed in the filesystem namespace, unlike
>> sockets (sockfs can't be mounted by userspace, and the socket objects
>> themselves have their own class, so there is no ambiguity).  Currently
>> the only such files that are labeled with the same SID as the associated
>> task are /proc files.  So if we label the kdbusfs files with the same
>> SID, then you can't allow read/write to kdbusfs nodes owned by another
>> task without also exposing its /proc/pid files in the same manner.
>> Doubt we want that.  Probably should compute a transition from the task
>> SID and the kdbusfs SID.
>
> Okay, that was one of my main concerns; your suggestion makes sense to me.
>
> I'm also thinking that is we do a file transition using the task label and the
> kdbusfs superblock label we should limit it to just the inode label and not
> the kdbus endpoint as I suggested in the comment above (the bit about
> kdbus_node_init()), yes?

Yes, it only needs to be done for the inode, not the endpoint.
Analogy with sockets:  Can I write to the socket file (kdbus file) bound 
to the socket (endpoint)?  Can I connectto/sendto the socket (endpoint)?

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Stephen Smalley]

On 10/09/2015 11:39 AM, Paul Moore wrote:
> On Friday, October 09, 2015 11:05:58 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>>> +static int selinux_kdbus_init_inode(struct inode *inode,
>>> +				    const struct cred *creds)
>>> +{
>>> +	struct inode_security_struct *isec = inode->i_security;
>>> +	u32 sid = cred_sid(creds);
>>> +
>>> +	/* XXX - this is very simple, e.g. no transitions, no special object
>>> +	 *       class, etc. since this inode is basically an IPC socket ...
>>> +	 *       however, is this too simple?  do we want transitions?  if we
>>> +	 *       do, we should do the transition in kdbus_node_init() and not
>>> +	 *       here so that endpoint is labeled correctly and not just this
>>> +	 *       inode */
>>> +
>>> +	isec->inode = inode;
>>> +	isec->task_sid = sid;
>>> +	isec->sid = sid;
>>> +	isec->sclass = SECCLASS_FILE;
>>> +	isec->initialized = 1;
>>
>> These are used for files exposed in the filesystem namespace, unlike
>> sockets (sockfs can't be mounted by userspace, and the socket objects
>> themselves have their own class, so there is no ambiguity).  Currently
>> the only such files that are labeled with the same SID as the associated
>> task are /proc files.  So if we label the kdbusfs files with the same
>> SID, then you can't allow read/write to kdbusfs nodes owned by another
>> task without also exposing its /proc/pid files in the same manner.
>> Doubt we want that.  Probably should compute a transition from the task
>> SID and the kdbusfs SID.
>
> Okay, that was one of my main concerns; your suggestion makes sense to me.
>
> I'm also thinking that is we do a file transition using the task label and the
> kdbusfs superblock label we should limit it to just the inode label and not
> the kdbus endpoint as I suggested in the comment above (the bit about
> kdbus_node_init()), yes?

Yes, it only needs to be done for the inode, not the endpoint.
Analogy with sockets:  Can I write to the socket file (kdbus file) bound 
to the socket (endpoint)?  Can I connectto/sendto the socket (endpoint)?

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Paul Moore]

On Friday, October 09, 2015 04:17:17 PM Stephen Smalley wrote:
> On 10/09/2015 11:39 AM, Paul Moore wrote:
> > On Friday, October 09, 2015 11:05:58 AM Stephen Smalley wrote:
> >> On 10/07/2015 07:08 PM, Paul Moore wrote:
> >>> +static int selinux_kdbus_init_inode(struct inode *inode,
> >>> +				    const struct cred *creds)
> >>> +{
> >>> +	struct inode_security_struct *isec = inode->i_security;
> >>> +	u32 sid = cred_sid(creds);
> >>> +
> >>> +	/* XXX - this is very simple, e.g. no transitions, no special object
> >>> +	 *       class, etc. since this inode is basically an IPC socket ...
> >>> +	 *       however, is this too simple?  do we want transitions?  if 
we
> >>> +	 *       do, we should do the transition in kdbus_node_init() and 
not
> >>> +	 *       here so that endpoint is labeled correctly and not just 
this
> >>> +	 *       inode */
> >>> +
> >>> +	isec->inode = inode;
> >>> +	isec->task_sid = sid;
> >>> +	isec->sid = sid;
> >>> +	isec->sclass = SECCLASS_FILE;
> >>> +	isec->initialized = 1;
> >> 
> >> These are used for files exposed in the filesystem namespace, unlike
> >> sockets (sockfs can't be mounted by userspace, and the socket objects
> >> themselves have their own class, so there is no ambiguity).  Currently
> >> the only such files that are labeled with the same SID as the associated
> >> task are /proc files.  So if we label the kdbusfs files with the same
> >> SID, then you can't allow read/write to kdbusfs nodes owned by another
> >> task without also exposing its /proc/pid files in the same manner.
> >> Doubt we want that.  Probably should compute a transition from the task
> >> SID and the kdbusfs SID.
> > 
> > Okay, that was one of my main concerns; your suggestion makes sense to me.
> > 
> > I'm also thinking that is we do a file transition using the task label and
> > the kdbusfs superblock label we should limit it to just the inode label
> > and not the kdbus endpoint as I suggested in the comment above (the bit
> > about kdbus_node_init()), yes?
> 
> Yes, it only needs to be done for the inode, not the endpoint.
> Analogy with sockets:  Can I write to the socket file (kdbus file) bound
> to the socket (endpoint)?  Can I connectto/sendto the socket (endpoint)?

Yep.

I'll make these changes and work to get another draft out next week.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 5/5] selinux: introduce kdbus access controls

[Paul Moore]

On Friday, October 09, 2015 04:17:17 PM Stephen Smalley wrote:
> On 10/09/2015 11:39 AM, Paul Moore wrote:
> > On Friday, October 09, 2015 11:05:58 AM Stephen Smalley wrote:
> >> On 10/07/2015 07:08 PM, Paul Moore wrote:
> >>> +static int selinux_kdbus_init_inode(struct inode *inode,
> >>> +				    const struct cred *creds)
> >>> +{
> >>> +	struct inode_security_struct *isec = inode->i_security;
> >>> +	u32 sid = cred_sid(creds);
> >>> +
> >>> +	/* XXX - this is very simple, e.g. no transitions, no special object
> >>> +	 *       class, etc. since this inode is basically an IPC socket ...
> >>> +	 *       however, is this too simple?  do we want transitions?  if 
we
> >>> +	 *       do, we should do the transition in kdbus_node_init() and 
not
> >>> +	 *       here so that endpoint is labeled correctly and not just 
this
> >>> +	 *       inode */
> >>> +
> >>> +	isec->inode = inode;
> >>> +	isec->task_sid = sid;
> >>> +	isec->sid = sid;
> >>> +	isec->sclass = SECCLASS_FILE;
> >>> +	isec->initialized = 1;
> >> 
> >> These are used for files exposed in the filesystem namespace, unlike
> >> sockets (sockfs can't be mounted by userspace, and the socket objects
> >> themselves have their own class, so there is no ambiguity).  Currently
> >> the only such files that are labeled with the same SID as the associated
> >> task are /proc files.  So if we label the kdbusfs files with the same
> >> SID, then you can't allow read/write to kdbusfs nodes owned by another
> >> task without also exposing its /proc/pid files in the same manner.
> >> Doubt we want that.  Probably should compute a transition from the task
> >> SID and the kdbusfs SID.
> > 
> > Okay, that was one of my main concerns; your suggestion makes sense to me.
> > 
> > I'm also thinking that is we do a file transition using the task label and
> > the kdbusfs superblock label we should limit it to just the inode label
> > and not the kdbus endpoint as I suggested in the comment above (the bit
> > about kdbus_node_init()), yes?
> 
> Yes, it only needs to be done for the inode, not the endpoint.
> Analogy with sockets:  Can I write to the socket file (kdbus file) bound
> to the socket (endpoint)?  Can I connectto/sendto the socket (endpoint)?

Yep.

I'll make these changes and work to get another draft out next week.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Paul Moore]

On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index ef63d65..1cb87b3 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
> > kdbus_ep *ep,> 
> >   	if (!owner && (creds || pids || seclabel))
> >   	
> >   		return ERR_PTR(-EPERM);
> > 
> > +	ret = security_kdbus_conn_new(get_cred(file->f_cred),
> 
> You only need to use get_cred() if saving a reference; otherwise, you'll
> leak one here.

Yes, that was a typo on my part, thanks.

> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
> what is used by their own checks; the former is typically the same as
> current cred IIUC).  For that matter, what about ep->node.creds vs ep->bus-
> node.creds vs. ep->bus->domain->node.creds?  Can they differ?  Do we care?

We don't want file->f_cred, per our previous discussions.  I was working on 
this patchset in small chunks and while I added credential storing in the 
nodes, I forgot to update the hooks before I hit send, my apologies.

My current thinking is to pass both the endpoint and bus credentials, as I 
believe they can differ.  Both the bus and the endpoint inherit their security 
labels from their creator and while I don't have any specifics, I think it is 
reasonable to imagine those two processes having different security labels.  
Assuming we pass both credentials down to the LSM, I'm currently thinking of 
the following SELinux access controls for this hook:

  allow <current> bus_t:kdbus { connect };
  allow <current> ep_t:kdbus { use privileged activator monitor policy };

... besides the additional label, I added the kdbus:use permission and dropped 
the kdbus:owner permission.  Considering that the endpoint label, ep_t, in the 
examples above, could be different from the current process, it seemed 
reasonable to want to control that interaction and I felt the fd:use 
permission was the closest existing control so I reused the permission name.  
I decided to drop the "owner" permission as it really wasn't the useful for 
anything anymore, it simply indicates that the current task is the DAC owner 
of the endpoint.

> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn
> > *conn,> 
> >   			return false;
> >   	
> >   	}
> > 
> > -	if (conn->owner)
> > -		return true;
> > +	if (!conn->owner &&
> > +	    kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
> > +			       hash) < KDBUS_POLICY_OWN)
> > +		return false;
> > 
> > -	res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
> > -				 name, hash);
> > -	return res >= KDBUS_POLICY_OWN;
> > +	return (security_kdbus_own_name(conn_creds, name) == 0);
> 
> Similar question here.  conn_creds is the credentials of the creator of
> the connection, typically the client/sender, right?
> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
> we want to ask "Can I own this name on this bus?".

Yes, I think so.

>From a SELinux point of view I imagine we would want access controls along the 
lines of the following:

  allow current name_t:kdbus { own_name };
  allow current bus_t:kdbus { own_name };

... do we want to use different permissions?  I doubt it would matter much 
either way.

> Note that their policy checks are based on conn->ep->policy_db, i.e. the
> policy associated with the endpoint, and conn->owner is only true if the
> connection creator has the same uid as the bus.

I don't think this is significant for us.

> > @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn
> > *conn,> 
> >   					 to, KDBUS_POLICY_TALK))
> >   		
> >   		return false;
> > 
> > -	if (conn->owner)
> > -		return true;
> > -	if (uid_eq(conn_creds->euid, to->cred->uid))
> > -		return true;
> > +	if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
> > +	    !kdbus_conn_policy_query_all(conn, conn_creds,
> > +					 &conn->ep->bus->policy_db, to,
> > +					 KDBUS_POLICY_TALK))
> > +		return false;
> > 
> > -	return kdbus_conn_policy_query_all(conn, conn_creds,
> > -					   &conn->ep->bus->policy_db, to,
> > -					   KDBUS_POLICY_TALK);
> > +	return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
> 
> Here at least we have a notion of client and peer.  But we still aren't
> considering conn->ep or conn->ep->bus, whereas they are querying both
> policy dbs for their decision.  The parallel would be checking access to
> the labels of both I suppose, unless we institute a control up front
> over the relationship between the label of the endpoint and the label of
> the bus.

While accidental, as I forgot to update this patch as mentioned previously, I 
think the differences between kdbus DAC and kdbus MAC is okay.  At this point 
we've already authorized the individual nodes to connect to the bus and now we 
are authorizing them to talk to each other; at this control point, it is the 
interaction between the two nodes that we care about, I don't think we care 
about what bus they use, do we?

> > @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct
> > kdbus_conn *conn,> 
> >   					 const struct cred *conn_creds,
> >   					 const char *name)
> >   
> >   {
> > 
> > -	int res;
> > +	if (!conn_creds)
> > +		conn_creds = conn->cred;
> > 
> >   	/*
> >   	
> >   	 * By default, all names are visible on a bus. SEE policies can only be
> >   	 * installed on custom endpoints, where by default no name is visible.
> >   	 */
> > 
> > -	if (!conn->ep->user)
> > -		return true;
> > +	if (conn->ep->user &&
> > +	    kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
> > +					kdbus_strhash(name)) < KDBUS_POLICY_SEE)
> > +		return false;
> > 
> > -	res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
> > -					  conn_creds ? : conn->cred,
> > -					  name, kdbus_strhash(name));
> > -	return res >= KDBUS_POLICY_SEE;
> > +	return (security_kdbus_conn_see_name(conn_creds, name) == 0);
> 
> Here they only define policy based on endpoints, not bus.  Not sure what
> we want, but we need at least one of their creds.  Same for the rest.

Once again, I'm not sure we care about the underlying bus at this point, do 
we?  We've already authorized both the service and the client to connect to 
the bus, now I think all we care about is can the client see the service name.  
Do we really care about the label of the bus here?

> > @@ -1530,10 +1541,13 @@ static bool kdbus_conn_policy_see(struct
> > kdbus_conn *conn,> 
> >   	 * peers from each other, unless you see at least _one_ name of the
> >   	 * peer.
> >   	 */
> > 
> > -	return !conn->ep->user ||
> > -	       kdbus_conn_policy_query_all(conn, conn_creds,
> > -					   &conn->ep->policy_db, whom,
> > -					   KDBUS_POLICY_SEE);
> > +	if (conn->ep->user &&
> > +	    !kdbus_conn_policy_query_all(conn, conn_creds,
> > +					 &conn->ep->policy_db, whom,
> > +					 KDBUS_POLICY_SEE))
> > +		return false;
> > +
> > +	return (security_kdbus_conn_see(conn_creds, whom->cred) == 0);
> > 
> >   }

Very similar to the kdbus_conn_talk hook above, the credentials above still 
look reasonable to me.

> > @@ -1567,18 +1584,22 @@ bool kdbus_conn_policy_see_notification(struct
> > kdbus_conn *conn,> 
> >   	case KDBUS_ITEM_NAME_ADD:
> >   	case KDBUS_ITEM_NAME_REMOVE:
> > 
> >   	case KDBUS_ITEM_NAME_CHANGE:
> > -		return kdbus_conn_policy_see_name(conn, conn_creds,
> > -					msg->items[0].name_change.name);
> > +		if (!kdbus_conn_policy_see_name(conn, conn_creds,
> > +						msg->items[0].name_change.name))
> > +			return false;
> > 
> >   	case KDBUS_ITEM_ID_ADD:
> > 
> >   	case KDBUS_ITEM_ID_REMOVE:
> > -		return true;
> > +		/* fall through for the LSM check */
> > +		break;
> > 
> >   	default:
> >   		WARN(1, "Invalid type for notification broadcast: %llu\n",
> >   		
> >   		     (unsigned long long)msg->items[0].type);
> >   		
> >   		return false;
> >   	
> >   	}
> > 
> > +
> > +	return (security_kdbus_conn_see_notification(conn_creds) == 0);
> > 
> >   }

This also seems okay.

> > diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
> > index 68818a8..4e84e89 100644
> > --- a/ipc/kdbus/fs.c
> > +++ b/ipc/kdbus/fs.c
> > @@ -200,6 +202,10 @@ static struct inode *fs_inode_get(struct super_block
> > *sb,> 
> >   	if (!(inode->i_state & I_NEW))
> >   	
> >   		return inode;
> > 
> > +	ret = security_kdbus_init_inode(inode, node->creds);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> 
> Need to put the inode.

Thanks.  It looks like I need to make a call to iget_failed() before 
returning.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Paul Moore]

On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index ef63d65..1cb87b3 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
> > kdbus_ep *ep,> 
> >   	if (!owner && (creds || pids || seclabel))
> >   	
> >   		return ERR_PTR(-EPERM);
> > 
> > +	ret = security_kdbus_conn_new(get_cred(file->f_cred),
> 
> You only need to use get_cred() if saving a reference; otherwise, you'll
> leak one here.

Yes, that was a typo on my part, thanks.

> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
> what is used by their own checks; the former is typically the same as
> current cred IIUC).  For that matter, what about ep->node.creds vs ep->bus-
> node.creds vs. ep->bus->domain->node.creds?  Can they differ?  Do we care?

We don't want file->f_cred, per our previous discussions.  I was working on 
this patchset in small chunks and while I added credential storing in the 
nodes, I forgot to update the hooks before I hit send, my apologies.

My current thinking is to pass both the endpoint and bus credentials, as I 
believe they can differ.  Both the bus and the endpoint inherit their security 
labels from their creator and while I don't have any specifics, I think it is 
reasonable to imagine those two processes having different security labels.  
Assuming we pass both credentials down to the LSM, I'm currently thinking of 
the following SELinux access controls for this hook:

  allow <current> bus_t:kdbus { connect };
  allow <current> ep_t:kdbus { use privileged activator monitor policy };

... besides the additional label, I added the kdbus:use permission and dropped 
the kdbus:owner permission.  Considering that the endpoint label, ep_t, in the 
examples above, could be different from the current process, it seemed 
reasonable to want to control that interaction and I felt the fd:use 
permission was the closest existing control so I reused the permission name.  
I decided to drop the "owner" permission as it really wasn't the useful for 
anything anymore, it simply indicates that the current task is the DAC owner 
of the endpoint.

> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn
> > *conn,> 
> >   			return false;
> >   	
> >   	}
> > 
> > -	if (conn->owner)
> > -		return true;
> > +	if (!conn->owner &&
> > +	    kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
> > +			       hash) < KDBUS_POLICY_OWN)
> > +		return false;
> > 
> > -	res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
> > -				 name, hash);
> > -	return res >= KDBUS_POLICY_OWN;
> > +	return (security_kdbus_own_name(conn_creds, name) == 0);
> 
> Similar question here.  conn_creds is the credentials of the creator of
> the connection, typically the client/sender, right?
> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
> we want to ask "Can I own this name on this bus?".

Yes, I think so.

>From a SELinux point of view I imagine we would want access controls along the 
lines of the following:

  allow current name_t:kdbus { own_name };
  allow current bus_t:kdbus { own_name };

... do we want to use different permissions?  I doubt it would matter much 
either way.

> Note that their policy checks are based on conn->ep->policy_db, i.e. the
> policy associated with the endpoint, and conn->owner is only true if the
> connection creator has the same uid as the bus.

I don't think this is significant for us.

> > @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn
> > *conn,> 
> >   					 to, KDBUS_POLICY_TALK))
> >   		
> >   		return false;
> > 
> > -	if (conn->owner)
> > -		return true;
> > -	if (uid_eq(conn_creds->euid, to->cred->uid))
> > -		return true;
> > +	if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
> > +	    !kdbus_conn_policy_query_all(conn, conn_creds,
> > +					 &conn->ep->bus->policy_db, to,
> > +					 KDBUS_POLICY_TALK))
> > +		return false;
> > 
> > -	return kdbus_conn_policy_query_all(conn, conn_creds,
> > -					   &conn->ep->bus->policy_db, to,
> > -					   KDBUS_POLICY_TALK);
> > +	return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
> 
> Here at least we have a notion of client and peer.  But we still aren't
> considering conn->ep or conn->ep->bus, whereas they are querying both
> policy dbs for their decision.  The parallel would be checking access to
> the labels of both I suppose, unless we institute a control up front
> over the relationship between the label of the endpoint and the label of
> the bus.

While accidental, as I forgot to update this patch as mentioned previously, I 
think the differences between kdbus DAC and kdbus MAC is okay.  At this point 
we've already authorized the individual nodes to connect to the bus and now we 
are authorizing them to talk to each other; at this control point, it is the 
interaction between the two nodes that we care about, I don't think we care 
about what bus they use, do we?

> > @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct
> > kdbus_conn *conn,> 
> >   					 const struct cred *conn_creds,
> >   					 const char *name)
> >   
> >   {
> > 
> > -	int res;
> > +	if (!conn_creds)
> > +		conn_creds = conn->cred;
> > 
> >   	/*
> >   	
> >   	 * By default, all names are visible on a bus. SEE policies can only be
> >   	 * installed on custom endpoints, where by default no name is visible.
> >   	 */
> > 
> > -	if (!conn->ep->user)
> > -		return true;
> > +	if (conn->ep->user &&
> > +	    kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
> > +					kdbus_strhash(name)) < KDBUS_POLICY_SEE)
> > +		return false;
> > 
> > -	res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
> > -					  conn_creds ? : conn->cred,
> > -					  name, kdbus_strhash(name));
> > -	return res >= KDBUS_POLICY_SEE;
> > +	return (security_kdbus_conn_see_name(conn_creds, name) == 0);
> 
> Here they only define policy based on endpoints, not bus.  Not sure what
> we want, but we need at least one of their creds.  Same for the rest.

Once again, I'm not sure we care about the underlying bus at this point, do 
we?  We've already authorized both the service and the client to connect to 
the bus, now I think all we care about is can the client see the service name.  
Do we really care about the label of the bus here?

> > @@ -1530,10 +1541,13 @@ static bool kdbus_conn_policy_see(struct
> > kdbus_conn *conn,> 
> >   	 * peers from each other, unless you see at least _one_ name of the
> >   	 * peer.
> >   	 */
> > 
> > -	return !conn->ep->user ||
> > -	       kdbus_conn_policy_query_all(conn, conn_creds,
> > -					   &conn->ep->policy_db, whom,
> > -					   KDBUS_POLICY_SEE);
> > +	if (conn->ep->user &&
> > +	    !kdbus_conn_policy_query_all(conn, conn_creds,
> > +					 &conn->ep->policy_db, whom,
> > +					 KDBUS_POLICY_SEE))
> > +		return false;
> > +
> > +	return (security_kdbus_conn_see(conn_creds, whom->cred) == 0);
> > 
> >   }

Very similar to the kdbus_conn_talk hook above, the credentials above still 
look reasonable to me.

> > @@ -1567,18 +1584,22 @@ bool kdbus_conn_policy_see_notification(struct
> > kdbus_conn *conn,> 
> >   	case KDBUS_ITEM_NAME_ADD:
> >   	case KDBUS_ITEM_NAME_REMOVE:
> > 
> >   	case KDBUS_ITEM_NAME_CHANGE:
> > -		return kdbus_conn_policy_see_name(conn, conn_creds,
> > -					msg->items[0].name_change.name);
> > +		if (!kdbus_conn_policy_see_name(conn, conn_creds,
> > +						msg->items[0].name_change.name))
> > +			return false;
> > 
> >   	case KDBUS_ITEM_ID_ADD:
> > 
> >   	case KDBUS_ITEM_ID_REMOVE:
> > -		return true;
> > +		/* fall through for the LSM check */
> > +		break;
> > 
> >   	default:
> >   		WARN(1, "Invalid type for notification broadcast: %llu\n",
> >   		
> >   		     (unsigned long long)msg->items[0].type);
> >   		
> >   		return false;
> >   	
> >   	}
> > 
> > +
> > +	return (security_kdbus_conn_see_notification(conn_creds) == 0);
> > 
> >   }

This also seems okay.

> > diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
> > index 68818a8..4e84e89 100644
> > --- a/ipc/kdbus/fs.c
> > +++ b/ipc/kdbus/fs.c
> > @@ -200,6 +202,10 @@ static struct inode *fs_inode_get(struct super_block
> > *sb,> 
> >   	if (!(inode->i_state & I_NEW))
> >   	
> >   		return inode;
> > 
> > +	ret = security_kdbus_init_inode(inode, node->creds);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> 
> Need to put the inode.

Thanks.  It looks like I need to make a call to iget_failed() before 
returning.

-- 
paul moore
security @ redhat

Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Stephen Smalley]

On Mon, Oct 19, 2015 at 6:29 PM, Paul Moore <pmoore@redhat.com> wrote:
> On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
>> > index ef63d65..1cb87b3 100644
>> > --- a/ipc/kdbus/connection.c
>> > +++ b/ipc/kdbus/connection.c
>> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
>> > kdbus_ep *ep,>
>> >     if (!owner && (creds || pids || seclabel))
>> >
>> >             return ERR_PTR(-EPERM);
>> >
>> > +   ret = security_kdbus_conn_new(get_cred(file->f_cred),
>>
>> You only need to use get_cred() if saving a reference; otherwise, you'll
>> leak one here.
>
> Yes, that was a typo on my part, thanks.
>
>> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
>> what is used by their own checks; the former is typically the same as
>> current cred IIUC).  For that matter, what about ep->node.creds vs ep->bus-
>> node.creds vs. ep->bus->domain->node.creds?  Can they differ?  Do we care?
>
> We don't want file->f_cred, per our previous discussions.  I was working on
> this patchset in small chunks and while I added credential storing in the
> nodes, I forgot to update the hooks before I hit send, my apologies.
>
> My current thinking is to pass both the endpoint and bus credentials, as I
> believe they can differ.  Both the bus and the endpoint inherit their security
> labels from their creator and while I don't have any specifics, I think it is
> reasonable to imagine those two processes having different security labels.
> Assuming we pass both credentials down to the LSM, I'm currently thinking of
> the following SELinux access controls for this hook:
>
>   allow <current> bus_t:kdbus { connect };
>   allow <current> ep_t:kdbus { use privileged activator monitor policy };

I think it would be simpler to apply an associate check when the
endpoint is created between the endpoint label and the bus label
(which will typically be the same), and then only check based on
endpoint label for all subsequent permission checks involving that
endpoint.  Then you don't have to worry about which label to use for
all the other permission checks. And you get finer-grained control -
per-endpoint rather than only per-bus.  In the common case, the labels
will be the same and it won't matter, but if you want to support some
MAC form of their 'custom endpoints' with further restrictions, you
can do that by labeling the endpoints uniquely and using those labels
in your permission checks.

Otherwise, I have to ask whether you need two different classes above
(bus vs endpoint), and whether the privileged/activator/monitor/policy
permissions properly belong with the endpoint label or the bus label.
At least some of those are bus-wide properties, not endpoint-specific,
but that's ok as long as we have established a relationship between
the endpoint label and bus label.

> ... besides the additional label, I added the kdbus:use permission and dropped
> the kdbus:owner permission.  Considering that the endpoint label, ep_t, in the
> examples above, could be different from the current process, it seemed
> reasonable to want to control that interaction and I felt the fd:use
> permission was the closest existing control so I reused the permission name.
> I decided to drop the "owner" permission as it really wasn't the useful for
> anything anymore, it simply indicates that the current task is the DAC owner
> of the endpoint.

Can you 'use' an endpoint in any way other than to connect via it?
If not, I'd just call that connect (won't conflict if you get rid of
the separate bus check above), or distinguish it via separate classes
or as connectthrough vs connectto.

conn->owner is used to determine whether the caller can fake
credentials, skip kdbus policy checking, create an activator, monitor,
or policy holder connection, etc.  Our options are:
1. Apply a SELinux check when it is set to see if the caller is
allowed to own the bus based on MAC labels and policy, and if not,
refuse to create the connection (that's what checking the owner
permission was doing).

2. Separately apply MAC checks over each of those abilities (fake
creds, override policy, create an activator, monitor, or policy
holder, etc) when there is an attempt to exercise them (not all during
connection creation), and selectively deny that ability.  More
invasive, more potential for breakage for applications that don't
expect failure if they could create the connection in the first place.

3. Treat faking of DAC credentials and skipping of kdbus policy
checking as not of interest to MAC, leaving it only controlled by the
existing uid match or CAP_IPC_OWNER check.  Simple, but seems to lose
some of the potential benefit of using SELinux for confining processes
using kdbus.

privileged actually seems less interesting than owner these days,
although I haven't done a thorough analysis.

IIUC, creating a monitor or policy holder connection has bus-wide
implications and we ought to control it so we can limit it to specific
processes, not just all uid-0 processes for the system bus and not
just all uid-<user> processes for the per-user bus.  I'm not actually
clear on the implications of activator connections or why they are
limited in the same way.

>
>> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn
>> > *conn,>
>> >                     return false;
>> >
>> >     }
>> >
>> > -   if (conn->owner)
>> > -           return true;
>> > +   if (!conn->owner &&
>> > +       kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
>> > +                          hash) < KDBUS_POLICY_OWN)
>> > +           return false;
>> >
>> > -   res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
>> > -                            name, hash);
>> > -   return res >= KDBUS_POLICY_OWN;
>> > +   return (security_kdbus_own_name(conn_creds, name) == 0);
>>
>> Similar question here.  conn_creds is the credentials of the creator of
>> the connection, typically the client/sender, right?
>> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
>> we want to ask "Can I own this name on this bus?".
>
> Yes, I think so.
>
> From a SELinux point of view I imagine we would want access controls along the
> lines of the following:
>
>   allow current name_t:kdbus { own_name };
>   allow current bus_t:kdbus { own_name };
>
> ... do we want to use different permissions?  I doubt it would matter much
> either way.

If we keep them both, then they need separate classes or separate
permissions; I can't think of another case where we reuse the same
class and permission for two different objects.

Not clear that this kind of pairwise checking will work well.  For
example, I should be able to own any name I like on my own bus, but
not on the system bus.

>> Note that their policy checks are based on conn->ep->policy_db, i.e. the
>> policy associated with the endpoint, and conn->owner is only true if the
>> connection creator has the same uid as the bus.
>
> I don't think this is significant for us.
>
>> > @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn
>> > *conn,>
>> >                                      to, KDBUS_POLICY_TALK))
>> >
>> >             return false;
>> >
>> > -   if (conn->owner)
>> > -           return true;
>> > -   if (uid_eq(conn_creds->euid, to->cred->uid))
>> > -           return true;
>> > +   if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
>> > +       !kdbus_conn_policy_query_all(conn, conn_creds,
>> > +                                    &conn->ep->bus->policy_db, to,
>> > +                                    KDBUS_POLICY_TALK))
>> > +           return false;
>> >
>> > -   return kdbus_conn_policy_query_all(conn, conn_creds,
>> > -                                      &conn->ep->bus->policy_db, to,
>> > -                                      KDBUS_POLICY_TALK);
>> > +   return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
>>
>> Here at least we have a notion of client and peer.  But we still aren't
>> considering conn->ep or conn->ep->bus, whereas they are querying both
>> policy dbs for their decision.  The parallel would be checking access to
>> the labels of both I suppose, unless we institute a control up front
>> over the relationship between the label of the endpoint and the label of
>> the bus.
>
> While accidental, as I forgot to update this patch as mentioned previously, I
> think the differences between kdbus DAC and kdbus MAC is okay.  At this point
> we've already authorized the individual nodes to connect to the bus and now we
> are authorizing them to talk to each other; at this control point, it is the
> interaction between the two nodes that we care about, I don't think we care
> about what bus they use, do we?

Perhaps not.

>
>> > @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct
>> > kdbus_conn *conn,>
>> >                                      const struct cred *conn_creds,
>> >                                      const char *name)
>> >
>> >   {
>> >
>> > -   int res;
>> > +   if (!conn_creds)
>> > +           conn_creds = conn->cred;
>> >
>> >     /*
>> >
>> >      * By default, all names are visible on a bus. SEE policies can only be
>> >      * installed on custom endpoints, where by default no name is visible.
>> >      */
>> >
>> > -   if (!conn->ep->user)
>> > -           return true;
>> > +   if (conn->ep->user &&
>> > +       kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
>> > +                                   kdbus_strhash(name)) < KDBUS_POLICY_SEE)
>> > +           return false;
>> >
>> > -   res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
>> > -                                     conn_creds ? : conn->cred,
>> > -                                     name, kdbus_strhash(name));
>> > -   return res >= KDBUS_POLICY_SEE;
>> > +   return (security_kdbus_conn_see_name(conn_creds, name) == 0);
>>
>> Here they only define policy based on endpoints, not bus.  Not sure what
>> we want, but we need at least one of their creds.  Same for the rest.
>
> Once again, I'm not sure we care about the underlying bus at this point, do
> we?  We've already authorized both the service and the client to connect to
> the bus, now I think all we care about is can the client see the service name.
> Do we really care about the label of the bus here?

Well, here we again have the issue that we should be able to see all
names on our own bus, but not necessarily on the system bus.

Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Stephen Smalley]

On Mon, Oct 19, 2015 at 6:29 PM, Paul Moore <pmoore@redhat.com> wrote:
> On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
>> > index ef63d65..1cb87b3 100644
>> > --- a/ipc/kdbus/connection.c
>> > +++ b/ipc/kdbus/connection.c
>> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
>> > kdbus_ep *ep,>
>> >     if (!owner && (creds || pids || seclabel))
>> >
>> >             return ERR_PTR(-EPERM);
>> >
>> > +   ret = security_kdbus_conn_new(get_cred(file->f_cred),
>>
>> You only need to use get_cred() if saving a reference; otherwise, you'll
>> leak one here.
>
> Yes, that was a typo on my part, thanks.
>
>> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
>> what is used by their own checks; the former is typically the same as
>> current cred IIUC).  For that matter, what about ep->node.creds vs ep->bus-
>> node.creds vs. ep->bus->domain->node.creds?  Can they differ?  Do we care?
>
> We don't want file->f_cred, per our previous discussions.  I was working on
> this patchset in small chunks and while I added credential storing in the
> nodes, I forgot to update the hooks before I hit send, my apologies.
>
> My current thinking is to pass both the endpoint and bus credentials, as I
> believe they can differ.  Both the bus and the endpoint inherit their security
> labels from their creator and while I don't have any specifics, I think it is
> reasonable to imagine those two processes having different security labels.
> Assuming we pass both credentials down to the LSM, I'm currently thinking of
> the following SELinux access controls for this hook:
>
>   allow <current> bus_t:kdbus { connect };
>   allow <current> ep_t:kdbus { use privileged activator monitor policy };

I think it would be simpler to apply an associate check when the
endpoint is created between the endpoint label and the bus label
(which will typically be the same), and then only check based on
endpoint label for all subsequent permission checks involving that
endpoint.  Then you don't have to worry about which label to use for
all the other permission checks. And you get finer-grained control -
per-endpoint rather than only per-bus.  In the common case, the labels
will be the same and it won't matter, but if you want to support some
MAC form of their 'custom endpoints' with further restrictions, you
can do that by labeling the endpoints uniquely and using those labels
in your permission checks.

Otherwise, I have to ask whether you need two different classes above
(bus vs endpoint), and whether the privileged/activator/monitor/policy
permissions properly belong with the endpoint label or the bus label.
At least some of those are bus-wide properties, not endpoint-specific,
but that's ok as long as we have established a relationship between
the endpoint label and bus label.

> ... besides the additional label, I added the kdbus:use permission and dropped
> the kdbus:owner permission.  Considering that the endpoint label, ep_t, in the
> examples above, could be different from the current process, it seemed
> reasonable to want to control that interaction and I felt the fd:use
> permission was the closest existing control so I reused the permission name.
> I decided to drop the "owner" permission as it really wasn't the useful for
> anything anymore, it simply indicates that the current task is the DAC owner
> of the endpoint.

Can you 'use' an endpoint in any way other than to connect via it?
If not, I'd just call that connect (won't conflict if you get rid of
the separate bus check above), or distinguish it via separate classes
or as connectthrough vs connectto.

conn->owner is used to determine whether the caller can fake
credentials, skip kdbus policy checking, create an activator, monitor,
or policy holder connection, etc.  Our options are:
1. Apply a SELinux check when it is set to see if the caller is
allowed to own the bus based on MAC labels and policy, and if not,
refuse to create the connection (that's what checking the owner
permission was doing).

2. Separately apply MAC checks over each of those abilities (fake
creds, override policy, create an activator, monitor, or policy
holder, etc) when there is an attempt to exercise them (not all during
connection creation), and selectively deny that ability.  More
invasive, more potential for breakage for applications that don't
expect failure if they could create the connection in the first place.

3. Treat faking of DAC credentials and skipping of kdbus policy
checking as not of interest to MAC, leaving it only controlled by the
existing uid match or CAP_IPC_OWNER check.  Simple, but seems to lose
some of the potential benefit of using SELinux for confining processes
using kdbus.

privileged actually seems less interesting than owner these days,
although I haven't done a thorough analysis.

IIUC, creating a monitor or policy holder connection has bus-wide
implications and we ought to control it so we can limit it to specific
processes, not just all uid-0 processes for the system bus and not
just all uid-<user> processes for the per-user bus.  I'm not actually
clear on the implications of activator connections or why they are
limited in the same way.

>
>> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn
>> > *conn,>
>> >                     return false;
>> >
>> >     }
>> >
>> > -   if (conn->owner)
>> > -           return true;
>> > +   if (!conn->owner &&
>> > +       kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
>> > +                          hash) < KDBUS_POLICY_OWN)
>> > +           return false;
>> >
>> > -   res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
>> > -                            name, hash);
>> > -   return res >= KDBUS_POLICY_OWN;
>> > +   return (security_kdbus_own_name(conn_creds, name) == 0);
>>
>> Similar question here.  conn_creds is the credentials of the creator of
>> the connection, typically the client/sender, right?
>> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
>> we want to ask "Can I own this name on this bus?".
>
> Yes, I think so.
>
> From a SELinux point of view I imagine we would want access controls along the
> lines of the following:
>
>   allow current name_t:kdbus { own_name };
>   allow current bus_t:kdbus { own_name };
>
> ... do we want to use different permissions?  I doubt it would matter much
> either way.

If we keep them both, then they need separate classes or separate
permissions; I can't think of another case where we reuse the same
class and permission for two different objects.

Not clear that this kind of pairwise checking will work well.  For
example, I should be able to own any name I like on my own bus, but
not on the system bus.

>> Note that their policy checks are based on conn->ep->policy_db, i.e. the
>> policy associated with the endpoint, and conn->owner is only true if the
>> connection creator has the same uid as the bus.
>
> I don't think this is significant for us.
>
>> > @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn
>> > *conn,>
>> >                                      to, KDBUS_POLICY_TALK))
>> >
>> >             return false;
>> >
>> > -   if (conn->owner)
>> > -           return true;
>> > -   if (uid_eq(conn_creds->euid, to->cred->uid))
>> > -           return true;
>> > +   if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
>> > +       !kdbus_conn_policy_query_all(conn, conn_creds,
>> > +                                    &conn->ep->bus->policy_db, to,
>> > +                                    KDBUS_POLICY_TALK))
>> > +           return false;
>> >
>> > -   return kdbus_conn_policy_query_all(conn, conn_creds,
>> > -                                      &conn->ep->bus->policy_db, to,
>> > -                                      KDBUS_POLICY_TALK);
>> > +   return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
>>
>> Here at least we have a notion of client and peer.  But we still aren't
>> considering conn->ep or conn->ep->bus, whereas they are querying both
>> policy dbs for their decision.  The parallel would be checking access to
>> the labels of both I suppose, unless we institute a control up front
>> over the relationship between the label of the endpoint and the label of
>> the bus.
>
> While accidental, as I forgot to update this patch as mentioned previously, I
> think the differences between kdbus DAC and kdbus MAC is okay.  At this point
> we've already authorized the individual nodes to connect to the bus and now we
> are authorizing them to talk to each other; at this control point, it is the
> interaction between the two nodes that we care about, I don't think we care
> about what bus they use, do we?

Perhaps not.

>
>> > @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct
>> > kdbus_conn *conn,>
>> >                                      const struct cred *conn_creds,
>> >                                      const char *name)
>> >
>> >   {
>> >
>> > -   int res;
>> > +   if (!conn_creds)
>> > +           conn_creds = conn->cred;
>> >
>> >     /*
>> >
>> >      * By default, all names are visible on a bus. SEE policies can only be
>> >      * installed on custom endpoints, where by default no name is visible.
>> >      */
>> >
>> > -   if (!conn->ep->user)
>> > -           return true;
>> > +   if (conn->ep->user &&
>> > +       kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
>> > +                                   kdbus_strhash(name)) < KDBUS_POLICY_SEE)
>> > +           return false;
>> >
>> > -   res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
>> > -                                     conn_creds ? : conn->cred,
>> > -                                     name, kdbus_strhash(name));
>> > -   return res >= KDBUS_POLICY_SEE;
>> > +   return (security_kdbus_conn_see_name(conn_creds, name) == 0);
>>
>> Here they only define policy based on endpoints, not bus.  Not sure what
>> we want, but we need at least one of their creds.  Same for the rest.
>
> Once again, I'm not sure we care about the underlying bus at this point, do
> we?  We've already authorized both the service and the client to connect to
> the bus, now I think all we care about is can the client see the service name.
> Do we really care about the label of the bus here?

Well, here we again have the issue that we should be able to see all
names on our own bus, but not necessarily on the system bus.

Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Paul Moore]

On Tuesday, October 20, 2015 04:41:14 PM Stephen Smalley wrote:
> On Mon, Oct 19, 2015 at 6:29 PM, Paul Moore <pmoore@redhat.com> wrote:
> > On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
> >> On 10/07/2015 07:08 PM, Paul Moore wrote:
> >> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> >> > index ef63d65..1cb87b3 100644
> >> > --- a/ipc/kdbus/connection.c
> >> > +++ b/ipc/kdbus/connection.c
> >> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
> >> > kdbus_ep *ep,>
> >> > 
> >> >     if (!owner && (creds || pids || seclabel))
> >> >     
> >> >             return ERR_PTR(-EPERM);
> >> > 
> >> > +   ret = security_kdbus_conn_new(get_cred(file->f_cred),
> >> 
> >> You only need to use get_cred() if saving a reference; otherwise, you'll
> >> leak one here.
> > 
> > Yes, that was a typo on my part, thanks.
> > 
> >> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
> >> what is used by their own checks; the former is typically the same as
> >> current cred IIUC).  For that matter, what about ep->node.creds vs
> >> ep->bus-
> >> node.creds vs. ep->bus->domain->node.creds?  Can they differ?  Do we
> >> care?
> > 
> > We don't want file->f_cred, per our previous discussions.  I was working
> > on this patchset in small chunks and while I added credential storing in
> > the nodes, I forgot to update the hooks before I hit send, my apologies.
> > 
> > My current thinking is to pass both the endpoint and bus credentials, as I
> > believe they can differ.  Both the bus and the endpoint inherit their
> > security labels from their creator and while I don't have any specifics,
> > I think it is reasonable to imagine those two processes having different
> > security labels. Assuming we pass both credentials down to the LSM, I'm
> > currently thinking of> 
> > the following SELinux access controls for this hook:
> >   allow <current> bus_t:kdbus { connect };
> >   allow <current> ep_t:kdbus { use privileged activator monitor policy };
> 
> I think it would be simpler to apply an associate check when the
> endpoint is created between the endpoint label and the bus label
> (which will typically be the same), and then only check based on
> endpoint label for all subsequent permission checks involving that
> endpoint.  Then you don't have to worry about which label to use for
> all the other permission checks. And you get finer-grained control -
> per-endpoint rather than only per-bus.

After thinking about this for a bit, I agree.

> > ... besides the additional label, I added the kdbus:use permission and
> > dropped the kdbus:owner permission.  Considering that the endpoint label,
> > ep_t, in the examples above, could be different from the current process,
> > it seemed reasonable to want to control that interaction and I felt the
> > fd:use permission was the closest existing control so I reused the
> > permission name. I decided to drop the "owner" permission as it really
> > wasn't the useful for anything anymore, it simply indicates that the
> > current task is the DAC owner of the endpoint.
> 
> Can you 'use' an endpoint in any way other than to connect via it?
> If not, I'd just call that connect (won't conflict if you get rid of
> the separate bus check above), or distinguish it via separate classes
> or as connectthrough vs connectto.

I don't believe so; my understanding is that the main point of endpoints is to 
define special kdbus DAC policy.
 
> conn->owner is used to determine whether the caller can fake
> credentials, skip kdbus policy checking, create an activator, monitor,
> or policy holder connection, etc.  Our options are:
>
> 1. Apply a SELinux check when it is set to see if the caller is
> allowed to own the bus based on MAC labels and policy, and if not,
> refuse to create the connection (that's what checking the owner
> permission was doing).
> 
> 2. Separately apply MAC checks over each of those abilities (fake
> creds, override policy, create an activator, monitor, or policy
> holder, etc) when there is an attempt to exercise them (not all during
> connection creation), and selectively deny that ability.  More
> invasive, more potential for breakage for applications that don't
> expect failure if they could create the connection in the first place.
> 
> 3. Treat faking of DAC credentials and skipping of kdbus policy
> checking as not of interest to MAC, leaving it only controlled by the
> existing uid match or CAP_IPC_OWNER check.  Simple, but seems to lose
> some of the potential benefit of using SELinux for confining processes
> using kdbus.

I agree with you on #3 so let's rule that out.  The same with #2 as I fear we 
would likely get a nasty surprise at some point when the kdbus folks changed 
something and forgot to update the LSM hooks.  That leaves us with option #1 
and the following operations:

* fake credentials
We are already checking this with the kdbus:impersonate permission.

* skip kdbus policy checks
Being an "owner" allows you to skip the kdbus DAC checks, but the operation 
should still be subject to the LSM hooks.  This really is an issue regardless 
of what we do with respect to checking ownership.

* create an activator
This is still passed as a flag into the security_kdbus_conn_new() hook.

* create a monitor
This is still passed as a flag into the security_kdbus_conn_new() hook.

* create a policy holder
This is still passed as a flag into the security_kdbus_conn_new() hook.

For the sake of completeness we can still keep passing it down via the LSM 
hook, I'm just not sure that adding it to the SELinux policy provides any real 
benefit.

> privileged actually seems less interesting than owner these days,
> although I haven't done a thorough analysis.

The only thing that it appears to offer is the ability to set the connection's 
creating user to a specified value.  From what I'm able to tell, this seems 
more relevant to the DAC side of things than the MAC side, although if we feel 
that "owner" is significant then privileged will remain important as well.

> IIUC, creating a monitor or policy holder connection has bus-wide
> implications and we ought to control it so we can limit it to specific
> processes, not just all uid-0 processes for the system bus and not
> just all uid-<user> processes for the per-user bus.

Agreed.  We still pass those flags to the LSM.

> I'm not actually clear on the implications of activator connections or why
> they are limited in the same way.

My understanding is that they serve as some sort of placeholder for kdbus 
services.  Regardless, we continue to pass the flag through the LSM hook.

> >> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct
> >> > kdbus_conn
> >> > *conn,>
> >> > 
> >> >                     return false;
> >> >     
> >> >     }
> >> > 
> >> > -   if (conn->owner)
> >> > -           return true;
> >> > +   if (!conn->owner &&
> >> > +       kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
> >> > +                          hash) < KDBUS_POLICY_OWN)
> >> > +           return false;
> >> > 
> >> > -   res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
> >> > -                            name, hash);
> >> > -   return res >= KDBUS_POLICY_OWN;
> >> > +   return (security_kdbus_own_name(conn_creds, name) == 0);
> >> 
> >> Similar question here.  conn_creds is the credentials of the creator of
> >> the connection, typically the client/sender, right?
> >> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
> >> we want to ask "Can I own this name on this bus?".
> > 
> > Yes, I think so.
> > 
> > From a SELinux point of view I imagine we would want access controls along
> > the lines of the following:
> >   allow current name_t:kdbus { own_name };
> >   allow current bus_t:kdbus { own_name };
> > 
> > ... do we want to use different permissions?  I doubt it would matter much
> > either way.
> 
> If we keep them both, then they need separate classes or separate
> permissions; I can't think of another case where we reuse the same
> class and permission for two different objects.
> 
> Not clear that this kind of pairwise checking will work well.  For
> example, I should be able to own any name I like on my own bus, but
> not on the system bus.

If we care both about the service and the bus on which it lives, I think we 
need to somehow factor both into the SELinux bus/service name mapping. 

Perhaps we transition the service names based on the bus; for example, it is 
really on the system bus services that we are likely to care about having 
special/non-default names, isn't it?  Service names on a user bus could simply 
take on the same label as the process.

If we don't think the transitions are the right solution, then I think we'll 
need to add the bus information to the service name/label mapping.  Perhaps 
<bus_label>/<service_name> to <service_label>?  Although my current thinking 
is that the transitions are the way to go.

> >> > @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct
> >> > kdbus_conn *conn,>
> >> > 
> >> >                                      const struct cred *conn_creds,
> >> >                                      const char *name)
> >> >   
> >> >   {
> >> > 
> >> > -   int res;
> >> > +   if (!conn_creds)
> >> > +           conn_creds = conn->cred;
> >> > 
> >> >     /*
> >> >     
> >> >      * By default, all names are visible on a bus. SEE policies can
> >> >      only be
> >> >      * installed on custom endpoints, where by default no name is
> >> >      visible.
> >> >      */
> >> > 
> >> > -   if (!conn->ep->user)
> >> > -           return true;
> >> > +   if (conn->ep->user &&
> >> > +       kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds,
> >> > name,
> >> > +                                   kdbus_strhash(name)) <
> >> > KDBUS_POLICY_SEE) +           return false;
> >> > 
> >> > -   res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
> >> > -                                     conn_creds ? : conn->cred,
> >> > -                                     name, kdbus_strhash(name));
> >> > -   return res >= KDBUS_POLICY_SEE;
> >> > +   return (security_kdbus_conn_see_name(conn_creds, name) == 0);
> >> 
> >> Here they only define policy based on endpoints, not bus.  Not sure what
> >> we want, but we need at least one of their creds.  Same for the rest.
> > 
> > Once again, I'm not sure we care about the underlying bus at this point,
> > do we?  We've already authorized both the service and the client to
> > connect to the bus, now I think all we care about is can the client see
> > the service name. Do we really care about the label of the bus here?
> 
> Well, here we again have the issue that we should be able to see all
> names on our own bus, but not necessarily on the system bus.

See my comment above, but I think we need to map both the bus and service name 
to a service label.

-- 
paul moore
security @ redhat