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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130410184609.GG28504@madcap2.tricolour.ca>
Date:	Wed, 10 Apr 2013 14:46:09 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-audit@...hat.com,
	Eric Paris <eparis@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in
 audit rules and data

On Wed, Apr 10, 2013 at 11:02:43AM -0700, Eric W. Biederman wrote:
> Richard Guy Briggs <rgb@...hat.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@...hat.com>
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/