From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Paris <eparis@redhat.com>
Subject: Re: [PATCH 2/8] integrity: IMA as an integrity service provider
Date: Fri, 06 Mar 2009 17:07:11 -0500
Message-ID: <1236377231.3602.8.camel@localhost.localdomain>
References: <cover.1233946566.git.zohar@linux.vnet.ibm.com>
	<3323eec921efd815178a23107ab63588c605c0b2.1233946566.git.zohar@linux.vnet.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Return-path: <linux-audit-bounces@redhat.com>
In-Reply-To: <3323eec921efd815178a23107ab63588c605c0b2.1233946566.git.zohar@linux.vnet.ibm.com>
List-Unsubscribe: <https://www.redhat.com/mailman/listinfo/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=unsubscribe>
List-Archive: <https://www.redhat.com/archives/linux-audit>
List-Post: <mailto:linux-audit@redhat.com>
List-Help: <mailto:linux-audit-request@redhat.com?subject=help>
List-Subscribe: <https://www.redhat.com/mailman/listinfo/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=subscribe>
Sender: linux-audit-bounces@redhat.com
Errors-To: linux-audit-bounces@redhat.com
To: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Safford <safford@watson.ibm.com>, linux-audit@redhat.com, James Morris <jmorris@namei.org>, Mimi Zohar <zohar@us.ibm.com>
List-Id: linux-audit@redhat.com

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....

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?

> +               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);