From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mimi Zohar Subject: Re: [PATCH 2/8] integrity: IMA as an integrity service provider Date: Mon, 09 Mar 2009 07:07:30 -0400 Message-ID: <1236596850.2944.20.camel@localhost.localdomain> References: <3323eec921efd815178a23107ab63588c605c0b2.1233946566.git.zohar@linux.vnet.ibm.com> <1236377231.3602.8.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1236377231.3602.8.camel@localhost.localdomain> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Eric Paris Cc: David Safford , linux-audit@redhat.com, James Morris , Mimi Zohar List-Id: linux-audit@redhat.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