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

[Richard Guy Briggs]

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 at 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 at 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 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