Re: [PATCH 02/13] AppArmor: basic auditing infrastructure.

[John Johansen <john.johansen@canonical.com>]

On 07/15/2010 08:18 AM, Eric Paris wrote:
> On Wed, Jul 14, 2010 at 8:43 PM, John Johansen
> <john.johansen@canonical.com> wrote:
>> Update lsm_audit for AppArmor specific data, and add the core routines for
>> AppArmor uses for auditing.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
> 
>> + * Currently AppArmor auditing is fed straight into the audit framework.
>> + *
>> + * TODO:
>> + * convert to LSM audit
> 
> 
oops, I missed that one, it should have been removed.

>> +       if (sa->aad.profile) {
>> +               struct aa_profile *profile = sa->aad.profile;
>> +               pid_t pid;
>> +               rcu_read_lock();
>> +               pid = tsk->real_parent->pid;
>> +               rcu_read_unlock();
>> +               audit_log_format(ab, " parent=%d", pid);
>> +               audit_log_format(ab, " profile=");
>> +               if (profile->ns != root_ns) {
>> +                       audit_log_format(ab, ":");
>> +                       audit_log_untrustedstring(ab, profile->ns->base.hname);
>> +                       audit_log_format(ab, "://");
>> +               }
>> +               audit_log_untrustedstring(ab, profile->base.hname);
>> +       }
> 
> what does this message look like?  I don't think it fits the nice
> key=value rules of the audit system....   Are you sure this is what
> you want?
>
it looks like
profile=:ns_name://profile_name

which could be fed straight back in as a valid profile name.  Profile names can
be expressed relative to the namespace, or absolute with the profile namespace name
prepended.  In this case relative namespace reporting is used only for the root_ns
because that is what was traditionally done.

So it is a single value following the rules.

The alternative of outputting the profile namespace name as a separate is also valid,
and I am actually indifferent as to which way it is reported.


>> +#define COMMON_AUDIT_DATA_INIT_NONE(_d) \
>> +       do { \
>> +               memset((_d), 0, sizeof(struct common_audit_data)); \
>> +               (_d)->type = LSM_AUDIT_DATA_NONE; \
>> +       } while (0)
>> +
>> +#endif /* __AA_AUDIT_H */
> 
> Why are you redefining this?  You should just use
> COMMON_AUDIT_DATA_INIT(sa, NONE);

oops, I actually have that patch, and missed applying to the branch, this was
originally added when COMMON_AUDIT_DATA_INIT(sa, NONE) didn't work.  

thanks Eric