Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; wrote: > On 05/30/2018 07:34 PM, Richard Guy Briggs wrote: >> On 2018-05-30 17:38, Stefan Berger wrote: >>> On 05/30/2018 05:22 PM, Paul Moore wrote: >>>> On Wed, May 30, 2018 at 9:08 AM, Stefan Berger >>>> <stefanb(a)linux.vnet.ibm.com&gt; wrote: >>>>> On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: >>>>>> On 2018-05-24 16:11, Stefan Berger wrote: >>>>>>> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >>>>>>> the IMA "audit" policy action. This patch defines >>>>>>> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >>>>>>> >>>>>>> With this change we now call integrity_audit_msg_common() to get >>>>>>> common integrity auditing fields. This now produces the following >>>>>>> record when parsing an IMA policy rule: >>>>>>> >>>>>>> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure >>>>>>> \ >>>>>>> fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >>>>>>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>>>>>> op=policy_update cause=parse_rule comm="echo" >>>>>>> exe="/usr/bin/echo" \ >>>>>>> tty=tty2 res=1 >>>>>>> >>>>>>> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; >>>>>>> --- >>>>>>> include/uapi/linux/audit.h | 3 ++- >>>>>>> security/integrity/ima/ima_policy.c | 5 +++-- >>>>>>> 2 files changed, 5 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >>>>>>> index 4e61a9e05132..776e0abd35cf 100644 >>>>>>> --- a/include/uapi/linux/audit.h >>>>>>> +++ b/include/uapi/linux/audit.h >>>>>>> @@ -146,7 +146,8 @@ >>>>>>> #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity >>>>>>> enable >>>>>>> status */ >>>>>>> #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ >>>>>>> #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs >>>>>>> */ >>>>>>> -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ >>>>>>> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy >>>>>>> msgs */ >>>>>>> +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ >>>>>>> #define AUDIT_KERNEL 2000 /* Asynchronous >>>>>>> audit >>>>>>> record. NOT A REQUEST. */ >>>>>>> diff --git a/security/integrity/ima/ima_policy.c >>>>>>> b/security/integrity/ima/ima_policy.c >>>>>>> index 3aed25a7178a..a8ae47a386b4 100644 >>>>>>> --- a/security/integrity/ima/ima_policy.c >>>>>>> +++ b/security/integrity/ima/ima_policy.c >>>>>>> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct >>>>>>> ima_rule_entry *entry) >>>>>>> int result = 0; >>>>>>> ab = integrity_audit_log_start(NULL, GFP_KERNEL, >>>>>>> - AUDIT_INTEGRITY_RULE); >>>>>>> + AUDIT_INTEGRITY_POLICY_RULE); >>>>>> Is it possible to connect this record to a syscall by replacing the >>>>>> first parameter (NULL) by current->context? >>>> We're likely going to need to "associate" this record (audit speak for >>>> making the first parameter non-NULL) with others for the audit >>>> container ID work. If you do it now, Richard's patches will likely >>>> get a few lines smaller and that will surely make him a bit happier :) >>> Richard is also introducing a local context that we can then create and >>> use >>> instead of the NULL. Can we not use that then? >> That is for records for which there is no syscall or user associated. >> >> In fact there is another recent change that would be better to use than >> current->audit_context, which is the function audit_context(). >> See commit cdfb6b3 ("audit: use inline function to get audit context"). >> >>> Steven seems to say: "We don't want to add syscall records to everything. >>> That messes up schemas and existing code. The integrity events are 1 >>> record >>> in size and should stay that way. This saves disk space and improves >>> readability." >>> >>>>> We would have to fix current->context in this case since it is NULL. We >>>>> get >>>>> to this location by root cat'ing a policy or writing a policy filename >>>>> into >>>>> /sys/kernel/security/ima/policy. >>>> Perhaps I'm missing something, but current in this case should point >>>> to the process which is writing to the policy file, yes? >>> Yes, but current->context is NULL for some reason. >> Is it always this way? If it isn't, which it should not be, we should >> find out why. Well, we should find out why this is NULL anyways, since >> it shouldn't be. > > When someone writes a policy for IMA into securityfs, it's always NULL. > There's another location where IMA uses the current->audit_context, and > that's here: > > https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima... > > At this location we sometimes see a (background) process with an > audit_context but in the majority of cases it's current->audit_context is > NULL. Starting a process as root or also non-root user, with the appropriate > IMA audit policy rules set, we always see a NULL audit_context here. What does your audit configuration look like? Depending on your configuration a NULL audit_context can be expected, see audit_dummy_context(). I believe the default Fedora audit config will leave you with a NULL audit_context for all processes. I also believe that unless you explicitly set "audit=1" on the kernel command line the init/systemd process will have a NULL audit_context (there was actually a range of kernels where even setting "audit=1" wouldn't be sufficient due to a bug we fixed a little while ago). Look at the audit_alloc() function, it is called when a new process is fork'd and is responsible for allocating a new audit_context. If the currently loaded audit config dictates that auditing is to be disabled for this new process (state == AUDIT_DISABLED) then an audit_context is not allocated and current->context remains NULL.