| 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: <c4416dfa-ed1c-479d-9558-252775f3b8b6@linux.microsoft.com>
Date: Fri, 13 Dec 2024 17:14:03 -0500
From: Daniel Burgener <dburgener@...ux.microsoft.com>
To: cgzones@...glemail.com, selinux@...r.kernel.org
Cc: Paul Moore <paul@...l-moore.com>,
Stephen Smalley <stephen.smalley.work@...il.com>,
Ondrej Mosnacek <omosnace@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 22/22] selinux: restrict policy strings
On 11/15/2024 8:35 AM, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@...glemail.com>
>
> Validate the characters and the lengths of strings parsed from binary
> policies.
>
> * Disallow control characters
> * Limit characters of identifiers to alphanumeric, underscore, dash,
> and dot
> * Limit identifiers in length to 128, expect types to 1024 and
> categories to 32, characters (excluding NUL-terminator)
>
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
> ---
> security/selinux/ss/conditional.c | 2 +-
> security/selinux/ss/policydb.c | 60 ++++++++++++++++++++-----------
> security/selinux/ss/policydb.h | 5 ++-
> 3 files changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index d37b4bdf6ba9..346102417cbf 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -280,7 +280,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
>
> len = le32_to_cpu(buf[2]);
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> if (rc)
> goto err;
>
It would be nice if these limits were named constants instead of magic
numbers. Right now it's hard to tell if all the "128"s are essentially
the same limit referenced in different places, or if they could (in
theory) be changed independently.
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 917b468c5144..d98dfa6c3f30 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1221,8 +1221,9 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
> * binary representation file.
> */
>
> -int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> +int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, int kind, u32 max_len)
> {
> + u32 i;
> int rc;
> char *str;
>
> @@ -1232,19 +1233,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> if (oom_check(sizeof(char), len, fp))
> return -EINVAL;
>
> + if (max_len != 0 && len > max_len)
> + return -EINVAL;
> +
> str = kmalloc(len + 1, flags | __GFP_NOWARN);
> if (!str)
> return -ENOMEM;
>
> rc = next_entry(str, fp, len);
> - if (rc) {
> - kfree(str);
> - return rc;
> + if (rc)
> + goto bad_str;
> +
> + rc = -EINVAL;
> + for (i = 0; i < len; i++) {
> + if (iscntrl(str[i]))
> + goto bad_str;
> +
> + if (kind == STR_IDENTIFIER &&
> + !(isalnum(str[i]) || str[i] == '_' || str[i] == '-' || str[i] == '.'))
> + goto bad_str;
> +
> }
>
> str[len] = '\0';
> *strp = str;
> return 0;
> +
> +bad_str:
> + kfree(str);
> + return rc;
> }
>
> static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
> @@ -1269,7 +1286,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
> if (perdatum->value < 1 || perdatum->value > 32)
> goto bad;
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> if (rc)
> goto bad;
>
> @@ -1315,7 +1332,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
> goto bad;
> comdatum->permissions.nprim = le32_to_cpu(buf[2]);
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> if (rc)
> goto bad;
>
> @@ -1552,12 +1569,12 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>
> ncons = le32_to_cpu(buf[5]);
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> if (rc)
> goto bad;
>
> if (len2) {
> - rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
> + rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2, STR_IDENTIFIER, 128);
> if (rc)
> goto bad;
>
> @@ -1691,7 +1708,7 @@ static int role_read(struct policydb *p, struct symtab *s, struct policy_file *f
> if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
> role->bounds = le32_to_cpu(buf[2]);
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> if (rc)
> goto bad;
>
> @@ -1758,7 +1775,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
> typdatum->primary = le32_to_cpu(buf[2]);
> }
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 1024);
> if (rc)
> goto bad;
>
> @@ -1822,7 +1839,7 @@ static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f
> if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
> usrdatum->bounds = le32_to_cpu(buf[2]);
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> if (rc)
> goto bad;
>
> @@ -1871,7 +1888,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
> goto bad;
> levdatum->isalias = val;
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> if (rc)
> goto bad;
>
> @@ -1914,7 +1931,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
> goto bad;
> catdatum->isalias = val;
>
> - rc = str_read(&key, GFP_KERNEL, fp, len);
> + rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 32);
> if (rc)
> goto bad;
The category restriction is more tight than the sensitivity one because
a context may have many categories? I guess that makes sense, but it
feels counterintuitive from a user perspective, because I feel like
users tend to think of categories and sensitivities as essentially the
same thing. Would dropping the sensitivity limit to 32 to match the
category limit make sense?
Is there a more strict limit on the number of categories a context can
have than the U32_MAX from symtab.nprim? Because that will allow
exceeding the page size using too many categories regardless of length
distinctions, which is a concern if the motivation here is about
potential future untrusted policy loaders in a namespaced environment.
-Daniel
Powered by blists - more mailing lists