Re: [PATCH 2/8] integrity: IMA as an integrity service provider

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Fri, 2009-03-06 at 17:07 -0500, Eric Paris wrote:
> I'm very slow to the game, I know, but today was the first kernel that I
> built from linux-next with IMA on.  I have a comment, and hopefully more
> to come....

np

> On Fri, 2009-02-06 at 14:52 -0500, Mimi Zohar wrote:
> > +void integrity_audit_msg(int audit_msgno, struct inode *inode,
> > +                        const unsigned char *fname, const char *op,
> > +                        const char *cause, int result, int audit_info)
> > +{
> > +       struct audit_buffer *ab;
> > +
> > +       if (!ima_audit && audit_info == 1) /* Skip informational messages */
> > +               return;
> > +
> > +       ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
> > +       audit_log_format(ab, "integrity: pid=%d uid=%u auid=%u",
> > +                        current->pid, current->cred->uid,
> > +                        audit_get_loginuid(current));
> > +       audit_log_task_context(ab);
> > +       switch (audit_msgno) {
> > +       case AUDIT_INTEGRITY_DATA:
> > +       case AUDIT_INTEGRITY_METADATA:
> > +       case AUDIT_INTEGRITY_PCR:
> > +               audit_log_format(ab, " op=%s cause=%s", op, cause);
> 
> There are cases where (at least) cause can be a multiword string.  These
> should be using audit_log_string() instead of %s.  I'm starting to frown
> on %s in audit log more and more heavily these days, actually I don't
> think I should let any more users of %s in at all.  If you want I'll go
> through and change all of these, but I want to make sure that you will
> be ok with multiword string being either "" or translated.  An example
> of the problem I see would be in say ima_update_policy() where you have
> const char *cause = "already exists";  Currently that record will get
> emitted like:
> 
> blah cause=already exists blah
> 
> which screws up parsers.  It should be:
> 
> blah cause="already exists" blah
> 
> make sense?

Oops, that should have been hyphenated, like "open_writers" below, but
quoting it, and "open writers", definitely makes more sense. 
 
type=INTEGRITY_PCR msg=audit(1235867814.206:6): integrity: pid=2089
uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:initrc_t:s0
op=invalid_pcr cause=open_writers comm="update-reader.c"
name="sh-thd-1235847728" dev=sda6 ino=1635223 res=0

> > +               break;
> > +       case AUDIT_INTEGRITY_HASH:
> > +               audit_log_format(ab, " op=%s hash=%s", op, cause);

> > +               break;
> > +       case AUDIT_INTEGRITY_STATUS:
> > +       default:
> > +               audit_log_format(ab, " op=%s", op);
> > +       }
> > +       audit_log_format(ab, " comm=");
> > +       audit_log_untrustedstring(ab, current->comm);
> > +       if (fname) {
> > +               audit_log_format(ab, " name=");
> > +               audit_log_untrustedstring(ab, fname);
> > +       }
> > +       if (inode)
> > +               audit_log_format(ab, " dev=%s ino=%lu",
> > +                                inode->i_sb->s_id, inode->i_ino);
> > +       audit_log_format(ab, " res=%d", result);
> > +       audit_log_end(ab);

Thanks,

Mimi