[RFC PATCH ghak73 V1] audit: re-structure audit field valid checks

[Ondrej Mosnacek]

On Mon, May 6, 2019 at 4:48 PM Richard Guy Briggs <rgb at redhat.com> wrote:
> Multiple checks were being done in one switch case statement that
> started to cause some redundancies and awkward exceptions.  Separate the
> valid field and op check from the select valid values checks.
>
> Enforce the elimination of meaningless bitwise and greater/lessthan
> checks on string fields and other fields with unrelated scalar values.
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/73
>
> Signed-off-by: Richard Guy Briggs <rgb at redhat.com>
> ---
> Passes audit-testsuite on f30.
> Note: This does not necessarily completely resolve ghak73, but enables
> ghak64.
>
>  kernel/auditfilter.c | 42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 1bc6410413e6..17cfccd9ee27 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -358,9 +358,16 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>                 }
>         }
>
> +       /* Check for valid field type and op */
>         switch(f->type) {
> -       default:
> -               return -EINVAL;
> +       case AUDIT_ARG0:
> +       case AUDIT_ARG1:
> +       case AUDIT_ARG2:
> +       case AUDIT_ARG3:
> +       case AUDIT_PERS: /* <uapi/linux/personality.h> */
> +       case AUDIT_DEVMINOR:
> +               /* all ops are valid */
> +               break;
>         case AUDIT_UID:
>         case AUDIT_EUID:
>         case AUDIT_SUID:
> @@ -373,11 +380,9 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>         case AUDIT_FSGID:
>         case AUDIT_OBJ_GID:
>         case AUDIT_PID:
> -       case AUDIT_PERS:
>         case AUDIT_MSGTYPE:
>         case AUDIT_PPID:
>         case AUDIT_DEVMAJOR:
> -       case AUDIT_DEVMINOR:

The fact that AUDIT_DEVMAJOR and AUDIT_DEVMINOR support different set
of operators irks me a bit, but I understand the motivation (minor
numbers are allocated in a more "ordered" fashion than major numbers),
so I'm neutral about it.

>         case AUDIT_EXIT:
>         case AUDIT_SUCCESS:
>         case AUDIT_INODE:
> @@ -386,10 +391,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>                 if (f->op == Audit_bitmask || f->op == Audit_bittest)
>                         return -EINVAL;
>                 break;
> -       case AUDIT_ARG0:
> -       case AUDIT_ARG1:
> -       case AUDIT_ARG2:
> -       case AUDIT_ARG3:
>         case AUDIT_SUBJ_USER:
>         case AUDIT_SUBJ_ROLE:
>         case AUDIT_SUBJ_TYPE:
> @@ -403,16 +404,28 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>         case AUDIT_WATCH:
>         case AUDIT_DIR:
>         case AUDIT_FILTERKEY:
> -               break;
>         case AUDIT_LOGINUID_SET:
> -               if ((f->val != 0) && (f->val != 1))
> -                       return -EINVAL;
> -       /* FALL THROUGH */
>         case AUDIT_ARCH:
>         case AUDIT_FSTYPE:
> +       case AUDIT_PERM:
> +       case AUDIT_FILETYPE:

Looking at [1], it seems that f->op is actually ignored for AUDIT_PERM
and AUDIT_FILETYPE... This is probably out of scope for this patch,
but maybe AUDIT_PERM should be fixed to properly support bitmask
operators (and the allowed ops should be only bitmask, bittest, equal,
and not_equal) and AUDIT_FILETYPE to distinguish between the equal and
not_equal operator. (Maybe you have already filed an issue for this,
I'm losing track of all the ghaks :)

Other than that the patch looks good to me.

[1] https://elixir.bootlin.com/linux/v5.1/source/kernel/auditsc.c#L685

> +       case AUDIT_FIELD_COMPARE:
> +       case AUDIT_EXE:
> +               /* only equal and not equal valid ops */
>                 if (f->op != Audit_not_equal && f->op != Audit_equal)
>                         return -EINVAL;
>                 break;
> +       default:
> +               /* field not recognized */
> +               return -EINVAL;
> +       }
> +
> +       /* Check for select valid field values */
> +       switch(f->type) {
> +       case AUDIT_LOGINUID_SET:
> +               if ((f->val != 0) && (f->val != 1))
> +                       return -EINVAL;
> +               break;
>         case AUDIT_PERM:
>                 if (f->val & ~15)
>                         return -EINVAL;
> @@ -425,11 +438,10 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>                 if (f->val > AUDIT_MAX_FIELD_COMPARE)
>                         return -EINVAL;
>                 break;
> -       case AUDIT_EXE:
> -               if (f->op != Audit_not_equal && f->op != Audit_equal)
> -                       return -EINVAL;
> +       default:
>                 break;
>         }
> +
>         return 0;
>  }
>
> --
> 1.8.3.1

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.