[PATCH] audit: fix use-after-free in audit_add_watch

[Ronny Chevalier]

audit_add_watch stores locally krule->watch without taking a reference
on watch. Then, it calls audit_add_to_parent, and uses the watch stored
locally.

Unfortunately, it is possible that audit_add_to_parent updates
krule->watch.
When it happens, it also drops a reference of watch which
could free the watch.

How to reproduce (with KASAN enabled):

auditctl -w /etc/passwd -F success=0 -k test_passwd
auditctl -w /etc/passwd -F success=1 -k test_passwd2

The second call to auditctl triggers the use-after-free, because
audit_to_parent updates krule->watch to use a previous existing watch
and drops the reference to the newly created watch.

To fix the issue, we grab a reference of watch and we release it at the
end of the function.

Signed-off-by: Ronny Chevalier <***@hp.com>
---
kernel/audit_watch.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f1ba88994508..4bb8d7718fbc 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -421,6 +421,14 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)

mutex_unlock(&audit_filter_mutex);

+ /*
+ * When we will be calling audit_add_to_parent, krule->watch might have
+ * been updated and watch might have been freed.
+ * So we need to keep a reference of watch.
+ */
+
+ audit_get_watch(watch);
+
/* Avoid calling path_lookup under audit_filter_mutex. */
ret = audit_get_nd(watch, &parent_path);

@@ -446,6 +454,7 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
*list = &audit_inode_hash[h];
error:
path_put(&parent_path);
+ audit_put_watch(watch);
return ret;
}

[Richard Guy Briggs]

Post by Ronny Chevalier
audit_add_watch stores locally krule->watch without taking a reference
on watch. Then, it calls audit_add_to_parent, and uses the watch stored
locally.
Unfortunately, it is possible that audit_add_to_parent updates
krule->watch.
When it happens, it also drops a reference of watch which
could free the watch.
auditctl -w /etc/passwd -F success=0 -k test_passwd
auditctl -w /etc/passwd -F success=1 -k test_passwd2
The second call to auditctl triggers the use-after-free, because
audit_to_parent updates krule->watch to use a previous existing watch
and drops the reference to the newly created watch.
To fix the issue, we grab a reference of watch and we release it at the
end of the function.

Nice catch.

This doesn't quite look right though. What if audit_get_nd() fails?
How about we put that audit_get_watch(watch) right before
audit_find_parent()?

Post by Ronny Chevalier
---
kernel/audit_watch.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f1ba88994508..4bb8d7718fbc 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -421,6 +421,14 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
mutex_unlock(&audit_filter_mutex);
+ /*
+ * When we will be calling audit_add_to_parent, krule->watch might have
+ * been updated and watch might have been freed.
+ * So we need to keep a reference of watch.
+ */
+
+ audit_get_watch(watch);
+
/* Avoid calling path_lookup under audit_filter_mutex. */
ret = audit_get_nd(watch, &parent_path);
@@ -446,6 +454,7 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
*list = &audit_inode_hash[h];
path_put(&parent_path);
+ audit_put_watch(watch);
return ret;
}

- RGB

--
Richard Guy Briggs <***@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

[Ronny Chevalier]

Post by Richard Guy Briggs
Nice catch.
This doesn't quite look right though. What if audit_get_nd() fails?

Oh, yes, obviously. I don't know why I thought there was a goto instead of a return there.

Post by Richard Guy Briggs
How about we put that audit_get_watch(watch) right before
audit_find_parent()?

Sure, I will send a V2.

[Paul Moore]

Post by Richard Guy Briggs

Post by Ronny Chevalier
audit_add_watch stores locally krule->watch without taking a reference
on watch. Then, it calls audit_add_to_parent, and uses the watch stored
locally.
Unfortunately, it is possible that audit_add_to_parent updates
krule->watch.
When it happens, it also drops a reference of watch which
could free the watch.
auditctl -w /etc/passwd -F success=0 -k test_passwd
auditctl -w /etc/passwd -F success=1 -k test_passwd2
The second call to auditctl triggers the use-after-free, because
audit_to_parent updates krule->watch to use a previous existing watch
and drops the reference to the newly created watch.
To fix the issue, we grab a reference of watch and we release it at the
end of the function.

Nice catch.
This doesn't quite look right though. What if audit_get_nd() fails?
How about we put that audit_get_watch(watch) right before
audit_find_parent()?

Since we end up using the watch in audit_get_nd() shouldn't we bump
the refcnt in the watch before we call audit_get_nd()? Further, I
think we need to bump the refcnt before we drop audit_filter_mutex so
we don't end up in a race with audit_find_parent(), yes?

Post by Richard Guy Briggs

Post by Ronny Chevalier
---
kernel/audit_watch.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f1ba88994508..4bb8d7718fbc 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -421,6 +421,14 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
mutex_unlock(&audit_filter_mutex);
+ /*
+ * When we will be calling audit_add_to_parent, krule->watch might have
+ * been updated and watch might have been freed.
+ * So we need to keep a reference of watch.
+ */
+
+ audit_get_watch(watch);
+
/* Avoid calling path_lookup under audit_filter_mutex. */
ret = audit_get_nd(watch, &parent_path);
@@ -446,6 +454,7 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
*list = &audit_inode_hash[h];
path_put(&parent_path);
+ audit_put_watch(watch);
return ret;
}

- RGB
--
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
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit