From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Guy Briggs Subject: Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI Date: Tue, 9 Dec 2014 11:30:14 -0500 Message-ID: <20141209163014.GA29998@madcap2.tricolour.ca> References: <12023790.4atqQrk5y0@sifl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <12023790.4atqQrk5y0@sifl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore Cc: linux-audit@redhat.com, ebiederm@xmission.com List-Id: linux-audit@redhat.com 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 > > --- > > 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@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs 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