LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Add security hooks to binder and implement the hooks for SELinux.
@ 2015-01-21 15:54 Stephen Smalley
  2015-01-22  8:51 ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2015-01-21 15:54 UTC (permalink / raw)
  To: linux-kernel
  Cc: gregkh, arve, nnk, paul, selinux, linux-security-module, jmorris,
	Stephen Smalley

Add security hooks to the binder and implement the hooks for SELinux.
The security hooks enable security modules such as SELinux to implement
controls over binder IPC.  The security hooks include support for
controlling what process can become the binder context manager
(binder_set_context_mgr), controlling the ability of a process
to invoke a binder transaction/IPC to another process (binder_transaction),
controlling the ability of a process to transfer a binder reference to
another process (binder_transfer_binder), and controlling the ability
of a process to transfer an open file to another process (binder_transfer_file).

These hooks have been included in the Android kernel trees since Android 4.3.

(Updated to reflect upstream relocation and changes to the binder driver,
changes to the LSM audit data structures, coding style cleanups, and
to add inline documentation for the hooks).

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 drivers/android/binder.c            | 26 +++++++++++++
 include/linux/security.h            | 58 +++++++++++++++++++++++++++++
 security/capability.c               | 27 ++++++++++++++
 security/security.c                 | 23 ++++++++++++
 security/selinux/hooks.c            | 73 +++++++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h |  2 +
 6 files changed, 209 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 8c43521..33b09b6 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -37,6 +37,7 @@
 #include <linux/vmalloc.h>
 #include <linux/slab.h>
 #include <linux/pid_namespace.h>
+#include <linux/security.h>
 
 #ifdef CONFIG_ANDROID_BINDER_IPC_32BIT
 #define BINDER_IPC_32BIT 1
@@ -1400,6 +1401,11 @@ static void binder_transaction(struct binder_proc *proc,
 			return_error = BR_DEAD_REPLY;
 			goto err_dead_binder;
 		}
+		if (security_binder_transaction(proc->tsk,
+						target_proc->tsk) < 0) {
+			return_error = BR_FAILED_REPLY;
+			goto err_invalid_target_handle;
+		}
 		if (!(tr->flags & TF_ONE_WAY) && thread->transaction_stack) {
 			struct binder_transaction *tmp;
 
@@ -1551,6 +1557,11 @@ static void binder_transaction(struct binder_proc *proc,
 				return_error = BR_FAILED_REPLY;
 				goto err_binder_get_ref_for_node_failed;
 			}
+			if (security_binder_transfer_binder(proc->tsk,
+							    target_proc->tsk)) {
+				return_error = BR_FAILED_REPLY;
+				goto err_binder_get_ref_for_node_failed;
+			}
 			ref = binder_get_ref_for_node(target_proc, node);
 			if (ref == NULL) {
 				return_error = BR_FAILED_REPLY;
@@ -1581,6 +1592,11 @@ static void binder_transaction(struct binder_proc *proc,
 				return_error = BR_FAILED_REPLY;
 				goto err_binder_get_ref_failed;
 			}
+			if (security_binder_transfer_binder(proc->tsk,
+							    target_proc->tsk)) {
+				return_error = BR_FAILED_REPLY;
+				goto err_binder_get_ref_failed;
+			}
 			if (ref->node->proc == target_proc) {
 				if (fp->type == BINDER_TYPE_HANDLE)
 					fp->type = BINDER_TYPE_BINDER;
@@ -1638,6 +1654,13 @@ static void binder_transaction(struct binder_proc *proc,
 				return_error = BR_FAILED_REPLY;
 				goto err_fget_failed;
 			}
+			if (security_binder_transfer_file(proc->tsk,
+							  target_proc->tsk,
+							  file) < 0) {
+				fput(file);
+				return_error = BR_FAILED_REPLY;
+				goto err_get_unused_fd_failed;
+			}
 			target_fd = task_get_unused_fd_flags(target_proc, O_CLOEXEC);
 			if (target_fd < 0) {
 				fput(file);
@@ -2675,6 +2698,9 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp)
 		ret = -EBUSY;
 		goto out;
 	}
+	ret = security_binder_set_context_mgr(proc->tsk);
+	if (ret < 0)
+		goto out;
 	if (uid_valid(binder_context_mgr_uid)) {
 		if (!uid_eq(binder_context_mgr_uid, curr_euid)) {
 			pr_err("BINDER_SET_CONTEXT_MGR bad uid %d != %d\n",
diff --git a/include/linux/security.h b/include/linux/security.h
index ba96471..a1b7dbd 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1281,6 +1281,25 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@alter contains the flag indicating whether changes are to be made.
  *	Return 0 if permission is granted.
  *
+ * @binder_set_context_mgr
+ *	Check whether @mgr is allowed to be the binder context manager.
+ *	@mgr contains the task_struct for the task being registered.
+ *	Return 0 if permission is granted.
+ * @binder_transaction
+ *	Check whether @from is allowed to invoke a binder transaction call
+ *	to @to.
+ *	@from contains the task_struct for the sending task.
+ *	@to contains the task_struct for the receiving task.
+ * @binder_transfer_binder
+ *	Check whether @from is allowed to transfer a binder reference to @to.
+ *	@from contains the task_struct for the sending task.
+ *	@to contains the task_struct for the receiving task.
+ * @binder_transfer_file
+ *	Check whether @from is allowed to transfer @file to @to.
+ *	@from contains the task_struct for the sending task.
+ *	@file contains the struct file being transferred.
+ *	@to contains the task_struct for the receiving task.
+ *
  * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
@@ -1441,6 +1460,14 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
+	int (*binder_set_context_mgr) (struct task_struct *mgr);
+	int (*binder_transaction) (struct task_struct *from,
+				   struct task_struct *to);
+	int (*binder_transfer_binder) (struct task_struct *from,
+				       struct task_struct *to);
+	int (*binder_transfer_file) (struct task_struct *from,
+				     struct task_struct *to, struct file *file);
+
 	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
 	int (*ptrace_traceme) (struct task_struct *parent);
 	int (*capget) (struct task_struct *target,
@@ -1739,6 +1766,13 @@ extern void __init security_fixup_ops(struct security_operations *ops);
 
 
 /* Security operations */
+int security_binder_set_context_mgr(struct task_struct *mgr);
+int security_binder_transaction(struct task_struct *from,
+				struct task_struct *to);
+int security_binder_transfer_binder(struct task_struct *from,
+				    struct task_struct *to);
+int security_binder_transfer_file(struct task_struct *from,
+				  struct task_struct *to, struct file *file);
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
@@ -1927,6 +1961,30 @@ static inline int security_init(void)
 	return 0;
 }
 
+static inline int security_binder_set_context_mgr(struct task_struct *mgr)
+{
+	return 0;
+}
+
+static inline int security_binder_transaction(struct task_struct *from,
+					      struct task_struct *to)
+{
+	return 0;
+}
+
+static inline int security_binder_transfer_binder(struct task_struct *from,
+						  struct task_struct *to)
+{
+	return 0;
+}
+
+static inline int security_binder_transfer_file(struct task_struct *from,
+						struct task_struct *to,
+						struct file *file)
+{
+	return 0;
+}
+
 static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
diff --git a/security/capability.c b/security/capability.c
index d68c57a..070dd46 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -12,6 +12,29 @@
 
 #include <linux/security.h>
 
+static int cap_binder_set_context_mgr(struct task_struct *mgr)
+{
+	return 0;
+}
+
+static int cap_binder_transaction(struct task_struct *from,
+				  struct task_struct *to)
+{
+	return 0;
+}
+
+static int cap_binder_transfer_binder(struct task_struct *from,
+				      struct task_struct *to)
+{
+	return 0;
+}
+
+static int cap_binder_transfer_file(struct task_struct *from,
+				    struct task_struct *to, struct file *file)
+{
+	return 0;
+}
+
 static int cap_syslog(int type)
 {
 	return 0;
@@ -930,6 +953,10 @@ static void cap_audit_rule_free(void *lsmrule)
 
 void __init security_fixup_ops(struct security_operations *ops)
 {
+	set_to_cap_if_null(ops, binder_set_context_mgr);
+	set_to_cap_if_null(ops, binder_transaction);
+	set_to_cap_if_null(ops, binder_transfer_binder);
+	set_to_cap_if_null(ops, binder_transfer_file);
 	set_to_cap_if_null(ops, ptrace_access_check);
 	set_to_cap_if_null(ops, ptrace_traceme);
 	set_to_cap_if_null(ops, capget);
diff --git a/security/security.c b/security/security.c
index 18b35c6..b196de3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -135,6 +135,29 @@ int __init register_security(struct security_operations *ops)
 
 /* Security operations */
 
+int security_binder_set_context_mgr(struct task_struct *mgr)
+{
+	return security_ops->binder_set_context_mgr(mgr);
+}
+
+int security_binder_transaction(struct task_struct *from,
+				struct task_struct *to)
+{
+	return security_ops->binder_transaction(from, to);
+}
+
+int security_binder_transfer_binder(struct task_struct *from,
+				    struct task_struct *to)
+{
+	return security_ops->binder_transfer_binder(from, to);
+}
+
+int security_binder_transfer_file(struct task_struct *from,
+				  struct task_struct *to, struct file *file)
+{
+	return security_ops->binder_transfer_file(from, to, file);
+}
+
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 #ifdef CONFIG_SECURITY_YAMA_STACKED
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6da7532..9d984bf 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1933,6 +1933,74 @@ static inline u32 open_file_to_av(struct file *file)
 
 /* Hook functions begin here. */
 
+static int selinux_binder_set_context_mgr(struct task_struct *mgr)
+{
+	u32 mysid = current_sid();
+	u32 mgrsid = task_sid(mgr);
+
+	return avc_has_perm(mysid, mgrsid, SECCLASS_BINDER,
+			    BINDER__SET_CONTEXT_MGR, NULL);
+}
+
+static int selinux_binder_transaction(struct task_struct *from,
+				      struct task_struct *to)
+{
+	u32 mysid = current_sid();
+	u32 fromsid = task_sid(from);
+	u32 tosid = task_sid(to);
+	int rc;
+
+	if (mysid != fromsid) {
+		rc = avc_has_perm(mysid, fromsid, SECCLASS_BINDER,
+				  BINDER__IMPERSONATE, NULL);
+		if (rc)
+			return rc;
+	}
+
+	return avc_has_perm(fromsid, tosid, SECCLASS_BINDER, BINDER__CALL,
+			    NULL);
+}
+
+static int selinux_binder_transfer_binder(struct task_struct *from,
+					  struct task_struct *to)
+{
+	u32 fromsid = task_sid(from);
+	u32 tosid = task_sid(to);
+
+	return avc_has_perm(fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
+			    NULL);
+}
+
+static int selinux_binder_transfer_file(struct task_struct *from,
+					struct task_struct *to,
+					struct file *file)
+{
+	u32 sid = task_sid(to);
+	struct file_security_struct *fsec = file->f_security;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	struct inode_security_struct *isec = inode->i_security;
+	struct common_audit_data ad;
+	int rc;
+
+	ad.type = LSM_AUDIT_DATA_PATH;
+	ad.u.path = file->f_path;
+
+	if (sid != fsec->sid) {
+		rc = avc_has_perm(sid, fsec->sid,
+				  SECCLASS_FD,
+				  FD__USE,
+				  &ad);
+		if (rc)
+			return rc;
+	}
+
+	if (unlikely(IS_PRIVATE(inode)))
+		return 0;
+
+	return avc_has_perm(sid, isec->sid, isec->sclass, file_to_av(file),
+			    &ad);
+}
+
 static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
@@ -5810,6 +5878,11 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 static struct security_operations selinux_ops = {
 	.name =				"selinux",
 
+	.binder_set_context_mgr =	selinux_binder_set_context_mgr,
+	.binder_transaction =		selinux_binder_transaction,
+	.binder_transfer_binder =	selinux_binder_transfer_binder,
+	.binder_transfer_file =		selinux_binder_transfer_file,
+
 	.ptrace_access_check =		selinux_ptrace_access_check,
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index be491a7..eccd61b 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -151,5 +151,7 @@ struct security_class_mapping secclass_map[] = {
 	{ "kernel_service", { "use_as_override", "create_files_as", NULL } },
 	{ "tun_socket",
 	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },
+	{ "binder", { "impersonate", "call", "set_context_mgr", "transfer",
+		      NULL } },
 	{ NULL }
   };
-- 
1.9.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-21 15:54 [PATCH] Add security hooks to binder and implement the hooks for SELinux Stephen Smalley
@ 2015-01-22  8:51 ` Greg KH
  2015-01-22 17:37   ` Jeffrey Vander Stoep
                     ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Greg KH @ 2015-01-22  8:51 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: linux-kernel, arve, nnk, paul, selinux, linux-security-module, jmorris

On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
> Add security hooks to the binder and implement the hooks for SELinux.
> The security hooks enable security modules such as SELinux to implement
> controls over binder IPC.  The security hooks include support for
> controlling what process can become the binder context manager
> (binder_set_context_mgr), controlling the ability of a process
> to invoke a binder transaction/IPC to another process (binder_transaction),
> controlling the ability of a process to transfer a binder reference to
> another process (binder_transfer_binder), and controlling the ability
> of a process to transfer an open file to another process (binder_transfer_file).
> 
> These hooks have been included in the Android kernel trees since Android 4.3.

Very interesting, I missed the fact that these were added in that tree,
thanks for digging it out and submitting it.

I'd like some acks from some Android developers before I take these.
Or, if it's easier for them to go through the security tree, that's fine
with me as well.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-22  8:51 ` Greg KH
@ 2015-01-22 17:37   ` Jeffrey Vander Stoep
  2015-01-22 17:48   ` Nick Kralevich
  2015-01-22 18:09   ` Casey Schaufler
  2 siblings, 0 replies; 9+ messages in thread
From: Jeffrey Vander Stoep @ 2015-01-22 17:37 UTC (permalink / raw)
  To: Greg KH
  Cc: Stephen Smalley, linux-security-module, linux-kernel, arve, SELinux

ACK.

This has been in the android tree since Nov 2012.

Forward port of commit: 6639e3d91a05bafa2a85c24c211c43fcaa1b17c5
in https://android.googlesource.com/kernel/common.git

Apologies for the double send. Forgot to disable formatting.

Thanks,
Jeff

On Thu, Jan 22, 2015 at 12:51 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
>> Add security hooks to the binder and implement the hooks for SELinux.
>> The security hooks enable security modules such as SELinux to implement
>> controls over binder IPC.  The security hooks include support for
>> controlling what process can become the binder context manager
>> (binder_set_context_mgr), controlling the ability of a process
>> to invoke a binder transaction/IPC to another process (binder_transaction),
>> controlling the ability of a process to transfer a binder reference to
>> another process (binder_transfer_binder), and controlling the ability
>> of a process to transfer an open file to another process (binder_transfer_file).
>>
>> These hooks have been included in the Android kernel trees since Android 4.3.
>
> Very interesting, I missed the fact that these were added in that tree,
> thanks for digging it out and submitting it.
>
> I'd like some acks from some Android developers before I take these.
> Or, if it's easier for them to go through the security tree, that's fine
> with me as well.
>
> thanks,
>
> greg k-h
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-22  8:51 ` Greg KH
  2015-01-22 17:37   ` Jeffrey Vander Stoep
@ 2015-01-22 17:48   ` Nick Kralevich
  2015-01-22 18:09   ` Casey Schaufler
  2 siblings, 0 replies; 9+ messages in thread
From: Nick Kralevich @ 2015-01-22 17:48 UTC (permalink / raw)
  To: Greg KH
  Cc: Stephen Smalley, lkml, Arve Hjønnevåg, paul, SELinux,
	linux-security-module, jmorris

Acked-By: Nick Kralevich <nnk@google.com>

On Thu, Jan 22, 2015 at 12:51 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
>> Add security hooks to the binder and implement the hooks for SELinux.
>> The security hooks enable security modules such as SELinux to implement
>> controls over binder IPC.  The security hooks include support for
>> controlling what process can become the binder context manager
>> (binder_set_context_mgr), controlling the ability of a process
>> to invoke a binder transaction/IPC to another process (binder_transaction),
>> controlling the ability of a process to transfer a binder reference to
>> another process (binder_transfer_binder), and controlling the ability
>> of a process to transfer an open file to another process (binder_transfer_file).
>>
>> These hooks have been included in the Android kernel trees since Android 4.3.
>
> Very interesting, I missed the fact that these were added in that tree,
> thanks for digging it out and submitting it.
>
> I'd like some acks from some Android developers before I take these.
> Or, if it's easier for them to go through the security tree, that's fine
> with me as well.
>
> thanks,
>
> greg k-h



-- 
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-22  8:51 ` Greg KH
  2015-01-22 17:37   ` Jeffrey Vander Stoep
  2015-01-22 17:48   ` Nick Kralevich
@ 2015-01-22 18:09   ` Casey Schaufler
  2015-01-22 18:47     ` Stephen Smalley
  2 siblings, 1 reply; 9+ messages in thread
From: Casey Schaufler @ 2015-01-22 18:09 UTC (permalink / raw)
  To: Greg KH, Stephen Smalley
  Cc: linux-kernel, arve, nnk, paul, selinux, linux-security-module,
	jmorris, Casey Schaufler

On 1/22/2015 12:51 AM, Greg KH wrote:
> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
>> Add security hooks to the binder and implement the hooks for SELinux.
>> The security hooks enable security modules such as SELinux to implement
>> controls over binder IPC.  The security hooks include support for
>> controlling what process can become the binder context manager
>> (binder_set_context_mgr), controlling the ability of a process
>> to invoke a binder transaction/IPC to another process (binder_transaction),
>> controlling the ability of a process to transfer a binder reference to
>> another process (binder_transfer_binder), and controlling the ability
>> of a process to transfer an open file to another process (binder_transfer_file).
>>
>> These hooks have been included in the Android kernel trees since Android 4.3.
> Very interesting, I missed the fact that these were added in that tree,
> thanks for digging it out and submitting it.
>
> I'd like some acks from some Android developers before I take these.
> Or, if it's easier for them to go through the security tree, that's fine
> with me as well.

My only concern is that we're about to see a set of hooks proposed
for kdbus as well, and it would be a shame if we had two sets of hooks
that do roughly the same thing (ok, *very roughly*) introduced back to back.

>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-22 18:09   ` Casey Schaufler
@ 2015-01-22 18:47     ` Stephen Smalley
  2015-01-23  2:30       ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2015-01-22 18:47 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Greg KH, Stephen Smalley, Linux Kernel, arve, Nick Kralevich,
	Paul Moore, selinux, linux-security-module, James Morris

On Thu, Jan 22, 2015 at 1:09 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/22/2015 12:51 AM, Greg KH wrote:
>> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
>>> Add security hooks to the binder and implement the hooks for SELinux.
>>> The security hooks enable security modules such as SELinux to implement
>>> controls over binder IPC.  The security hooks include support for
>>> controlling what process can become the binder context manager
>>> (binder_set_context_mgr), controlling the ability of a process
>>> to invoke a binder transaction/IPC to another process (binder_transaction),
>>> controlling the ability of a process to transfer a binder reference to
>>> another process (binder_transfer_binder), and controlling the ability
>>> of a process to transfer an open file to another process (binder_transfer_file).
>>>
>>> These hooks have been included in the Android kernel trees since Android 4.3.
>> Very interesting, I missed the fact that these were added in that tree,
>> thanks for digging it out and submitting it.
>>
>> I'd like some acks from some Android developers before I take these.
>> Or, if it's easier for them to go through the security tree, that's fine
>> with me as well.
>
> My only concern is that we're about to see a set of hooks proposed
> for kdbus as well, and it would be a shame if we had two sets of hooks
> that do roughly the same thing (ok, *very roughly*) introduced back to back.

Not sure how much commonality there truly is among them (based on the
last set of proposed kdbus lsm hooks that I saw, admittedly a while
ago) and modules may want to distinguish between the two
forms of IPC regardless.  The binder hooks have been in place in the
Android kernel trees for quite some time, so this patch is just making
the mainline binder driver consistent with what is already in Android.
If it turns out that there is significant duplication when the kdbus
lsm hooks land, I'd be happy to help coalesce them.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-22 18:47     ` Stephen Smalley
@ 2015-01-23  2:30       ` Greg KH
  2015-01-23 21:56         ` Casey Schaufler
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2015-01-23  2:30 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Casey Schaufler, Stephen Smalley, Linux Kernel, arve,
	Nick Kralevich, Paul Moore, selinux, linux-security-module,
	James Morris

On Thu, Jan 22, 2015 at 01:47:29PM -0500, Stephen Smalley wrote:
> On Thu, Jan 22, 2015 at 1:09 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 1/22/2015 12:51 AM, Greg KH wrote:
> >> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
> >>> Add security hooks to the binder and implement the hooks for SELinux.
> >>> The security hooks enable security modules such as SELinux to implement
> >>> controls over binder IPC.  The security hooks include support for
> >>> controlling what process can become the binder context manager
> >>> (binder_set_context_mgr), controlling the ability of a process
> >>> to invoke a binder transaction/IPC to another process (binder_transaction),
> >>> controlling the ability of a process to transfer a binder reference to
> >>> another process (binder_transfer_binder), and controlling the ability
> >>> of a process to transfer an open file to another process (binder_transfer_file).
> >>>
> >>> These hooks have been included in the Android kernel trees since Android 4.3.
> >> Very interesting, I missed the fact that these were added in that tree,
> >> thanks for digging it out and submitting it.
> >>
> >> I'd like some acks from some Android developers before I take these.
> >> Or, if it's easier for them to go through the security tree, that's fine
> >> with me as well.
> >
> > My only concern is that we're about to see a set of hooks proposed
> > for kdbus as well, and it would be a shame if we had two sets of hooks
> > that do roughly the same thing (ok, *very roughly*) introduced back to back.
> 
> Not sure how much commonality there truly is among them (based on the
> last set of proposed kdbus lsm hooks that I saw, admittedly a while
> ago) and modules may want to distinguish between the two
> forms of IPC regardless.  The binder hooks have been in place in the
> Android kernel trees for quite some time, so this patch is just making
> the mainline binder driver consistent with what is already in Android.
> If it turns out that there is significant duplication when the kdbus
> lsm hooks land, I'd be happy to help coalesce them.

Yeah, I would wait and see what happens with how the kdbus hooks look
before worrying about this all that much.  As people are using the
binder hooks today, and binder is in the kernel tree, but kdbus isn't,
let's not worry about kdbus until that is merged.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-23  2:30       ` Greg KH
@ 2015-01-23 21:56         ` Casey Schaufler
  2015-01-23 23:45           ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Casey Schaufler @ 2015-01-23 21:56 UTC (permalink / raw)
  To: Greg KH, Stephen Smalley
  Cc: Stephen Smalley, Linux Kernel, arve, Nick Kralevich, Paul Moore,
	selinux, linux-security-module, James Morris, Casey Schaufler

On 1/22/2015 6:30 PM, Greg KH wrote:
> On Thu, Jan 22, 2015 at 01:47:29PM -0500, Stephen Smalley wrote:
>> On Thu, Jan 22, 2015 at 1:09 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 1/22/2015 12:51 AM, Greg KH wrote:
>>>> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
>>>>> Add security hooks to the binder and implement the hooks for SELinux.
>>>>> The security hooks enable security modules such as SELinux to implement
>>>>> controls over binder IPC.  The security hooks include support for
>>>>> controlling what process can become the binder context manager
>>>>> (binder_set_context_mgr), controlling the ability of a process
>>>>> to invoke a binder transaction/IPC to another process (binder_transaction),
>>>>> controlling the ability of a process to transfer a binder reference to
>>>>> another process (binder_transfer_binder), and controlling the ability
>>>>> of a process to transfer an open file to another process (binder_transfer_file).
>>>>>
>>>>> These hooks have been included in the Android kernel trees since Android 4.3.
>>>> Very interesting, I missed the fact that these were added in that tree,
>>>> thanks for digging it out and submitting it.
>>>>
>>>> I'd like some acks from some Android developers before I take these.
>>>> Or, if it's easier for them to go through the security tree, that's fine
>>>> with me as well.
>>> My only concern is that we're about to see a set of hooks proposed
>>> for kdbus as well, and it would be a shame if we had two sets of hooks
>>> that do roughly the same thing (ok, *very roughly*) introduced back to back.
>> Not sure how much commonality there truly is among them (based on the
>> last set of proposed kdbus lsm hooks that I saw, admittedly a while
>> ago) and modules may want to distinguish between the two
>> forms of IPC regardless.  The binder hooks have been in place in the
>> Android kernel trees for quite some time, so this patch is just making
>> the mainline binder driver consistent with what is already in Android.
>> If it turns out that there is significant duplication when the kdbus
>> lsm hooks land, I'd be happy to help coalesce them.
> Yeah, I would wait and see what happens with how the kdbus hooks look
> before worrying about this all that much.  As people are using the
> binder hooks today, and binder is in the kernel tree, but kdbus isn't,
> let's not worry about kdbus until that is merged.

That seems like a sane plan to me in light of the situation. I am somewhat
concerned about the growth in special case security behavior. The tun hooks,
now the binder hooks and later the kdbus hooks. It looks like we're going
in the direction of special policies for each kind of communication rather
than a system IPC policy. On the other hand, that appears to be the trend in
system security overall, so even I can see that it may be prudent.


>
> thanks,
>
> greg k-h
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.
  2015-01-23 21:56         ` Casey Schaufler
@ 2015-01-23 23:45           ` Greg KH
  0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2015-01-23 23:45 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Stephen Smalley, Stephen Smalley, Linux Kernel, arve,
	Nick Kralevich, Paul Moore, selinux, linux-security-module,
	James Morris

On Fri, Jan 23, 2015 at 01:56:28PM -0800, Casey Schaufler wrote:
> On 1/22/2015 6:30 PM, Greg KH wrote:
> > On Thu, Jan 22, 2015 at 01:47:29PM -0500, Stephen Smalley wrote:
> >> On Thu, Jan 22, 2015 at 1:09 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> On 1/22/2015 12:51 AM, Greg KH wrote:
> >>>> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
> >>>>> Add security hooks to the binder and implement the hooks for SELinux.
> >>>>> The security hooks enable security modules such as SELinux to implement
> >>>>> controls over binder IPC.  The security hooks include support for
> >>>>> controlling what process can become the binder context manager
> >>>>> (binder_set_context_mgr), controlling the ability of a process
> >>>>> to invoke a binder transaction/IPC to another process (binder_transaction),
> >>>>> controlling the ability of a process to transfer a binder reference to
> >>>>> another process (binder_transfer_binder), and controlling the ability
> >>>>> of a process to transfer an open file to another process (binder_transfer_file).
> >>>>>
> >>>>> These hooks have been included in the Android kernel trees since Android 4.3.
> >>>> Very interesting, I missed the fact that these were added in that tree,
> >>>> thanks for digging it out and submitting it.
> >>>>
> >>>> I'd like some acks from some Android developers before I take these.
> >>>> Or, if it's easier for them to go through the security tree, that's fine
> >>>> with me as well.
> >>> My only concern is that we're about to see a set of hooks proposed
> >>> for kdbus as well, and it would be a shame if we had two sets of hooks
> >>> that do roughly the same thing (ok, *very roughly*) introduced back to back.
> >> Not sure how much commonality there truly is among them (based on the
> >> last set of proposed kdbus lsm hooks that I saw, admittedly a while
> >> ago) and modules may want to distinguish between the two
> >> forms of IPC regardless.  The binder hooks have been in place in the
> >> Android kernel trees for quite some time, so this patch is just making
> >> the mainline binder driver consistent with what is already in Android.
> >> If it turns out that there is significant duplication when the kdbus
> >> lsm hooks land, I'd be happy to help coalesce them.
> > Yeah, I would wait and see what happens with how the kdbus hooks look
> > before worrying about this all that much.  As people are using the
> > binder hooks today, and binder is in the kernel tree, but kdbus isn't,
> > let's not worry about kdbus until that is merged.
> 
> That seems like a sane plan to me in light of the situation. I am somewhat
> concerned about the growth in special case security behavior. The tun hooks,
> now the binder hooks and later the kdbus hooks. It looks like we're going
> in the direction of special policies for each kind of communication rather
> than a system IPC policy. On the other hand, that appears to be the trend in
> system security overall, so even I can see that it may be prudent.

If you know of some way to make a more "unified" system IPC policy, that
would be great to have, I don't know if anyone is even considering that
work, as it seems people are just more concerned about addressing the
more real issues of the different IPC methods these days.

Maybe it would be a good research project for someone to write a thesis
on?  :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-01-23 23:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-21 15:54 [PATCH] Add security hooks to binder and implement the hooks for SELinux Stephen Smalley
2015-01-22  8:51 ` Greg KH
2015-01-22 17:37   ` Jeffrey Vander Stoep
2015-01-22 17:48   ` Nick Kralevich
2015-01-22 18:09   ` Casey Schaufler
2015-01-22 18:47     ` Stephen Smalley
2015-01-23  2:30       ` Greg KH
2015-01-23 21:56         ` Casey Schaufler
2015-01-23 23:45           ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).