[RFC PATCH v3 2/5] lsm: introduce hooks for kdbus
Stephen Smalley
sds at tycho.nsa.gov
Fri Oct 9 14:56:12 UTC 2015
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 at 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;
More information about the Linux-audit
mailing list