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

[Richard Guy Briggs]

On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> writes:
> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@xxxxxxxxxx> 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@xxxxxxxxxx>

and recommend a:

Reported-By: Steve Grubb <sbrubb@xxxxxxxxxx>


> Eric
> 
> From: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> 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@xxxxxxxxxx> wrote:
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/