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

[Paul Moore]

On Mon, Jun 18, 2018 at 2:37 PM Richard Guy Briggs <rgb at redhat.com> wrote:
>
> On 2018-06-18 15:27, 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.
>
> 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?

> > Signed-off-by: Ronny Chevalier <ronny.chevalier at 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;
> >  }
> >
>
> - RGB
>
> --
> Richard Guy Briggs <rgb at 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
>
> --
> Linux-audit mailing list
> Linux-audit at redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com