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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit