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

From: Stephen Smalley
Date: Thu Dec 12 2024 - 09:17:53 EST

On Wed, Oct 23, 2024 at 11:27 AM Christian Göttsche
<cgoettsche@xxxxxxxxxxxxx> wrote:
>
> From: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>

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

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