[libvirt] [PATCH] initgroups() in qemudOpenAsUID()

[Laine Stump]

  On 10/17/2010 04:58 PM, Dan Kenigsberg wrote:
> qemudOpenAsUID is intended to open a file with the credentials of a
> specified uid. Current implementation fails if the file is accessible to
> one of uid's groups but not owned by uid.
>
> This patch replaces the supplementary group list that the child process
> inherited from libvirtd with the default group list of uid.


Urr. Yet another twist in this ugly saga. Thanks for figuring it out!


> ---
>   src/qemu/qemu_driver.c |   16 ++++++++++++++++
>   1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0ce2d40..a1027d4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6353,6 +6353,7 @@ parent_cleanup:
>       char *buf = NULL;
>       size_t bufsize = 1024 * 1024;
>       int bytesread;
> +    struct passwd *pwd;
>
>       /* child doesn't need the read side of the pipe */
>       close(pipefd[0]);
> @@ -6365,6 +6366,21 @@ parent_cleanup:
>           goto child_cleanup;
>       }
>
> +    /* we can avoid getpwuid_r() in threadless child */
> +    if ((pwd = getpwuid(uid)) == NULL) {
> +        exit_code = errno;
> +        virReportSystemError(errno,
> +                             _("cannot setuid(%d) to read '%s'"),
> +                             uid, path);


The error message is misleading. It should rather be something like:

+        virReportSystemError(errno,
+                             _("getpwuid(%d) failed reading '%s'"),
+                             uid, path);


(BTW, thanks for the comment about getpwuid_r())


> +        goto child_cleanup;
> +    }
> +    if (initgroups(pwd->pw_name, pwd->pw_gid) != 0) {
> +        exit_code = errno;
> +        virReportSystemError(errno,
> +                             _("cannot setuid(%d) to read '%s'"),
> +                             uid, path);


Similarly, the error message should be:

+        virReportSystemError(errno,
+                             _("initgroups(\"%s\", %d) failed reading '%s'"),
+                             pwd->pw_name, pwd->pw_gid, path);


> +        goto child_cleanup;
> +    }
>       if (setuid(uid) != 0) {
>           exit_code = errno;
>           virReportSystemError(errno,

I give my ACK with the above error message fixes, with the reservation 
that I'm not that familiar with getpwuid and initgroups, and any 
restrictions they may have when called in a non-root process. 
(apparently everything is in order in this case, since presumably your 
testing was successful ;-)