* [PATCH v3] audit: fix use-after-free in audit_add_watch
@ 2018-07-11 12:39 Ronny Chevalier
2018-07-13 20:57 ` Richard Guy Briggs
0 siblings, 1 reply; 3+ messages in thread
From: Ronny Chevalier @ 2018-07-11 12:39 UTC (permalink / raw)
To: linux-audit; +Cc: ronny.chevalier, rgb
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 <ronny.chevalier@hp.com>
---
v3:
- Move audit_get_watch before audit_get_nd since it is using it
and call audit_put_watch if it fails.
v2:
- Move audit_get_watch before audit_find_parent. In the case of
audit_get_nd failing.
---
kernel/audit_watch.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 6f249bdf2d84..787c7afdf829 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -420,6 +420,13 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
struct path parent_path;
int h, ret = 0;
+ /*
+ * 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);
+
mutex_unlock(&audit_filter_mutex);
/* Avoid calling path_lookup under audit_filter_mutex. */
@@ -428,8 +435,10 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
/* caller expects mutex locked */
mutex_lock(&audit_filter_mutex);
- if (ret)
+ if (ret) {
+ audit_put_watch(watch);
return ret;
+ }
/* either find an old parent or attach a new one */
parent = audit_find_parent(d_backing_inode(parent_path.dentry));
@@ -447,6 +456,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;
}
--
2.18.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] audit: fix use-after-free in audit_add_watch
2018-07-11 12:39 [PATCH v3] audit: fix use-after-free in audit_add_watch Ronny Chevalier
@ 2018-07-13 20:57 ` Richard Guy Briggs
2018-07-18 15:50 ` Paul Moore
0 siblings, 1 reply; 3+ messages in thread
From: Richard Guy Briggs @ 2018-07-13 20:57 UTC (permalink / raw)
To: Ronny Chevalier; +Cc: linux-audit
On 2018-07-11 14:39, Ronny Chevalier wrote:
> 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 <ronny.chevalier@hp.com>
> ---
Ronnie, This looks good to me, but let's see how Jan Kara's patches
interplay with it.
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> v3:
> - Move audit_get_watch before audit_get_nd since it is using it
> and call audit_put_watch if it fails.
> v2:
> - Move audit_get_watch before audit_find_parent. In the case of
> audit_get_nd failing.
> ---
> kernel/audit_watch.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 6f249bdf2d84..787c7afdf829 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -420,6 +420,13 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
> struct path parent_path;
> int h, ret = 0;
>
> + /*
> + * 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);
> +
> mutex_unlock(&audit_filter_mutex);
>
> /* Avoid calling path_lookup under audit_filter_mutex. */
> @@ -428,8 +435,10 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
> /* caller expects mutex locked */
> mutex_lock(&audit_filter_mutex);
>
> - if (ret)
> + if (ret) {
> + audit_put_watch(watch);
> return ret;
> + }
>
> /* either find an old parent or attach a new one */
> parent = audit_find_parent(d_backing_inode(parent_path.dentry));
> @@ -447,6 +456,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;
> }
>
> --
> 2.18.0
>
- RGB
--
Richard Guy Briggs <rgb@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] audit: fix use-after-free in audit_add_watch
2018-07-13 20:57 ` Richard Guy Briggs
@ 2018-07-18 15:50 ` Paul Moore
0 siblings, 0 replies; 3+ messages in thread
From: Paul Moore @ 2018-07-18 15:50 UTC (permalink / raw)
To: ronny.chevalier; +Cc: rgb, linux-audit
On Fri, Jul 13, 2018 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-11 14:39, Ronny Chevalier wrote:
> > 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 <ronny.chevalier@hp.com>
> > ---
>
> Ronnie, This looks good to me, but let's see how Jan Kara's patches
> interplay with it.
>
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
Agreed, this looks better, thanks Ronny, I'm merging this into audit/next.
Yes, this obviously treads in the same space as Jan's patches, but we
will deal with that as Jan's patches are reviewed; in the meantime
this fixes a trivially reproduced problem, the additional reference is
limited to a single function instantiation, and is easily understood.
> > v3:
> > - Move audit_get_watch before audit_get_nd since it is using it
> > and call audit_put_watch if it fails.
> > v2:
> > - Move audit_get_watch before audit_find_parent. In the case of
> > audit_get_nd failing.
> > ---
> > kernel/audit_watch.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 6f249bdf2d84..787c7afdf829 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -420,6 +420,13 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
> > struct path parent_path;
> > int h, ret = 0;
> >
> > + /*
> > + * 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);
> > +
> > mutex_unlock(&audit_filter_mutex);
> >
> > /* Avoid calling path_lookup under audit_filter_mutex. */
> > @@ -428,8 +435,10 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
> > /* caller expects mutex locked */
> > mutex_lock(&audit_filter_mutex);
> >
> > - if (ret)
> > + if (ret) {
> > + audit_put_watch(watch);
> > return ret;
> > + }
> >
> > /* either find an old parent or attach a new one */
> > parent = audit_find_parent(d_backing_inode(parent_path.dentry));
> > @@ -447,6 +456,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;
> > }
> >
> > --
> > 2.18.0
> >
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@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
www.paul-moore.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-07-18 15:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-11 12:39 [PATCH v3] audit: fix use-after-free in audit_add_watch Ronny Chevalier
2018-07-13 20:57 ` Richard Guy Briggs
2018-07-18 15:50 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.