Re: [RFC 5/8] kdbus: use LSM hooks in kdbus code

On 07/08/2015 09:37 AM, Stephen Smalley wrote:

On 07/08/2015 06:25 AM, Paul Osmialowski wrote:

quoted

Originates from:

https://github.com/lmctl/kdbus.git (branch: kdbus-lsm-v4.for-systemd-v212)
commit: aa0885489d19be92fa41c6f0a71df28763228a40

Signed-off-by: Karol Lewandowski <redacted>
Signed-off-by: Paul Osmialowski <redacted>
---
 ipc/kdbus/bus.c        | 12 ++++++++++-
 ipc/kdbus/bus.h        |  3 +++
 ipc/kdbus/connection.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 ipc/kdbus/connection.h |  4 ++++
 ipc/kdbus/domain.c     |  9 ++++++++-
 ipc/kdbus/domain.h     |  2 ++
 ipc/kdbus/endpoint.c   | 11 ++++++++++
 ipc/kdbus/names.c      | 11 ++++++++++
 ipc/kdbus/queue.c      | 30 ++++++++++++++++++----------
 9 files changed, 124 insertions(+), 12 deletions(-)

quoted

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 9993753..b85cdc7 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c

@@ -31,6 +31,7 @@
 #include <linux/slab.h>
 #include <linux/syscalls.h>
 #include <linux/uio.h>
+#include <linux/security.h>
 
 #include "bus.h"
 #include "connection.h"

@@ -73,6 +74,8 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged,
 	bool is_activator;
 	bool is_monitor;
 	struct kvec kvec;
+	u32 sid, len;
+	char *label;
 	int ret;
 
 	struct {

@@ -222,6 +225,14 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged,
 		}
 	}
 
+	security_task_getsecid(current, &sid);
+	security_secid_to_secctx(sid, &label, &len);
+	ret = security_kdbus_connect(conn, label, len);
+	if (ret) {
+		ret = -EPERM;
+		goto exit_unref;
+	}

This seems convoluted and expensive.  If you always want the label of
the current task here, then why not just have security_kdbus_connect()
internally extract the label of the current task?

quoted

@@ -1107,6 +1119,12 @@ static int kdbus_conn_reply(struct kdbus_conn *src, struct kdbus_kmsg *kmsg)
 	if (ret < 0)
 		goto exit;
 
+	ret = security_kdbus_talk(src, dst);
+	if (ret) {
+		ret = -EPERM;
+		goto exit;
+	}

Where does kdbus apply its uid-based or other restrictions on
connections?  Why do we need to insert separate hooks into each of these
functions?  Is there no central chokepoint already for permission
checking that we can hook?

For example, why wouldn't you insert a single hook into
kdbus_conn_policy_talk() where they perform their DAC checking?
You would need to restructure it slightly to ensure that the security
hook is only called if it passes the DAC (privileged || uid_eq) check so
that we do not trigger MAC denials when DAC wouldn't have allowed it
anyway.  Also, kdbus_conn_policy_talk() takes a separate conn_creds
argument - that should be passed through to the hook as well.