| 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: <CAEjxPJ6uxn+nmYruOJvrAHMNQYQhx1tVZ1eG5_YC2CAqEiQisg@mail.gmail.com>
Date: Wed, 14 May 2025 13:26:47 -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,
Eric Suen <ericsu@...ux.microsoft.com>
Subject: Re: [PATCH v3 04/14] selinux: check length fields in policies
On Sun, May 11, 2025 at 1:31 PM Christian Göttsche
<cgoettsche@...tendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@...glemail.com>
>
> In multiple places the binary policy announces how many items of some
> kind are to be expected next. Before reading them the kernel already
> allocates enough memory for that announced size. Validate that the
> remaining input size can actually fit the announced items, to avoid OOM
> issues on malformed binary policies.
>
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@...il.com>
> ---
> v3:
> - fix error branch by returning directly instead of jumping to goto
> label, see
> https://lore.kernel.org/all/202412241405.LK8YTZqp-lkp@intel.com/
> - rename to size_check()
> - add comments for magic values
> ---
> security/selinux/ss/avtab.c | 5 +++++
> security/selinux/ss/conditional.c | 17 ++++++++++++++++
> security/selinux/ss/policydb.c | 32 +++++++++++++++++++++++++++++++
> security/selinux/ss/policydb.h | 13 +++++++++++++
> 4 files changed, 67 insertions(+)
>
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 33556922f15e..50df8b69de2b 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -548,6 +548,11 @@ int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol)
> goto bad;
> }
>
> + /* avtab_read_item() reads at least 96 bytes for any valid entry */
> + rc = size_check(3 * sizeof(u32), nel, fp);
> + if (rc)
> + goto bad;
> +
> rc = avtab_alloc(a, nel);
> if (rc)
> goto bad;
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index db30462ed6a3..92ed4f23a217 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -12,6 +12,7 @@
>
> #include "security.h"
> #include "conditional.h"
> +#include "policydb.h"
> #include "services.h"
>
> /*
> @@ -329,6 +330,11 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
> if (len == 0)
> return 0;
>
> + /* avtab_read_item() reads at least 96 bytes for any valid entry */
> + rc = size_check(3 * sizeof(u32), len, fp);
> + if (rc)
> + return rc;
> +
> list->nodes = kcalloc(len, sizeof(*list->nodes), GFP_KERNEL);
> if (!list->nodes)
> return -ENOMEM;
> @@ -379,6 +385,12 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, struct pol
>
> /* expr */
> len = le32_to_cpu(buf[1]);
> +
> + /* we will read 64 bytes per node */
> + rc = size_check(2 * sizeof(u32), len, fp);
> + if (rc)
> + return rc;
> +
> node->expr.nodes = kcalloc(len, sizeof(*node->expr.nodes), GFP_KERNEL);
> if (!node->expr.nodes)
> return -ENOMEM;
> @@ -417,6 +429,11 @@ int cond_read_list(struct policydb *p, struct policy_file *fp)
>
> len = le32_to_cpu(buf[0]);
>
> + /* cond_read_node() reads at least 128 bytes for any valid node */
> + rc = size_check(4 * sizeof(u32), len, fp);
> + if (rc)
> + return rc;
> +
> p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
> if (!p->cond_list)
> return -ENOMEM;
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 3e4a28a2928b..46c010afd44f 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1100,6 +1100,9 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> if ((len == 0) || (len == (u32)-1))
> return -EINVAL;
>
> + if (size_check(sizeof(char), len, fp))
> + return -EINVAL;
> +
> str = kmalloc(len + 1, flags | __GFP_NOWARN);
> if (!str)
> return -ENOMEM;
> @@ -1174,6 +1177,11 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
> if (nel > SEL_VEC_MAX)
> goto bad;
>
> + /* perm_read() reads at least 64 bytes for any valid permission */
> + rc = size_check(2 * sizeof(u32), nel, fp);
> + if (rc)
> + goto bad;
> +
> rc = symtab_init(&comdatum->permissions, nel);
> if (rc)
> goto bad;
> @@ -1348,6 +1356,11 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
> goto bad;
> cladatum->value = val;
>
> + /* perm_read() reads at least 64 bytes for any valid permission */
> + rc = size_check(2 * sizeof(u32), nel, fp);
> + if (rc)
> + goto bad;
> +
> rc = symtab_init(&cladatum->permissions, nel);
> if (rc)
> goto bad;
> @@ -1921,6 +1934,13 @@ static int range_read(struct policydb *p, struct policy_file *fp)
>
> nel = le32_to_cpu(buf[0]);
>
> + /* we read at least 64 bytes and mls_read_range_helper() 32 bytes
> + * for any valid range-transition
> + */
> + rc = size_check(3 * sizeof(u32), nel, fp);
> + if (rc)
> + return rc;
> +
> rc = hashtab_init(&p->range_tr, nel);
> if (rc)
> return rc;
> @@ -2689,6 +2709,13 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
> nprim = le32_to_cpu(buf[0]);
> nel = le32_to_cpu(buf[1]);
>
> + /* every read_f() implementation reads at least 128 bytes
> + * for any valid entry
> + */
> + rc = size_check(4 * sizeof(u32), nel, fp);
> + if (rc)
> + goto out;
> +
> rc = symtab_init(&p->symtab[i], nel);
> if (rc)
> goto out;
> @@ -2730,6 +2757,11 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
> goto bad;
> nel = le32_to_cpu(buf[0]);
>
> + /* we read at least 96 bytes for any valid role-transition */
> + rc = size_check(3 * sizeof(u32), nel, fp);
> + if (rc)
> + goto bad;
> +
> rc = hashtab_init(&p->role_tr, nel);
> if (rc)
> goto bad;
> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 9b3cc393a979..bb96a6cb6101 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -353,6 +353,19 @@ struct policy_data {
> struct policy_file *fp;
> };
>
> +static inline int size_check(size_t bytes, size_t num, const struct policy_file *fp)
> +{
> + size_t len;
> +
> + if (unlikely(check_mul_overflow(bytes, num, &len)))
> + return -EINVAL;
> +
> + if (unlikely(len > fp->len))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static inline int next_entry(void *buf, struct policy_file *fp, size_t bytes)
> {
> if (bytes > fp->len)
> --
> 2.49.0
>
Powered by blists - more mailing lists