6 a.m.
Hello,
Tony pointed me to a long-standing use after free issue in audit watch
subsystem. The first patch should fix this. When analyzing and fixing
that issue, I've noticed that audit watch does not received FS_UNMOUNT
event although from the code it seems it expects to receive it - patch
two fixes that. The patches survive some basic testing but especially
for the second patch I'd like someone with more experience with audit
subsystem to have a look whether the patch makes sense.
Honza
6 a.m.
New subject: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()
audit_remove_watch_rule() drops watch's reference to parent but then
continues to work with it. That is not safe as parent can get freed once
we drop our reference. The following is a trivial reproducer:
mount -o loop image /mnt
touch /mnt/file
auditctl -w /mnt/file -p wax
umount /mnt
auditctl -D
<crash in fsnotify_destroy_mark()>
Grab our own reference in audit_remove_watch_rule() earlier to make sure
mark does not get freed under us.
CC: stable(a)vger.kernel.org
Reported-by: Tony Jones <tonyj(a)suse.de>
Signed-off-by: Jan Kara <jack(a)suse.cz>
---
kernel/audit_watch.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 62d686d96581..ed748ee40029 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -457,13 +457,15 @@ void audit_remove_watch_rule(struct audit_krule *krule)
list_del(&krule->rlist);
if (list_empty(&watch->rules)) {
+ /*
+ * audit_remove_watch() drops our reference to 'parent' which
+ * can get freed. Grab our own reference to be safe.
+ */
+ audit_get_parent(parent);
audit_remove_watch(watch);
-
- if (list_empty(&parent->watches)) {
- audit_get_parent(parent);
+ if (list_empty(&parent->watches))
fsnotify_destroy_mark(&parent->mark, audit_watch_group);
- audit_put_parent(parent);
- }
+ audit_put_parent(parent);
}
}
--
2.12.3
1:43 p.m.
New subject: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()
On 08/15/2017 04:00 AM, Jan Kara wrote:
audit_remove_watch_rule() drops watch's reference to parent but
then
continues to work with it. That is not safe as parent can get freed once
we drop our reference. The following is a trivial reproducer:
mount -o loop image /mnt
touch /mnt/file
auditctl -w /mnt/file -p wax
umount /mnt
auditctl -D
<crash in fsnotify_destroy_mark()>
Grab our own reference in audit_remove_watch_rule() earlier to make sure
mark does not get freed under us.
CC: stable(a)vger.kernel.org
Reported-by: Tony Jones <tonyj(a)suse.de>
Signed-off-by: Jan Kara <jack(a)suse.cz>
---
Tested-by: Tony Jones <tonyj(a)suse.de>
Fix tested and verified against v3.0 and mainline
2:52 p.m.
New subject: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()
On Tue, Aug 15, 2017 at 7:00 AM, Jan Kara <jack(a)suse.cz> wrote:
audit_remove_watch_rule() drops watch's reference to parent but
then
continues to work with it. That is not safe as parent can get freed once
we drop our reference. The following is a trivial reproducer:
mount -o loop image /mnt
touch /mnt/file
auditctl -w /mnt/file -p wax
umount /mnt
auditctl -D
<crash in fsnotify_destroy_mark()>
Grab our own reference in audit_remove_watch_rule() earlier to make sure
mark does not get freed under us.
CC: stable(a)vger.kernel.org
Reported-by: Tony Jones <tonyj(a)suse.de>
Signed-off-by: Jan Kara <jack(a)suse.cz>
---
kernel/audit_watch.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
This looks good to me, thanks for the fix. I'm going to merge this
into the audit/stable-4.13 branch and assuming it passes the audit
tests I'll send it up to Linus later this week.
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 62d686d96581..ed748ee40029 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -457,13 +457,15 @@ void audit_remove_watch_rule(struct audit_krule *krule)
list_del(&krule->rlist);
if (list_empty(&watch->rules)) {
+ /*
+ * audit_remove_watch() drops our reference to 'parent' which
+ * can get freed. Grab our own reference to be safe.
+ */
+ audit_get_parent(parent);
audit_remove_watch(watch);
-
- if (list_empty(&parent->watches)) {
- audit_get_parent(parent);
+ if (list_empty(&parent->watches))
fsnotify_destroy_mark(&parent->mark, audit_watch_group);
- audit_put_parent(parent);
- }
+ audit_put_parent(parent);
}
}
--
2.12.3
--
paul moore
www.paul-moore.com
12:09 a.m.
New subject: [PATCH 1/2] audit: Fix use after free in audit_remove_watch_rule()
On 2017-08-15 13:00, Jan Kara wrote:
audit_remove_watch_rule() drops watch's reference to parent but
then
continues to work with it. That is not safe as parent can get freed once
we drop our reference. The following is a trivial reproducer:
mount -o loop image /mnt
touch /mnt/file
auditctl -w /mnt/file -p wax
umount /mnt
auditctl -D
<crash in fsnotify_destroy_mark()>
Grab our own reference in audit_remove_watch_rule() earlier to make sure
mark does not get freed under us.
CC: stable(a)vger.kernel.org
Reported-by: Tony Jones <tonyj(a)suse.de>
Signed-off-by: Jan Kara <jack(a)suse.cz>
Reviewed-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit_watch.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 62d686d96581..ed748ee40029 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -457,13 +457,15 @@ void audit_remove_watch_rule(struct audit_krule *krule)
list_del(&krule->rlist);
if (list_empty(&watch->rules)) {
+ /*
+ * audit_remove_watch() drops our reference to 'parent' which
+ * can get freed. Grab our own reference to be safe.
+ */
+ audit_get_parent(parent);
audit_remove_watch(watch);
-
- if (list_empty(&parent->watches)) {
- audit_get_parent(parent);
+ if (list_empty(&parent->watches))
fsnotify_destroy_mark(&parent->mark, audit_watch_group);
- audit_put_parent(parent);
- }
+ audit_put_parent(parent);
}
}
--
2.12.3
--
Linux-audit mailing list
Linux-audit(a)redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
6 a.m.
New subject: [PATCH 2/2] audit: Receive unmount event
Although audit_watch_handle_event() can handle FS_UNMOUNT event, it is
not part of AUDIT_FS_WATCH mask and thus such event never gets to
audit_watch_handle_event(). Thus fsnotify marks are deleted by fsnotify
subsystem on unmount without audit being notified about that which leads
to a strange state of existing audit rules with dead fsnotify marks.
Add FS_UNMOUNT to the mask of events to be received so that audit can
clean up its state accordingly.
Signed-off-by: Jan Kara <jack(a)suse.cz>
---
kernel/audit_watch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index ed748ee40029..9eb8b3511636 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -66,7 +66,7 @@ static struct fsnotify_group *audit_watch_group;
/* fsnotify events we care about. */
#define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
- FS_MOVE_SELF | FS_EVENT_ON_CHILD)
+ FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
static void audit_free_parent(struct audit_parent *parent)
{
--
2.12.3
2:55 p.m.
New subject: [PATCH 2/2] audit: Receive unmount event
On Tue, Aug 15, 2017 at 7:00 AM, Jan Kara <jack(a)suse.cz> wrote:
Although audit_watch_handle_event() can handle FS_UNMOUNT event, it
is
not part of AUDIT_FS_WATCH mask and thus such event never gets to
audit_watch_handle_event(). Thus fsnotify marks are deleted by fsnotify
subsystem on unmount without audit being notified about that which leads
to a strange state of existing audit rules with dead fsnotify marks.
Add FS_UNMOUNT to the mask of events to be received so that audit can
clean up its state accordingly.
Signed-off-by: Jan Kara <jack(a)suse.cz>
---
kernel/audit_watch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
It's funny how the rest of the audit code handles the FS_UNMOUNT
event, but it isn't in the mask. It looks like it was lost in the
inotify to fanotify conversion. Since I'm likely sending your other
patch up to Linus later this week, and I think this is a reasonable
bug-fix, I'm going to include this in the audit/stable-4.13 branch.
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index ed748ee40029..9eb8b3511636 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -66,7 +66,7 @@ static struct fsnotify_group *audit_watch_group;
/* fsnotify events we care about. */
#define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
- FS_MOVE_SELF | FS_EVENT_ON_CHILD)
+ FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
static void audit_free_parent(struct audit_parent *parent)
{
--
2.12.3
--
paul moore
www.paul-moore.com
12:12 a.m.
New subject: [PATCH 2/2] audit: Receive unmount event
On 2017-08-15 15:55, Paul Moore wrote:
On Tue, Aug 15, 2017 at 7:00 AM, Jan Kara <jack(a)suse.cz>
wrote:
> Although audit_watch_handle_event() can handle FS_UNMOUNT event, it is
> not part of AUDIT_FS_WATCH mask and thus such event never gets to
> audit_watch_handle_event(). Thus fsnotify marks are deleted by fsnotify
> subsystem on unmount without audit being notified about that which leads
> to a strange state of existing audit rules with dead fsnotify marks.
>
> Add FS_UNMOUNT to the mask of events to be received so that audit can
> clean up its state accordingly.
>
> Signed-off-by: Jan Kara <jack(a)suse.cz>
Reviewed-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> kernel/audit_watch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
It's funny how the rest of the audit code handles the FS_UNMOUNT
event, but it isn't in the mask. It looks like it was lost in the
inotify to fanotify conversion. Since I'm likely sending your other
patch up to Linus later this week, and I think this is a reasonable
bug-fix, I'm going to include this in the audit/stable-4.13 branch.
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index ed748ee40029..9eb8b3511636 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -66,7 +66,7 @@ static struct fsnotify_group *audit_watch_group;
>
> /* fsnotify events we care about. */
> #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
> - FS_MOVE_SELF | FS_EVENT_ON_CHILD)
> + FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
>
> static void audit_free_parent(struct audit_parent *parent)
> {
> --
> 2.12.3
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit(a)redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rgb(a)redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
2591
days inactive
2594
days old
linux-audit@lists.linux-audit.osci.io
7 comments
4 participants
participants (4)
-
Jan Kara -
Paul Moore -
Richard Guy Briggs -
Tony Jones