| 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
| ||
|
Message-ID: <CAEjxPJ6OWqjhyMS47cwRUT3OphknTdmc1FXRMA130njxajjsKQ@mail.gmail.com>
Date: Wed, 14 May 2025 13:51:23 -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,
Thiébaud Weksteen <tweek@...gle.com>,
Casey Schaufler <casey@...aufler-ca.com>, Canfeng Guo <guocanfeng@...ontech.com>,
Takaya Saeki <takayas@...omium.org>
Subject: Re: [PATCH v3 05/14] selinux: validate constraints
On Sun, May 11, 2025 at 1:31 PM Christian Göttsche
<cgoettsche@...tendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@...glemail.com>
>
> Validate constraint expressions during reading the policy.
> Avoid the usage of BUG() on constraint evaluation, to mitigate malformed
> policies halting the system.
>
> Closes: https://github.com/SELinuxProject/selinux-testsuite/issues/76
>
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
> ---
> security/selinux/ss/policydb.c | 61 ++++++++++++++++++++++++++++++++--
> security/selinux/ss/services.c | 55 +++++++++++++++---------------
> 2 files changed, 88 insertions(+), 28 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 46c010afd44f..a8397ed66109 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1288,15 +1290,70 @@ static int read_cons_helper(struct policydb *p, struct constraint_node **nodep,
> depth--;
> break;
> case CEXPR_ATTR:
> - if (depth == (CEXPR_MAXDEPTH - 1))
> + if (depth >= (CEXPR_MAXDEPTH - 1))
How would the > case be possible? If it isn't something that can be
done by a malformed policy, drop it. Same applies later too.
> return -EINVAL;
> depth++;
> break;
> +
> + switch (e->op) {
Something went wrong here, probably a merge failure. Statement after a break?
> + case CEXPR_EQ:
> + case CEXPR_NEQ:
> + break;
> + case CEXPR_DOM:
> + case CEXPR_DOMBY:
> + case CEXPR_INCOMP:
> + if ((e->attr & CEXPR_USER) || (e->attr & CEXPR_TYPE))
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (e->attr) {
> + case CEXPR_USER:
> + case CEXPR_ROLE:
> + case CEXPR_TYPE:
> + case CEXPR_L1L2:
> + case CEXPR_L1H2:
> + case CEXPR_H1L2:
> + case CEXPR_H1H2:
> + case CEXPR_L1H1:
> + case CEXPR_L2H2:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + break;
> case CEXPR_NAMES:
> if (!allowxtarget && (e->attr & CEXPR_XTARGET))
> return -EINVAL;
> - if (depth == (CEXPR_MAXDEPTH - 1))
> + if (depth >= (CEXPR_MAXDEPTH - 1))
> + return -EINVAL;
> +
> + switch (e->op) {
> + case CEXPR_EQ:
> + case CEXPR_NEQ:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (e->attr) {
> + case CEXPR_USER:
> + case CEXPR_USER | CEXPR_TARGET:
> + case CEXPR_USER | CEXPR_XTARGET:
> + case CEXPR_ROLE:
> + case CEXPR_ROLE | CEXPR_TARGET:
> + case CEXPR_ROLE | CEXPR_XTARGET:
> + case CEXPR_TYPE:
> + case CEXPR_TYPE | CEXPR_TARGET:
> + case CEXPR_TYPE | CEXPR_XTARGET:
> + break;
> + default:
> return -EINVAL;
> + }
> +
> depth++;
> rc = ebitmap_read(&e->names, fp);
> if (rc)
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 0f67a030b49b..3fb971fe4fd9 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -279,22 +279,25 @@ static int constraint_expr_eval(struct policydb *policydb,
> for (e = cexpr; e; e = e->next) {
> switch (e->expr_type) {
> case CEXPR_NOT:
> - BUG_ON(sp < 0);
> + if (unlikely(sp < 0))
> + goto invalid;
If you have ensured that this is not possible at policy load time,
then we don't need to recheck it at runtime. Same applies later.
> s[sp] = !s[sp];
> break;
> case CEXPR_AND:
> - BUG_ON(sp < 1);
> + if (unlikely(sp < 1))
> + goto invalid;
> sp--;
> s[sp] &= s[sp + 1];
> break;
> case CEXPR_OR:
> - BUG_ON(sp < 1);
> + if (unlikely(sp < 1))
> + goto invalid;
> sp--;
> s[sp] |= s[sp + 1];
> break;
> case CEXPR_ATTR:
> - if (sp == (CEXPR_MAXDEPTH - 1))
> - return 0;
> + if (unlikely(sp >= (CEXPR_MAXDEPTH - 1)))
> + goto invalid;
> switch (e->attr) {
> case CEXPR_USER:
> val1 = scontext->user;
> @@ -370,13 +373,11 @@ static int constraint_expr_eval(struct policydb *policydb,
> s[++sp] = mls_level_incomp(l2, l1);
> continue;
> default:
> - BUG();
> - return 0;
> + goto invalid;
> }
> break;
> default:
> - BUG();
> - return 0;
> + goto invalid;
> }
>
> switch (e->op) {
> @@ -387,22 +388,19 @@ static int constraint_expr_eval(struct policydb *policydb,
> s[++sp] = (val1 != val2);
> break;
> default:
> - BUG();
> - return 0;
> + goto invalid;
> }
> break;
> case CEXPR_NAMES:
> - if (sp == (CEXPR_MAXDEPTH-1))
> - return 0;
> + if (unlikely(sp >= (CEXPR_MAXDEPTH-1)))
> + goto invalid;
> c = scontext;
> if (e->attr & CEXPR_TARGET)
> c = tcontext;
> else if (e->attr & CEXPR_XTARGET) {
> c = xcontext;
> - if (!c) {
> - BUG();
> - return 0;
> - }
> + if (unlikely(!c))
> + goto invalid;
> }
> if (e->attr & CEXPR_USER)
> val1 = c->user;
> @@ -410,10 +408,8 @@ static int constraint_expr_eval(struct policydb *policydb,
> val1 = c->role;
> else if (e->attr & CEXPR_TYPE)
> val1 = c->type;
> - else {
> - BUG();
> - return 0;
> - }
> + else
> + goto invalid;
>
> switch (e->op) {
> case CEXPR_EQ:
> @@ -423,18 +419,25 @@ static int constraint_expr_eval(struct policydb *policydb,
> s[++sp] = !ebitmap_get_bit(&e->names, val1 - 1);
> break;
> default:
> - BUG();
> - return 0;
> + goto invalid;
> }
> break;
> default:
> - BUG();
> - return 0;
> + goto invalid;
> }
> }
>
> - BUG_ON(sp != 0);
> + if (unlikely(sp != 0))
> + goto invalid;
> +
> return s[0];
> +
> +invalid:
> + /* Should *never* be reached, cause malformed constraints should
> + * have been filtered by read_cons_helper().
> + */
> + WARN_ONCE(true, "SELinux: invalid constraint passed validation\n");
> + return 0;
> }
>
> /*
> --
> 2.49.0
>
Powered by blists - more mailing lists