On Mon, Apr 15, 2024 at 12:53 PM Christian Göttsche
<cgoettsche@xxxxxxxxxxxxx> wrote:
>
> From: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
>
> Validate the symbol tables for permissions of security classes and
> common classes:
> * check their value is valid
> * check their values are unique
> * check permission values of classes do not reuse values from
> inherited permissions
>
> This simplifies validating permissions of access vectors a lot, since it
> is now only a binary and against the valid permission mask of the class.
>
> Use UINT32_MAX instead of 0 as the special value for validating
> constraints signaling a validate-trans rule, since classes with no
> permissions are permitted, but they must not have a normal constraint
> attached.
>
> Reported-by: oss-fuzz (issue 67893)
> Improves: 8c64e5bb6fe7 ("libsepol: validate access vector permissions")
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> v2:
> - move check independent of individual constraints out of the loop
> - change nperms parameter type of validate_constraint_nodes() from
> unsigned int to uint32_t
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
Acked-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> libsepol/src/policydb_validate.c | 104 ++++++++++++++++++++-----------
> 1 file changed, 68 insertions(+), 36 deletions(-)
>
> diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
> index c4f8c300..e1623172 100644
> --- a/libsepol/src/policydb_validate.c
> +++ b/libsepol/src/policydb_validate.c
> @@ -11,6 +11,7 @@
>
> #define bool_xor(a, b) (!(a) != !(b))
> #define bool_xnor(a, b) (!bool_xor(a, b))
> +#define PERMISSION_MASK(nprim) ((nprim) == PERM_SYMTAB_SIZE ? (~UINT32_C(0)) : ((UINT32_C(1) << (nprim)) - 1))
>
> typedef struct validate {
> uint32_t nprim;
> @@ -23,6 +24,12 @@ typedef struct map_arg {
> const policydb_t *policy;
> } map_arg_t;
>
> +typedef struct perm_arg {
> + uint32_t visited;
> + const uint32_t nprim;
> + const uint32_t inherited_nprim;
> +} perm_arg_t;
> +
> static int create_gap_ebitmap(char **val_to_name, uint32_t nprim, ebitmap_t *gaps)
> {
> uint32_t i;
> @@ -227,17 +234,21 @@ bad:
> return -1;
> }
>
> -static int validate_constraint_nodes(sepol_handle_t *handle, unsigned int nperms, const constraint_node_t *cons, validate_t flavors[])
> +static int validate_constraint_nodes(sepol_handle_t *handle, uint32_t nperms, const constraint_node_t *cons, validate_t flavors[])
> {
> const constraint_expr_t *cexp;
> + const int is_validatetrans = (nperms == UINT32_MAX);
> int depth;
>
> + if (cons && nperms == 0)
> + goto bad;
> +
> for (; cons; cons = cons->next) {
> - if (nperms == 0 && cons->permissions != 0)
> + if (is_validatetrans && cons->permissions != 0)
> goto bad;
> - if (nperms > 0 && cons->permissions == 0)
> + if (!is_validatetrans && cons->permissions == 0)
> goto bad;
> - if (nperms > 0 && nperms != PERM_SYMTAB_SIZE && cons->permissions >= (UINT32_C(1) << nperms))
> + if (!is_validatetrans && nperms != PERM_SYMTAB_SIZE && cons->permissions >= (UINT32_C(1) << nperms))
> goto bad;
>
> if (!cons->expr)
> @@ -251,7 +262,7 @@ static int validate_constraint_nodes(sepol_handle_t *handle, unsigned int nperms
> goto bad;
> depth++;
>
> - if (cexp->attr & CEXPR_XTARGET && nperms != 0)
> + if (cexp->attr & CEXPR_XTARGET && !is_validatetrans)
> goto bad;
> if (!(cexp->attr & CEXPR_TYPE)) {
> if (validate_empty_type_set(cexp->type_names))
> @@ -366,11 +377,49 @@ bad:
> return -1;
> }
>
> +static int perm_visit(__attribute__((__unused__)) hashtab_key_t k, hashtab_datum_t d, void *args)
> +{
> + perm_arg_t *pargs = args;
> + const perm_datum_t *perdatum = d;
> +
> + if (!value_isvalid(perdatum->s.value, pargs->nprim))
> + return -1;
> +
> + if (pargs->inherited_nprim != 0 && value_isvalid(perdatum->s.value, pargs->inherited_nprim))
> + return -1;
> +
> + if ((UINT32_C(1) << (perdatum->s.value - 1)) & pargs->visited)
> + return -1;
> +
> + pargs->visited |= (UINT32_C(1) << (perdatum->s.value - 1));
> + return 0;
> +}
> +
> +static int validate_permission_symtab(sepol_handle_t *handle, const symtab_t *permissions, uint32_t inherited_nprim)
> +{
> + /* Check each entry has a different valid value and is not overriding an inherited one */
> +
> + perm_arg_t pargs = { .visited = 0, .nprim = permissions->nprim, .inherited_nprim = inherited_nprim };
> +
> + if (hashtab_map(permissions->table, perm_visit, &pargs))
> + goto bad;
> +
> + return 0;
> +
> +bad:
> + ERR(handle, "Invalid permission table");
> + return -1;
> +}
> +
> static int validate_common_datum(sepol_handle_t *handle, const common_datum_t *common, validate_t flavors[])
> {
> if (validate_value(common->s.value, &flavors[SYM_COMMONS]))
> goto bad;
> - if (common->permissions.table->nel == 0 || common->permissions.nprim > PERM_SYMTAB_SIZE)
> + if (common->permissions.nprim == 0 || common->permissions.nprim > PERM_SYMTAB_SIZE)
> + goto bad;
> + if (common->permissions.nprim != common->permissions.table->nel)
> + goto bad;
> + if (validate_permission_symtab(handle, &common->permissions, 0))
> goto bad;
>
> return 0;
> @@ -393,11 +442,17 @@ static int validate_class_datum(sepol_handle_t *handle, const class_datum_t *cla
> goto bad;
> if (class->comdatum && validate_common_datum(handle, class->comdatum, flavors))
> goto bad;
> - if (class->permissions.nprim > PERM_SYMTAB_SIZE)
> + /* empty classes are permitted */
> + if (class->permissions.nprim > PERM_SYMTAB_SIZE || class->permissions.table->nel > PERM_SYMTAB_SIZE)
> + goto bad;
> + if (class->permissions.nprim !=
> + (class->permissions.table->nel + (class->comdatum ? class->comdatum->permissions.table->nel : 0)))
> + goto bad;
> + if (validate_permission_symtab(handle, &class->permissions, class->comdatum ? class->comdatum->permissions.nprim : 0))
> goto bad;
> if (validate_constraint_nodes(handle, class->permissions.nprim, class->constraints, flavors))
> goto bad;
> - if (validate_constraint_nodes(handle, 0, class->validatetrans, flavors))
> + if (validate_constraint_nodes(handle, UINT32_MAX, class->validatetrans, flavors))
> goto bad;
>
> switch (class->default_user) {
> @@ -877,46 +932,23 @@ bad:
> return -1;
> }
>
> -static int perm_match(__attribute__ ((unused)) hashtab_key_t key, hashtab_datum_t datum, void *data)
> -{
> - const uint32_t *v = data;
> - const perm_datum_t *perdatum = datum;
> -
> - return *v == perdatum->s.value;
> -}
> -
> static int validate_access_vector(sepol_handle_t *handle, const policydb_t *p, sepol_security_class_t tclass,
> sepol_access_vector_t av)
> {
> const class_datum_t *cladatum = p->class_val_to_struct[tclass - 1];
> - uint32_t i;
>
> /*
> * Check that at least one permission bit is valid.
> * Older compilers might set invalid bits for the wildcard permission.
> */
> - for (i = 0; i < cladatum->permissions.nprim; i++) {
> - if (av & (UINT32_C(1) << i)) {
> - uint32_t v = i + 1;
> - int rc;
> -
> - rc = hashtab_map(cladatum->permissions.table, perm_match, &v);
> - if (rc == 1)
> - goto good;
> -
> - if (cladatum->comdatum) {
> - rc = hashtab_map(cladatum->comdatum->permissions.table, perm_match, &v);
> - if (rc == 1)
> - goto good;
> - }
> - }
> - }
> + if (!(av & PERMISSION_MASK(cladatum->permissions.nprim)))
> + goto bad;
>
> + return 0;
> +
> +bad:
> ERR(handle, "Invalid access vector");
> return -1;
> -
> -good:
> - return 0;
> }
>
> static int validate_avtab_key_and_datum(avtab_key_t *k, avtab_datum_t *d, void *args)
> --
> 2.43.0
>
>
