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

[Stephen Smalley <sds@tycho.nsa.gov>]

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;