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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <559D27AB.4010402@tycho.nsa.gov>
Date:	Wed, 08 Jul 2015 09:37:47 -0400
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	Paul Osmialowski <p.osmialowsk@...sung.com>,
	Paul Moore <pmoore@...hat.com>,
	James Morris <james.l.morris@...cle.com>,
	Casey Schaufler <casey@...aufler-ca.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	Kees Cook <keescook@...omium.org>,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	Neil Brown <neilb@...e.de>,
	Mark Rustad <mark.d.rustad@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Mack <daniel@...que.org>,
	David Herrmann <dh.herrmann@...glemail.com>,
	Djalal Harouni <tixxdz@...ndz.org>,
	Shuah Khan <shuahkh@....samsung.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org
CC:	Karol Lewandowski <k.lewandowsk@...sung.com>,
	Lukasz Skalski <l.skalski@...sung.com>
Subject: Re: [RFC 5/8] kdbus: use LSM hooks in kdbus code

On 07/08/2015 06:25 AM, Paul Osmialowski wrote:
> Originates from:
> 
> https://github.com/lmctl/kdbus.git (branch: kdbus-lsm-v4.for-systemd-v212)
> commit: aa0885489d19be92fa41c6f0a71df28763228a40
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk@...sung.com>
> Signed-off-by: Paul Osmialowski <p.osmialowsk@...sung.com>
> ---
>  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(-)
> 
>

> 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?

> @@ -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?

> diff --git a/ipc/kdbus/connection.h b/ipc/kdbus/connection.h
> index d1ffe90..1f91d39 100644
> --- a/ipc/kdbus/connection.h
> +++ b/ipc/kdbus/connection.h
> @@ -19,6 +19,7 @@
>  #include <linux/kref.h>
>  #include <linux/lockdep.h>
>  #include <linux/path.h>
> +#include <uapi/linux/kdbus.h>
>  
>  #include "limits.h"
>  #include "metadata.h"
> @@ -73,6 +74,7 @@ struct kdbus_kmsg;
>   * @names_queue_list:	Well-known names this connection waits for
>   * @privileged:		Whether this connection is privileged on the bus
>   * @faked_meta:		Whether the metadata was faked on HELLO
> + * @security:		LSM security blob
>   */
>  struct kdbus_conn {
>  	struct kref kref;
> @@ -113,6 +115,8 @@ struct kdbus_conn {
>  
>  	bool privileged:1;
>  	bool faked_meta:1;
> +
> +	void *security;
>  };

Unless I missed it, you may have missed the most important thing of all:
 controlling kdbus's notion of "privileged".  kdbus sets privileged to
true if the process has CAP_IPC_OWNER or the process euid matches the
uid of the bus creator, and then it allows those processes to do many
dangerous things, including monitoring all traffic, impersonating
credentials, pids, or seclabel, etc.

I don't believe we should ever permit impersonating seclabel information.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/