On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote: > On 11/21/2016 04:50 PM, Paul Moore wrote: > > On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@xxxxxxxxxx> wrote: > >> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same > >> thing > >> as the event type and is missing the uid and results field. The bigger > >> issue is that in some failure cases no event is emitted. This patch > >> fixes the noted problems. > > A potential problem with this patch is that it changes the semantic > meaning of this audit record, from meaning "a policy was loaded into the > kernel" to "there was an attempt to load a policy, check the res= field > to determine whether it succeeded". So anything in userspace that used > the presence of this audit record to determine whether or not policy was > successfully loaded (e.g. audit2allow -l) will be confused. I really can't have implicit success. I need to have a field to point to that says yes/no. It can be hard coded to res=1 (success), but it needs to be there. > While there were failure cases that would still generate the audit record > previously, those were all selinuxfs node creation failures; the policy > would nonetheless have been loaded into the kernel and would be active > at that point, so saying res=0 is somewhat misleading. OK. We can move the point where res=1 is set. But I would think that its a requirement to have an audit record that states that policy failed to load. FMT_MSA.3 Static Attribute Initialization. Auditable events: All modifications of the initial value of security attributes. I would think this means changes such as booleans, modifying labels, loading a new policy, or failure to load a policy. > This overlapswith https://github.com/SELinuxProject/selinux-kernel/issues/1, > which highlights the fact that we can end up in an intermediate state where > policy is loaded but selinuxfs (particularly booleans, class/*, and > policy_capabilities/*) has not been regenerated. I see. That should be an audited event. If you have a datacenter with a thousand machines, its best to get this in the audit trail so it can be alerted on at a central collector. So, what should we do about the patch? I'm willing to modify it. -Steve > >> Signed-off-by: Steve Grubb <sgrubb@xxxxxxxxxx> > > > > First off, for patches such as these, I think it is good to CC the > > affected subsystem, SELinux in this case (fixed). > > > > Beyond that, I'm a little concerned that you adding fields to record > > in the middle. In the past you've warned against inserting fields in > > the middle of the record, or reordering fields in general ("you'll > > break the world") due to some poor userspace practices, yet you do > > these exact things when it suits you. > > > > We need a consistent message when it comes to userspace record > > processing so we know what we can do in the kernel without causing > > massive failure. > > > >> --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c 2016-11-16 > >> 15:16:34.738723900 -0500 +++ > >> linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c > >> 2016-11-21 12:16:08.046787604 -0500 @@ -494,6 +494,7 @@ static ssize_t > >> sel_write_load(struct fil > >> > >> { > >> > >> ssize_t length; > >> void *data = NULL; > >> > >> + unsigned int result = 0; > >> > >> mutex_lock(&sel_mutex); > >> > >> @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil > >> > >> length = sel_make_bools(); > >> if (length) > >> > >> - goto out1; > >> + goto out; > >> > >> length = sel_make_classes(); > >> if (length) > >> > >> - goto out1; > >> + goto out; > >> > >> length = sel_make_policycap(); > >> if (length) > >> > >> - goto out1; > >> + goto out; > >> > >> length = count; > >> > >> + result = 1; > >> > >> -out1: > >> > >> +out: > >> audit_log(current->audit_context, GFP_KERNEL, > >> AUDIT_MAC_POLICY_LOAD, > >> > >> - "policy loaded auid=%u ses=%u", > >> + "uid=%u auid=%u ses=%u res=%u", > >> + from_kuid(&init_user_ns, task_uid(current)), > >> > >> from_kuid(&init_user_ns, audit_get_loginuid(current)), > >> > >> - audit_get_sessionid(current)); > >> -out: > >> + audit_get_sessionid(current), result); > >> + > >> > >> mutex_unlock(&sel_mutex); > >> vfree(data); > >> return length; > >> > >> -- > >> Linux-audit mailing list > >> Linux-audit@xxxxxxxxxx > >> https://www.redhat.com/mailman/listinfo/linux-audit _______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
