[libvirt] [PATCH 3/3] util: virfile: Only setuid for virFileRemove if on NFS

[John Ferlan]

On 03/09/2016 12:39 PM, Cole Robinson wrote:
> NFS with root-squash is the only reason we need to do setuid/setgid
> crazyness in virFileRemove, so limit that behavior to the NFS case.
> ---
> I'm not sure though if NFS is the only case we care about this here,
> or if we want to conditionalize this path on NFS since that makes it
> more of a pain to test... It's not required to fix the initial bug
> 
>  src/util/virfile.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index cea2674..3d1b118 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2322,7 +2322,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
>   * owned by the passed uid/gid pair. Needed for NFS with root-squash
>   */
>  static bool
> -virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
> +virFileRemoveNeedsSetuid(const char *path, uid_t uid, gid_t gid)

You added a new parameter to document...

ACK with that adjustment.

John

More outloud typing follows...

>  {
>      /* If running unprivileged, setuid isn't going to work */
>      if (geteuid() != 0)
> @@ -2336,6 +2336,12 @@ virFileRemoveNeedsSetuid(uid_t uid, gid_t gid)
>      if (uid == geteuid() && gid == getegid())
>          return false;
>  
> +    /* Only perform the setuid stuff for NFS, which is the only case
> +       that may actually need it. This can error, but just be safe and
> +       only check for a clear negative result. */
> +    if (virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS) == 0)
> +        return false;
> +

So at this point we know we're root, the provided uid/gid isn't 0:0 and
it's not -1:-1.  So it's something else...

Now if the target isn't a volume on an NFS server, then since we're root
we can delete it regardless of what uid/gid is set on the file under the
premise that we changed it to something, but the XML wasn't updated.  At
some future point in time perhaps we can track uid/gid changes such as this.

>      return true;
>  }
>  
> @@ -2361,7 +2367,7 @@ virFileRemove(const char *path,
>      gid_t *groups;
>      int ngroups;
>  
> -    if (!virFileRemoveNeedsSetuid(uid, gid)) {
> +    if (!virFileRemoveNeedsSetuid(path, uid, gid)) {
>          if (virFileIsDir(path))
>              return rmdir(path);
>          else
>