Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data
Steve Grubb sgr...@redhat.com writes: On Tuesday, April 09, 2013 02:39:32 AM Eric W. Biederman wrote: Andrew Morton a...@linux-foundation.org writes: On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs r...@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 -- 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
On Wed, Apr 10, 2013 at 11:02:43AM -0700, Eric W. Biederman wrote: Richard Guy Briggs r...@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 rbri...@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
