| 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: <CAEjxPJ4Q8_PjvvXR9JAs307A0n-9xxS8LkLWE5NmeLgiF-NrdA@mail.gmail.com>
Date: Wed, 14 May 2025 15:53:16 -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 14/14] selinux: harden MLS context string generation
against overflows
On Sun, May 11, 2025 at 1:31 PM Christian Göttsche
<cgoettsche@...tendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@...glemail.com>
>
> Check the length accumulator for the MLS component of security contexts
> does not overflow in mls_compute_context_len() resulting in
> out-of-buffer writes in mls_sid_to_context().
>
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
> ---
> v3: add patch
> ---
> security/selinux/ss/mls.c | 18 +++++++++-----
> security/selinux/ss/services.c | 43 +++++++++++++++++++++++++++++-----
> 2 files changed, 49 insertions(+), 12 deletions(-)
>
> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 3cd36e2015fa..aa25724f0b0f 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -42,7 +42,8 @@ int mls_compute_context_len(struct policydb *p, struct context *context)
> len = 1; /* for the beginning ":" */
> for (l = 0; l < 2; l++) {
> u32 index_sens = context->range.level[l].sens;
> - len += strlen(sym_name(p, SYM_LEVELS, index_sens - 1));
> + if (check_add_overflow(len, strlen(sym_name(p, SYM_LEVELS, index_sens - 1)), &len))
> + return -EOVERFLOW;
I don't believe these checks are necessary, especially once the other
limits on identifier lengths land.
The same applies throughout.
>
> /* categories */
> head = -2;
> @@ -54,24 +55,29 @@ int mls_compute_context_len(struct policydb *p, struct context *context)
> /* one or more negative bits are skipped */
> if (head != prev) {
> nm = sym_name(p, SYM_CATS, prev);
> - len += strlen(nm) + 1;
> + if (check_add_overflow(len, strlen(nm) + 1, &len))
> + return -EOVERFLOW;
> }
> nm = sym_name(p, SYM_CATS, i);
> - len += strlen(nm) + 1;
> + if (check_add_overflow(len, strlen(nm) + 1, &len))
> + return -EOVERFLOW;
> head = i;
> }
> prev = i;
> }
> if (prev != head) {
> nm = sym_name(p, SYM_CATS, prev);
> - len += strlen(nm) + 1;
> + if (check_add_overflow(len, strlen(nm) + 1, &len))
> + return -EOVERFLOW;
> }
> if (l == 0) {
> if (mls_level_eq(&context->range.level[0],
> &context->range.level[1]))
> break;
> - else
> - len++;
> + else {
> + if (check_add_overflow(len, 1, &len))
> + return -EOVERFLOW;
> + }
> }
> }
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 464a4663c993..dc6dce2eb7d2 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1247,10 +1247,12 @@ static int context_struct_to_string(struct policydb *p,
> char **scontext, u32 *scontext_len)
> {
> char *scontextp;
> + size_t len;
> + int mls_len;
>
> if (scontext)
> *scontext = NULL;
> - *scontext_len = 0;
> + len = 0;
>
> if (context->len) {
> *scontext_len = context->len;
> @@ -1263,16 +1265,45 @@ static int context_struct_to_string(struct policydb *p,
> }
>
> /* Compute the size of the context. */
> - *scontext_len += strlen(sym_name(p, SYM_USERS, context->user - 1)) + 1;
> - *scontext_len += strlen(sym_name(p, SYM_ROLES, context->role - 1)) + 1;
> - *scontext_len += strlen(sym_name(p, SYM_TYPES, context->type - 1)) + 1;
> - *scontext_len += mls_compute_context_len(p, context);
> + len += strlen(sym_name(p, SYM_USERS, context->user - 1)) + 1;
> + len += strlen(sym_name(p, SYM_ROLES, context->role - 1)) + 1;
> + len += strlen(sym_name(p, SYM_TYPES, context->type - 1)) + 1;
> +
> + mls_len = mls_compute_context_len(p, context);
> + if (unlikely(mls_len < 0)) {
> + pr_warn_ratelimited("SELinux: %s: MLS security context component too large [%s:%s:%s[:[%s:%d]-[%s:%d]]]\n",
> + __func__,
> + sym_name(p, SYM_USERS, context->user - 1),
> + sym_name(p, SYM_ROLES, context->role - 1),
> + sym_name(p, SYM_TYPES, context->type - 1),
> + sym_name(p, SYM_LEVELS, context->range.level[0].sens - 1),
> + ebitmap_length(&context->range.level[0].cat),
> + sym_name(p, SYM_LEVELS, context->range.level[1].sens - 1),
> + ebitmap_length(&context->range.level[1].cat));
> + return -EOVERFLOW;
> + }
> +
> + if (unlikely(check_add_overflow(len, mls_len, &len) || len > CONTEXT_MAXLENGTH)) {
> + pr_warn_ratelimited("SELinux: %s: security context string of length %zu too large [%s:%s:%s[:[%s:%d]-[%s:%d]]]\n",
> + __func__,
> + len,
> + sym_name(p, SYM_USERS, context->user - 1),
> + sym_name(p, SYM_ROLES, context->role - 1),
> + sym_name(p, SYM_TYPES, context->type - 1),
> + sym_name(p, SYM_LEVELS, context->range.level[0].sens - 1),
> + ebitmap_length(&context->range.level[0].cat),
> + sym_name(p, SYM_LEVELS, context->range.level[1].sens - 1),
> + ebitmap_length(&context->range.level[1].cat));
> + return -EOVERFLOW;
> + }
> +
> + *scontext_len = len;
>
> if (!scontext)
> return 0;
>
> /* Allocate space for the context; caller must free this space. */
> - scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
> + scontextp = kmalloc(len, GFP_ATOMIC);
> if (!scontextp)
> return -ENOMEM;
> *scontext = scontextp;
> --
> 2.49.0
>
Powered by blists - more mailing lists