Re: [PATCH] audit: add containerid support for IMA-audit
On 05/18/2018 08:53 AM, Mimi Zohar wrote:
On Fri, 2018-05-18 at 07:49 -0400, Stefan Berger wrote:
> On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:
[...]
>>>> auxiliary record either by being converted to a syscall auxiliary record
>>>> by using current->audit_context rather than NULL when calling
>>>> audit_log_start(), or creating a local audit_context and calling
>>> ima_parse_rule() is invoked when setting a policy by writing it into
>>> /sys/kernel/security/ima/policy. We unfortunately don't have the
>>> current->audit_context in this case.
>> Sure you do. What process writes to that file? That's the one we care
>> about, unless it is somehow handed off to a queue and processed later in
>> a different context.
> I just printk'd it again. current->audit_context is NULL in this case.
The builtin policy rules are loaded at __init. Subsequently a custom
policy can replace the builtin policy rules by writing to the
securityfs file. Is the audit_context NULL in both cases?
The builtin policy rules are not parsed from what I can see. They seem
to be encoded in the format the parser would produce.
I get current->audit_context == NULL in the case the user cat's the
policy into IMA's policy securityfs file.
>>> If so, which ones? We could probably refactor the current
>>> integrity_audit_message() and have ima_parse_rule() call into it to get
>>> those fields as well. I suppose adding new fields to it wouldn't be
>>> considered breaking user space?
>> Changing the order of existing fields or inserting fields could break
>> stuff and is strongly discouraged without a good reason, but appending
>> fields is usually the right way to add information.
>>
>> There are exceptions, and in this case, I'd pick the "more
standard" of
>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
>> with that, abandoning the other format, renaming the less standard
>> version of the record (ima_parse_rule?) and perhpas adopting that
>> abandonned format for the new record type while using
>> current->audit_context.
This sounds right, other than "type=INTEGRITY_RULE" (1805) for
ima_audit_measurement(). Could we rename type=1805 to be
So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good name
for the 'type'. Now in this case I would not want to 'break user space'.
The only change I was going to make was to what ima_parse_rule() produces.
INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit
message could be named INTEGRITY_RULE or, if that would be confusing,
INTEGRITY_POLICY_RULE.
For 1806, as we would use it in ima_parse_rule(), we could change that
in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better
for IMA to produce but that's inconsistent then.
> 1806 would be in sync with INTEGRITY_RULE now for process related info.
> If this looks good, I'll remove the dependency on your local context
> creation and post the series.
>
> The justification for the change is that the INTEGRITY_RULE, as produced
> by ima_parse_rule(), is broken.
Post which series? The IMA namespacing patch set? This change should
be upstreamed independently of IMA namespacing.
Without Richard's local context patch it may just be one or two patches.
Stefan
Mimi