From mboxrd@z Thu Jan  1 00:00:00 1970
From: ebiederm@xmission.com (Eric W. Biederman)
Subject: Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data
Date: Wed, 10 Apr 2013 11:02:43 -0700
Message-ID: <871uaiz2kc.fsf@xmission.com>
References: <1363807097-13073-1-git-send-email-rgb@redhat.com>
	<20130408164622.284f48c65341396aa8dbd220@linux-foundation.org>
	<87ip3w59gr.fsf@xmission.com>
	<20130409201558.GE31242@madcap2.tricolour.ca>
Mime-Version: 1.0
Content-Type: text/plain
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20130409201558.GE31242@madcap2.tricolour.ca> (Richard Guy
	Briggs's message of "Tue, 9 Apr 2013 16:15:58 -0400")
Sender: linux-kernel-owner@vger.kernel.org
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, linux-audit@redhat.com, Eric Paris <eparis@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>
List-Id: linux-audit@redhat.com

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