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

[Richard Guy Briggs]

On 2019-05-07 11:23, Ondrej Mosnacek wrote:
> 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.

Agreed it felt a bit weird, but figured the ordered nature could justify
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 :)

Good catch.  WATCH, DIR and EXE also fall into this same issue.  Is it
worth addressing audit_watch_perm(), audit_watch_filetype(),
audit_watch_compare(), audit_tree_refs() in the same manner as
audit_exe_compare() has been addressed?

> 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.

- RGB

--
Richard Guy Briggs <rgb at redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635