On Fri, 13 Dec 2024 at 23:14, Daniel Burgener
<dburgener@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On 11/15/2024 8:35 AM, Christian Göttsche wrote:
> > From: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > ---
> > 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?
Yes, I'll change the limit for sensitivities to 32 in v2.
> 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.
It seems the limit of categories a context can have in the total
number of categories defined in the policy, which seems to be U32_MAX
(or one or two less).
But even today in the common policies on can reach this limit, e.g. via
$ for ((j=1; j<1000; j++)); do echo "limit $j";
ctx=system_u:system_r:init_t:s0:c0; for ((i=1; i<j; i++)); do
ctx+=,c$i; done; echo -n $ctx > /sys/fs/selinux/context || j=1000;
done;
For me with more than 834 categories (which have a maximum identifier
length of 4 (c833)) the context hits the page size limit.
So the maximum length of 32 is an attempt to minimize the practical likelihood.
> -Daniel
>
