[PATCH] audit: restore AUDIT_LOGINUID unset ABI
Richard Guy Briggs
rgb at redhat.com
Tue Dec 9 16:30:14 UTC 2014
On 14/12/08, Paul Moore wrote:
> On Wednesday, September 24, 2014 10:07:20 AM Richard Guy Briggs wrote:
> > A regression was caused by commit 780a7654cee8:
> > audit: Make testing for a valid loginuid explicit.
> > (which in turn attempted to fix a regression caused by e1760bd)
> >
> > When audit_krule_to_data() fills in the rules to get a listing, there was a
> > missing clause to convert back from AUDIT_LOGINUID_SET to AUDIT_LOGINUID.
> >
> > This broke userspace by not returning the same information that was sent and
> > expected.
> >
> > The rule:
> > auditctl -a exit,never -F auid=-1
> > gives:
> > auditctl -l
> > LIST_RULES: exit,never f24=0 syscall=all
> > when it should give:
> > LIST_RULES: exit,never auid=-1 (0xffffffff) syscall=all
> >
> > Tag it so that it is reported the same way it was set.
> >
> > Note: move the field validation call ahead of the mutation code to have it
> > work on the original field set.
> >
> > Signed-off-by: Richard Guy Briggs <rgb at redhat.com>
> > ---
> > include/uapi/linux/audit.h | 3 +++
> > kernel/auditfilter.c | 19 +++++++++++++------
> > kernel/auditsc.c | 2 +-
> > 3 files changed, 17 insertions(+), 7 deletions(-)
>
> Sorry for the delays, I'm working on cleaning out the audit bug/patch backlog
> and it took me a little while to get to this topic ... so, this looks a messy
> little problem; Richard, could you help me understand things a bit better?
>
> As I understand it, when old userspace would set a filter with AUDIT_LOGINUID
> but when it listed the audit rules in the kernel it would see
> AUDIT_LOGINUID_SET, yes? This patch attempts to fix this by marking a legacy
> userspace with the AUDIT_LOGINUID_LEGACY bitmask on the internal kernel
> representation so that when the rules are dumped to userspace the
> AUDIT_LOGINUID_SET rule can be rewritten as AUDIT_LOGINUID, yes?
Correct.
> However, there are some things that are not immediately obvious to me:
>
> * Why are we using a bit in audit_field->type to indicate the legacy nature of
> userspace?
Convenience. Adding a new member to audit_field or audit_krule seemed
unnecessary memory overhead (however, it then complicates other code...).
> * Why are we reusing the AUDIT_NEGATE bit in the type field to indicate a
> legacy userspace?
It wasn't reaped when commit 18900909 went through... (first introduced
with original audit in b7b0074c, 2004-04-11). It would have been more
clear if I had sent a first patch to remove AUDIT_NEGATE altogether and
re-introduce it with a new name in this patch.
> * Why are we not using something in audit_krule? Without looking to in depth
> it would appear that there are multiple fields which might be useful, e.g.
> "vers_ops", "flags"?
audit_krule applies to the set of all fields for this rule. I wanted
something that localized it very unambiguously to this one field.
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 4d100c8..860df86 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -274,6 +274,9 @@
> >
> > #define AUDIT_FILTERKEY 210
> >
> > +/* Flag to indicate legacy AUDIT_LOGINUID unset usage */
> > +#define AUDIT_LOGINUID_LEGACY 0x80000000
> > +
> > #define AUDIT_NEGATE 0x80000000
> >
> > /* These are the supported operators.
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 40ed981..39ce3e6 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -438,9 +438,13 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data, f->type = data->fields[i];
> > f->val = data->values[i];
> >
> > + err = audit_field_valid(entry, f);
> > + if (err)
> > + goto exit_free;
> > +
> > /* Support legacy tests for a valid loginuid */
> > if ((f->type == AUDIT_LOGINUID) && (f->val == AUDIT_UID_UNSET)) {
> > - f->type = AUDIT_LOGINUID_SET;
> > + f->type = AUDIT_LOGINUID_SET | AUDIT_LOGINUID_LEGACY;
> > f->val = 0;
> > }
> >
> > @@ -457,10 +461,6 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data, rcu_read_unlock();
> > }
> >
> > - err = audit_field_valid(entry, f);
> > - if (err)
> > - goto exit_free;
> > -
> > err = -EINVAL;
> > switch (f->type) {
> > case AUDIT_LOGINUID:
> > @@ -630,6 +630,13 @@ static struct audit_rule_data
> > *audit_krule_to_data(struct audit_krule *krule) data->buflen +=
> > data->values[i] =
> > audit_pack_string(&bufp, krule->filterkey);
> > break;
> > + case AUDIT_LOGINUID_SET | AUDIT_LOGINUID_LEGACY:
> > + if (!f->val) {
> > + data->fields[i] = AUDIT_LOGINUID;
> > + data->values[i] = AUDIT_UID_UNSET;
> > + break;
> > + }
> > + /* fallthrough if set */
> > default:
> > data->values[i] = f->val;
> > }
> > @@ -1270,7 +1277,7 @@ static int audit_filter_user_rules(struct audit_krule
> > *rule, int type, int result = 0;
> > u32 sid;
> >
> > - switch (f->type) {
> > + switch (f->type & ~AUDIT_LOGINUID_LEGACY) {
> > case AUDIT_PID:
> > pid = task_pid_nr(current);
> > result = audit_comparator(pid, f->op, f->val);
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 8933572..ef25cbc 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -452,7 +452,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> > int result = 0;
> > pid_t pid;
> >
> > - switch (f->type) {
> > + switch (f->type & ~AUDIT_LOGINUID_LEGACY) {
> > case AUDIT_PID:
> > pid = task_pid_nr(tsk);
> > result = audit_comparator(pid, f->op, f->val);
>
> --
> paul moore
> security and virtualization @ redhat
>
> --
> Linux-audit mailing list
> Linux-audit at redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rbriggs at redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
More information about the Linux-audit
mailing list