From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Smalley Subject: Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus Date: Fri, 9 Oct 2015 10:56:12 -0400 Message-ID: <5617D58C.2070402@tycho.nsa.gov> References: <20151007230615.7823.74519.stgit@localhost> <20151007230829.7823.14385.stgit@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151007230829.7823.14385.stgit@localhost> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore , linux-security-module@vger.kernel.org, linux-audit@redhat.com, selinux@tycho.nsa.gov Cc: Paul Osmialowski List-Id: linux-audit@redhat.com 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 > > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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 > #include > #include > +#include > #include > > #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;