On 01/17/2012 06:44 PM, Laine Stump wrote:

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

s/arae/are/

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()

Hmm. I'm not familiar enough with root-squash to know the answer off-hand. I know that root-squash prevents uid 0 from open()ing a remote file (by instead opening the file under the nobody account), but once the file is open, can root proceed to fchown() or even fchmod() the file? fchmod() is unlikely to succeed except in the case of swapping the gid_t ownership if done as non-root, so I can understand pulling fchown() out to the public function after the child has already passed the fd. And hopefully root can fchmod() an fd that it obtained from a child; but if not, then you have to move the fchmod() back into the child code.

(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.

fchown and fchmod are async-safe. (Most syscalls in 'man 2' fall in this category; it's when you get to 'man 3' that you are likely to be introducing library functions that hold locks and aren't atomic where you are no longer async-safe).

* 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).

Sounds like a nice cleanup, especially since we previously had several call sites that had to dance around virFileOpenAs twice to get the same behavior. Refactoring the double attempt into the common code is good.

(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.)

For a pre-existing file, all that matters is that you get an fd open; it doesn't matter who opened it. (Unless we really _do_ want to change ownerships as part of opening a file.) For creating a new file, it matters that the resulting fd is owned by the correct user. If root creates the file, but the file will be passed to qemu, then we must change ownership (fchmod or grant an ACL); or we must open the file as qemu in the first place. The former will be prevented by root-squash NFS, so NFS already has the right behavior; anywhere else, root is powerful enough to be able to fchown the file over to the right user. I guess I'll see more on this as I continue reviewing.

* 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.

Yes, I think that makes sense. We really are doing two things at once: 'give me an fd', and 'ensure that the right people down the road can call open and get a new fd to the same file'; separating the fchown and making it only happen when explicitly requested means that by default we just get the fd. Callers passing O_CREAT | O_TRUNC don't care if the file previously existed, so they probably won't pass the ownership flag. Callers passing O_CREAT | O_EXCL are trying to create a new file, so they probably care about ownership on the new file. But making them be explicit about it makes it easier to review.

(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) {

Based solely on the commit message, this is a valid conversion (I guess when I get to the actual function, to see if the new implementation matches the commit, determines whether I have to change my mind :)

@@ -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) {

Here, we're creating a file, but I guess we're okay if it ends up being owned by root and not some other user.

+++ 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;

Per the commit message, this is preserving semantics of the old behavior.

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) {

And this keeps things as two calls for now, for minimal code churn in the clients, so that this patch focuses on the implementation (I'm guessing 2/2 simplifies the clients).

@@ -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,

Hmm, we still have the problem that we are storing _virStoragePerms as int mode, int uid, and int gid, instead of the better mode_t mode, uid_t uid, and gid_t gid (and given our recent problems with 64-bit pid_t, it's not too much of a stretch to imagine a system with 64-bit uid_t, even though mingw64 is thankfully not such a system). But that's a separate cleanup, and I'll probably be working on it in parallel with my pid_t cleanups. So I'm okay with this change.

-/* 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

I'm going to review this more on the basis of post-patch, rather than on the diff (as you mentioned, it is enough of a change to make the diff difficult). But unfortunately, I've run out of time at the moment; I'll see if I can get back to the review later tonight, otherwise it will be tomorrow morning.

+++ 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),

The new flag names make sense. It looked like in this round, you used NOFORK and FORK as mutually exclusive flags; I'm guessing that the next patch mixes the two as the way to state that you want to use a double-open attempt, And to some degree, FORCE_PERMS maps to FORCE_MODE, and AS_UID maps to FORCE_OWNER, but with clearer semantics - if the flag is present, guarantee an fchmod() or fchown(), whether or not a file was created; if the flag is not present, focus only on getting the fd. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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 are in a separate function. This makes the public function easier to understand. * 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). --- V3 differences - Eric Blake wondered if fchmod() issued by uid 0 of a file opened as a different user on a root-squashed NFS would actually work. It turns out it won't, so I put fchmod back into the child process. And since there are already so many other pieces of code related with this that would try to log a message on failure (e.g. virSetUIDGID()), I backed off on that as well, and have put the log messages in child process code back in - that's a bigger problem for another day. src/libxl/libxl_driver.c | 4 +- src/qemu/qemu_driver.c | 8 +- src/storage/storage_backend.c | 12 +- src/util/util.c | 357 +++++++++++++++++++++++++---------------- src/util/util.h | 6 +- 5 files changed, 233 insertions(+), 154 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index f7f45c7..1fd2b82 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 608e82a..08ebd0d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2311,6 +2311,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(); @@ -2320,6 +2321,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 @@ -2340,8 +2342,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 @@ -2385,7 +2387,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 c00c2f9..b493beb 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" @@ -753,115 +754,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, unsigned int flags) { pid_t pid; int waitret, status, ret = 0; @@ -869,13 +769,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 @@ -890,28 +783,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) @@ -922,31 +809,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 */ @@ -955,10 +838,22 @@ parenterror: goto childerror; } - ret = virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); - if (ret < 0) + if ((fd = open(path, openflags, mode)) < 0) { + ret = -errno; + virReportSystemError(errno, _("child process failed to create file '%s'"), + path); + goto childerror; + } + + /* File is successfully open. Set permissions if requested. */ + if ((flags & VIR_FILE_OPEN_FORCE_MODE) + && (fchmod(fd, mode) < 0)) { + ret = -errno; + virReportSystemError(errno, + _("child process cannot set mode of '%s' to %04o"), + path, mode); goto childerror; - fd = ret; + } do { ret = sendfd(pair[1], fd); @@ -966,6 +861,8 @@ parenterror: if (ret < 0) { ret = -errno; + virReportSystemError(errno, "%s", + _("child process failed to send fd to parent")); goto childerror; } @@ -975,13 +872,193 @@ 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; + } + } + /* 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; + } + } + } + + /* 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, flags)) < 0) { + ret = fd; + fd = -1; + goto error; + } + } + + /* File is successfully opened */ + 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 94d9282..1784c15 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -92,8 +92,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

On 01/27/2012 02:20 PM, Eric Blake wrote:

...

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:

V3 differences - Eric Blake wondered if fchmod() issued by uid 0 of a file opened as a different user on a root-squashed NFS would actually work. It turns out it won't, so I put fchmod back into the child process. And since there are already so many other pieces of code related with this that would try to log a message on failure (e.g. virSetUIDGID()), I backed off on that as well, and have put the log messages in child process code back in - that's a bigger problem for another day. Sometimes it seems like those big nasty problems are there until someone actually hits them in practice. Oh well. Even if we don't make it 100%

On 01/27/2012 10:43 AM, Laine Stump wrote: perfect, I still think that your rewrite makes it easier to understand, and therefore easier to maintain, should we come across the day where we need to fix the nasty problems.

And for anyone in the future re-reading this thread while trying to fix the problems (hello there!), here's an idea - virSetUIDGID should be broken into two functions, virSetUIDGIDRaw, which only uses async-safe functions and returns -errno on failure, and virSetUIDGID that formats errors into log message for normal users.

...

} 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 @@ -2385,7 +2387,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) {

You weren't kidding about the difference in 0600 vs. 0660 permissions between the two attempts. My gut feel is that we want 0660 (that's why we invented virSetUIDGID in the first place - people like to use group permissions and membership in multiple groups as a reasonable form of access management); but I also agree with your decision to keep this patch semantically unchanged, and leave any permissions modifications for a later patch in isolation.

...

-/* 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) Here, I'll just paste the code post-patch and review that in isolation, rather than caring about the delta:

...

/* 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 virFileOpenForked(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) { pid_t pid; int waitret, status, ret = 0; int fd = -1; int pair[2] = { -1, -1 }; int forkRet;

/* parent is running as root, but caller requested that the * file be created as some other user and/or group). The Comment is out-of-date; s/created/opened/

...

* following dance avoids problems caused by root-squashing * NFS servers. */

if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair)< 0) { ret = -errno; virReportSystemError(errno, _("failed to create socket needed for '%s'"), path); return ret; }

forkRet = virFork(&pid); if (pid< 0) return -errno;

if (pid) { /* parent */ VIR_FORCE_CLOSE(pair[1]);

do { fd = recvfd(pair[0], 0); } while (fd< 0&& errno == EINTR); VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */

if (fd< 0&& errno != EACCES) { ret = -errno; while (waitpid(pid, NULL, 0) == -1&& errno == EINTR);

Hmm, this might do better as virPidAbort(pid).

...

return ret; }

And from here...

...

/* wait for child to complete, and retrieve its exit code */ while ((waitret = waitpid(pid,&status, 0) == -1) && (errno == EINTR)); if (waitret == -1) { ret = -errno; virReportSystemError(errno, _("failed to wait for child creating '%s'"), path); VIR_FORCE_CLOSE(fd); return ret; } if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || fd == -1) {

...to here, we could probably replace a good chunk of code with the simpler virPidWait(pid,&status). But both of those should be separate cleanups, as this part of the function was not touched by your refactoring of the public interface.

Agreed.

...

/* fall back to the simpler method, which works better in * some cases */ VIR_FORCE_CLOSE(fd); if ((fd = open(path, openflags, mode))< 0)

I would tweak things to only attempt this if !(flags& VIR_FILE_OPEN_NOFORK). That is, if NOFORK was requested, the we already tried this open once, before even deciding to call this helper function, so since it failed once it will fail again. Only if NOFORK was not requested is it worth this fallback in the parent, but then you have the issue that the parent needs to honor fchmod() somehow.

Right. I think I've decided that this bit of code (trying the open un-forked after trying it forked fails) is really an artifact of the old code that I don't want to keep around; the only existing code that took advantage of it in a useful way was virStorageBackendCreateRaw, and now that virFileOpenAs is setup to "officially" try both ways in a single call, that can actually be served *better* by just letting virFileOpenAs try both in the normal order. However, I agree with you that, since it's a semantic change, I will make that a separate patch. (The only situation I can think of where trying open() as root first would cause problems would be if there was a directory on root-squashed NFS where user nobody was allowed to create files - the initial open() as root would succeed, leaving a file owned by "nobody", and we would then be unable to chown it to "qemu". I don't think I've ever heard of anyone in their right mind giving user nobody write access to anything important on an NFS share, so this doesn't seem like a scenario we need to worry about.)

...

return -errno; } 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; }

/* set desired uid/gid, then attempt to create the file */

if (virSetUIDGID(uid, gid)< 0) { ret = -errno; goto childerror; }

if ((fd = open(path, openflags, mode))< 0) { ret = -errno; virReportSystemError(errno, _("child process failed to create file '%s'"), path);

Repeating myself, logging in the child needs fixing someday, but it is no worse than the code you were refactoring, so I'm okay with leaving things as they were and keeping this patch focused on the interface.

...

goto childerror; }

/* File is successfully open. Set permissions if requested. */ if ((flags& VIR_FILE_OPEN_FORCE_MODE) && (fchmod(fd, mode)< 0)) { ret = -errno; virReportSystemError(errno, _("child process cannot set mode of '%s' to %04o"), path, mode); goto childerror; }

do { ret = sendfd(pair[1], fd); } while (ret< 0&& errno == EINTR);

if (ret< 0) { ret = -errno; virReportSystemError(errno, "%s", _("child process failed to send fd to parent")); goto childerror; }

childerror: /* ret tracks -errno on failure, but exit value must be positive. * If the child exits with EACCES, then the parent tries again. */ /* 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);

If we do any cleanups to fix that XXX comment, they can also be as separate patches.

...

}

/** * 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 s/is/are/

...

* @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 s/ ;/;/

...

* 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)) Safe, since we know uid and gid are not -1 at this point. (It took me two tries to pick up on that point, though, so adding a comment might be useful).

Well, about 20 lines further up, the documentation (otherwise know as "source code") says "if (uid == -1) uid = getuid()". For me, that's pretty close to English :-P

...

&& (fchown(fd, uid, gid)< 0)) { ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), path, (unsigned int) uid, (unsigned int) gid); goto error; } } /* File is successfully open. Set permissions if requested. */ if ((flags& VIR_FILE_OPEN_FORCE_MODE) && (fchmod(fd, mode)< 0)) {

I'm wondering if we should hoist the fstat, and only attempt the fchmod if needed. That is, arrange the logic:

} else { // fd is open if (flags& (force_owner | force_mode)) { if fstat fails error if (flags& force_owner&& owner is wrong) { if fchown fails error } if (flags& force_mode&& mode is wrong) { if fchmod fails error } } }

Yeah, I agree on that one. What's in there is just another artifact of the old code that got moved around.

...

ret = -errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), path, mode); 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, flags))< 0) { ret = fd; fd = -1; goto error;

Hmm, this means that force_mode has to be done in the helper function.

I'm almost thinking you should move the fchmod/fchown code into a helper routine, and call it in two places: in virFileOpenForked, in the child, after fd is successfully opened

in virFileOpenAs, _after_ you have a working fd (whether from NOFORK or FORK code paths)

and if the fchmod/fchown code is a no-op when modes are already correct, then you won't have issues with root-squash being unable to fchmod in the parent.

I'm making a helper function, but in order to make the code more understandable when I later eliminate that extra "last-ditch" open(), I'm calling it from 3 different places - in each case immediately after the open() succeeds.

...

} }

/* File is successfully opened */ 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);

Hmm, should we unlink() the file if O_CREAT was specified and we were the ones that ended up creating it? That's a tough call.

I greatly prefer APIs that either complete the entire task they were given, or leave the universe unchanged aside from returning an error code. The problem in this case is that (if the open was done in a child process), we can't always know whether it was us who created the file - in many cases the parent will be unable to successfully stat() the file beforehand even if it did already exist, so the only way to know for sure is if 1) a stat() of the file failed prior to a successful call to open() in the parent, or 2) if O_EXCL is also specified, and open() is successful (in the child or parent). (I guess there is also 3) if stat() of the file in the child failed prior to a successful open() in the child, but there's currently no way for the child to indicate that to the parent). Otherwise we're just taking a guess. (I noticed the "needUnlink" stuff in qemuOpenFile, and actually ended up with the feeling that it couldn't get the answer 100% correct either)

...

} return ret; }

I still think there's a bit of work needed; are you up for trying a v4, or should I try it on top of what you have?

I'm going to post a v4 which basically addresses the issues you raised without saying "should be in a separate patch". If you don't like that version, maybe it will be time to eliminate more back-forth by having the next version from you.