linux-kernel - Re: [PATCH v2] audit: vfs: fix audit records error when write to a file

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] [day] [month] [year] [list]
Message-ID: <541A40B6.30507@huawei.com>
Date:	Thu, 18 Sep 2014 10:17:26 +0800
From:	hujianyang <hujianyang@...wei.com>
To:	<linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<rgb@...hat.com>
CC:	<viro@...iv.linux.org.uk>, <eparis@...hat.com>,
	<jlayton@...marydata.com>
Subject: Re: [PATCH v2] audit: vfs: fix audit records error when write to
 a file

>
> Could you add me to your Cc: list on this thread, please?  I'm
> interested in the outcome...  Thanks!
>

Hi Richard,

I've resend a v2 patch and now quote it in the end of this mail
for you.

I'm sorry to say the previously work of mine seems useless. Moving
audit_inode() to the O_CREAT case in lookup_open() may miss some
conditions when we create a file.

fs/namei.c lookup_open() line 2828
"""
        if ((nd->flags & LOOKUP_OPEN) && dir_inode->i_op->atomic_open) {
                return atomic_open(nd, dentry, path, file, op, got_write,
                                   need_lookup, opened);
        }
"""

We will miss this branch and other potential cases if we apply my
patch.

There is something wrong with audit, I think. But I don't know where.
One good way to solve this problem is go back to commit bfcec708 as
Jeff mentioned in commit 33e2208a and find the reason why the audit
of create operations is wrong. Do we have another way to fix it than
changing the third parameter of audit_inode() to LOOKUP_PARENT which
lead the audit of write confused?

I don't know much about audit. One of my colleagues is dealing with
this problem now. I'm very glad if someone else could help us.

Thanks~!

Hu


On 2014/9/9 10:34, hujianyang wrote:
> Changes from v1:
> 
>    * Move audit_inode() to the beginning of O_CREAT case in
>      lookup_open() to avoid missing audit for ROFS error. This
>      lack is spotted by Jeff Layton <jeff.layton@...marydata.com>
> 
> commit 33e2208acfc1
> 
> audit: vfs: fix audit_inode call in O_CREAT case of do_last
> 
> fix a regression in auditing of open(..., O_CREAT) syscalls but
> introduce a new problem which lead the records of write operation
> confusion.
> 
> This error can be reproduced by these steps:
> 
> 	touch /etc/test
> 	echo "-w /etc/test" >>/etc/audit/audit.rules
> 	/etc/init.d/auditd restart
> 
> 	echo "abc" >> /etc/test
> 
> audit_name records are:
> 
> type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097 dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
> type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> 
> but if we revert commit 33e2208acfc1, records are correct:
> 
> type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> 
> We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
> of do_last() because this branch don't really know if vfs need to
> create a new file. There is no need to do vfs_create() if we open
> an existing file with O_CREAT flag and write to it. But this
> audit_inode() in O_CREAT case will record a msg as we create a new
> file and confuse the records of write.
> 
> This patch moves the audit for create operation to where a file
> really need to be created, the O_CREAT case in lookup_open().
> We have to add the pointer of struct filename as a parameter of
> lookup_open(). By doing this, the records of both create and write
> are correct.
> 
> Signed-off-by: hujianyang <hujianyang@...wei.com>
> ---
>  fs/namei.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index a996bb4..ca4a831 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2808,7 +2808,8 @@ looked_up:
>  static int lookup_open(struct nameidata *nd, struct path *path,
>  			struct file *file,
>  			const struct open_flags *op,
> -			bool got_write, int *opened)
> +			bool got_write, int *opened,
> +			struct filename *name)
>  {
>  	struct dentry *dir = nd->path.dentry;
>  	struct inode *dir_inode = dir->d_inode;
> @@ -2841,6 +2842,8 @@ static int lookup_open(struct nameidata *nd, struct path *path,
>  	/* Negative dentry, just create the file */
>  	if (!dentry->d_inode && (op->open_flag & O_CREAT)) {
>  		umode_t mode = op->mode;
> +
> +		audit_inode(name, dir, LOOKUP_PARENT);
>  		if (!IS_POSIXACL(dir->d_inode))
>  			mode &= ~current_umask();
>  		/*
> @@ -2926,7 +2929,6 @@ static int do_last(struct nameidata *nd, struct path *path,
>  		if (error)
>  			return error;
> 
> -		audit_inode(name, dir, LOOKUP_PARENT);
>  		error = -EISDIR;
>  		/* trailing slashes? */
>  		if (nd->last.name[nd->last.len])
> @@ -2945,7 +2947,7 @@ retry_lookup:
>  		 */
>  	}
>  	mutex_lock(&dir->d_inode->i_mutex);
> -	error = lookup_open(nd, path, file, op, got_write, opened);
> +	error = lookup_open(nd, path, file, op, got_write, opened, name);
>  	mutex_unlock(&dir->d_inode->i_mutex);
> 
>  	if (error <= 0) {
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/