Re: [PATCH] libselinux: restorecon_xattr: reset dir_xattr_list on every call

On Thu, Aug 6, 2026 at 3:41 PM Vit Mojzis <[email protected]> wrote:
>
> selinux_restorecon_xattr(3) documents that "xattr_list must be set to
> NULL before calling selinux_restorecon_xattr(3). The caller is
> responsible for freeing the returned xattr_list entries." Commit
> b5a23d7f30c1 ("libselinux: restorecon_xattr: clear dir_xattr_* after
> freeing") reset the dir_xattr_list/dir_xattr_last statics after
> freeing them on the internal error-cleanup path of the recursive
> walk, but not on the normal success path.
>
> After a successful call, dir_xattr_list/dir_xattr_last are left
> pointing at the just-returned, now caller-owned list. Once the caller
> frees it as required and calls the function again, add_xattr_entry()
> finds dir_xattr_list non-NULL and appends the next entry through
> dir_xattr_last->next, a dangling pointer into memory the caller has
> already freed - a use-after-free write.
>
> Reset both pointers to NULL at the top of every call instead of only
> in the error path: by the API description, any list from a prior call
> is not valid.
>
> Co-Authored-By: Claude Sonnet 5 <[email protected]>
>
> Signed-off-by: Vit Mojzis <[email protected]>

Acked-by: Stephen Smalley <[email protected]>

> ---
>
> Verified with an AddressSanitizer harness that calls
> selinux_restorecon_xattr() on one directory, frees the result,
> then calls it again on a second directory: reliably crashes
> with a heap-use-after-free (when compiled with -fsanitize=address)
> before this fix (reproduced on libselinux-3.10 and libselinux-3.11)
> and runs clean after rebuilding with this change applied.
>
> https://github.com/vmojzis/selinuxproject_selinux/blob/libselinux_AISLE/reproducers/RHEL-217622/reproduce.c
>
>
>
>  libselinux/src/selinux_restorecon.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> index 4233ba65..49b8ff7e 100644
> --- a/libselinux/src/selinux_restorecon.c
> +++ b/libselinux/src/selinux_restorecon.c
> @@ -1968,6 +1968,18 @@ int selinux_restorecon_xattr(const char *pathname, unsigned int xattr_flags,
>         if (!fc_sehandle)
>                 return -1;
>
> +       /*
> +        * The API contract requires the caller to have already freed any
> +        * xattr_list returned by a previous call before calling again.
> +        * Forget our own head/tail pointers to that now caller-owned memory
> +        * here instead of carrying them over: leaving them set would make
> +        * the next add_xattr_entry() append a new entry through
> +        * dir_xattr_last, which would be dangling once the caller has freed
> +        * it, resulting in a use-after-free.
> +        */
> +       dir_xattr_list = NULL;
> +       dir_xattr_last = NULL;
> +
>         if (lstat(pathname, &sb) < 0) {
>                 if (errno == ENOENT)
>                         return 0;
> --
> 2.53.0
>
>