linux-kernel - Re: [PATCH RFC v2 12/22] selinux: check length fields in policies

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: <fdaab30bbed2fe6565fc78227d17cdfa@paul-moore.com>
Date: Tue, 07 Jan 2025 22:00:07 -0500
From: Paul Moore <paul@...l-moore.com>
To: Christian Göttsche <cgoettsche@...tendoof.de>, selinux@...r.kernel.org
Cc: Christian Göttsche <cgzones@...glemail.com>, Stephen Smalley <stephen.smalley.work@...il.com>, Ondrej Mosnacek <omosnace@...hat.com>, Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, Thiébaud Weksteen <tweek@...gle.com>, Bram Bonné <brambonne@...gle.com>, Masahiro Yamada <masahiroy@...nel.org>, linux-kernel@...r.kernel.org, llvm@...ts.linux.dev, Eric Suen <ericsu@...ux.microsoft.com>
Subject: Re: [PATCH RFC v2 12/22] selinux: check length fields in policies

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@...tendoof.de> wrote:
> 
> 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>
> ---
>  security/selinux/ss/avtab.c       |  4 ++++
>  security/selinux/ss/conditional.c | 14 ++++++++++++++
>  security/selinux/ss/policydb.c    | 23 +++++++++++++++++++++++
>  security/selinux/ss/policydb.h    | 13 +++++++++++++
>  4 files changed, 54 insertions(+)
> 
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 3bd949a200ef..a7bf0ceb45d4 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -550,6 +550,10 @@ int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol)
>  		goto bad;
>  	}
>  
> +	rc = oom_check(2 * 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 35442f4ceedf..de29948efb48 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,10 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
>  	if (len == 0)
>  		return 0;
>  
> +	rc = oom_check(2 * sizeof(u32), len, fp);
> +	if (rc)
> +		return rc;

Magic number, we should make it obvious why '2' is being used, if we
can't do that we should add a comment.

This comment applies several other places in this patch, I'll refrain
from mentioning all of them.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 1275fd7d9148..4bc1e225f2fe 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1174,6 +1177,10 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
>  	if (nel > 32)
>  		goto bad;
>  
> +	rc = oom_check(/*guaranteed read by perm_read()*/2 * sizeof(u32), nel, fp);
> +	if (rc)
> +		goto bad;

Please don't add a comment *inside* code like that, it makes the code
awful to read.

> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 690dc4a00cf3..828fef98e340 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -352,6 +352,19 @@ struct policy_data {
>  	struct policy_file *fp;
>  };
>  
> +static inline int oom_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;
> +}

I'd prefer if we could use a different name than "oom_check()", perhaps
"size_check()"?

--
paul-moore.com