7:44 p.m.
virFileOpenAs previously would only try opening a file as the current
user, or as a different user, but wouldn't try both methods in a
single call. This made it cumbersome to use as a replacement for
open(2). Additionally, it had a lot of historical baggage that led to
it being difficult to understand.
This patch refactors virFileOpenAs in the following ways:
* reorganize the code so that everything dealing with both the parent
and child sides of the "fork+setuid+setgid+open" method arae in a
separate function. This makes the public function easier to understand.
* Reduce what is executed in the context of the child process to just
the bare minimum necessary to open the file and send its fd back to
the parent. In particular, the older version had also done fchmod()
(and had code to call fchown(), although that would never actually
be executed in the case of forking prior to the open). This code is
now in the main public function, and also executed in the context of
the parent process, thus reducing the chance that we may end up
trying to call some async-unsafe function from the child.
* Allow a single call to virFileOpenAs() to first attempt the open as
the current user, and if that fails to automatically re-try after
doing fork+setuid (if deemed appropriate, i.e. errno indicates it
would now be successful, and the file is on a networkFS). This makes
it possible (in many, but possibly not all, cases) to drop-in
virFileOpenAs() as a replacement for open(2).
(NB: currently qemuOpenFile() calls virFileOpenAs() twice, once
without forking, then again with forking. That unfortunately can't
be changed without at least some discussion of the ramifications,
because the requested file permissions are different in each case,
which is something that a single call to virFileOpenAs() can't deal
with.)
* Add a flag so that any fchown() of the file to a different uid:gid
is explicitly requested when the function is called, rather than it
being implied by the presence of the O_CREAT flag. This just makes
for less subtle surprises to consumers. (Commit
b1643dc15c5de886fefe56ad18608d65f1325a2c added the check for O_CREAT
before forcing ownership. This patch just makes that restriction
more explicit.)
* If either the uid or gid is specified as "-1", virFileOpenAs will
interpret this to mean "the current [gu]id".
All current consumers of virFileOpenAs should retain their present
behavior (after a few minor changes to their setup code and
arguments).
---
src/libxl/libxl_driver.c | 4 +-
src/qemu/qemu_driver.c | 8 +-
src/storage/storage_backend.c | 12 +-
src/util/util.c | 341 ++++++++++++++++++++++++-----------------
src/util/util.h | 6 +-
5 files changed, 217 insertions(+), 154 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 0500ed0..d7325c3 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -216,7 +216,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
libxlSavefileHeader hdr;
char *xml = NULL;
- if ((fd = virFileOpenAs(from, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) {
+ if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
libxlError(VIR_ERR_OPERATION_FAILED,
"%s", _("cannot read domain image"));
goto error;
@@ -1827,7 +1827,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
}
if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR,
- getuid(), getgid(), 0)) < 0) {
+ -1, -1, 0)) < 0) {
virReportSystemError(-fd,
_("Failed to create domain save file
'%s'"), to);
goto cleanup;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 712f1fc..9a5b36e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2306,6 +2306,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int
oflags,
bool is_reg = true;
bool need_unlink = false;
bool bypass_security = false;
+ unsigned int vfoflags = 0;
int fd = -1;
uid_t uid = getuid();
gid_t gid = getgid();
@@ -2315,6 +2316,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int
oflags,
* in the failure case */
if (oflags & O_CREAT) {
need_unlink = true;
+ vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
if (stat(path, &sb) == 0) {
is_reg = !!S_ISREG(sb.st_mode);
/* If the path is regular file which exists
@@ -2335,8 +2337,8 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int
oflags,
goto cleanup;
}
} else {
- if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR,
- uid, gid, 0)) < 0) {
+ if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid,
+ vfoflags | VIR_FILE_OPEN_NOFORK)) < 0) {
/* If we failed as root, and the error was permission-denied
(EACCES or EPERM), assume it's on a network-connected share
where root access is restricted (eg, root-squashed NFS). If the
@@ -2380,7 +2382,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int
oflags,
if ((fd = virFileOpenAs(path, oflags,
S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
driver->user, driver->group,
- VIR_FILE_OPEN_AS_UID)) < 0) {
+ vfoflags | VIR_FILE_OPEN_FORK)) < 0) {
virReportSystemError(-fd,
_("Error from child process creating
'%s'"),
path);
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index d7394e0..0ea050f 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -380,8 +380,6 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
{
int ret = -1;
int fd = -1;
- uid_t uid;
- gid_t gid;
int operation_flags;
virCheckFlags(0, -1);
@@ -393,15 +391,15 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
goto cleanup;
}
- uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
- gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
- operation_flags = VIR_FILE_OPEN_FORCE_PERMS;
+ operation_flags = VIR_FILE_OPEN_FORCE_MODE | VIR_FILE_OPEN_FORCE_OWNER;
if (pool->def->type == VIR_STORAGE_POOL_NETFS)
- operation_flags |= VIR_FILE_OPEN_AS_UID;
+ operation_flags |= VIR_FILE_OPEN_FORK;
if ((fd = virFileOpenAs(vol->target.path,
O_RDWR | O_CREAT | O_EXCL,
- vol->target.perms.mode, uid, gid,
+ vol->target.perms.mode,
+ vol->target.perms.uid,
+ vol->target.perms.gid,
operation_flags)) < 0) {
virReportSystemError(-fd,
_("cannot create path '%s'"),
diff --git a/src/util/util.c b/src/util/util.c
index 6f46d53..3f1b4ef 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -70,6 +70,7 @@
#include "logging.h"
#include "buf.h"
#include "util.h"
+#include "storage_file.h"
#include "memory.h"
#include "threads.h"
#include "verify.h"
@@ -732,115 +733,14 @@ childerror:
_exit(ret);
}
-/* return -errno on failure, or 0 on success */
+/* virFileOpenForked() - an internal utility function called only by
+ * virFileOpenAs(). It forks, then the child does setuid+setgid to
+ * given uid:gid and attempts to open the file, while the parent just
+ * calls recvfd to get the open fd back from the child. returns the
+ * fd, or -errno if there is an error. */
static int
-virFileOpenAsNoFork(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid, unsigned int flags)
-{
- int fd = -1;
- int ret = 0;
-
- if ((fd = open(path, openflags, mode)) < 0) {
- ret = -errno;
- virReportSystemError(errno, _("failed to create file '%s'"),
- path);
- goto error;
- }
-
- /* VIR_FILE_OPEN_AS_UID in flags means we are running in a child process
- * owned by uid and gid */
- if (!(flags & VIR_FILE_OPEN_AS_UID)) {
- struct stat st;
-
- if (fstat(fd, &st) == -1) {
- ret = -errno;
- virReportSystemError(errno, _("stat of '%s' failed"),
path);
- goto error;
- }
- if (((st.st_uid != uid) || (st.st_gid != gid))
- && (openflags & O_CREAT)
- && (fchown(fd, uid, gid) < 0)) {
- ret = -errno;
- virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
- path, (unsigned int) uid, (unsigned int) gid);
- goto error;
- }
- }
-
- if ((flags & VIR_FILE_OPEN_FORCE_PERMS)
- && (fchmod(fd, mode) < 0)) {
- ret = -errno;
- virReportSystemError(errno,
- _("cannot set mode of '%s' to %04o"),
- path, mode);
- goto error;
- }
- return fd;
-
-error:
- VIR_FORCE_CLOSE(fd);
- return ret;
-}
-
-/* return -errno on failure, or 0 on success */
-static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid,
- unsigned int flags) {
- int ret = 0;
- struct stat st;
-
- if ((mkdir(path, mode) < 0)
- && !((errno == EEXIST) && (flags &
VIR_DIR_CREATE_ALLOW_EXIST))) {
- ret = -errno;
- virReportSystemError(errno, _("failed to create directory
'%s'"),
- path);
- goto error;
- }
-
- if (stat(path, &st) == -1) {
- ret = -errno;
- virReportSystemError(errno, _("stat of '%s' failed"), path);
- goto error;
- }
- if (((st.st_uid != uid) || (st.st_gid != gid))
- && (chown(path, uid, gid) < 0)) {
- ret = -errno;
- virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
- path, (unsigned int) uid, (unsigned int) gid);
- goto error;
- }
- if ((flags & VIR_DIR_CREATE_FORCE_PERMS)
- && (chmod(path, mode) < 0)) {
- ret = -errno;
- virReportSystemError(errno,
- _("cannot set mode of '%s' to %04o"),
- path, mode);
- goto error;
- }
-error:
- return ret;
-}
-
-/**
- * virFileOpenAs:
- * @path: file to open or create
- * @openflags: flags to pass to open
- * @mode: mode to use on creation or when forcing permissions
- * @uid: uid that should own file on creation
- * @gid: gid that should own file
- * @flags: bit-wise or of VIR_FILE_OPEN_* flags
- *
- * Open @path, and execute an optional callback @hook on the open file
- * description. @hook must return 0 on success, or -errno on failure.
- * If @flags includes VIR_FILE_OPEN_AS_UID, then open the file while the
- * effective user id is @uid (by using a child process); this allows
- * one to bypass root-squashing NFS issues. If @flags includes
- * VIR_FILE_OPEN_FORCE_PERMS, then ensure that @path has those
- * permissions before the callback, even if the file already existed
- * with different permissions. The return value (if non-negative)
- * is the file descriptor, left open. Return -errno on failure. */
-int
-virFileOpenAs(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid, unsigned int flags)
+virFileOpenForked(const char *path, int openflags, mode_t mode,
+ uid_t uid, gid_t gid)
{
pid_t pid;
int waitret, status, ret = 0;
@@ -848,13 +748,6 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
int pair[2] = { -1, -1 };
int forkRet;
- if ((!(flags & VIR_FILE_OPEN_AS_UID))
- || (getuid() != 0)
- || ((uid == 0) && (gid == 0))) {
- flags &= ~VIR_FILE_OPEN_AS_UID;
- return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
- }
-
/* parent is running as root, but caller requested that the
* file be created as some other user and/or group). The
* following dance avoids problems caused by root-squashing
@@ -869,28 +762,22 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
}
forkRet = virFork(&pid);
-
- if (pid < 0) {
- ret = -errno;
- return ret;
- }
+ if (pid < 0)
+ return -errno;
if (pid) { /* parent */
VIR_FORCE_CLOSE(pair[1]);
do {
- ret = recvfd(pair[0], 0);
- } while (ret < 0 && errno == EINTR);
+ fd = recvfd(pair[0], 0);
+ } while (fd < 0 && errno == EINTR);
+ VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */
- if (ret < 0 && errno != EACCES) {
+ if (fd < 0 && errno != EACCES) {
ret = -errno;
- VIR_FORCE_CLOSE(pair[0]);
while (waitpid(pid, NULL, 0) == -1 && errno == EINTR);
- goto parenterror;
- } else {
- fd = ret;
+ return ret;
}
- VIR_FORCE_CLOSE(pair[0]);
/* wait for child to complete, and retrieve its exit code */
while ((waitret = waitpid(pid, &status, 0) == -1)
@@ -901,31 +788,27 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
_("failed to wait for child creating
'%s'"),
path);
VIR_FORCE_CLOSE(fd);
- goto parenterror;
+ return ret;
}
if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES ||
fd == -1) {
/* fall back to the simpler method, which works better in
* some cases */
VIR_FORCE_CLOSE(fd);
- flags &= ~VIR_FILE_OPEN_AS_UID;
- return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
+ if ((fd = open(path, openflags, mode)) < 0)
+ return -errno;
}
- if (!ret)
- ret = fd;
-parenterror:
- return ret;
+ return fd;
}
-
/* child */
+ VIR_FORCE_CLOSE(pair[0]); /* preserves errno */
if (forkRet < 0) {
/* error encountered and logged in virFork() after the fork. */
ret = -errno;
goto childerror;
}
- VIR_FORCE_CLOSE(pair[0]);
/* set desired uid/gid, then attempt to create the file */
@@ -934,10 +817,10 @@ parenterror:
goto childerror;
}
- ret = virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
- if (ret < 0)
+ if ((fd = open(path, openflags, mode)) < 0) {
+ ret = --errno;
goto childerror;
- fd = ret;
+ }
do {
ret = sendfd(pair[1], fd);
@@ -954,13 +837,191 @@ childerror:
/* XXX This makes assumptions about errno being < 255, which is
* not true on Hurd. */
VIR_FORCE_CLOSE(pair[1]);
+ if (ret < 0) {
+ VIR_FORCE_CLOSE(fd);
+ }
ret = -ret;
if ((ret & 0xff) != ret) {
VIR_WARN("unable to pass desired return value %d", ret);
ret = 0xff;
}
_exit(ret);
+}
+
+/**
+ * virFileOpenAs:
+ * @path: file to open or create
+ * @openflags: flags to pass to open
+ * @mode: mode to use on creation or when forcing permissions
+ * @uid: uid that should own file on creation
+ * @gid: gid that should own file
+ * @flags: bit-wise or of VIR_FILE_OPEN_* flags
+ *
+ * Open @path, and return an fd to the open file. @openflags are the flags
+ * normally passed to open(2), while @flags is used internally. If
+ * @flags includes VIR_FILE_OPEN_NOFORK, then try opening the file
+ * while executing with the current uid:gid (i.e. don't
+ * fork+setuid+setgid before the call to open()). If @flags includes
+ * VIR_FILE_OPEN_FORK, then try opening the file while the effective
+ * user id is @uid (by forking a child process); this allows one to
+ * bypass root-squashing NFS issues ; NOFORK is always tried before
+ * FORK (the absence of both flags is treated identically to
+ * (VIR_FILE_OPEN_NOFORK | VIR_FILE_OPEN_FORK)). If @flags includes
+ * VIR_FILE_OPEN_FORCE_OWNER, then ensure that @path is owned by
+ * uid:gid before returning (even if it already existed with a
+ * different owner). If @flags includes VIR_FILE_OPEN_FORCE_MODE,
+ * ensure it has those permissions before returning (again, even if
+ * the file already existed with different permissions). The return
+ * value (if non-negative) is the file descriptor, left open. Returns
+ * -errno on failure. */
+int
+virFileOpenAs(const char *path, int openflags, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags)
+{
+ int ret = 0, fd = -1;
+
+ /* allow using -1 to mean "current value" */
+ if (uid == (uid_t) -1)
+ uid = getuid();
+ if (gid == (gid_t) -1)
+ gid = getgid();
+ /* treat absence of both flags as presence of both for simpler
+ * calling. */
+ if (!(flags & (VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK)))
+ flags |= VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK;
+
+ if ((flags & VIR_FILE_OPEN_NOFORK)
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+
+ if ((fd = open(path, openflags, mode)) < 0) {
+ ret = -errno;
+ } else if (flags & VIR_FILE_OPEN_FORCE_OWNER) {
+ /* successfully opened as current user. Try fchown if
+ * appropriate. */
+ struct stat st;
+
+ if (fstat(fd, &st) == -1) {
+ ret = -errno;
+ virReportSystemError(errno, _("stat of '%s' failed"),
path);
+ goto error;
+ }
+ if (((st.st_uid != uid) || (st.st_gid != gid))
+ && (fchown(fd, uid, gid) < 0)) {
+ ret = -errno;
+ virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
+ path, (unsigned int) uid,
+ (unsigned int) gid);
+ goto error;
+ }
+ }
+ }
+
+ /* If we either 1) didn't try opening as current user at all, or
+ * 2) failed, and errno/virStorageFileIsSharedFS indicate we might
+ * be successful if we try as a different uid, then try doing
+ * fork+setuid+setgid before opening.
+ */
+ if ((fd < 0) && (flags & VIR_FILE_OPEN_FORK)) {
+
+ if (ret < 0) {
+ /* An open(2) that failed due to insufficient permissions
+ * could return one or the other of these depending on OS
+ * version and circumstances. Any other errno indicates a
+ * problem that couldn't be remedied by fork+setuid
+ * anyway. */
+ if (ret != -EACCES && ret != -EPERM)
+ goto error;
+
+ /* On Linux we can also verify the FS-type of the
+ * directory. (this is a NOP on other platforms). */
+ switch (virStorageFileIsSharedFS(path)) {
+ case 1:
+ /* it was on a network share, so we'll re-try */
+ break;
+ case -1:
+ /* failure detecting fstype */
+ virReportSystemError(errno, _("couldn't determine fs type
of"
+ "of mount containing
'%s'"), path);
+ goto error;
+ case 0:
+ default:
+ /* file isn't on a recognized network FS */
+ goto error;
+ }
+ }
+
+ /* passed all prerequisites - retry the open w/fork+setuid */
+ if ((fd = virFileOpenForked(path, openflags, mode, uid, gid)) < 0) {
+ ret = fd;
+ fd = -1;
+ goto error;
+ }
+ }
+
+ /* File is successfully open. Set permissions if requested. */
+ if ((flags & VIR_FILE_OPEN_FORCE_MODE)
+ && (fchmod(fd, mode) < 0)) {
+ ret = -errno;
+ virReportSystemError(errno,
+ _("cannot set mode of '%s' to %04o"),
+ path, mode);
+ goto error;
+ }
+
+ return fd;
+
+error:
+ if (fd < 0) {
+ /* whoever failed the open last has already set ret = -errno */
+ virReportSystemError(-ret, openflags & O_CREAT
+ ? _("failed to create file '%s'")
+ : _("failed to open file '%s'"),
+ path);
+ } else {
+ /* some other failure after the open succeeded */
+ VIR_FORCE_CLOSE(fd);
+ }
+ return ret;
+}
+
+/* return -errno on failure, or 0 on success */
+static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid,
+ unsigned int flags) {
+ int ret = 0;
+ struct stat st;
+
+ if ((mkdir(path, mode) < 0)
+ && !((errno == EEXIST) && (flags &
VIR_DIR_CREATE_ALLOW_EXIST))) {
+ ret = -errno;
+ virReportSystemError(errno, _("failed to create directory
'%s'"),
+ path);
+ goto error;
+ }
+
+ if (stat(path, &st) == -1) {
+ ret = -errno;
+ virReportSystemError(errno, _("stat of '%s' failed"), path);
+ goto error;
+ }
+ if (((st.st_uid != uid) || (st.st_gid != gid))
+ && (chown(path, uid, gid) < 0)) {
+ ret = -errno;
+ virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
+ path, (unsigned int) uid, (unsigned int) gid);
+ goto error;
+ }
+ if ((flags & VIR_DIR_CREATE_FORCE_PERMS)
+ && (chmod(path, mode) < 0)) {
+ ret = -errno;
+ virReportSystemError(errno,
+ _("cannot set mode of '%s' to %04o"),
+ path, mode);
+ goto error;
+ }
+error:
+ return ret;
}
/* return -errno on failure, or 0 on success */
diff --git a/src/util/util.h b/src/util/util.h
index c9c785b..a5d5228 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -90,8 +90,10 @@ char *virFileSanitizePath(const char *path);
enum {
VIR_FILE_OPEN_NONE = 0,
- VIR_FILE_OPEN_AS_UID = (1 << 0),
- VIR_FILE_OPEN_FORCE_PERMS = (1 << 1),
+ VIR_FILE_OPEN_NOFORK = (1 << 0),
+ VIR_FILE_OPEN_FORK = (1 << 1),
+ VIR_FILE_OPEN_FORCE_MODE = (1 << 2),
+ VIR_FILE_OPEN_FORCE_OWNER = (1 << 3),
};
int virFileAccessibleAs(const char *path, int mode,
uid_t uid, gid_t gid)
--
1.7.7.5