Discussion:
[PATCH v2] audit: fix use-after-free in audit_add_watch
Richard Guy Briggs
2018-06-18 19:15:51 UTC
Permalink
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.
---
- Move audit_get_watch before audit_find_parent. In the case of
audit_get_nd failing.
---
kernel/audit_watch.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f1ba88994508..6d9b3f2bb1e2 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -430,6 +430,14 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
if (ret)
return ret;
+ /*
+ * 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);
+
/* either find an old parent or attach a new one */
parent = audit_find_parent(d_backing_inode(parent_path.dentry));
if (!parent) {
@@ -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;
}
--
2.17.1
- 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
Paul Moore
2018-07-03 15:34:29 UTC
Permalink
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.
---
- Move audit_get_watch before audit_find_parent. In the case of
audit_get_nd failing.
---
kernel/audit_watch.c | 9 +++++++++
1 file changed, 9 insertions(+)
I haven't made it through Jan's latest patches, which may change
things related to this patch, but did you see my comment on the
original version of your patch?

* https://www.redhat.com/archives/linux-audit/2018-June/msg00166.html
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f1ba88994508..6d9b3f2bb1e2 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -430,6 +430,14 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
if (ret)
return ret;
+ /*
+ * 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);
+
/* either find an old parent or attach a new one */
parent = audit_find_parent(d_backing_inode(parent_path.dentry));
if (!parent) {
@@ -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;
}
--
2.17.1
--
paul moore
www.paul-moore.com
Ronny Chevalier
2018-07-03 15:56:23 UTC
Permalink
Post by Paul Moore
I haven't made it through Jan's latest patches, which may change
things related to this patch, but did you see my comment on the
original version of your patch?
* https://www.redhat.com/archives/linux-audit/2018-June/msg00166.html
Yes, I have, but I did not have time to work on it yet.
(this patch was sent before your comments)
Paul Moore
2018-07-03 16:51:56 UTC
Permalink
Post by Ronny Chevalier
Post by Paul Moore
I haven't made it through Jan's latest patches, which may change
things related to this patch, but did you see my comment on the
original version of your patch?
* https://www.redhat.com/archives/linux-audit/2018-June/msg00166.html
Yes, I have, but I did not have time to work on it yet.
(this patch was sent before your comments)
No worries, or rush, I just wanted to make sure.

Thanks for your help.
--
paul moore
www.paul-moore.com
Continue reading on narkive:
Loading...