[PATCH] fsnotify: Fix handling of renames in audit

[PATCH] fsnotify: Fix handling of renames in audit

[Jan Kara]

Commit e9fd702a58c4 (audit: convert audit watches to use fsnotify
instead of inotify) broke handling of renames in audit. Audit code wants
to update inode number of an inode corresponding to watched name in a
directory. When something gets renamed into a directory to a watched
name, inotify previously passed moved inode to audit code however new
fsnotify code passes directory inode where the change happened. That
confuses audit and it starts watching parent directory instead of a file
in a directory.

This can be observed for example by doing:
cd /tmp
touch foo bar
auditctl -w /tmp/foo
touch foo
mv bar foo
touch foo

In audit log we see events like:
type=CONFIG_CHANGE msg=audit(1423563584.155:90): auid=1000 ses=2
op="updated rules" path="/tmp/foo" key=(null) list=4 res=1
...
type=PATH msg=audit(1423563584.155:91): item=2 name="bar" inode=1046884
dev=08:0
2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=DELETE
type=PATH msg=audit(1423563584.155:91): item=3 name="foo" inode=1046842
dev=08:0
2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=DELETE
type=PATH msg=audit(1423563584.155:91): item=4 name="foo" inode=1046884
dev=08:0
2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=CREATE
...
and that's it - we see event for the first touch after creating the
audit rule, we see events for rename but we don't see any event for the
last touch. However we start seeing events for unrelated stuff happening
in /tmp.

Fix the problem by passing moved inode as data in the FS_MOVED_FROM and
FS_MOVED_TO events instead of the directory where the change happens.
This doesn't introduce any new problems because noone besides
audit_watch.c cares about the passed value:
fs/notify/fanotify/fanotify.c cares only about FSNOTIFY_EVENT_PATH events.
fs/notify/dnotify/dnotify.c doesn't care about passed 'data' value at all.
fs/notify/inotify/inotify_fsnotify.c uses 'data' only for FSNOTIFY_EVENT_PATH.
kernel/audit_tree.c doesn't care about passed 'data' at all.
kernel/audit_watch.c expects moved inode as 'data'.

CC: linux-audit@redhat.com
CC: Paul Moore <paul@paul-moore.com>
CC: Eric Paris <eparis@redhat.com>
CC: stable@vger.kernel.org
Fixes: e9fd702a58c49dbb14481dca88dad44758da393a
Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/fsnotify.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 1c804b057fb1..7ee1774edee5 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -101,8 +101,10 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 		new_dir_mask |= FS_ISDIR;
 	}
 
-	fsnotify(old_dir, old_dir_mask, old_dir, FSNOTIFY_EVENT_INODE, old_name, fs_cookie);
-	fsnotify(new_dir, new_dir_mask, new_dir, FSNOTIFY_EVENT_INODE, new_name, fs_cookie);
+	fsnotify(old_dir, old_dir_mask, source, FSNOTIFY_EVENT_INODE, old_name,
+		 fs_cookie);
+	fsnotify(new_dir, new_dir_mask, source, FSNOTIFY_EVENT_INODE, new_name,
+		 fs_cookie);
 
 	if (target)
 		fsnotify_link_count(target);
-- 
2.1.4

Re: [PATCH] fsnotify: Fix handling of renames in audit

[Paul Moore]

On Tuesday, February 10, 2015 04:00:12 PM Jan Kara wrote:
> Commit e9fd702a58c4 (audit: convert audit watches to use fsnotify
> instead of inotify) broke handling of renames in audit. Audit code wants
> to update inode number of an inode corresponding to watched name in a
> directory. When something gets renamed into a directory to a watched
> name, inotify previously passed moved inode to audit code however new
> fsnotify code passes directory inode where the change happened. That
> confuses audit and it starts watching parent directory instead of a file
> in a directory.
> 
> This can be observed for example by doing:
> cd /tmp
> touch foo bar
> auditctl -w /tmp/foo
> touch foo
> mv bar foo
> touch foo
> 
> In audit log we see events like:
> type=CONFIG_CHANGE msg=audit(1423563584.155:90): auid=1000 ses=2
> op="updated rules" path="/tmp/foo" key=(null) list=4 res=1
> ...
> type=PATH msg=audit(1423563584.155:91): item=2 name="bar" inode=1046884
> dev=08:0
> 2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=DELETE
> type=PATH msg=audit(1423563584.155:91): item=3 name="foo" inode=1046842
> dev=08:0
> 2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=DELETE
> type=PATH msg=audit(1423563584.155:91): item=4 name="foo" inode=1046884
> dev=08:0
> 2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=CREATE
> ...
> and that's it - we see event for the first touch after creating the
> audit rule, we see events for rename but we don't see any event for the
> last touch. However we start seeing events for unrelated stuff happening
> in /tmp.
> 
> Fix the problem by passing moved inode as data in the FS_MOVED_FROM and
> FS_MOVED_TO events instead of the directory where the change happens.
> This doesn't introduce any new problems because noone besides
> audit_watch.c cares about the passed value:
> fs/notify/fanotify/fanotify.c cares only about FSNOTIFY_EVENT_PATH events.
> fs/notify/dnotify/dnotify.c doesn't care about passed 'data' value at all.
> fs/notify/inotify/inotify_fsnotify.c uses 'data' only for
> FSNOTIFY_EVENT_PATH. kernel/audit_tree.c doesn't care about passed 'data'
> at all.
> kernel/audit_watch.c expects moved inode as 'data'.
> 
> CC: linux-audit@redhat.com
> CC: Paul Moore <paul@paul-moore.com>
> CC: Eric Paris <eparis@redhat.com>
> CC: stable@vger.kernel.org
> Fixes: e9fd702a58c49dbb14481dca88dad44758da393a
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  include/linux/fsnotify.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

I can confirm this is indeed broken.

Thanks for the patch.  I'm building a test kernel as I type this, assuming all 
goes well I'll send this to Linus with the other audit patches for v3.20.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] fsnotify: Fix handling of renames in audit

[Jan Kara]

On Tue 10-02-15 19:58:35, Paul Moore wrote:
> On Tuesday, February 10, 2015 04:00:12 PM Jan Kara wrote:
> > Commit e9fd702a58c4 (audit: convert audit watches to use fsnotify
> > instead of inotify) broke handling of renames in audit. Audit code wants
> > to update inode number of an inode corresponding to watched name in a
> > directory. When something gets renamed into a directory to a watched
> > name, inotify previously passed moved inode to audit code however new
> > fsnotify code passes directory inode where the change happened. That
> > confuses audit and it starts watching parent directory instead of a file
> > in a directory.
> > 
> > This can be observed for example by doing:
> > cd /tmp
> > touch foo bar
> > auditctl -w /tmp/foo
> > touch foo
> > mv bar foo
> > touch foo
> > 
> > In audit log we see events like:
> > type=CONFIG_CHANGE msg=audit(1423563584.155:90): auid=1000 ses=2
> > op="updated rules" path="/tmp/foo" key=(null) list=4 res=1
> > ...
> > type=PATH msg=audit(1423563584.155:91): item=2 name="bar" inode=1046884
> > dev=08:0
> > 2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=DELETE
> > type=PATH msg=audit(1423563584.155:91): item=3 name="foo" inode=1046842
> > dev=08:0
> > 2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=DELETE
> > type=PATH msg=audit(1423563584.155:91): item=4 name="foo" inode=1046884
> > dev=08:0
> > 2 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=CREATE
> > ...
> > and that's it - we see event for the first touch after creating the
> > audit rule, we see events for rename but we don't see any event for the
> > last touch. However we start seeing events for unrelated stuff happening
> > in /tmp.
> > 
> > Fix the problem by passing moved inode as data in the FS_MOVED_FROM and
> > FS_MOVED_TO events instead of the directory where the change happens.
> > This doesn't introduce any new problems because noone besides
> > audit_watch.c cares about the passed value:
> > fs/notify/fanotify/fanotify.c cares only about FSNOTIFY_EVENT_PATH events.
> > fs/notify/dnotify/dnotify.c doesn't care about passed 'data' value at all.
> > fs/notify/inotify/inotify_fsnotify.c uses 'data' only for
> > FSNOTIFY_EVENT_PATH. kernel/audit_tree.c doesn't care about passed 'data'
> > at all.
> > kernel/audit_watch.c expects moved inode as 'data'.
> > 
> > CC: linux-audit@redhat.com
> > CC: Paul Moore <paul@paul-moore.com>
> > CC: Eric Paris <eparis@redhat.com>
> > CC: stable@vger.kernel.org
> > Fixes: e9fd702a58c49dbb14481dca88dad44758da393a
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  include/linux/fsnotify.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> I can confirm this is indeed broken.
> 
> Thanks for the patch.  I'm building a test kernel as I type this, assuming all 
> goes well I'll send this to Linus with the other audit patches for v3.20.
  Actually, Andrew has picked up the patch and already sent it to Linus.
So you don't have to put it in your tree. I sent it to Andrew since he
usually takes care of pushing fsnotify fixes to Linus and although this was
an audit bug, the problem was really in fsnotify...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] fsnotify: Fix handling of renames in audit

[Paul Moore]

On Wed, Feb 11, 2015 at 1:36 AM, Jan Kara <jack@suse.cz> wrote:
> On Tue 10-02-15 19:58:35, Paul Moore wrote:
>> I can confirm this is indeed broken.
>>
>> Thanks for the patch.  I'm building a test kernel as I type this, assuming all
>> goes well I'll send this to Linus with the other audit patches for v3.20.
>
>   Actually, Andrew has picked up the patch and already sent it to Linus.
> So you don't have to put it in your tree. I sent it to Andrew since he
> usually takes care of pushing fsnotify fixes to Linus and although this was
> an audit bug, the problem was really in fsnotify...

Okay, great.  As long as it gets to Linus eventually I'm happy.

FWIW, your patch tested just fine on my system.

-- 
paul moore
www.paul-moore.com