| 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: <CAP+JOzQpWav+a-DmA3Sh22JAPHmX0U9HMvNqpW-LU9sGj-9dbA@mail.gmail.com>
Date: Wed, 14 May 2025 16:02:19 -0400
From: James Carter <jwcart2@...il.com>
To: Stephen Smalley <stephen.smalley.work@...il.com>
Cc: cgzones@...glemail.com, selinux@...r.kernel.org,
Paul Moore <paul@...l-moore.com>, Ondrej Mosnacek <omosnace@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 13/14] selinux: restrict policy strings
On Wed, May 14, 2025 at 3:40 PM Stephen Smalley
<stephen.smalley.work@...il.com> wrote:
>
> On Sun, May 11, 2025 at 1:31 PM Christian Göttsche
> <cgoettsche@...tendoof.de> 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 64, expect types to 1024,
> > sensitivities to 32 and categories to 16, characters
> > (excluding NUL-terminator)
>
> (added James Carter to explicit cc for comparison with any
> userspace-imposed restrictions)
>
> I think we could easily go lower than 1024 characters for type names.
>
CIL has a limit of 2,048 for identifiers. Checkpolicy has a limit of 8,192.
We definitely could have lower limits. I think that I would rather
have one limit rather than a bunch of different limits, but it
wouldn't be too hard to do different limits if there is a good reason
to do that.
Even a limit of 256 would seem to be sufficient (at least until AI takes over).
Jim
> >
> > Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
> > ---
> > v3:
> > - introduce a central limits.h header
> > - add limits for all kinds of string: filesystem names, filetrans
> > keys, genfs paths, infiniband device names
> > v2:
> > - add wrappers for str_read() to minimize the usage of magic numbers
> > - limit sensitivities to a length of 32, to match categories,
> > suggested by Daniel
> > ---
> > security/selinux/include/limits.h | 90 +++++++++++++++++++++++++++++++
> > security/selinux/ss/conditional.c | 5 +-
> > security/selinux/ss/conditional.h | 2 -
> > security/selinux/ss/constraint.h | 2 -
> > security/selinux/ss/policydb.c | 78 ++++++++++++++++++---------
> > security/selinux/ss/policydb.h | 51 +++++++++++++++++-
> > 6 files changed, 196 insertions(+), 32 deletions(-)
> > create mode 100644 security/selinux/include/limits.h
> >
> > diff --git a/security/selinux/include/limits.h b/security/selinux/include/limits.h
> > new file mode 100644
> > index 000000000000..d267c0c64f49
> > --- /dev/null
> > +++ b/security/selinux/include/limits.h
> > @@ -0,0 +1,90 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Limits for various policy database elements.
> > + */
> > +
> > +/*
> > + * Maximum supported depth of conditional expressions.
> > + */
> > +#define COND_EXPR_MAXDEPTH 10
> > +
> > +/*
> > + * Maximum supported depth for constraint expressions.
> > + */
> > +#define CEXPR_MAXDEPTH 5
> > +
> > +/*
> > + * Maximum supported identifier value.
> > + *
> > + * Reasoning: The most used symbols are types and they need to fit into
> > + * an u16 for the avtab entries. Keep U16_MAX as special value
> > + * and U16_MAX-1 to avoid accidental overflows into U16_MAX.
>
> This seems rather arbitrary and unnecessary to me? Unless userspace
> does the same?
>
> > + */
> > +#define IDENTIFIER_MAXVALUE (U16_MAX - 2)
> > +
> > +/*
> > + * Maximum supported length of security context strings.
> > + *
> > + * Reasoning: The string must fir into a PAGE_SIZE.
>
> s/fir/fit/
> s/into a/under/
>
> > + */
> > +#define CONTEXT_MAXLENGTH 4000
>
> Any particular reason to not just make it PAGE_SIZE then?
>
> > +
> > +/*
> > + * Maximum supported boolean name length.
> > + */
> > +#define BOOLEAN_NAME_MAXLENGTH 64
> > +
> > +/*
> > + * Maximum supported security class and common class name length.
> > + */
> > +#define CLASS_NAME_MAXLENGTH 64
> > +
> > +/*
> > + * Maximum supported permission name length.
> > + */
> > +#define PERMISSION_NAME_MAXLENGTH 64
> > +
> > +/*
> > + * Maximum supported user name length.
> > + */
> > +#define USER_NAME_MAXLENGTH 64
> > +
> > +/*
> > + * Maximum supported role name length.
> > + */
> > +#define ROLE_NAME_MAXLENGTH 64
> > +
> > +/*
> > + * Maximum supported type name length.
> > + */
> > +#define TYPE_NAME_MAXLENGTH 1024
>
> Would advocate for a lower limit unless we know of a policy that would
> exceed it.
>
> > +
> > +/*
> > + * Maximum supported sensitivity name length.
> > + */
> > +#define SENSITIVITY_NAME_MAXLENGTH 32
> > +
> > +/*
> > + * Maximum supported category name length.
> > + */
> > +#define CATEGORY_NAME_MAXLENGTH 16
> > +
> > +/*
> > + * Maximum supported path name length for keys in filename transitions.
> > + */
> > +#define FILETRANSKEY_NAME_MAXLENGTH 1024
>
> These are component names only, right, not multi-component pathnames?
> In that case open to lower limit or using something defined by fs layer.
>
> > +
> > +/*
> > + * Maximum supported filesystem name length.
>
> s/filesystem/filesystem type/
>
> > + */
> > +#define FILESYSTEM_NAME_MAXLENGTH 128
>
> If we can find a limit imposed by the kernel elsewhere for fstype
> names, we should just reuse that.
>
> > +
> > +/*
> > + * Maximum supported path prefix length for genfs statements.
> > + */
> > +#define GENFS_PATH_MAXLENGTH 1024
>
> Should just use PATH_MAX or similar definition from elsewhere.
>
> > +
> > +/*
> > + * Maximum supported Infiniband device name length.
> > + */
> > +#define INFINIBAND_DEVNAME_MAXLENGTH 256
>
> Would use a limit from infiniband subsystem if one exists.
>
> > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> > index ce0281cce739..c0a2814dafdb 100644
> > --- a/security/selinux/ss/conditional.c
> > +++ b/security/selinux/ss/conditional.c
> > @@ -245,7 +245,8 @@ int cond_index_bool(void *key, void *datum, void *datap)
> > booldatum = datum;
> > p = datap;
> >
> > - if (!booldatum->value || booldatum->value > p->p_bools.nprim)
> > + if (!booldatum->value || booldatum->value > p->p_bools.nprim ||
> > + booldatum->value > IDENTIFIER_MAXVALUE)
> > return -EINVAL;
> >
> > p->sym_val_to_name[SYM_BOOLS][booldatum->value - 1] = key;
> > @@ -280,7 +281,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_bool(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto err;
> >
> > diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> > index 468e98ad3ea1..d5aefcbaa1eb 100644
> > --- a/security/selinux/ss/conditional.h
> > +++ b/security/selinux/ss/conditional.h
> > @@ -12,8 +12,6 @@
> > #include "policydb.h"
> > #include "../include/conditional.h"
> >
> > -#define COND_EXPR_MAXDEPTH 10
> > -
> > /*
> > * A conditional expression is a list of operators and operands
> > * in reverse polish notation.
> > diff --git a/security/selinux/ss/constraint.h b/security/selinux/ss/constraint.h
> > index 1d75a8a044df..f986156de856 100644
> > --- a/security/selinux/ss/constraint.h
> > +++ b/security/selinux/ss/constraint.h
> > @@ -19,8 +19,6 @@
> >
> > #include "ebitmap.h"
> >
> > -#define CEXPR_MAXDEPTH 5
> > -
> > struct constraint_expr {
> > #define CEXPR_NOT 1 /* not expr */
> > #define CEXPR_AND 2 /* expr and expr */
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index 2b098d9abf17..e64254985762 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -552,7 +552,8 @@ static int common_index(void *key, void *datum, void *datap)
> >
> > comdatum = datum;
> > p = datap;
> > - if (!comdatum->value || comdatum->value > p->p_commons.nprim)
> > + if (!comdatum->value || comdatum->value > p->p_commons.nprim ||
> > + comdatum->value > IDENTIFIER_MAXVALUE)
> > return -EINVAL;
> >
> > p->sym_val_to_name[SYM_COMMONS][comdatum->value - 1] = key;
> > @@ -567,7 +568,8 @@ static int class_index(void *key, void *datum, void *datap)
> >
> > cladatum = datum;
> > p = datap;
> > - if (!cladatum->value || cladatum->value > p->p_classes.nprim)
> > + if (!cladatum->value || cladatum->value > p->p_classes.nprim ||
> > + cladatum->value > IDENTIFIER_MAXVALUE)
> > return -EINVAL;
> >
> > p->sym_val_to_name[SYM_CLASSES][cladatum->value - 1] = key;
> > @@ -583,6 +585,7 @@ static int role_index(void *key, void *datum, void *datap)
> > role = datum;
> > p = datap;
> > if (!role->value || role->value > p->p_roles.nprim ||
> > + role->value > IDENTIFIER_MAXVALUE ||
> > role->bounds > p->p_roles.nprim)
> > return -EINVAL;
> >
> > @@ -601,6 +604,7 @@ static int type_index(void *key, void *datum, void *datap)
> >
> > if (typdatum->primary) {
> > if (!typdatum->value || typdatum->value > p->p_types.nprim ||
> > + typdatum->value > IDENTIFIER_MAXVALUE ||
> > typdatum->bounds > p->p_types.nprim)
> > return -EINVAL;
> > p->sym_val_to_name[SYM_TYPES][typdatum->value - 1] = key;
> > @@ -618,6 +622,7 @@ static int user_index(void *key, void *datum, void *datap)
> > usrdatum = datum;
> > p = datap;
> > if (!usrdatum->value || usrdatum->value > p->p_users.nprim ||
> > + usrdatum->value > IDENTIFIER_MAXVALUE ||
> > usrdatum->bounds > p->p_users.nprim)
> > return -EINVAL;
> >
> > @@ -634,7 +639,8 @@ static int sens_index(void *key, void *datum, void *datap)
> > levdatum = datum;
> > p = datap;
> >
> > - if (!levdatum->level.sens || levdatum->level.sens > p->p_levels.nprim)
> > + if (!levdatum->level.sens || levdatum->level.sens > p->p_levels.nprim ||
> > + levdatum->level.sens > IDENTIFIER_MAXVALUE)
> > return -EINVAL;
> >
> > if (!levdatum->isalias)
> > @@ -651,7 +657,8 @@ static int cat_index(void *key, void *datum, void *datap)
> > catdatum = datum;
> > p = datap;
> >
> > - if (!catdatum->value || catdatum->value > p->p_cats.nprim)
> > + if (!catdatum->value || catdatum->value > p->p_cats.nprim ||
> > + catdatum->value > IDENTIFIER_MAXVALUE)
> > return -EINVAL;
> >
> > if (!catdatum->isalias)
> > @@ -1226,8 +1233,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;
> >
> > @@ -1237,19 +1245,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > if (size_check(sizeof(char), len, fp))
> > return -EINVAL;
> >
> > + if (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)
> > @@ -1274,7 +1298,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
> > if (perdatum->value < 1 || perdatum->value > SEL_VEC_MAX)
> > goto bad;
> >
> > - rc = str_read(&key, GFP_KERNEL, fp, len);
> > + rc = str_read_perm(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > @@ -1321,7 +1345,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_class(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > @@ -1559,12 +1583,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_class(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > if (len2) {
> > - rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
> > + rc = str_read_class(&cladatum->comkey, GFP_KERNEL, fp, len2);
> > if (rc)
> > goto bad;
> >
> > @@ -1698,7 +1722,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_role(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > @@ -1765,7 +1789,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_type(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > @@ -1829,7 +1853,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_user(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > @@ -1878,7 +1902,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_sens(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > @@ -1921,7 +1945,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_cat(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > goto bad;
> >
> > @@ -2230,7 +2254,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
> > len = le32_to_cpu(buf[0]);
> >
> > /* path component string */
> > - rc = str_read(&name, GFP_KERNEL, fp, len);
> > + rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, FILETRANSKEY_NAME_MAXLENGTH);
> > if (rc)
> > return rc;
> >
> > @@ -2329,7 +2353,7 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
> > len = le32_to_cpu(buf[0]);
> >
> > /* path component string */
> > - rc = str_read(&name, GFP_KERNEL, fp, len);
> > + rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, FILETRANSKEY_NAME_MAXLENGTH);
> > if (rc)
> > return rc;
> >
> > @@ -2483,7 +2507,7 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
> > if (!newgenfs)
> > goto out;
> >
> > - rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len);
> > + rc = str_read_fsname(&newgenfs->fstype, GFP_KERNEL, fp, len);
> > if (rc)
> > goto out;
> >
> > @@ -2522,7 +2546,8 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
> > if (!newc)
> > goto out;
> >
> > - rc = str_read(&newc->u.name, GFP_KERNEL, fp, len);
> > + rc = str_read(&newc->u.name, GFP_KERNEL, fp, len,
> > + STR_UNCONSTRAINT, GENFS_PATH_MAXLENGTH);
> > if (rc)
> > goto out;
> >
> > @@ -2625,7 +2650,7 @@ static int ocontext_read(struct policydb *p,
> > goto out;
> > len = le32_to_cpu(buf[0]);
> >
> > - rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
> > + rc = str_read_fsname(&c->u.name, GFP_KERNEL, fp, len);
> > if (rc)
> > goto out;
> >
> > @@ -2693,7 +2718,7 @@ static int ocontext_read(struct policydb *p,
> > goto out;
> >
> > len = le32_to_cpu(buf[1]);
> > - rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
> > + rc = str_read_fsname(&c->u.name, GFP_KERNEL, fp, len);
> > if (rc)
> > goto out;
> >
> > @@ -2759,7 +2784,9 @@ static int ocontext_read(struct policydb *p,
> > len = le32_to_cpu(buf[0]);
> >
> > rc = str_read(&c->u.ibendport.dev_name,
> > - GFP_KERNEL, fp, len);
> > + GFP_KERNEL, fp, len,
> > + STR_UNCONSTRAINT,
> > + INFINIBAND_DEVNAME_MAXLENGTH);
> > if (rc)
> > goto out;
> >
> > @@ -2827,7 +2854,8 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
> > goto bad;
> > }
> >
> > - rc = str_read(&policydb_str, GFP_KERNEL, fp, len);
> > + rc = str_read(&policydb_str, GFP_KERNEL, fp, len,
> > + STR_UNCONSTRAINT, strlen(POLICYDB_STRING));
> > if (rc) {
> > if (rc == -ENOMEM) {
> > pr_err("SELinux: unable to allocate memory for policydb string of length %d\n",
> > diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> > index b4f0c1a754cf..e901ec648cbf 100644
> > --- a/security/selinux/ss/policydb.h
> > +++ b/security/selinux/ss/policydb.h
> > @@ -27,6 +27,7 @@
> > #include "mls_types.h"
> > #include "context.h"
> > #include "constraint.h"
> > +#include "limits.h"
> >
> > /*
> > * A datum type is defined for each kind of symbol
> > @@ -408,7 +409,55 @@ static inline bool val_is_boolean(u32 value)
> > return value == 0 || value == 1;
> > }
> >
> > -extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len);
> > +#define STR_UNCONSTRAINT 0
> > +#define STR_IDENTIFIER 1
> > +extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len,
> > + int kind, u32 max_len);
> > +
> > +static inline int str_read_bool(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, BOOLEAN_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_cat(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, CATEGORY_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_class(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, CLASS_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_perm(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, PERMISSION_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_role(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, ROLE_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_sens(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, SENSITIVITY_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_type(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, TYPE_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_user(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, USER_NAME_MAXLENGTH);
> > +}
> > +
> > +static inline int str_read_fsname(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +{
> > + return str_read(strp, flags, fp, len, STR_IDENTIFIER, FILESYSTEM_NAME_MAXLENGTH);
> > +}
> >
> > extern u16 string_to_security_class(struct policydb *p, const char *name);
> > extern u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name);
> > --
> > 2.49.0
> >
Powered by blists - more mailing lists