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

[Paul Moore]

On Mon, Jun 18, 2018 at 3:12 PM Ronny Chevalier <ronny.chevalier at hp.com> 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 at hp.com>
> ---
> v2:
> - 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];
>  error:
>         path_put(&parent_path);
> +       audit_put_watch(watch);
>         return ret;
>  }
>
> --
> 2.17.1
>


-- 
paul moore
www.paul-moore.com