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

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

[Christian Göttsche]

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(-)

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,
-- 
2.43.0

[PATCH 1/2] selinux: constify source policy in cond_policydb_dup()

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

cond_policydb_dup() duplicates conditional parts of an existing policy.
Declare the source policy const, since it should not be modified.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/conditional.c | 12 ++++++------
 security/selinux/ss/conditional.h |  2 +-
 security/selinux/ss/hashtab.c     |  9 +++++----
 security/selinux/ss/hashtab.h     |  4 ++--
 4 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index e868fc403d75..d53c34021dbe 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -603,7 +603,7 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
 	}
 }
 
-static int cond_dup_av_list(struct cond_av_list *new, struct cond_av_list *orig,
+static int cond_dup_av_list(struct cond_av_list *new, const struct cond_av_list *orig,
 			    struct avtab *avtab)
 {
 	u32 i;
@@ -626,7 +626,7 @@ static int cond_dup_av_list(struct cond_av_list *new, struct cond_av_list *orig,
 }
 
 static int duplicate_policydb_cond_list(struct policydb *newp,
-					struct policydb *origp)
+					const struct policydb *origp)
 {
 	int rc;
 	u32 i;
@@ -643,7 +643,7 @@ static int duplicate_policydb_cond_list(struct policydb *newp,
 
 	for (i = 0; i < origp->cond_list_len; i++) {
 		struct cond_node *newn = &newp->cond_list[i];
-		struct cond_node *orign = &origp->cond_list[i];
+		const struct cond_node *orign = &origp->cond_list[i];
 
 		newp->cond_list_len++;
 
@@ -683,7 +683,7 @@ static int cond_bools_destroy(void *key, void *datum, void *args)
 	return 0;
 }
 
-static int cond_bools_copy(struct hashtab_node *new, struct hashtab_node *orig,
+static int cond_bools_copy(struct hashtab_node *new, const struct hashtab_node *orig,
 			   void *args)
 {
 	struct cond_bool_datum *datum;
@@ -710,7 +710,7 @@ static int cond_bools_index(void *key, void *datum, void *args)
 }
 
 static int duplicate_policydb_bools(struct policydb *newdb,
-				    struct policydb *orig)
+				    const struct policydb *orig)
 {
 	struct cond_bool_datum **cond_bool_array;
 	int rc;
@@ -743,7 +743,7 @@ void cond_policydb_destroy_dup(struct policydb *p)
 	cond_policydb_destroy(p);
 }
 
-int cond_policydb_dup(struct policydb *new, struct policydb *orig)
+int cond_policydb_dup(struct policydb *new, const struct policydb *orig)
 {
 	cond_policydb_init(new);
 
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index b972ce40db18..8827715bad75 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -79,6 +79,6 @@ void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key,
 			 struct extended_perms_decision *xpermd);
 void evaluate_cond_nodes(struct policydb *p);
 void cond_policydb_destroy_dup(struct policydb *p);
-int cond_policydb_dup(struct policydb *new, struct policydb *orig);
+int cond_policydb_dup(struct policydb *new, const struct policydb *orig);
 
 #endif /* _CONDITIONAL_H_ */
diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
index 754bedbde133..836642f789ab 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -136,11 +136,12 @@ void hashtab_stat(struct hashtab *h, struct hashtab_info *info)
 }
 #endif /* CONFIG_SECURITY_SELINUX_DEBUG */
 
-int hashtab_duplicate(struct hashtab *new, struct hashtab *orig,
+int hashtab_duplicate(struct hashtab *new, const struct hashtab *orig,
 		      int (*copy)(struct hashtab_node *new,
-				  struct hashtab_node *orig, void *args),
+				  const struct hashtab_node *orig, void *args),
 		      int (*destroy)(void *k, void *d, void *args), void *args)
 {
+	const struct hashtab_node *orig_cur;
 	struct hashtab_node *cur, *tmp, *tail;
 	u32 i;
 	int rc;
@@ -155,12 +156,12 @@ int hashtab_duplicate(struct hashtab *new, struct hashtab *orig,
 
 	for (i = 0; i < orig->size; i++) {
 		tail = NULL;
-		for (cur = orig->htable[i]; cur; cur = cur->next) {
+		for (orig_cur = orig->htable[i]; orig_cur; orig_cur = orig_cur->next) {
 			tmp = kmem_cache_zalloc(hashtab_node_cachep,
 						GFP_KERNEL);
 			if (!tmp)
 				goto error;
-			rc = copy(tmp, cur, args);
+			rc = copy(tmp, orig_cur, args);
 			if (rc) {
 				kmem_cache_free(hashtab_node_cachep, tmp);
 				goto error;
diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h
index 5f74dcc1360f..deba82d78c3a 100644
--- a/security/selinux/ss/hashtab.h
+++ b/security/selinux/ss/hashtab.h
@@ -136,9 +136,9 @@ void hashtab_destroy(struct hashtab *h);
 int hashtab_map(struct hashtab *h, int (*apply)(void *k, void *d, void *args),
 		void *args);
 
-int hashtab_duplicate(struct hashtab *new, struct hashtab *orig,
+int hashtab_duplicate(struct hashtab *new, const struct hashtab *orig,
 		      int (*copy)(struct hashtab_node *new,
-				  struct hashtab_node *orig, void *args),
+				  const struct hashtab_node *orig, void *args),
 		      int (*destroy)(void *k, void *d, void *args), void *args);
 
 #ifdef CONFIG_SECURITY_SELINUX_DEBUG
-- 
2.43.0

Re: [PATCH 1/2] selinux: constify source policy in cond_policydb_dup()

[Paul Moore]

On Apr  5, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> cond_policydb_dup() duplicates conditional parts of an existing policy.
> Declare the source policy const, since it should not be modified.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/ss/conditional.c | 12 ++++++------
>  security/selinux/ss/conditional.h |  2 +-
>  security/selinux/ss/hashtab.c     |  9 +++++----
>  security/selinux/ss/hashtab.h     |  4 ++--
>  4 files changed, 14 insertions(+), 13 deletions(-)

I had to do some line length fixups, but otherwise this looked good
to me, merged into selinux/dev.  Thanks!

--
paul-moore.com

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

[Christian Göttsche]

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

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

[Stephen Smalley]

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.

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

[Stephen Smalley]

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.

[PATCH v2] selinux: add support for xperms in conditional policies

[Christian Göttsche]

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>
---
v2:
  rebased onto the netlink xperm patch
---
 security/selinux/include/security.h |  3 ++-
 security/selinux/ss/avtab.c         | 11 +++++++++--
 security/selinux/ss/avtab.h         |  2 +-
 security/selinux/ss/conditional.c   |  2 +-
 security/selinux/ss/policydb.c      |  5 +++++
 security/selinux/ss/services.c      | 12 ++++++++----
 6 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index c7f2731abd03..10949df22fa4 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 8e400dd736b7..83add633f92a 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,13 @@ 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 +530,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 f4407185401c..a7cbb80a11eb 100644
--- a/security/selinux/ss/avtab.h
+++ b/security/selinux/ss/avtab.h
@@ -108,7 +108,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 64ba95e40a6f..c9a3060f08a4 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 9652aec400cb..66d2472d3874 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -946,7 +946,7 @@ static void avd_init(struct selinux_policy *policy, struct av_decision *avd)
 }
 
 static void update_xperms_extended_data(u8 specified,
-					struct extended_perms_data *from,
+					const struct extended_perms_data *from,
 					struct extended_perms_data *xp_data)
 {
 	unsigned int i;
@@ -967,6 +967,8 @@ static void update_xperms_extended_data(u8 specified,
 void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
 					struct avtab_node *node)
 {
+	u16 specified;
+
 	switch (node->datum.u.xperms->specified) {
 	case AVTAB_XPERMS_IOCTLFUNCTION:
 	case AVTAB_XPERMS_NLMSG:
@@ -982,17 +984,19 @@ 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;
 		update_xperms_extended_data(node->datum.u.xperms->specified,
 					    &node->datum.u.xperms->perms,
 					    xpermd->allowed);
-	} else if (node->key.specified == AVTAB_XPERMS_AUDITALLOW) {
+	} else if (specified == AVTAB_XPERMS_AUDITALLOW) {
 		xpermd->used |= XPERMS_AUDITALLOW;
 		update_xperms_extended_data(node->datum.u.xperms->specified,
 					    &node->datum.u.xperms->perms,
 					    xpermd->auditallow);
-	} else if (node->key.specified == AVTAB_XPERMS_DONTAUDIT) {
+	} else if (specified == AVTAB_XPERMS_DONTAUDIT) {
 		xpermd->used |= XPERMS_DONTAUDIT;
 		update_xperms_extended_data(node->datum.u.xperms->specified,
 					    &node->datum.u.xperms->perms,
-- 
2.45.2

Re: [PATCH v2] selinux: add support for xperms in conditional policies

[Paul Moore]

On Wed, Oct 23, 2024 at 11:27 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>
> ---
> v2:
>   rebased onto the netlink xperm patch
> ---
>  security/selinux/include/security.h |  3 ++-
>  security/selinux/ss/avtab.c         | 11 +++++++++--
>  security/selinux/ss/avtab.h         |  2 +-
>  security/selinux/ss/conditional.c   |  2 +-
>  security/selinux/ss/policydb.c      |  5 +++++
>  security/selinux/ss/services.c      | 12 ++++++++----
>  6 files changed, 26 insertions(+), 9 deletions(-)

This looks fine to me, but I believe there are some outstanding
userspace issues that need to be resolved?

-- 
paul-moore.com

Re: [PATCH v2] selinux: add support for xperms in conditional policies

[Christian Göttsche]

On Thu, 31 Oct 2024 at 23:20, Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Oct 23, 2024 at 11:27 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>
> > ---
> > v2:
> >   rebased onto the netlink xperm patch
> > ---
> >  security/selinux/include/security.h |  3 ++-
> >  security/selinux/ss/avtab.c         | 11 +++++++++--
> >  security/selinux/ss/avtab.h         |  2 +-
> >  security/selinux/ss/conditional.c   |  2 +-
> >  security/selinux/ss/policydb.c      |  5 +++++
> >  security/selinux/ss/services.c      | 12 ++++++++----
> >  6 files changed, 26 insertions(+), 9 deletions(-)
>
> This looks fine to me, but I believe there are some outstanding
> userspace issues that need to be resolved?

Hi,

I know it's very late in the development cycle, but I wanted to ask if
there is a chance this could be merged for 6.13?
The userspace patches are merged and currently part of 3.8-rc1, and
these kernel changes are quite simple, since most of the needed
functionality was already in place.
I created a testsuite patch over at
https://github.com/SELinuxProject/selinux-testsuite/pull/98.

>
> --
> paul-moore.com

Re: [PATCH v2] selinux: add support for xperms in conditional policies

[Paul Moore]

On Thu, Nov 28, 2024 at 7:49 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
> On Thu, 31 Oct 2024 at 23:20, Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Oct 23, 2024 at 11:27 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>
> > > ---
> > > v2:
> > >   rebased onto the netlink xperm patch
> > > ---
> > >  security/selinux/include/security.h |  3 ++-
> > >  security/selinux/ss/avtab.c         | 11 +++++++++--
> > >  security/selinux/ss/avtab.h         |  2 +-
> > >  security/selinux/ss/conditional.c   |  2 +-
> > >  security/selinux/ss/policydb.c      |  5 +++++
> > >  security/selinux/ss/services.c      | 12 ++++++++----
> > >  6 files changed, 26 insertions(+), 9 deletions(-)
> >
> > This looks fine to me, but I believe there are some outstanding
> > userspace issues that need to be resolved?
>
> Hi,
>
> I know it's very late in the development cycle, but I wanted to ask if
> there is a chance this could be merged for 6.13?

I'm sorry, but it is/was too late for those changes to be merged into
the kernel.  I'm sure you've seen this already, but the process is
documented in the README.md file which is linked below:

* https://github.com/SELinuxProject/selinux-kernel/blob/main/README.md

The relevant potion is copied below:

"During the development cycle that starts with the close of the kernel
merge window and ends with the tagged kernel release, patches will be
accepted into the stable-X.Y and dev branches as described in their
respective sections in this document. While patches will be accepted
into the stable-X.Y branch at any point in time, significant changes
will likely not be accepted into the dev branch when there are two or
less weeks left in the development cycle; this typically means that
only critical bugfixes are accepted once the vX.Y-rc6 kernel is
released."

> The userspace patches are merged and currently part of 3.8-rc1, and
> these kernel changes are quite simple, since most of the needed
> functionality was already in place.
> I created a testsuite patch over at
> https://github.com/SELinuxProject/selinux-testsuite/pull/98.

Thank you!

-- 
paul-moore.com

Re: [PATCH v2] selinux: add support for xperms in conditional policies

[Stephen Smalley]

On Wed, Oct 23, 2024 at 11:27 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>

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Tested-by: Stephen Smalley <stephen.smalley.work@gmail.com>

> ---
> v2:
>   rebased onto the netlink xperm patch
> ---
>  security/selinux/include/security.h |  3 ++-
>  security/selinux/ss/avtab.c         | 11 +++++++++--
>  security/selinux/ss/avtab.h         |  2 +-
>  security/selinux/ss/conditional.c   |  2 +-
>  security/selinux/ss/policydb.c      |  5 +++++
>  security/selinux/ss/services.c      | 12 ++++++++----
>  6 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index c7f2731abd03..10949df22fa4 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 8e400dd736b7..83add633f92a 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,13 @@ 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 +530,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 f4407185401c..a7cbb80a11eb 100644
> --- a/security/selinux/ss/avtab.h
> +++ b/security/selinux/ss/avtab.h
> @@ -108,7 +108,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 64ba95e40a6f..c9a3060f08a4 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 9652aec400cb..66d2472d3874 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -946,7 +946,7 @@ static void avd_init(struct selinux_policy *policy, struct av_decision *avd)
>  }
>
>  static void update_xperms_extended_data(u8 specified,
> -                                       struct extended_perms_data *from,
> +                                       const struct extended_perms_data *from,
>                                         struct extended_perms_data *xp_data)
>  {
>         unsigned int i;
> @@ -967,6 +967,8 @@ static void update_xperms_extended_data(u8 specified,
>  void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
>                                         struct avtab_node *node)
>  {
> +       u16 specified;
> +
>         switch (node->datum.u.xperms->specified) {
>         case AVTAB_XPERMS_IOCTLFUNCTION:
>         case AVTAB_XPERMS_NLMSG:
> @@ -982,17 +984,19 @@ 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;
>                 update_xperms_extended_data(node->datum.u.xperms->specified,
>                                             &node->datum.u.xperms->perms,
>                                             xpermd->allowed);
> -       } else if (node->key.specified == AVTAB_XPERMS_AUDITALLOW) {
> +       } else if (specified == AVTAB_XPERMS_AUDITALLOW) {
>                 xpermd->used |= XPERMS_AUDITALLOW;
>                 update_xperms_extended_data(node->datum.u.xperms->specified,
>                                             &node->datum.u.xperms->perms,
>                                             xpermd->auditallow);
> -       } else if (node->key.specified == AVTAB_XPERMS_DONTAUDIT) {
> +       } else if (specified == AVTAB_XPERMS_DONTAUDIT) {
>                 xpermd->used |= XPERMS_DONTAUDIT;
>                 update_xperms_extended_data(node->datum.u.xperms->specified,
>                                             &node->datum.u.xperms->perms,
> --
> 2.45.2
>

Re: [PATCH v2] selinux: add support for xperms in conditional policies

[Paul Moore]

On Oct 23, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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>
> ---
> v2:
>   rebased onto the netlink xperm patch
> ---
>  security/selinux/include/security.h |  3 ++-
>  security/selinux/ss/avtab.c         | 11 +++++++++--
>  security/selinux/ss/avtab.h         |  2 +-
>  security/selinux/ss/conditional.c   |  2 +-
>  security/selinux/ss/policydb.c      |  5 +++++
>  security/selinux/ss/services.c      | 12 ++++++++----
>  6 files changed, 26 insertions(+), 9 deletions(-)

Merged into selinux/dev, thanks for working on this and your patience!

--
paul-moore.com