Re: [libvirt] [PATCH] initgroups() in qemudOpenAsUID()
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 ;-)