[libvirt] [PATCH 2/3] util: virfile: Clarify setuid usage for virFileRemove

[John Ferlan]

On 03/09/2016 12:38 PM, Cole Robinson wrote:
> Break these checks out into their own function, and clearly document
> each one. This shouldn't change behavior
> ---
>  src/util/virfile.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f45e18f..cea2674 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2314,6 +2314,32 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
>  }
>  
>  
> +/* virFileRemoveNeedsSetuid:
> + * @uid: file uid to check
> + * @gid: file gid to check
> + *
> + * Return true if we should use setuid/setgid before deleting a file
> + * owned by the passed uid/gid pair. Needed for NFS with root-squash
> + */
> +static bool
> +virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
> +{
> +    /* If running unprivileged, setuid isn't going to work */
> +    if (geteuid() != 0)
> +        return false;
> +
> +    /* uid/gid weren'd specified */

weren't

ACK - with the nit fixed... The rest is me typing out my thoughts...

John

> +    if ((uid == (uid_t) -1) && (gid == (gid_t) -1))
> +        return false;

This "should" only happen as failure path of
virStorageBackendCreateExecCommand when uid/gid not provided in the XML.
The virStorageBackendCreateRaw failure path expects creation to have
occurred where virFileOpenAs would have set the owner/mode due to the
operation_flags setting.

> +
> +    /* already running as proper uid/gid */
> +    if (uid == geteuid() && gid == getegid())
> +        return false;
> +

If the XML provided uid/gid and it's root - that's what we want

> +    return true;
> +}
> +
> +
>  /* virFileRemove:
>   * @path: file to unlink or directory to remove
>   * @uid: uid that was used to create the file (not required)
> @@ -2335,12 +2361,7 @@ virFileRemove(const char *path,
>      gid_t *groups;
>      int ngroups;
>  
> -    /* If not running as root or if a non explicit uid/gid was being used for
> -     * the file/volume or the explicit uid/gid matches, then use unlink directly
> -     */
> -    if ((geteuid() != 0) ||
> -        ((uid == (uid_t) -1) && (gid == (gid_t) -1)) ||
> -        (uid == geteuid() && gid == getegid())) {
> +    if (!virFileRemoveNeedsSetuid(uid, gid)) {
>          if (virFileIsDir(path))
>              return rmdir(path);
>          else
>