On Wed, 8 Jan 2025 at 04:00, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@xxxxxxxxxxxxx> wrote:
> >
> > Be more strict during parsing of policies and reject invalid values.
> >
> > Add some error messages in the case of policy parse failures, to
> > enhance debugging, either on a malformed policy or a too strict check.
> >
> > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> > ---
> > v2:
> > accept unknown xperm specifiers to support backwards compatibility for
> > future ones, suggested by Thiébaud
> > ---
> > security/selinux/ss/avtab.c | 37 +++++--
> > security/selinux/ss/conditional.c | 18 ++--
> > security/selinux/ss/constraint.h | 2 +-
> > security/selinux/ss/policydb.c | 157 ++++++++++++++++++++++++------
> > security/selinux/ss/policydb.h | 19 +++-
> > security/selinux/ss/services.c | 2 -
> > 6 files changed, 182 insertions(+), 53 deletions(-)
> >
> > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> > index c2c31521cace..3bd949a200ef 100644
> > --- a/security/selinux/ss/avtab.c
> > +++ b/security/selinux/ss/avtab.c
> > @@ -349,7 +349,7 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
> > struct avtab_extended_perms xperms;
> > __le32 buf32[ARRAY_SIZE(xperms.perms.p)];
> > int rc;
> > - unsigned int set, vers = pol->policyvers;
> > + unsigned int vers = pol->policyvers;
> >
> > memset(&key, 0, sizeof(struct avtab_key));
> > memset(&datum, 0, sizeof(struct avtab_datum));
> > @@ -361,8 +361,8 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
> > return rc;
> > }
> > items2 = le32_to_cpu(buf32[0]);
> > - if (items2 > ARRAY_SIZE(buf32)) {
> > - pr_err("SELinux: avtab: entry overflow\n");
> > + if (items2 < 5 || items2 > ARRAY_SIZE(buf32)) {
> > + pr_err("SELinux: avtab: invalid item count\n");
> > return -EINVAL;
> > }
>
> A reminder that magic numbers are a bad thing, if we can't make it clear
> what the '5' in the conditional above represents by using a computed
> value, let's either use a #define with a helpful name or a comment to
> make this a bit more understandable.
>
> > @@ -444,9 +456,13 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
> > return -EINVAL;
> > }
> >
> > - set = hweight16(key.specified & (AVTAB_XPERMS | AVTAB_TYPE | AVTAB_AV));
> > - if (!set || set > 1) {
> > - pr_err("SELinux: avtab: more than one specifier\n");
> > + if (hweight16(key.specified & ~AVTAB_ENABLED) != 1) {
> > + pr_err("SELinux: avtab: not exactly one specifier\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (key.specified & ~(AVTAB_AV | AVTAB_TYPE | AVTAB_XPERMS | AVTAB_ENABLED)) {
> > + pr_err("SELinux: avtab: invalid specifier\n");
> > return -EINVAL;
> > }
>
> Let's define a macro in avtab.h with all of the allowed avtab key
> values, otherwise I think people are going to forget about this check
> when adding a new flag and they are going to get frustrated :)
>
> > @@ -471,6 +487,15 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
> > pr_err("SELinux: avtab: truncated entry\n");
> > return rc;
> > }
> > + switch (xperms.specified) {
> > + case AVTAB_XPERMS_IOCTLFUNCTION:
> > + case AVTAB_XPERMS_IOCTLDRIVER:
> > + case AVTAB_XPERMS_NLMSG:
> > + break;
> > + default:
> > + pr_warn_once_policyload(pol, "SELinux: avtab: unsupported xperm specifier %#x\n",
> > + xperms.specified);
> > + }
>
> Similar to the avtab flags discussion above, can we create a small
> inline function in avtab.h that checks to see if an xperm is valid?
>
> /* feel free to come up with a better name */
> static inline bool avtab_xpermspec_valid(u8 specified)
> {
> if (specified == AVTAB_XPERMS_IOCTLFUNCTION)
> return true;
> elif (...)
> return true;
>
> return false;
> }
>
> > diff --git a/security/selinux/ss/constraint.h b/security/selinux/ss/constraint.h
> > index 203033cfad67..26ffdb8c1c29 100644
> > --- a/security/selinux/ss/constraint.h
> > +++ b/security/selinux/ss/constraint.h
> > @@ -50,7 +50,7 @@ struct constraint_expr {
> > u32 op; /* operator */
> >
> > struct ebitmap names; /* names */
> > - struct type_set *type_names;
> > + struct type_set *type_names; /* (unused) */
>
> If we're not using this field, let's remove it. If for some odd reason
> we need to keep it here for size reasons, or something similar, let's
> turn it into a 'void *unused;' field.
This member (and the one later down this patch) is not used
internally, but forwarded to userspace via policydb_write() to
/sys/fs/selinux/policy.
>
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index eeca470cc90c..1275fd7d9148 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -634,13 +634,11 @@ static int sens_index(void *key, void *datum, void *datap)
> > levdatum = datum;
> > p = datap;
> >
> > - if (!levdatum->isalias) {
> > - if (!levdatum->level.sens ||
> > - levdatum->level.sens > p->p_levels.nprim)
> > - return -EINVAL;
> > + if (!levdatum->level.sens || levdatum->level.sens > p->p_levels.nprim)
> > + return -EINVAL;
> >
> > + if (!levdatum->isalias)
> > p->sym_val_to_name[SYM_LEVELS][levdatum->level.sens - 1] = key;
> > - }
> >
> > return 0;
> > }
>
> Hmm, I don't think the code above does the error checking in the same
> way, [...]
That is the point of this change: to also validate the sensitivities aliases.
>
> int sens_index(...)
> {
> if (isalias)
> return 0;
> if (!level->sens || level->send > levels.nprim)
> return -EINVAL;
> p = ...;
> return 0;
> }
>
> > @@ -653,12 +651,11 @@ static int cat_index(void *key, void *datum, void *datap)
> > catdatum = datum;
> > p = datap;
> >
> > - if (!catdatum->isalias) {
> > - if (!catdatum->value || catdatum->value > p->p_cats.nprim)
> > - return -EINVAL;
> > + if (!catdatum->value || catdatum->value > p->p_cats.nprim)
> > + return -EINVAL;
> >
> > + if (!catdatum->isalias)
> > p->sym_val_to_name[SYM_CATS][catdatum->value - 1] = key;
> > - }
> >
> > return 0;
> > }
>
> Similar to the sensitivity level comment above.
>
> > @@ -1136,6 +1133,9 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
> >
> > len = le32_to_cpu(buf[0]);
> > perdatum->value = le32_to_cpu(buf[1]);
> > + rc = -EINVAL;
> > + if (perdatum->value < 1 || perdatum->value > 32)
> > + goto bad;
>
> More magic number problems.
>
> > rc = str_read(&key, GFP_KERNEL, fp, len);
> > if (rc)
> > @@ -1170,6 +1170,9 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
> > len = le32_to_cpu(buf[0]);
> > comdatum->value = le32_to_cpu(buf[1]);
> > nel = le32_to_cpu(buf[3]);
> > + rc = -EINVAL;
> > + if (nel > 32)
> > + goto bad;
>
> Magic number.
>
> > rc = symtab_init(&comdatum->permissions, nel);
> > if (rc)
> > @@ -1335,6 +1338,9 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
> > len = le32_to_cpu(buf[0]);
> > len2 = le32_to_cpu(buf[1]);
> > nel = le32_to_cpu(buf[4]);
> > + rc = -EINVAL;
> > + if (nel > 32)
> > + goto bad;
>
> Again.
>
> > @@ -1527,7 +1578,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
> > * Read a MLS level structure from a policydb binary
> > * representation file.
> > */
> > -static int mls_read_level(struct mls_level *lp, struct policy_file *fp)
> > +static int mls_read_level(const struct policydb *p, struct mls_level *lp, struct policy_file *fp)
> > {
> > __le32 buf[1];
> > int rc;
>
> Why is this here? You don't use the @p parameter anywhere in this
> patch and it add some code churn in all of the callers.
>
> > @@ -1606,7 +1657,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
> > struct level_datum *levdatum;
> > int rc;
> > __le32 buf[2];
> > - u32 len;
> > + u32 len, val;
> >
> > levdatum = kzalloc(sizeof(*levdatum), GFP_KERNEL);
> > if (!levdatum)
> > @@ -1617,13 +1668,17 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
> > goto bad;
> >
> > len = le32_to_cpu(buf[0]);
> > - levdatum->isalias = le32_to_cpu(buf[1]);
> > + val = le32_to_cpu(buf[1]);
> > + rc = -EINVAL;
> > + if (val != 0 && val != 1)
> > + goto bad;
> > + levdatum->isalias = val;
>
> Should we have a simple inline function to do the integer boolean check?
>
> Considering all the places we check for 0 and 1, it seems like it might
> be a bit cleaner, and would help with self-documenting.
>
> > @@ -2221,7 +2303,7 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
> >
> > rc = -EINVAL;
> > val = le32_to_cpu(buf[0]);
> > - if (val >= U16_MAX)
> > + if (val >= U16_MAX || (val != 0 && !policydb_class_isvalid(p, val)))
> > goto out;
> > newc->v.sclass = val;
> > rc = context_read_and_validate(&newc->context[0], p,
>
> This should probably be in patch 10/22, yes?
>
> > @@ -110,15 +110,15 @@ struct role_allow {
> > /* Type attributes */
> > struct type_datum {
> > u32 value; /* internal type value */
> > - u32 bounds; /* boundary of type */
> > - unsigned char primary; /* primary name? */
> > + u32 bounds; /* boundary of type, 0 for none */
> > + unsigned char primary; /* primary name? (unused) */
>
> See my previous comment about unused fields.
>
> > #endif /* _SS_POLICYDB_H_ */
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 28c0bdf9fc9d..d5725c768d59 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -445,8 +445,6 @@ static int dump_masked_av_helper(void *k, void *d, void *args)
> > struct perm_datum *pdatum = d;
> > char **permission_names = args;
> >
> > - BUG_ON(pdatum->value < 1 || pdatum->value > 32);
>
> Do we need to convert this to a if-then check that does the proper error
> handling, or is it already handled in the other changes in this patch?
perm_read() now performs this check at policyload time.
>
> --
> paul-moore.com
