* [RFC PATCH ghak73 V1] audit: re-structure audit field valid checks
@ 2019-05-06 14:48 Richard Guy Briggs
2019-05-07 9:23 ` Ondrej Mosnacek
0 siblings, 1 reply; 3+ messages in thread
From: Richard Guy Briggs @ 2019-05-06 14:48 UTC (permalink / raw)
To: Linux-Audit Mailing List; +Cc: Richard Guy Briggs, eparis
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@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:
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:
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH ghak73 V1] audit: re-structure audit field valid checks
2019-05-06 14:48 [RFC PATCH ghak73 V1] audit: re-structure audit field valid checks Richard Guy Briggs
@ 2019-05-07 9:23 ` Ondrej Mosnacek
2019-05-07 11:59 ` Richard Guy Briggs
0 siblings, 1 reply; 3+ messages in thread
From: Ondrej Mosnacek @ 2019-05-07 9:23 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: Linux-Audit Mailing List, Eric Paris
On Mon, May 6, 2019 at 4:48 PM Richard Guy Briggs <rgb@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@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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH ghak73 V1] audit: re-structure audit field valid checks
2019-05-07 9:23 ` Ondrej Mosnacek
@ 2019-05-07 11:59 ` Richard Guy Briggs
0 siblings, 0 replies; 3+ messages in thread
From: Richard Guy Briggs @ 2019-05-07 11:59 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: Linux-Audit Mailing List, Eric Paris
On 2019-05-07 11:23, Ondrej Mosnacek wrote:
> On Mon, May 6, 2019 at 4:48 PM Richard Guy Briggs <rgb@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@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@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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-07 11:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-06 14:48 [RFC PATCH ghak73 V1] audit: re-structure audit field valid checks Richard Guy Briggs
2019-05-07 9:23 ` Ondrej Mosnacek
2019-05-07 11:59 ` Richard Guy Briggs
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.