[PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

audit rule additions containing "-F auid!=4294967295" were failing with EINVAL.

UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting and
testing against audit rules.  Remove the check for invalid uid and gid when
parsing rules and data for logging.

Revert part of ca57ec0f00c3f139c41bf6b0a5b9bcc95bbb2ad7 (2012-09-11) to fix
this.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditfilter.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f9fc54b..457ee39 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -360,10 +360,7 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
 			/* bit ops not implemented for uid comparisons */
 			if (f->op == Audit_bitmask || f->op == Audit_bittest)
 				goto exit_free;
-
 			f->uid = make_kuid(current_user_ns(), f->val);
-			if (!uid_valid(f->uid))
-				goto exit_free;
 			break;
 		case AUDIT_GID:
 		case AUDIT_EGID:
@@ -372,10 +369,7 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
 			/* bit ops not implemented for gid comparisons */
 			if (f->op == Audit_bitmask || f->op == Audit_bittest)
 				goto exit_free;
-
 			f->gid = make_kgid(current_user_ns(), f->val);
-			if (!gid_valid(f->gid))
-				goto exit_free;
 			break;
 		case AUDIT_PID:
 		case AUDIT_PERS:
@@ -469,10 +463,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			/* bit ops not implemented for uid comparisons */
 			if (f->op == Audit_bitmask || f->op == Audit_bittest)
 				goto exit_free;
-
 			f->uid = make_kuid(current_user_ns(), f->val);
-			if (!uid_valid(f->uid))
-				goto exit_free;
 			break;
 		case AUDIT_GID:
 		case AUDIT_EGID:
@@ -482,10 +473,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			/* bit ops not implemented for gid comparisons */
 			if (f->op == Audit_bitmask || f->op == Audit_bittest)
 				goto exit_free;
-
 			f->gid = make_kgid(current_user_ns(), f->val);
-			if (!gid_valid(f->gid))
-				goto exit_free;
 			break;
 		case AUDIT_PID:
 		case AUDIT_PERS:
-- 
1.7.1

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Andrew Morton]

On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> wrote:

> audit rule additions containing "-F auid!=4294967295" were failing with EINVAL.
> 
> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting and
> testing against audit rules.  Remove the check for invalid uid and gid when
> parsing rules and data for logging.
> 
> Revert part of ca57ec0f00c3f139c41bf6b0a5b9bcc95bbb2ad7 (2012-09-11) to fix
> this.

Eric, can you please take a look?

> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/auditfilter.c |   12 ------------
>  1 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index f9fc54b..457ee39 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -360,10 +360,7 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>  			/* bit ops not implemented for uid comparisons */
>  			if (f->op == Audit_bitmask || f->op == Audit_bittest)
>  				goto exit_free;
> -
>  			f->uid = make_kuid(current_user_ns(), f->val);
> -			if (!uid_valid(f->uid))
> -				goto exit_free;

It concerns me that map_id_down() can return -1 on error and that this
change causes the kernel to no longer notice that error?


>  			break;
>  		case AUDIT_GID:
>  		case AUDIT_EGID:
> @@ -372,10 +369,7 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>  			/* bit ops not implemented for gid comparisons */
>  			if (f->op == Audit_bitmask || f->op == Audit_bittest)
>  				goto exit_free;
> -
>  			f->gid = make_kgid(current_user_ns(), f->val);
> -			if (!gid_valid(f->gid))
> -				goto exit_free;
>  			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:
> @@ -469,10 +463,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
>  			/* bit ops not implemented for uid comparisons */
>  			if (f->op == Audit_bitmask || f->op == Audit_bittest)
>  				goto exit_free;
> -
>  			f->uid = make_kuid(current_user_ns(), f->val);
> -			if (!uid_valid(f->uid))
> -				goto exit_free;
>  			break;
>  		case AUDIT_GID:
>  		case AUDIT_EGID:
> @@ -482,10 +473,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
>  			/* bit ops not implemented for gid comparisons */
>  			if (f->op == Audit_bitmask || f->op == Audit_bittest)
>  				goto exit_free;
> -
>  			f->gid = make_kgid(current_user_ns(), f->val);
> -			if (!gid_valid(f->gid))
> -				goto exit_free;
>  			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> wrote:
>
>> audit rule additions containing "-F auid!=4294967295" were failing with EINVAL.
>> 
>> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting and
>> testing against audit rules.  Remove the check for invalid uid and gid when
>> parsing rules and data for logging.

In general testing against invalid uid appears completely bogus, and
should always return true.  As it is and essentially always has been
incorrect to explicitly set any kernel uid to that value.

The only case where this appears to make the least little bit of sense
is if the goal of the test is to test to see if an audit logloginuid
has been set at all.  In which case depending on a test against
4294967295 is bogus because you are depending on an intimate internal
kernel implementation detail.

Certainly removing the gid_valid tests is completely gratitious in this
case.

>> Revert part of ca57ec0f00c3f139c41bf6b0a5b9bcc95bbb2ad7 (2012-09-11) to fix
>> this.
>
> Eric, can you please take a look?
>
>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> ---
>>  kernel/auditfilter.c |   12 ------------
>>  1 files changed, 0 insertions(+), 12 deletions(-)
>> 
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index f9fc54b..457ee39 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -360,10 +360,7 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>>  			/* bit ops not implemented for uid comparisons */
>>  			if (f->op == Audit_bitmask || f->op == Audit_bittest)
>>  				goto exit_free;
>> -
>>  			f->uid = make_kuid(current_user_ns(), f->val);
>> -			if (!uid_valid(f->uid))
>> -				goto exit_free;
>
> It concerns me that map_id_down() can return -1 on error and that this
> change causes the kernel to no longer notice that error?

Me too.  Where we only communicate with audit in the initial user
namespace right now it isn't absolutely broken but it certainly isn't a
habit I want to get into.

How about something like my untested patch below that add an explicit
operation to test if loginuid has been set?

Eric

From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Tue, 9 Apr 2013 02:22:10 -0700
Subject: [PATCH] audit: Make testing for a valid loginuid explicit.

audit rule additions containing "-F auid!=4294967295" were failing
with EINVAL.

Apparently some userland audit rule sets want to know if loginuid uid
has been set and are using a test for auid != 4294967295 to determine
that.

In practice that is a horrible way to ask if a value has been set,
because it relies on subtle implementation details and will break
every time the uid implementation in the kernel changes.

So add a clean way to test if the audit loginuid has been set, and
silently convert the old idiom to the cleaner and more comprehensible
new idiom.

Reported-By: Richard Guy Briggs <rgb@redhat.com> wrote:
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/audit.h      |    5 +++++
 include/uapi/linux/audit.h |    1 +
 kernel/auditfilter.c       |   29 +++++++++++++++++++++++++++++
 kernel/auditsc.c           |    5 ++++-
 4 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index a9fefe2..8a1ddde 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct *t)
 #define audit_signals 0
 #endif /* CONFIG_AUDITSYSCALL */
 
+static inline bool audit_loginuid_set(struct task_struct *tsk)
+{
+	return uid_valid(audit_get_loginuid(tsk));
+}
+
 #ifdef CONFIG_AUDIT
 /* These are defined in audit.c */
 				/* Public API */
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..9554a19 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -246,6 +246,7 @@
 #define AUDIT_OBJ_TYPE	21
 #define AUDIT_OBJ_LEV_LOW	22
 #define AUDIT_OBJ_LEV_HIGH	23
+#define AUDIT_LOGINUID_SET	24
 
 				/* These are ONLY useful when checking
 				 * at syscall exit time (AUDIT_AT_EXIT). */
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 540f986..6381d17 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
 		if (f->op == Audit_bad)
 			goto exit_free;
 
+		/* Support legacy tests for a valid loginuid */
+		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
+			f->type = AUDIT_LOGINUID_SET;
+			f->val = 0;
+		}
+
 		switch(f->type) {
 		default:
 			goto exit_free;
@@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
 			if (!gid_valid(f->gid))
 				goto exit_free;
 			break;
+		case AUDIT_LOGINUID_SET:
+			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
+				goto exit_free;
+			if ((f->val != 0) && (f->val != 1))
+				goto exit_free;
+			break;
 		case AUDIT_PID:
 		case AUDIT_PERS:
 		case AUDIT_MSGTYPE:
@@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 		f->gid = INVALID_GID;
 		f->lsm_str = NULL;
 		f->lsm_rule = NULL;
+
+		/* Support legacy tests for a valid loginuid */
+		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
+			f->type = AUDIT_LOGINUID_SET;
+			f->val = 0;
+		}
+
 		switch(f->type) {
 		case AUDIT_UID:
 		case AUDIT_EUID:
@@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			if (!gid_valid(f->gid))
 				goto exit_free;
 			break;
+		case AUDIT_LOGINUID_SET:
+			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
+				goto exit_free;
+			if ((f->val != 0) && (f->val != 1))
+				goto exit_free;
+			break;
 		case AUDIT_PID:
 		case AUDIT_PERS:
 		case AUDIT_MSGTYPE:
@@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct audit_krule *rule,
 			result = audit_uid_comparator(audit_get_loginuid(current),
 						  f->op, f->uid);
 			break;
+		case AUDIT_LOGINUID_SET:
+			result = audit_comparator(audit_loginuid_set(current),
+						  f->op, f->val);
+			break;
 		case AUDIT_SUBJ_USER:
 		case AUDIT_SUBJ_ROLE:
 		case AUDIT_SUBJ_TYPE:
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 3a11d34..27d0a50 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
 			if (ctx)
 				result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
 			break;
+		case AUDIT_LOGINUID_SET:
+			result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
+			break;
 		case AUDIT_SUBJ_USER:
 		case AUDIT_SUBJ_ROLE:
 		case AUDIT_SUBJ_TYPE:
@@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
 	unsigned int sessionid;
 
 #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
-	if (uid_valid(task->loginuid))
+	if (audit_loginuid_set(task))
 		return -EPERM;
 #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
 	if (!capable(CAP_AUDIT_CONTROL))
-- 
1.7.5.4

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> wrote:
> >> audit rule additions containing "-F auid!=4294967295" were failing with EINVAL.
> >> 
> >> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting and
> >> testing against audit rules.  Remove the check for invalid uid and gid when
> >> parsing rules and data for logging.
> 
> In general testing against invalid uid appears completely bogus, and
> should always return true.  As it is and essentially always has been
> incorrect to explicitly set any kernel uid to that value.

My understanding is that any process started by init has UID -1
(UID_INVALID).  I was a little uncomfortable with this fact when I
learned it, but understand that it is probably too late to change that
now to a reserved value in-band (some probably use -2 or 4294967294 for
nobody).

This leaves an out-of-band solution as has been partially suggested below...

> The only case where this appears to make the least little bit of sense
> is if the goal of the test is to test to see if an audit logloginuid
> has been set at all.  In which case depending on a test against
> 4294967295 is bogus because you are depending on an intimate internal
> kernel implementation detail.

I agree this is ugly.

> Certainly removing the gid_valid tests is completely gratitious in this
> case.

I think I understand that now.

> >> Revert part of ca57ec0f00c3f139c41bf6b0a5b9bcc95bbb2ad7 (2012-09-11) to fix
> >> this.
> >
> > Eric, can you please take a look?
> >
> >> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> ---
> >>  kernel/auditfilter.c |   12 ------------
> >>  1 files changed, 0 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index f9fc54b..457ee39 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -360,10 +360,7 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
> >>  			/* bit ops not implemented for uid comparisons */
> >>  			if (f->op == Audit_bitmask || f->op == Audit_bittest)
> >>  				goto exit_free;
> >> -
> >>  			f->uid = make_kuid(current_user_ns(), f->val);
> >> -			if (!uid_valid(f->uid))
> >> -				goto exit_free;
> >
> > It concerns me that map_id_down() can return -1 on error and that this
> > change causes the kernel to no longer notice that error?
> 
> Me too.  Where we only communicate with audit in the initial user
> namespace right now it isn't absolutely broken but it certainly isn't a
> habit I want to get into.

It will be soon if we try to get auditd in containers talking to the
host kernel, so I agree this needs a better approach.

> How about something like my untested patch below that add an explicit
> operation to test if loginuid has been set?
> 
> Eric
> 
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Tue, 9 Apr 2013 02:22:10 -0700
> Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> 
> audit rule additions containing "-F auid!=4294967295" were failing
> with EINVAL.
> 
> Apparently some userland audit rule sets want to know if loginuid uid
> has been set and are using a test for auid != 4294967295 to determine
> that.
> 
> In practice that is a horrible way to ask if a value has been set,
> because it relies on subtle implementation details and will break
> every time the uid implementation in the kernel changes.
> 
> So add a clean way to test if the audit loginuid has been set, and
> silently convert the old idiom to the cleaner and more comprehensible
> new idiom.
> 
> Reported-By: Richard Guy Briggs <rgb@redhat.com> wrote:

The reporting credit goes to Steve Grubb <sgrubb@redhat.com>.  I just
attempted a fix.

> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  include/linux/audit.h      |    5 +++++
>  include/uapi/linux/audit.h |    1 +
>  kernel/auditfilter.c       |   29 +++++++++++++++++++++++++++++
>  kernel/auditsc.c           |    5 ++++-
>  4 files changed, 39 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index a9fefe2..8a1ddde 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct *t)
>  #define audit_signals 0
>  #endif /* CONFIG_AUDITSYSCALL */
>  
> +static inline bool audit_loginuid_set(struct task_struct *tsk)
> +{
> +	return uid_valid(audit_get_loginuid(tsk));
> +}
> +
>  #ifdef CONFIG_AUDIT
>  /* These are defined in audit.c */
>  				/* Public API */
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9f096f1..9554a19 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -246,6 +246,7 @@
>  #define AUDIT_OBJ_TYPE	21
>  #define AUDIT_OBJ_LEV_LOW	22
>  #define AUDIT_OBJ_LEV_HIGH	23
> +#define AUDIT_LOGINUID_SET	24
>  
>  				/* These are ONLY useful when checking
>  				 * at syscall exit time (AUDIT_AT_EXIT). */
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 540f986..6381d17 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>  		if (f->op == Audit_bad)
>  			goto exit_free;
>  
> +		/* Support legacy tests for a valid loginuid */
> +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> +			f->type = AUDIT_LOGINUID_SET;
> +			f->val = 0;
> +		}
> +
>  		switch(f->type) {
>  		default:
>  			goto exit_free;
> @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>  			if (!gid_valid(f->gid))
>  				goto exit_free;
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> +				goto exit_free;
> +			if ((f->val != 0) && (f->val != 1))

Why the extra comparison to "1"?

Are you anticipating already a userspace process making a call using the
newof type AUDIT_LOGINUID_SET with a value of 1?

> +				goto exit_free;
> +			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:
>  		case AUDIT_MSGTYPE:
> @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
>  		f->gid = INVALID_GID;
>  		f->lsm_str = NULL;
>  		f->lsm_rule = NULL;
> +
> +		/* Support legacy tests for a valid loginuid */
> +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> +			f->type = AUDIT_LOGINUID_SET;
> +			f->val = 0;
> +		}
> +
>  		switch(f->type) {
>  		case AUDIT_UID:
>  		case AUDIT_EUID:
> @@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
>  			if (!gid_valid(f->gid))
>  				goto exit_free;
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> +				goto exit_free;
> +			if ((f->val != 0) && (f->val != 1))

(Again...)

> +				goto exit_free;
> +			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:
>  		case AUDIT_MSGTYPE:
> @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct audit_krule *rule,
>  			result = audit_uid_comparator(audit_get_loginuid(current),
>  						  f->op, f->uid);
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			result = audit_comparator(audit_loginuid_set(current),
> +						  f->op, f->val);
> +			break;
>  		case AUDIT_SUBJ_USER:
>  		case AUDIT_SUBJ_ROLE:
>  		case AUDIT_SUBJ_TYPE:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 3a11d34..27d0a50 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
>  			if (ctx)
>  				result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> +			break;

(OT: I assume the "if (ctx)" is wrong in the AUDIT_LOGINUID case above.)

>  		case AUDIT_SUBJ_USER:
>  		case AUDIT_SUBJ_ROLE:
>  		case AUDIT_SUBJ_TYPE:
> @@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
>  	unsigned int sessionid;
>  
>  #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
> -	if (uid_valid(task->loginuid))
> +	if (audit_loginuid_set(task))
>  		return -EPERM;
>  #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
>  	if (!capable(CAP_AUDIT_CONTROL))
> -- 
> 1.7.5.4
> 

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Steve Grubb]

On Tuesday, April 09, 2013 02:39:32 AM Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> 
wrote:
> >> audit rule additions containing "-F auid!=4294967295" were failing with
> >> EINVAL.
> >> 
> >> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting
> >> and
> >> testing against audit rules.  Remove the check for invalid uid and gid
> >> when
> >> parsing rules and data for logging.
> 
> In general testing against invalid uid appears completely bogus, and
> should always return true.  As it is and essentially always has been
> incorrect to explicitly set any kernel uid to that value.

This is the unset value that daemons would have. When a real person logs in, 
pam_loginuid writes the loginuid that was authenticated to. So, any time the 
value is -1, we are dealing with a daemon or system process. When it comes to 
auditing, people usually make an exception so that daemon and normal system 
activity is not recorded. So, you would make a rule something like

-a always,exit -S open -F exit=-EPERM -F auid>=500 -F auid!=-1


> The only case where this appears to make the least little bit of sense
> is if the goal of the test is to test to see if an audit logloginuid
> has been set at all.  In which case depending on a test against
> 4294967295 is bogus because you are depending on an intimate internal
> kernel implementation detail.

Its been this way and documented since at least 9 years ago. The audit system 
has been broken for all intents and purposes since the 3.7 kernel was 
released.

Thanks,
-Steve


> Certainly removing the gid_valid tests is completely gratitious in this
> case.
> 
> >> Revert part of ca57ec0f00c3f139c41bf6b0a5b9bcc95bbb2ad7 (2012-09-11) to
> >> fix
> >> this.
> > 
> > Eric, can you please take a look?
> > 
> >> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> ---
> >> 
> >>  kernel/auditfilter.c |   12 ------------
> >>  1 files changed, 0 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index f9fc54b..457ee39 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -360,10 +360,7 @@ static struct audit_entry
> >> *audit_rule_to_entry(struct audit_rule *rule)>> 
> >>  			/* bit ops not implemented for uid comparisons */
> >>  			if (f->op == Audit_bitmask || f->op == Audit_bittest)
> >>  			
> >>  				goto exit_free;
> >> 
> >> -
> >> 
> >>  			f->uid = make_kuid(current_user_ns(), f->val);
> >> 
> >> -			if (!uid_valid(f->uid))
> >> -				goto exit_free;
> > 
> > It concerns me that map_id_down() can return -1 on error and that this
> > change causes the kernel to no longer notice that error?
> 
> Me too.  Where we only communicate with audit in the initial user
> namespace right now it isn't absolutely broken but it certainly isn't a
> habit I want to get into.
> 
> How about something like my untested patch below that add an explicit
> operation to test if loginuid has been set?
> 
> Eric
> 
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Tue, 9 Apr 2013 02:22:10 -0700
> Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> 
> audit rule additions containing "-F auid!=4294967295" were failing
> with EINVAL.
> 
> Apparently some userland audit rule sets want to know if loginuid uid
> has been set and are using a test for auid != 4294967295 to determine
> that.
> 
> In practice that is a horrible way to ask if a value has been set,
> because it relies on subtle implementation details and will break
> every time the uid implementation in the kernel changes.
> 
> So add a clean way to test if the audit loginuid has been set, and
> silently convert the old idiom to the cleaner and more comprehensible
> new idiom.
> 
> Reported-By: Richard Guy Briggs <rgb@redhat.com> wrote:
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  include/linux/audit.h      |    5 +++++
>  include/uapi/linux/audit.h |    1 +
>  kernel/auditfilter.c       |   29 +++++++++++++++++++++++++++++
>  kernel/auditsc.c           |    5 ++++-
>  4 files changed, 39 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index a9fefe2..8a1ddde 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct *t)
>  #define audit_signals 0
>  #endif /* CONFIG_AUDITSYSCALL */
> 
> +static inline bool audit_loginuid_set(struct task_struct *tsk)
> +{
> +	return uid_valid(audit_get_loginuid(tsk));
> +}
> +
>  #ifdef CONFIG_AUDIT
>  /* These are defined in audit.c */
>  				/* Public API */
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9f096f1..9554a19 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -246,6 +246,7 @@
>  #define AUDIT_OBJ_TYPE	21
>  #define AUDIT_OBJ_LEV_LOW	22
>  #define AUDIT_OBJ_LEV_HIGH	23
> +#define AUDIT_LOGINUID_SET	24
> 
>  				/* These are ONLY useful when checking
>  				 * at syscall exit time (AUDIT_AT_EXIT). */
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 540f986..6381d17 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> audit_rule *rule) if (f->op == Audit_bad)
>  			goto exit_free;
> 
> +		/* Support legacy tests for a valid loginuid */
> +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> +			f->type = AUDIT_LOGINUID_SET;
> +			f->val = 0;
> +		}
> +
>  		switch(f->type) {
>  		default:
>  			goto exit_free;
> @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> audit_rule *rule) if (!gid_valid(f->gid))
>  				goto exit_free;
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> +				goto exit_free;
> +			if ((f->val != 0) && (f->val != 1))
> +				goto exit_free;
> +			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:
>  		case AUDIT_MSGTYPE:
> @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data, f->gid = INVALID_GID;
>  		f->lsm_str = NULL;
>  		f->lsm_rule = NULL;
> +
> +		/* Support legacy tests for a valid loginuid */
> +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> +			f->type = AUDIT_LOGINUID_SET;
> +			f->val = 0;
> +		}
> +
>  		switch(f->type) {
>  		case AUDIT_UID:
>  		case AUDIT_EUID:
> @@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data, if (!gid_valid(f->gid))
>  				goto exit_free;
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> +				goto exit_free;
> +			if ((f->val != 0) && (f->val != 1))
> +				goto exit_free;
> +			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:
>  		case AUDIT_MSGTYPE:
> @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct audit_krule
> *rule, result = audit_uid_comparator(audit_get_loginuid(current),
>  						  f->op, f->uid);
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			result = audit_comparator(audit_loginuid_set(current),
> +						  f->op, f->val);
> +			break;
>  		case AUDIT_SUBJ_USER:
>  		case AUDIT_SUBJ_ROLE:
>  		case AUDIT_SUBJ_TYPE:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 3a11d34..27d0a50 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
>  			if (ctx)
>  				result = audit_uid_comparator(tsk->loginuid, f->op, f-
>uid);
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			result = audit_comparator(audit_loginuid_set(tsk), f->op, f-
>val);
> +			break;
>  		case AUDIT_SUBJ_USER:
>  		case AUDIT_SUBJ_ROLE:
>  		case AUDIT_SUBJ_TYPE:
> @@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
>  	unsigned int sessionid;
> 
>  #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
> -	if (uid_valid(task->loginuid))
> +	if (audit_loginuid_set(task))
>  		return -EPERM;
>  #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
>  	if (!capable(CAP_AUDIT_CONTROL))

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Eric W. Biederman]

Steve Grubb <sgrubb@redhat.com> writes:

> On Tuesday, April 09, 2013 02:39:32 AM Eric W. Biederman wrote:
>> Andrew Morton <akpm@linux-foundation.org> writes:
>> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> 
> wrote:
>> >> audit rule additions containing "-F auid!=4294967295" were failing with
>> >> EINVAL.
>> >> 
>> >> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting
>> >> and
>> >> testing against audit rules.  Remove the check for invalid uid and gid
>> >> when
>> >> parsing rules and data for logging.
>> 
>> In general testing against invalid uid appears completely bogus, and
>> should always return true.  As it is and essentially always has been
>> incorrect to explicitly set any kernel uid to that value.
>
> This is the unset value that daemons would have. 

As their uid, or gid, or euid, or fsuid.  Not in the least.

> When a real person logs in, 
> pam_loginuid writes the loginuid that was authenticated to. So, any time the 
> value is -1, we are dealing with a daemon or system process. When it comes to 
> auditing, people usually make an exception so that daemon and normal system 
> activity is not recorded. So, you would make a rule something like

> -a always,exit -S open -F exit=-EPERM -F auid>=500 -F auid!=-1

My point is that -1 is a special case that applies only to loginuid, and
that when testing for -1 is not testing for a specific loginuid value
but instead it is testing to see if loginuid has been set.  Semantically
the last is very different.

>> The only case where this appears to make the least little bit of sense
>> is if the goal of the test is to test to see if an audit logloginuid
>> has been set at all.  In which case depending on a test against
>> 4294967295 is bogus because you are depending on an intimate internal
>> kernel implementation detail.
>
> Its been this way and documented since at least 9 years ago. The audit system 
> has been broken for all intents and purposes since the 3.7 kernel was 
> released.

I certainly haven't seen the documentation.  And no one has much cared
about the audit subsystem this "breakage" of the audit
subsystem. Despite things failing with a clear error code.  So there are
two choices.  We mark the audit subsystem as broken moving it to staging
and then delete it because no one cares enough to look after it and
maintain it.  Or we have a constructive conversation about what to do
with it.

I have proposed a patch that will preserve the existing behavior while
adding maintainable semantics.  Will someone who cares please test my
proposed fix?

Eric

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Tue, Apr 09, 2013 at 02:16:22PM -0700, Eric W. Biederman wrote:
> Steve Grubb <sgrubb@redhat.com> writes:
> 
> > On Tuesday, April 09, 2013 02:39:32 AM Eric W. Biederman wrote:
> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> 
> > wrote:
> >> >> audit rule additions containing "-F auid!=4294967295" were failing with
> >> >> EINVAL.
> >> >> 
> >> >> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting
> >> >> and
> >> >> testing against audit rules.  Remove the check for invalid uid and gid
> >> >> when
> >> >> parsing rules and data for logging.
> >> 
> >> In general testing against invalid uid appears completely bogus, and
> >> should always return true.  As it is and essentially always has been
> >> incorrect to explicitly set any kernel uid to that value.
> >
> > This is the unset value that daemons would have. 
> 
> As their uid, or gid, or euid, or fsuid.  Not in the least.

Point taken that only a value of UID_INVALID should be accepted for
auid.

> > When a real person logs in, 
> > pam_loginuid writes the loginuid that was authenticated to. So, any time the 
> > value is -1, we are dealing with a daemon or system process. When it comes to 
> > auditing, people usually make an exception so that daemon and normal system 
> > activity is not recorded. So, you would make a rule something like
> 
> > -a always,exit -S open -F exit=-EPERM -F auid>=500 -F auid!=-1
> 
> My point is that -1 is a special case that applies only to loginuid, and
> that when testing for -1 is not testing for a specific loginuid value
> but instead it is testing to see if loginuid has been set.  Semantically
> the last is very different.
> 
> >> The only case where this appears to make the least little bit of sense
> >> is if the goal of the test is to test to see if an audit logloginuid
> >> has been set at all.  In which case depending on a test against
> >> 4294967295 is bogus because you are depending on an intimate internal
> >> kernel implementation detail.
> >
> > Its been this way and documented since at least 9 years ago. The audit system 
> > has been broken for all intents and purposes since the 3.7 kernel was 
> > released.
> 
> I certainly haven't seen the documentation.

It is in the audit manpages.

> And no one has much cared
> about the audit subsystem this "breakage" of the audit
> subsystem. Despite things failing with a clear error code.  So there are
> two choices.  We mark the audit subsystem as broken moving it to staging
> and then delete it because no one cares enough to look after it and
> maintain it.  Or we have a constructive conversation about what to do
> with it.

Ok, politics aside...

> I have proposed a patch that will preserve the existing behavior while
> adding maintainable semantics.  Will someone who cares please test my
> proposed fix?

I'll test it.

> Eric

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Wed, Apr 10, 2013 at 12:20:18PM -0400, Richard Guy Briggs wrote:
> On Tue, Apr 09, 2013 at 02:16:22PM -0700, Eric W. Biederman wrote:
> > Steve Grubb <sgrubb@redhat.com> writes:
> > > On Tuesday, April 09, 2013 02:39:32 AM Eric W. Biederman wrote:
> > >> Andrew Morton <akpm@linux-foundation.org> writes:
> > >> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> wrote:
> > >> >> audit rule additions containing "-F auid!=4294967295" were failing with
> > >> >> EINVAL.
> > >> >> 
> > >> >> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting
> > >> >> and
> > >> >> testing against audit rules.  Remove the check for invalid uid and gid
> > >> >> when
> > >> >> parsing rules and data for logging.
> > >> 
> > >> In general testing against invalid uid appears completely bogus, and
> > >> should always return true.  As it is and essentially always has been
> > >> incorrect to explicitly set any kernel uid to that value.
> > >
> > > This is the unset value that daemons would have. 
> > 
> > As their uid, or gid, or euid, or fsuid.  Not in the least.
> 
> Point taken that only a value of UID_INVALID should be accepted for
> auid.

> > And no one has much cared
> > about the audit subsystem this "breakage" of the audit
> > subsystem. Despite things failing with a clear error code.  So there are
> > two choices.  We mark the audit subsystem as broken moving it to staging
> > and then delete it because no one cares enough to look after it and
> > maintain it.  Or we have a constructive conversation about what to do
> > with it.
> 
> Ok, politics aside...
> 
> > I have proposed a patch that will preserve the existing behavior while
> > adding maintainable semantics.  Will someone who cares please test my
> > proposed fix?
> 
> I'll test it.

Meanwhile, could you please respond to my other comments interlaced in my
previous reply earlier in the thread?  In particular the question about
f->val == 1.

> > Eric
> 
> - RGB

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Eric W. Biederman]

Richard Guy Briggs <rgb@redhat.com> writes:

> On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:

>> @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>>  			if (!gid_valid(f->gid))
>>  				goto exit_free;
>>  			break;
>> +		case AUDIT_LOGINUID_SET:
>> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
>> +				goto exit_free;
>> +			if ((f->val != 0) && (f->val != 1))
>
> Why the extra comparison to "1"?
>
> Are you anticipating already a userspace process making a call using the
> newof type AUDIT_LOGINUID_SET with a value of 1?

Sorry I missed this question the first time.  I am anticipating
AUDIT_LOGINUID_SET to return a value of 0 or 1 (a boolean) and so I
allow the operations and constants that are valid for a boolean.

In particuluar I allow the opeartions == !=  and the boolean constants 0 and 1.

>> @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct audit_krule *rule,
>>  			result = audit_uid_comparator(audit_get_loginuid(current),
>>  						  f->op, f->uid);
>>  			break;
>> +		case AUDIT_LOGINUID_SET:
>> +			result = audit_comparator(audit_loginuid_set(current),
>> +						  f->op, f->val);
>> +			break;
>>  		case AUDIT_SUBJ_USER:
>>  		case AUDIT_SUBJ_ROLE:
>>  		case AUDIT_SUBJ_TYPE:
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 3a11d34..27d0a50 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
>>  			if (ctx)
>>  				result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
>>  			break;
>> +		case AUDIT_LOGINUID_SET:
>> +			result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
>> +			break;
>
> (OT: I assume the "if (ctx)" is wrong in the AUDIT_LOGINUID case
> above.)

Good question.  I didn't see that when I was preparing my patch.

ctx is not necessary but I think ctx is set when a task is being audited
so it may serve a useful function.  But I have to admit it that if(ctx)
looks like a bug.

Eric

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Wed, Apr 10, 2013 at 11:02:43AM -0700, Eric W. Biederman wrote:
> Richard Guy Briggs <rgb@redhat.com> writes:
> > On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> >> @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
> >>  			if (!gid_valid(f->gid))
> >>  				goto exit_free;
> >>  			break;
> >> +		case AUDIT_LOGINUID_SET:
> >> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> >> +				goto exit_free;
> >> +			if ((f->val != 0) && (f->val != 1))
> >
> > Why the extra comparison to "1"?
> >
> > Are you anticipating already a userspace process making a call using the
> > newof type AUDIT_LOGINUID_SET with a value of 1?
> 
> Sorry I missed this question the first time.  I am anticipating
> AUDIT_LOGINUID_SET to return a value of 0 or 1 (a boolean) and so I
> allow the operations and constants that are valid for a boolean.
> 
> In particuluar I allow the opeartions == !=  and the boolean constants 0 and 1.

Duh, of course...  sorry for being thick.

> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index 3a11d34..27d0a50 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> >>  			if (ctx)
> >>  				result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
> >>  			break;
> >
> > (OT: I assume the "if (ctx)" is wrong in the AUDIT_LOGINUID case
> > above.)
> 
> Good question.  I didn't see that when I was preparing my patch.
> 
> ctx is not necessary but I think ctx is set when a task is being audited
> so it may serve a useful function.  But I have to admit it that if(ctx)
> looks like a bug.

Thanks...

> Eric

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> wrote:
> >> audit rule additions containing "-F auid!=4294967295" were failing with EINVAL.
> 
> The only case where this appears to make the least little bit of sense
> is if the goal of the test is to test to see if an audit logloginuid
> has been set at all.  In which case depending on a test against
> 4294967295 is bogus because you are depending on an intimate internal
> kernel implementation detail.
> 
> How about something like my untested patch below that add an explicit
> operation to test if loginuid has been set?

Sorry for the delay in testing this, I had another urgent bug and a
belligerent test box...

I like this approach better than mine now that I understand it.  I've
tested the patch below without any changes.  It works as expected with
my previous test case.  I don't know if a Signed-off-by: is appropriate
for me in this case, but I'll throw in a:

Tested-by: Richard Guy Briggs <rbriggs@redhat.com>

and recommend a:

Reported-By: Steve Grubb <sbrubb@redhat.com>


> Eric
> 
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Tue, 9 Apr 2013 02:22:10 -0700
> Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> 
> audit rule additions containing "-F auid!=4294967295" were failing
> with EINVAL.
> 
> Apparently some userland audit rule sets want to know if loginuid uid
> has been set and are using a test for auid != 4294967295 to determine
> that.
> 
> In practice that is a horrible way to ask if a value has been set,
> because it relies on subtle implementation details and will break
> every time the uid implementation in the kernel changes.
> 
> So add a clean way to test if the audit loginuid has been set, and
> silently convert the old idiom to the cleaner and more comprehensible
> new idiom.
> 
> Reported-By: Richard Guy Briggs <rgb@redhat.com> wrote:
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  include/linux/audit.h      |    5 +++++
>  include/uapi/linux/audit.h |    1 +
>  kernel/auditfilter.c       |   29 +++++++++++++++++++++++++++++
>  kernel/auditsc.c           |    5 ++++-
>  4 files changed, 39 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index a9fefe2..8a1ddde 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct *t)
>  #define audit_signals 0
>  #endif /* CONFIG_AUDITSYSCALL */
>  
> +static inline bool audit_loginuid_set(struct task_struct *tsk)
> +{
> +	return uid_valid(audit_get_loginuid(tsk));
> +}
> +
>  #ifdef CONFIG_AUDIT
>  /* These are defined in audit.c */
>  				/* Public API */
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9f096f1..9554a19 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -246,6 +246,7 @@
>  #define AUDIT_OBJ_TYPE	21
>  #define AUDIT_OBJ_LEV_LOW	22
>  #define AUDIT_OBJ_LEV_HIGH	23
> +#define AUDIT_LOGINUID_SET	24
>  
>  				/* These are ONLY useful when checking
>  				 * at syscall exit time (AUDIT_AT_EXIT). */
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 540f986..6381d17 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>  		if (f->op == Audit_bad)
>  			goto exit_free;
>  
> +		/* Support legacy tests for a valid loginuid */
> +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> +			f->type = AUDIT_LOGINUID_SET;
> +			f->val = 0;
> +		}
> +
>  		switch(f->type) {
>  		default:
>  			goto exit_free;
> @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
>  			if (!gid_valid(f->gid))
>  				goto exit_free;
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> +				goto exit_free;
> +			if ((f->val != 0) && (f->val != 1))
> +				goto exit_free;
> +			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:
>  		case AUDIT_MSGTYPE:
> @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
>  		f->gid = INVALID_GID;
>  		f->lsm_str = NULL;
>  		f->lsm_rule = NULL;
> +
> +		/* Support legacy tests for a valid loginuid */
> +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> +			f->type = AUDIT_LOGINUID_SET;
> +			f->val = 0;
> +		}
> +
>  		switch(f->type) {
>  		case AUDIT_UID:
>  		case AUDIT_EUID:
> @@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
>  			if (!gid_valid(f->gid))
>  				goto exit_free;
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> +				goto exit_free;
> +			if ((f->val != 0) && (f->val != 1))
> +				goto exit_free;
> +			break;
>  		case AUDIT_PID:
>  		case AUDIT_PERS:
>  		case AUDIT_MSGTYPE:
> @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct audit_krule *rule,
>  			result = audit_uid_comparator(audit_get_loginuid(current),
>  						  f->op, f->uid);
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			result = audit_comparator(audit_loginuid_set(current),
> +						  f->op, f->val);
> +			break;
>  		case AUDIT_SUBJ_USER:
>  		case AUDIT_SUBJ_ROLE:
>  		case AUDIT_SUBJ_TYPE:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 3a11d34..27d0a50 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
>  			if (ctx)
>  				result = audit_uid_comparator(tsk->loginuid, f->op, f->uid);
>  			break;
> +		case AUDIT_LOGINUID_SET:
> +			result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> +			break;
>  		case AUDIT_SUBJ_USER:
>  		case AUDIT_SUBJ_ROLE:
>  		case AUDIT_SUBJ_TYPE:
> @@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
>  	unsigned int sessionid;
>  
>  #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
> -	if (uid_valid(task->loginuid))
> +	if (audit_loginuid_set(task))
>  		return -EPERM;
>  #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
>  	if (!capable(CAP_AUDIT_CONTROL))
> -- 
> 1.7.5.4
> 

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Steve Grubb]

On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
> On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> > Andrew Morton <akpm@linux-foundation.org> writes:
> > > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> 
wrote:
> > >> audit rule additions containing "-F auid!=4294967295" were failing with
> > >> EINVAL.> 
> > The only case where this appears to make the least little bit of sense
> > is if the goal of the test is to test to see if an audit logloginuid
> > has been set at all.  In which case depending on a test against
> > 4294967295 is bogus because you are depending on an intimate internal
> > kernel implementation detail.
> > 
> > How about something like my untested patch below that add an explicit
> > operation to test if loginuid has been set?
> 
> Sorry for the delay in testing this, I had another urgent bug and a
> belligerent test box...
> 
> I like this approach better than mine now that I understand it.  I've
> tested the patch below without any changes.  It works as expected with
> my previous test case.  I don't know if a Signed-off-by: is appropriate
> for me in this case, but I'll throw in a:
> 
> Tested-by: Richard Guy Briggs <rbriggs@redhat.com>
> 
> and recommend a:
> 
> Reported-By: Steve Grubb <sbrubb@redhat.com>

If this is the approved patch, can it be put in stable? The audit system 
hasn't worked as intended since January.

Thanks,
-Steve


> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> > Date: Tue, 9 Apr 2013 02:22:10 -0700
> > Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> > 
> > audit rule additions containing "-F auid!=4294967295" were failing
> > with EINVAL.
> > 
> > Apparently some userland audit rule sets want to know if loginuid uid
> > has been set and are using a test for auid != 4294967295 to determine
> > that.
> > 
> > In practice that is a horrible way to ask if a value has been set,
> > because it relies on subtle implementation details and will break
> > every time the uid implementation in the kernel changes.
> > 
> > So add a clean way to test if the audit loginuid has been set, and
> > silently convert the old idiom to the cleaner and more comprehensible
> > new idiom.
> > 
> > Reported-By: Richard Guy Briggs <rgb@redhat.com> wrote:
> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > ---
> > 
> >  include/linux/audit.h      |    5 +++++
> >  include/uapi/linux/audit.h |    1 +
> >  kernel/auditfilter.c       |   29 +++++++++++++++++++++++++++++
> >  kernel/auditsc.c           |    5 ++++-
> >  4 files changed, 39 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index a9fefe2..8a1ddde 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct
> > *t)
> > 
> >  #define audit_signals 0
> >  #endif /* CONFIG_AUDITSYSCALL */
> > 
> > +static inline bool audit_loginuid_set(struct task_struct *tsk)
> > +{
> > +	return uid_valid(audit_get_loginuid(tsk));
> > +}
> > +
> > 
> >  #ifdef CONFIG_AUDIT
> >  /* These are defined in audit.c */
> >  
> >  				/* Public API */
> > 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 9f096f1..9554a19 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -246,6 +246,7 @@
> > 
> >  #define AUDIT_OBJ_TYPE	21
> >  #define AUDIT_OBJ_LEV_LOW	22
> >  #define AUDIT_OBJ_LEV_HIGH	23
> > 
> > +#define AUDIT_LOGINUID_SET	24
> > 
> >  				/* These are ONLY useful when checking
> >  				
> >  				 * at syscall exit time (AUDIT_AT_EXIT). */
> > 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 540f986..6381d17 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > audit_rule *rule)> 
> >  		if (f->op == Audit_bad)
> >  		
> >  			goto exit_free;
> > 
> > +		/* Support legacy tests for a valid loginuid */
> > +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > +			f->type = AUDIT_LOGINUID_SET;
> > +			f->val = 0;
> > +		}
> > +
> > 
> >  		switch(f->type) {
> >  		
> >  		default:
> >  			goto exit_free;
> > 
> > @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > audit_rule *rule)> 
> >  			if (!gid_valid(f->gid))
> >  			
> >  				goto exit_free;
> >  			
> >  			break;
> > 
> > +		case AUDIT_LOGINUID_SET:
> > +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > +				goto exit_free;
> > +			if ((f->val != 0) && (f->val != 1))
> > +				goto exit_free;
> > +			break;
> > 
> >  		case AUDIT_PID:
> >  		case AUDIT_PERS:
> > 
> >  		case AUDIT_MSGTYPE:
> > @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data,> 
> >  		f->gid = INVALID_GID;
> >  		f->lsm_str = NULL;
> >  		f->lsm_rule = NULL;
> > 
> > +
> > +		/* Support legacy tests for a valid loginuid */
> > +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > +			f->type = AUDIT_LOGINUID_SET;
> > +			f->val = 0;
> > +		}
> > +
> > 
> >  		switch(f->type) {
> >  		case AUDIT_UID:
> > 
> >  		case AUDIT_EUID:
> > @@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data,> 
> >  			if (!gid_valid(f->gid))
> >  			
> >  				goto exit_free;
> >  			
> >  			break;
> > 
> > +		case AUDIT_LOGINUID_SET:
> > +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > +				goto exit_free;
> > +			if ((f->val != 0) && (f->val != 1))
> > +				goto exit_free;
> > +			break;
> > 
> >  		case AUDIT_PID:
> >  		case AUDIT_PERS:
> > 
> >  		case AUDIT_MSGTYPE:
> > @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct
> > audit_krule *rule,> 
> >  			result = audit_uid_comparator(audit_get_loginuid(current),
> >  			
> >  						  f->op, f->uid);
> >  			
> >  			break;
> > 
> > +		case AUDIT_LOGINUID_SET:
> > +			result = audit_comparator(audit_loginuid_set(current),
> > +						  f->op, f->val);
> > +			break;
> > 
> >  		case AUDIT_SUBJ_USER:
> >  		case AUDIT_SUBJ_ROLE:
> > 
> >  		case AUDIT_SUBJ_TYPE:
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 3a11d34..27d0a50 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > 
> >  			if (ctx)
> >  			
> >  				result = audit_uid_comparator(tsk->loginuid, f->op, 
f->uid);
> >  			
> >  			break;
> > 
> > +		case AUDIT_LOGINUID_SET:
> > +			result = audit_comparator(audit_loginuid_set(tsk), f->op, 
f->val);
> > +			break;
> > 
> >  		case AUDIT_SUBJ_USER:
> >  		case AUDIT_SUBJ_ROLE:
> > 
> >  		case AUDIT_SUBJ_TYPE:
> > @@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
> > 
> >  	unsigned int sessionid;
> >  
> >  #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
> > 
> > -	if (uid_valid(task->loginuid))
> > +	if (audit_loginuid_set(task))
> > 
> >  		return -EPERM;
> >  
> >  #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
> >  
> >  	if (!capable(CAP_AUDIT_CONTROL))
> 
> - RGB
> 
> --
> Richard Guy Briggs <rbriggs@redhat.com>
> Senior Software Engineer
> AMER ENG Base Operating Systems
> Remote, Canada, Ottawa
> Voice: 1.647.777.2635
> Internal: (81) 32635
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Thu, May 09, 2013 at 09:29:18AM -0400, Steve Grubb wrote:
> On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
> > On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> > > Andrew Morton <akpm@linux-foundation.org> writes:
> > > > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> 
> wrote:
> > > >> audit rule additions containing "-F auid!=4294967295" were failing with
> > > >> EINVAL.> 
> > > The only case where this appears to make the least little bit of sense
> > > is if the goal of the test is to test to see if an audit logloginuid
> > > has been set at all.  In which case depending on a test against
> > > 4294967295 is bogus because you are depending on an intimate internal
> > > kernel implementation detail.
> > > 
> > > How about something like my untested patch below that add an explicit
> > > operation to test if loginuid has been set?
> > 
> > Sorry for the delay in testing this, I had another urgent bug and a
> > belligerent test box...
> > 
> > I like this approach better than mine now that I understand it.  I've
> > tested the patch below without any changes.  It works as expected with
> > my previous test case.  I don't know if a Signed-off-by: is appropriate
> > for me in this case, but I'll throw in a:
> > 
> > Tested-by: Richard Guy Briggs <rbriggs@redhat.com>
> > 
> > and recommend a:
> > 
> > Reported-By: Steve Grubb <sbrubb@redhat.com>
> 
> If this is the approved patch, can it be put in stable? The audit system 
> hasn't worked as intended since January.

Most certainly.  I should have added that.  I only thought about that
after sending.

> Thanks,
> -Steve
> 
> > > From: "Eric W. Biederman" <ebiederm@xmission.com>
> > > Date: Tue, 9 Apr 2013 02:22:10 -0700
> > > Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> > > 
> > > audit rule additions containing "-F auid!=4294967295" were failing
> > > with EINVAL.
> > > 
> > > Apparently some userland audit rule sets want to know if loginuid uid
> > > has been set and are using a test for auid != 4294967295 to determine
> > > that.
> > > 
> > > In practice that is a horrible way to ask if a value has been set,
> > > because it relies on subtle implementation details and will break
> > > every time the uid implementation in the kernel changes.
> > > 
> > > So add a clean way to test if the audit loginuid has been set, and
> > > silently convert the old idiom to the cleaner and more comprehensible
> > > new idiom.
> > > 
> > > Reported-By: Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > ---
> > > 
> > >  include/linux/audit.h      |    5 +++++
> > >  include/uapi/linux/audit.h |    1 +
> > >  kernel/auditfilter.c       |   29 +++++++++++++++++++++++++++++
> > >  kernel/auditsc.c           |    5 ++++-
> > >  4 files changed, 39 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index a9fefe2..8a1ddde 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct
> > > *t)
> > > 
> > >  #define audit_signals 0
> > >  #endif /* CONFIG_AUDITSYSCALL */
> > > 
> > > +static inline bool audit_loginuid_set(struct task_struct *tsk)
> > > +{
> > > +	return uid_valid(audit_get_loginuid(tsk));
> > > +}
> > > +
> > > 
> > >  #ifdef CONFIG_AUDIT
> > >  /* These are defined in audit.c */
> > >  
> > >  				/* Public API */
> > > 
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index 9f096f1..9554a19 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -246,6 +246,7 @@
> > > 
> > >  #define AUDIT_OBJ_TYPE	21
> > >  #define AUDIT_OBJ_LEV_LOW	22
> > >  #define AUDIT_OBJ_LEV_HIGH	23
> > > 
> > > +#define AUDIT_LOGINUID_SET	24
> > > 
> > >  				/* These are ONLY useful when checking
> > >  				
> > >  				 * at syscall exit time (AUDIT_AT_EXIT). */
> > > 
> > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > index 540f986..6381d17 100644
> > > --- a/kernel/auditfilter.c
> > > +++ b/kernel/auditfilter.c
> > > @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > > audit_rule *rule)> 
> > >  		if (f->op == Audit_bad)
> > >  		
> > >  			goto exit_free;
> > > 
> > > +		/* Support legacy tests for a valid loginuid */
> > > +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > > +			f->type = AUDIT_LOGINUID_SET;
> > > +			f->val = 0;
> > > +		}
> > > +
> > > 
> > >  		switch(f->type) {
> > >  		
> > >  		default:
> > >  			goto exit_free;
> > > 
> > > @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > > audit_rule *rule)> 
> > >  			if (!gid_valid(f->gid))
> > >  			
> > >  				goto exit_free;
> > >  			
> > >  			break;
> > > 
> > > +		case AUDIT_LOGINUID_SET:
> > > +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > > +				goto exit_free;
> > > +			if ((f->val != 0) && (f->val != 1))
> > > +				goto exit_free;
> > > +			break;
> > > 
> > >  		case AUDIT_PID:
> > >  		case AUDIT_PERS:
> > > 
> > >  		case AUDIT_MSGTYPE:
> > > @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct
> > > audit_rule_data *data,> 
> > >  		f->gid = INVALID_GID;
> > >  		f->lsm_str = NULL;
> > >  		f->lsm_rule = NULL;
> > > 
> > > +
> > > +		/* Support legacy tests for a valid loginuid */
> > > +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > > +			f->type = AUDIT_LOGINUID_SET;
> > > +			f->val = 0;
> > > +		}
> > > +
> > > 
> > >  		switch(f->type) {
> > >  		case AUDIT_UID:
> > > 
> > >  		case AUDIT_EUID:
> > > @@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct
> > > audit_rule_data *data,> 
> > >  			if (!gid_valid(f->gid))
> > >  			
> > >  				goto exit_free;
> > >  			
> > >  			break;
> > > 
> > > +		case AUDIT_LOGINUID_SET:
> > > +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > > +				goto exit_free;
> > > +			if ((f->val != 0) && (f->val != 1))
> > > +				goto exit_free;
> > > +			break;
> > > 
> > >  		case AUDIT_PID:
> > >  		case AUDIT_PERS:
> > > 
> > >  		case AUDIT_MSGTYPE:
> > > @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct
> > > audit_krule *rule,> 
> > >  			result = audit_uid_comparator(audit_get_loginuid(current),
> > >  			
> > >  						  f->op, f->uid);
> > >  			
> > >  			break;
> > > 
> > > +		case AUDIT_LOGINUID_SET:
> > > +			result = audit_comparator(audit_loginuid_set(current),
> > > +						  f->op, f->val);
> > > +			break;
> > > 
> > >  		case AUDIT_SUBJ_USER:
> > >  		case AUDIT_SUBJ_ROLE:
> > > 
> > >  		case AUDIT_SUBJ_TYPE:
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 3a11d34..27d0a50 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > > 
> > >  			if (ctx)
> > >  			
> > >  				result = audit_uid_comparator(tsk->loginuid, f->op, 
> f->uid);
> > >  			
> > >  			break;
> > > 
> > > +		case AUDIT_LOGINUID_SET:
> > > +			result = audit_comparator(audit_loginuid_set(tsk), f->op, 
> f->val);
> > > +			break;
> > > 
> > >  		case AUDIT_SUBJ_USER:
> > >  		case AUDIT_SUBJ_ROLE:
> > > 
> > >  		case AUDIT_SUBJ_TYPE:
> > > @@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
> > > 
> > >  	unsigned int sessionid;
> > >  
> > >  #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
> > > 
> > > -	if (uid_valid(task->loginuid))
> > > +	if (audit_loginuid_set(task))
> > > 
> > >  		return -EPERM;
> > >  
> > >  #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
> > >  
> > >  	if (!capable(CAP_AUDIT_CONTROL))
> > 
> > - RGB
> > 
> > --
> > Richard Guy Briggs <rbriggs@redhat.com>
> > Senior Software Engineer
> > AMER ENG Base Operating Systems
> > Remote, Canada, Ottawa
> > Voice: 1.647.777.2635
> > Internal: (81) 32635
> > 
> > --
> > 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
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Richard Guy Briggs]

On Thu, May 09, 2013 at 09:52:47AM -0400, Richard Guy Briggs wrote:
> On Thu, May 09, 2013 at 09:29:18AM -0400, Steve Grubb wrote:
> > On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
> > > On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> > > > Andrew Morton <akpm@linux-foundation.org> writes:
> > > > > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> 
> > wrote:
> > > > >> audit rule additions containing "-F auid!=4294967295" were failing with
> > > > >> EINVAL.> 
> > > > The only case where this appears to make the least little bit of sense
> > > > is if the goal of the test is to test to see if an audit logloginuid
> > > > has been set at all.  In which case depending on a test against
> > > > 4294967295 is bogus because you are depending on an intimate internal
> > > > kernel implementation detail.
> > > > 
> > > > How about something like my untested patch below that add an explicit
> > > > operation to test if loginuid has been set?
> > > 
> > > Sorry for the delay in testing this, I had another urgent bug and a
> > > belligerent test box...
> > > 
> > > I like this approach better than mine now that I understand it.  I've
> > > tested the patch below without any changes.  It works as expected with
> > > my previous test case.  I don't know if a Signed-off-by: is appropriate
> > > for me in this case, but I'll throw in a:
> > > 
> > > Tested-by: Richard Guy Briggs <rbriggs@redhat.com>
> > > 
> > > and recommend a:
> > > 
> > > Reported-By: Steve Grubb <sbrubb@redhat.com>
> > 
> > If this is the approved patch, can it be put in stable? The audit system 
> > hasn't worked as intended since January.
> 
> Most certainly.  I should have added that.  I only thought about that
> after sending.

No worries, Eric added it in his set in his pull request to Linus.

> > Thanks,
> > -Steve
> > 
> > > > From: "Eric W. Biederman" <ebiederm@xmission.com>
> > > > Date: Tue, 9 Apr 2013 02:22:10 -0700
> > > > Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> > > > 
> > > > audit rule additions containing "-F auid!=4294967295" were failing
> > > > with EINVAL.
> > > > 
> > > > Apparently some userland audit rule sets want to know if loginuid uid
> > > > has been set and are using a test for auid != 4294967295 to determine
> > > > that.
> > > > 
> > > > In practice that is a horrible way to ask if a value has been set,
> > > > because it relies on subtle implementation details and will break
> > > > every time the uid implementation in the kernel changes.
> > > > 
> > > > So add a clean way to test if the audit loginuid has been set, and
> > > > silently convert the old idiom to the cleaner and more comprehensible
> > > > new idiom.
> > > > 
> > > > Reported-By: Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > > ---
> > > > 
> > > >  include/linux/audit.h      |    5 +++++
> > > >  include/uapi/linux/audit.h |    1 +
> > > >  kernel/auditfilter.c       |   29 +++++++++++++++++++++++++++++
> > > >  kernel/auditsc.c           |    5 ++++-
> > > >  4 files changed, 39 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index a9fefe2..8a1ddde 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct
> > > > *t)
> > > > 
> > > >  #define audit_signals 0
> > > >  #endif /* CONFIG_AUDITSYSCALL */
> > > > 
> > > > +static inline bool audit_loginuid_set(struct task_struct *tsk)
> > > > +{
> > > > +	return uid_valid(audit_get_loginuid(tsk));
> > > > +}
> > > > +
> > > > 
> > > >  #ifdef CONFIG_AUDIT
> > > >  /* These are defined in audit.c */
> > > >  
> > > >  				/* Public API */
> > > > 
> > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > index 9f096f1..9554a19 100644
> > > > --- a/include/uapi/linux/audit.h
> > > > +++ b/include/uapi/linux/audit.h
> > > > @@ -246,6 +246,7 @@
> > > > 
> > > >  #define AUDIT_OBJ_TYPE	21
> > > >  #define AUDIT_OBJ_LEV_LOW	22
> > > >  #define AUDIT_OBJ_LEV_HIGH	23
> > > > 
> > > > +#define AUDIT_LOGINUID_SET	24
> > > > 
> > > >  				/* These are ONLY useful when checking
> > > >  				
> > > >  				 * at syscall exit time (AUDIT_AT_EXIT). */
> > > > 
> > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > > index 540f986..6381d17 100644
> > > > --- a/kernel/auditfilter.c
> > > > +++ b/kernel/auditfilter.c
> > > > @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > > > audit_rule *rule)> 
> > > >  		if (f->op == Audit_bad)
> > > >  		
> > > >  			goto exit_free;
> > > > 
> > > > +		/* Support legacy tests for a valid loginuid */
> > > > +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > > > +			f->type = AUDIT_LOGINUID_SET;
> > > > +			f->val = 0;
> > > > +		}
> > > > +
> > > > 
> > > >  		switch(f->type) {
> > > >  		
> > > >  		default:
> > > >  			goto exit_free;
> > > > 
> > > > @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > > > audit_rule *rule)> 
> > > >  			if (!gid_valid(f->gid))
> > > >  			
> > > >  				goto exit_free;
> > > >  			
> > > >  			break;
> > > > 
> > > > +		case AUDIT_LOGINUID_SET:
> > > > +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > > > +				goto exit_free;
> > > > +			if ((f->val != 0) && (f->val != 1))
> > > > +				goto exit_free;
> > > > +			break;
> > > > 
> > > >  		case AUDIT_PID:
> > > >  		case AUDIT_PERS:
> > > > 
> > > >  		case AUDIT_MSGTYPE:
> > > > @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct
> > > > audit_rule_data *data,> 
> > > >  		f->gid = INVALID_GID;
> > > >  		f->lsm_str = NULL;
> > > >  		f->lsm_rule = NULL;
> > > > 
> > > > +
> > > > +		/* Support legacy tests for a valid loginuid */
> > > > +		if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > > > +			f->type = AUDIT_LOGINUID_SET;
> > > > +			f->val = 0;
> > > > +		}
> > > > +
> > > > 
> > > >  		switch(f->type) {
> > > >  		case AUDIT_UID:
> > > > 
> > > >  		case AUDIT_EUID:
> > > > @@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct
> > > > audit_rule_data *data,> 
> > > >  			if (!gid_valid(f->gid))
> > > >  			
> > > >  				goto exit_free;
> > > >  			
> > > >  			break;
> > > > 
> > > > +		case AUDIT_LOGINUID_SET:
> > > > +			if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > > > +				goto exit_free;
> > > > +			if ((f->val != 0) && (f->val != 1))
> > > > +				goto exit_free;
> > > > +			break;
> > > > 
> > > >  		case AUDIT_PID:
> > > >  		case AUDIT_PERS:
> > > > 
> > > >  		case AUDIT_MSGTYPE:
> > > > @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct
> > > > audit_krule *rule,> 
> > > >  			result = audit_uid_comparator(audit_get_loginuid(current),
> > > >  			
> > > >  						  f->op, f->uid);
> > > >  			
> > > >  			break;
> > > > 
> > > > +		case AUDIT_LOGINUID_SET:
> > > > +			result = audit_comparator(audit_loginuid_set(current),
> > > > +						  f->op, f->val);
> > > > +			break;
> > > > 
> > > >  		case AUDIT_SUBJ_USER:
> > > >  		case AUDIT_SUBJ_ROLE:
> > > > 
> > > >  		case AUDIT_SUBJ_TYPE:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 3a11d34..27d0a50 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > > > 
> > > >  			if (ctx)
> > > >  			
> > > >  				result = audit_uid_comparator(tsk->loginuid, f->op, 
> > f->uid);
> > > >  			
> > > >  			break;
> > > > 
> > > > +		case AUDIT_LOGINUID_SET:
> > > > +			result = audit_comparator(audit_loginuid_set(tsk), f->op, 
> > f->val);
> > > > +			break;
> > > > 
> > > >  		case AUDIT_SUBJ_USER:
> > > >  		case AUDIT_SUBJ_ROLE:
> > > > 
> > > >  		case AUDIT_SUBJ_TYPE:
> > > > @@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > 
> > > >  	unsigned int sessionid;
> > > >  
> > > >  #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
> > > > 
> > > > -	if (uid_valid(task->loginuid))
> > > > +	if (audit_loginuid_set(task))
> > > > 
> > > >  		return -EPERM;
> > > >  
> > > >  #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
> > > >  
> > > >  	if (!capable(CAP_AUDIT_CONTROL))
> > > 
> > > - RGB
> > > 
> > > --
> > > Richard Guy Briggs <rbriggs@redhat.com>
> > > Senior Software Engineer
> > > AMER ENG Base Operating Systems
> > > Remote, Canada, Ottawa
> > > Voice: 1.647.777.2635
> > > Internal: (81) 32635
> > > 
> > > --
> > > 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
> AMER ENG Base Operating Systems
> Remote, Ottawa, Canada
> Voice: 1.647.777.2635
> Internal: (81) 32635

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data

[Eric Paris]

On Thu, 2013-05-09 at 09:29 -0400, Steve Grubb wrote:
> On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
> > On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> > > Andrew Morton <akpm@linux-foundation.org> writes:
> > > > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@redhat.com> 
> wrote:
> > > >> audit rule additions containing "-F auid!=4294967295" were failing with
> > > >> EINVAL.> 
> > > The only case where this appears to make the least little bit of sense
> > > is if the goal of the test is to test to see if an audit logloginuid
> > > has been set at all.  In which case depending on a test against
> > > 4294967295 is bogus because you are depending on an intimate internal
> > > kernel implementation detail.
> > > 
> > > How about something like my untested patch below that add an explicit
> > > operation to test if loginuid has been set?
> > 
> > Sorry for the delay in testing this, I had another urgent bug and a
> > belligerent test box...
> > 
> > I like this approach better than mine now that I understand it.  I've
> > tested the patch below without any changes.  It works as expected with
> > my previous test case.  I don't know if a Signed-off-by: is appropriate
> > for me in this case, but I'll throw in a:
> > 
> > Tested-by: Richard Guy Briggs <rbriggs@redhat.com>
> > 
> > and recommend a:
> > 
> > Reported-By: Steve Grubb <sbrubb@redhat.com>
> 
> If this is the approved patch, can it be put in stable? The audit system 
> hasn't worked as intended since January.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/auditsc.c?id=780a7654cee8d61819512385e778e4827db4bfbc

Should be queued for 3.7 and later now.