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.
> 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, how about something like this:
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?
--
paul-moore.com
