linux-kernel - Re: [PATCH v3 11/14] selinux: more strict bounds check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEjxPJ6p_JPqUyvcQJL1=Vfa+5C_zt-oB7yvpkBYLCBLfxi55w@mail.gmail.com>
Date: Wed, 14 May 2025 15:11:57 -0400
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: cgzones@...glemail.com
Cc: selinux@...r.kernel.org, Paul Moore <paul@...l-moore.com>, 
	Ondrej Mosnacek <omosnace@...hat.com>, linux-kernel@...r.kernel.org, 
	Casey Schaufler <casey@...aufler-ca.com>, Thiébaud Weksteen <tweek@...gle.com>, 
	Canfeng Guo <guocanfeng@...ontech.com>, Takaya Saeki <takayas@...omium.org>
Subject: Re: [PATCH v3 11/14] selinux: more strict bounds check

On Sun, May 11, 2025 at 1:31 PM Christian Göttsche
<cgoettsche@...tendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@...glemail.com>
>
> Validate the types used in bounds checks.
> Replace the usage of BUG(), to avoid halting the system on malformed
> polices.
>
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>

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

> ---
>  security/selinux/ss/policydb.c | 29 +++++++++++++++++++++++++++--
>  security/selinux/ss/policydb.h |  1 +
>  security/selinux/ss/services.c |  3 +++
>  3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 4559c8918134..7774f6da2ebe 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1020,6 +1020,15 @@ bool policydb_class_isvalid(const struct policydb *p, u16 class)
>         return true;
>  }
>
> +bool policydb_user_isvalid(const struct policydb *p, u32 user)
> +{
> +       if (!user || user > p->p_roles.nprim)
> +               return false;
> +       if (!p->sym_val_to_name[SYM_USERS][user - 1])
> +               return false;
> +       return true;
> +}
> +
>  bool policydb_role_isvalid(const struct policydb *p, u32 role)
>  {
>         if (!role || role > p->p_roles.nprim)
> @@ -1942,6 +1951,12 @@ static int user_bounds_sanity_check(void *key, void *datum, void *datap)
>                         return -EINVAL;
>                 }
>
> +               if (!policydb_user_isvalid(p, upper->bounds)) {
> +                       pr_err("SELinux: user %s: invalid boundary id %d\n",
> +                              (char *) key, upper->bounds);
> +                       return -EINVAL;
> +               }
> +
>                 upper = p->user_val_to_struct[upper->bounds - 1];
>                 ebitmap_for_each_positive_bit(&user->roles, node, bit)
>                 {
> @@ -1979,6 +1994,12 @@ static int role_bounds_sanity_check(void *key, void *datum, void *datap)
>                         return -EINVAL;
>                 }
>
> +               if (!policydb_role_isvalid(p, upper->bounds)) {
> +                       pr_err("SELinux: role %s: invalid boundary id %d\n",
> +                              (char *) key, upper->bounds);
> +                       return -EINVAL;
> +               }
> +
>                 upper = p->role_val_to_struct[upper->bounds - 1];
>                 ebitmap_for_each_positive_bit(&role->types, node, bit)
>                 {
> @@ -2013,9 +2034,13 @@ static int type_bounds_sanity_check(void *key, void *datum, void *datap)
>                         return -EINVAL;
>                 }
>
> -               upper = p->type_val_to_struct[upper->bounds - 1];
> -               BUG_ON(!upper);
> +               if (!policydb_type_isvalid(p, upper->bounds)) {
> +                       pr_err("SELinux: type %s: invalid boundary id %d\n",
> +                              (char *) key, upper->bounds);
> +                       return -EINVAL;
> +               }
>
> +               upper = p->type_val_to_struct[upper->bounds - 1];
>                 if (upper->attribute) {
>                         pr_err("SELinux: type %s: "
>                                "bounded by attribute %s\n",
> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 1367387beaa7..04acf414fffa 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -324,6 +324,7 @@ extern bool policydb_context_isvalid(const struct policydb *p, const struct cont
>  extern bool policydb_class_isvalid(const struct policydb *p, u16 class);
>  extern bool policydb_type_isvalid(const struct policydb *p, u32 type);
>  extern bool policydb_role_isvalid(const struct policydb *p, u32 role);
> +extern bool policydb_user_isvalid(const struct policydb *p, u32 user);
>  extern bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean);
>  extern int policydb_read(struct policydb *p, struct policy_file *fp);
>  extern int policydb_write(struct policydb *p, struct policy_file *fp);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 5b1d0e80d975..464a4663c993 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -718,6 +718,9 @@ static void context_struct_compute_av(struct policydb *policydb,
>          * If the given source and target types have boundary
>          * constraint, lazy checks have to mask any violated
>          * permission and notice it to userspace via audit.
> +        *
> +        * Infinite recursion is avoided via a depth pre-check in
> +        * type_bounds_sanity_check().
>          */
>         type_attribute_bounds_av(policydb, scontext, tcontext,
>                                  tclass, avd);
> --
> 2.49.0
>