linux-kernel - Re: [PATCH ghak109 V1] audit: link integrity evm_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Mar 2019 19:48:09 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, sgrubb@...hat.com,
        omosnace@...hat.com, Eric Paris <eparis@...isplace.org>,
        Serge Hallyn <serge@...lyn.com>, zohar@...ux.ibm.com,
        mjg59@...gle.com
Subject: Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record
 to syscall event

On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@...hat.com> wrote:
> In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> verified xattrs"), the call to audit_log_start() is missing a context to
> link it to an audit event. Since this event is in user context, add
> the process' syscall context to the record.
>
> In addition, the orphaned keyword "locked" appears in the record.
> Normalize this by changing it to "xattr=(locked)".
>
> Please see the github issue
> https://github.com/linux-audit/audit-kernel/issues/109
>
> Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> ---
>  security/integrity/evm/evm_secfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index 015aea8fdf1e..4171d174e9da 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -192,7 +192,8 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
>         if (count > XATTR_NAME_MAX)
>                 return -E2BIG;
>
> -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
> +       ab = audit_log_start(audit_context(), GFP_KERNEL,
> +                            AUDIT_INTEGRITY_EVM_XATTR);

This part is fine.

>         if (!ab)
>                 return -ENOMEM;
>
> @@ -222,7 +223,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
>                 inode_lock(inode);
>                 err = simple_setattr(evm_xattrs, &newattrs);
>                 inode_unlock(inode);
> -               audit_log_format(ab, "locked");
> +               audit_log_format(ab, "xattr=(locked)");

Two things come to mind:

* While we can clearly trust the string above, should we be logging
the xattr field value as an untrusted string so it is consistent with
how we record other xattr names?
* I'm not sure you can ever have parens in a xattr (I would hope not),
but if we are going to use the xattr field, perhaps we should simply
stick with the name as provided (".") so we don't ever run afoul of
xattr names?  I'm curious to hear what the IMA/EVM folks think of
this.

-- 
paul moore
www.paul-moore.com