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

Christian Göttsche posted 1 patch 1 year, 3 months ago
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(-)
[PATCH v2] selinux: add support for xperms in conditional policies
Posted by Christian Göttsche 1 year, 3 months ago
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
Posted by Paul Moore 1 year, 1 month ago
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
Re: [PATCH v2] selinux: add support for xperms in conditional policies
Posted by Stephen Smalley 1 year, 1 month ago
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
Posted by Paul Moore 1 year, 3 months ago
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
Posted by Christian Göttsche 1 year, 2 months ago
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
Posted by Paul Moore 1 year, 2 months ago
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