On Mon, Jun 18, 2018 at 2:37 PM Richard Guy Briggs <[email protected]> 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 <[email protected]> > > --- > > 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 <[email protected]> > 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 > [email protected] > https://www.redhat.com/mailman/listinfo/linux-audit -- paul moore www.paul-moore.com -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
