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

[Stephen Smalley <sds-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Paul Osmialowski <p.osmialowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  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.