diff mbox series

[2/2] selinux: add support for xperms in conditional policies

Message ID 20240405161042.260113-1-cgoettsche@seltendoof.de (mailing list archive)
State New, archived
Delegated to: Paul Moore
Headers show
Series [1/2] selinux: constify source policy in cond_policydb_dup() | expand

Commit Message

Christian Göttsche April 5, 2024, 4:10 p.m. UTC
From: Christian Göttsche <cgzones@googlemail.com>

Add support for extended permission rules in conditional policies.
Currently the kernel accepts such rules already, but evaluating a
security decision will hit a BUG() in
services_compute_xperms_decision().  Thus reject extended permission
rules in conditional policies for current policy versions.

Add a new policy version for this feature.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
Userspace patches are available at:
https://github.com/SELinuxProject/selinux/pull/432

Maybe the policy version 34 can be reused for the prefix/suffix filetrans
feature to avoid two new versions?
---
 security/selinux/include/security.h |  3 ++-
 security/selinux/ss/avtab.c         | 12 ++++++++++--
 security/selinux/ss/avtab.h         |  2 +-
 security/selinux/ss/conditional.c   |  2 +-
 security/selinux/ss/policydb.c      |  5 +++++
 security/selinux/ss/services.c      | 11 +++++++----
 security/selinux/ss/services.h      |  2 +-
 7 files changed, 27 insertions(+), 10 deletions(-)

Comments

Christian Göttsche Aug. 21, 2024, 1:07 p.m. UTC | #1
> From: Christian Göttsche <cgzones@googlemail.com>
>
> Add support for extended permission rules in conditional policies.
> Currently the kernel accepts such rules already, but evaluating a
> security decision will hit a BUG() in
> services_compute_xperms_decision().  Thus reject extended permission
> rules in conditional policies for current policy versions.
>
> Add a new policy version for this feature.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> Userspace patches are available at:
> https://github.com/SELinuxProject/selinux/pull/432
>
> Maybe the policy version 34 can be reused for the prefix/suffix filetrans
> feature to avoid two new versions?

Kindly ping.

Any comments?

This affects (improves?) also the netlink xperm proposal.
Stephen Smalley Aug. 21, 2024, 2:57 p.m. UTC | #2
On Wed, Aug 21, 2024 at 9:08 AM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > Add support for extended permission rules in conditional policies.
> > Currently the kernel accepts such rules already, but evaluating a
> > security decision will hit a BUG() in
> > services_compute_xperms_decision().  Thus reject extended permission
> > rules in conditional policies for current policy versions.
> >
> > Add a new policy version for this feature.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> > Userspace patches are available at:
> > https://github.com/SELinuxProject/selinux/pull/432
> >
> > Maybe the policy version 34 can be reused for the prefix/suffix filetrans
> > feature to avoid two new versions?
>
> Kindly ping.
>
> Any comments?
>
> This affects (improves?) also the netlink xperm proposal.

Do you know of anyone who plans to use this feature? Android does not
use conditional policies and it is the primary user of the current
extended permissions feature. I haven't seen any usage in refpolicy to
date.
Stephen Smalley Aug. 29, 2024, 1:12 p.m. UTC | #3
On Wed, Aug 21, 2024 at 10:57 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Aug 21, 2024 at 9:08 AM Christian Göttsche
> <cgoettsche@seltendoof.de> wrote:
> >
> > > From: Christian Göttsche <cgzones@googlemail.com>
> > >
> > > Add support for extended permission rules in conditional policies.
> > > Currently the kernel accepts such rules already, but evaluating a
> > > security decision will hit a BUG() in
> > > services_compute_xperms_decision().  Thus reject extended permission
> > > rules in conditional policies for current policy versions.
> > >
> > > Add a new policy version for this feature.
> > >
> > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > > ---
> > > Userspace patches are available at:
> > > https://github.com/SELinuxProject/selinux/pull/432
> > >
> > > Maybe the policy version 34 can be reused for the prefix/suffix filetrans
> > > feature to avoid two new versions?
> >
> > Kindly ping.
> >
> > Any comments?
> >
> > This affects (improves?) also the netlink xperm proposal.
>
> Do you know of anyone who plans to use this feature? Android does not
> use conditional policies and it is the primary user of the current
> extended permissions feature. I haven't seen any usage in refpolicy to
> date.

Not opposed to adding this support but absent an immediate user and
with the requirement to introduce a new policy version for it, I would
defer merging this until after the netlink_xperm proposal. I would
encourage you to re-base on that once it lands and also to post the
selinux userspace patches to the list if possible (breaking them up if
necessary). Agree it would be nice if we could combine with the
prefix/suffix support to avoid two policy version bumps but unclear
that one is going to move forward any time soon.
diff mbox series

Patch

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 289bf9233f71..3a385821c574 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -46,10 +46,11 @@ 
 #define POLICYDB_VERSION_INFINIBAND	     31
 #define POLICYDB_VERSION_GLBLUB		     32
 #define POLICYDB_VERSION_COMP_FTRANS	     33 /* compressed filename transitions */
+#define POLICYDB_VERSION_COND_XPERMS	     34 /* extended permissions in conditional policies */
 
 /* Range of policy versions we understand*/
 #define POLICYDB_VERSION_MIN POLICYDB_VERSION_BASE
-#define POLICYDB_VERSION_MAX POLICYDB_VERSION_COMP_FTRANS
+#define POLICYDB_VERSION_MAX POLICYDB_VERSION_COND_XPERMS
 
 /* Mask for just the mount related flags */
 #define SE_MNTMASK 0x0f
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 2ad98732d052..bc7f1aa3ebfb 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -339,7 +339,7 @@  static const uint16_t spec_order[] = {
 int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
 		    int (*insertf)(struct avtab *a, const struct avtab_key *k,
 				   const struct avtab_datum *d, void *p),
-		    void *p)
+		    void *p, bool conditional)
 {
 	__le16 buf16[4];
 	u16 enabled;
@@ -457,6 +457,14 @@  int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
 		       "was specified\n",
 		       vers);
 		return -EINVAL;
+	} else if ((vers < POLICYDB_VERSION_COND_XPERMS) &&
+	       (key.specified & AVTAB_XPERMS) &&
+	       conditional) {
+		pr_err("SELinux:  avtab:  policy version %u does not "
+		       "support extended permissions rules in conditional "
+		       "policies and one was specified\n",
+		       vers);
+		return -EINVAL;
 	} else if (key.specified & AVTAB_XPERMS) {
 		memset(&xperms, 0, sizeof(struct avtab_extended_perms));
 		rc = next_entry(&xperms.specified, fp, sizeof(u8));
@@ -523,7 +531,7 @@  int avtab_read(struct avtab *a, void *fp, struct policydb *pol)
 		goto bad;
 
 	for (i = 0; i < nel; i++) {
-		rc = avtab_read_item(a, fp, pol, avtab_insertf, NULL);
+		rc = avtab_read_item(a, fp, pol, avtab_insertf, NULL, false);
 		if (rc) {
 			if (rc == -ENOMEM)
 				pr_err("SELinux: avtab: out of memory\n");
diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
index 8e8820484c55..b48c15b3698c 100644
--- a/security/selinux/ss/avtab.h
+++ b/security/selinux/ss/avtab.h
@@ -107,7 +107,7 @@  struct policydb;
 int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
 		    int (*insert)(struct avtab *a, const struct avtab_key *k,
 				  const struct avtab_datum *d, void *p),
-		    void *p);
+		    void *p, bool conditional);
 
 int avtab_read(struct avtab *a, void *fp, struct policydb *pol);
 int avtab_write_item(struct policydb *p, const struct avtab_node *cur,
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index d53c34021dbe..ed4606e3af5d 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -349,7 +349,7 @@  static int cond_read_av_list(struct policydb *p, void *fp,
 	for (i = 0; i < len; i++) {
 		data.dst = &list->nodes[i];
 		rc = avtab_read_item(&p->te_cond_avtab, fp, p, cond_insertf,
-				     &data);
+				     &data, true);
 		if (rc) {
 			kfree(list->nodes);
 			list->nodes = NULL;
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 383f3ae82a73..3ba5506a3fff 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -155,6 +155,11 @@  static const struct policydb_compat_info policydb_compat[] = {
 		.sym_num = SYM_NUM,
 		.ocon_num = OCON_NUM,
 	},
+	{
+		.version = POLICYDB_VERSION_COND_XPERMS,
+		.sym_num = SYM_NUM,
+		.ocon_num = OCON_NUM,
+	},
 };
 
 static const struct policydb_compat_info *
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index e88b1b6c4adb..57f09f830a06 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -944,9 +944,10 @@  static void avd_init(struct selinux_policy *policy, struct av_decision *avd)
 }
 
 void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
-					struct avtab_node *node)
+					const struct avtab_node *node)
 {
 	unsigned int i;
+	u16 specified;
 
 	if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLFUNCTION) {
 		if (xpermd->driver != node->datum.u.xperms->driver)
@@ -959,7 +960,9 @@  void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
 		BUG();
 	}
 
-	if (node->key.specified == AVTAB_XPERMS_ALLOWED) {
+	specified = node->key.specified & ~(AVTAB_ENABLED | AVTAB_ENABLED_OLD);
+
+	if (specified == AVTAB_XPERMS_ALLOWED) {
 		xpermd->used |= XPERMS_ALLOWED;
 		if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) {
 			memset(xpermd->allowed->p, 0xff,
@@ -970,7 +973,7 @@  void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
 				xpermd->allowed->p[i] |=
 					node->datum.u.xperms->perms.p[i];
 		}
-	} else if (node->key.specified == AVTAB_XPERMS_AUDITALLOW) {
+	} else if (specified == AVTAB_XPERMS_AUDITALLOW) {
 		xpermd->used |= XPERMS_AUDITALLOW;
 		if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) {
 			memset(xpermd->auditallow->p, 0xff,
@@ -981,7 +984,7 @@  void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
 				xpermd->auditallow->p[i] |=
 					node->datum.u.xperms->perms.p[i];
 		}
-	} else if (node->key.specified == AVTAB_XPERMS_DONTAUDIT) {
+	} else if (specified == AVTAB_XPERMS_DONTAUDIT) {
 		xpermd->used |= XPERMS_DONTAUDIT;
 		if (node->datum.u.xperms->specified == AVTAB_XPERMS_IOCTLDRIVER) {
 			memset(xpermd->dontaudit->p, 0xff,
diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
index 93358e7a649c..a6d8a06fd13c 100644
--- a/security/selinux/ss/services.h
+++ b/security/selinux/ss/services.h
@@ -38,7 +38,7 @@  struct convert_context_args {
 void services_compute_xperms_drivers(struct extended_perms *xperms,
 				     struct avtab_node *node);
 void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
-				      struct avtab_node *node);
+				      const struct avtab_node *node);
 
 int services_convert_context(struct convert_context_args *args,
 			     struct context *oldc, struct context *newc,