Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Paul Moore <pmoore@redhat.com>]

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@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?

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?

* Why are we reusing the AUDIT_NEGATE bit in the type field to indicate a 
legacy userspace?

* 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"?

> 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