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: <559FF920.2020302@tycho.nsa.gov>
Date:	Fri, 10 Jul 2015 12:56:00 -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 09:37 AM, Stephen Smalley wrote:
> 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?

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.


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