[PATCH] audit: restore AUDIT_LOGINUID unset ABI

[PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Richard Guy Briggs]

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(-)

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);
-- 
1.7.1

Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Richard Guy Briggs]

On 14/09/24, 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)

Eric (Paris),  does tagging this field in the type member with a high
bit work for you?  It only has to be filtered out for
audit_filter_user_rules() and audit_filter_rules() to avoid changing the
normal semantics of that field.  I'd rather do this than add a new
member to the field struct.

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

I intend to add a Cc: stable 3.10-rc1 here.

> 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(-)
> 
> 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);
> -- 
> 1.7.1
> 

- RGB

--
Richard Guy Briggs <rbriggs@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

Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Paul Moore]

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

Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Richard Guy Briggs]

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@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@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@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

Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Paul Moore]

On Tuesday, December 09, 2014 11:30:14 AM Richard Guy Briggs wrote:
> On 14/12/08, Paul Moore wrote:
> > 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.

The problem is that AUDIT_NEGATE lives in the userspace visible header file 
which means it needs to live there for pretty much forever.  While I would 
like to see us remote it for clarity's sake, I think we're stuck with it.

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

You can only add or delete rules, right?  Not modify?  If you can only add or 
delete a rule, then if one of the fields in that rule is sent from legacy 
userspace I think it is safe to set an indicator in one of the audit_krule 
fields.  I understand your point, but I'm not sure it is something to worry 
too much about; I'd rather see the legacy indicator here than in the 
audit_field->type field where we might have to contend with userspace usage at 
some point.

I'd like to explore the idea of not using audit_field->type; I picked 
"vers_ops" and "flags" since they seemed like reasonable places to start.  The 
"vers_ops" field in particular appears to be almost unused in the current code 
and it seems like a good way to track userspace versions perhaps, e.g. 1 = 
legacy, 2 = now current, etc.?  I'm curious if this sounds reasonable to you.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Richard Guy Briggs]

On 14/12/09, Paul Moore wrote:
> On Tuesday, December 09, 2014 11:30:14 AM Richard Guy Briggs wrote:
> > On 14/12/08, Paul Moore wrote:
> > > 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.
> 
> The problem is that AUDIT_NEGATE lives in the userspace visible header file 
> which means it needs to live there for pretty much forever.  While I would 
> like to see us remote it for clarity's sake, I think we're stuck with it.

Ok, fair enough, the same goes for AUDIT_{LIST,ADD,DEL} since they are
no longer used, but still remain in the API.  I can use another value.

> > > * 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.
> 
> You can only add or delete rules, right?  Not modify?  If you can only add or 
> delete a rule, then if one of the fields in that rule is sent from legacy 
> userspace I think it is safe to set an indicator in one of the audit_krule 
> fields.  I understand your point, but I'm not sure it is something to worry 
> too much about; I'd rather see the legacy indicator here than in the 
> audit_field->type field where we might have to contend with userspace usage at 
> some point.
> 
> I'd like to explore the idea of not using audit_field->type; I picked 
> "vers_ops" and "flags" since they seemed like reasonable places to start.  The 
> "vers_ops" field in particular appears to be almost unused in the current code 
> and it seems like a good way to track userspace versions perhaps, e.g. 1 = 
> legacy, 2 = now current, etc.?  I'm curious if this sounds reasonable to you.

vers_ops appears to be assigned, copied and never otherwise read.  And
in fact *that* should have been removed with commit 18900909.  I'll just
prepare a patch to rip it out.

flags looks like a better choice...  and I'll have to do some similar
filtering that I did for type and as has been already done for
AUDIT_FILTER_PREPEND...  In fact, the inverse of AUDIT_FILTER_PREPEND.
And this patch is simpler than the one you are critiquing.  Bonus.

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@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

[PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Richard Guy Briggs]

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

Cc: stable@vger.kernel.org # v3.10-rc1+
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditfilter.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 40ed981..d0715a7 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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:
+			if (!f->val) {
+				data->fields[i] = AUDIT_LOGINUID;
+				data->values[i] = AUDIT_UID_UNSET;
+				break;
+			}
+			/* fallthrough if set */
 		default:
 			data->values[i] = f->val;
 		}
-- 
1.7.1