[libvirt] [PATCH 0/4] Remove open coding virProcessWait for root-squash
A followup of sorts to recently pushed patches regarding NFS root-squash. During libvirt-security list review it was pointed out that the new code was essentially open coding what virProcessWait does. However, since the model being used also was open coded and there was a time element, the change was allowed as is with the expectation that a cleanup patch would follow. Which is what leads into this series.... The series started out purely as removing the open code and replacing with the call to virProcessWait, but during that exercise I also realized that it was possible to create a 'netdir' in a NFS root-squash environment (eg, virDirCreate); however, the corrollary to remove the directory using a fork/exec didn't exist - in fact all that was called was rmdir, which failed to delete in the NFS root-squash environment. Rather than having a whole new interface, the first patch reworks virFileUnlink to check whether the target is a directory or a file and either call rmdir or unlink appropriately. The one common thread amongst the 3 API's changed here is they each looked to return an errno value, while typically virProcessWait consumers only return -1 and errno. Determining which failure in virProcessWait returns -1 is possible because one exit path uses virReportSystemError to report the error that caused the waitpid() to fail, while the other error path either receives the errno from the child process or if not present had already "assumed" EACCES, so these changes follow that model, except that if it's determined the waitpid failed, EINTR is returned similar to how virFileAccessibleAs sets errno and returns -1. John Ferlan (4): storage: Use virFileUnlink instead of rmdir virfile: Use virProcessWait in virFileOpenForked virfile: Use virProcessWait in virFileUnlink virfile: Use virProcessWait in virDirCreate src/storage/storage_backend_fs.c | 20 +++-- src/util/virfile.c | 153 ++++++++++++++++----------------------- 2 files changed, 71 insertions(+), 102 deletions(-) -- 2.1.0
Similar to commit id '35847860', it's possible to attempt to create a 'netfs' directory in an NFS root-squash environment which will cause the 'vol-delete' command to fail. It's also possible error paths from the 'vol-create' would result in an error to remove a created directory if the permissions were incorrect (and disallowed root access). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 20 +++++++++----------- src/util/virfile.c | 23 +++++++++++++++++------ 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f41a41e..388ac7e 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1203,25 +1203,23 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, switch ((virStorageVolType) vol->type) { case VIR_STORAGE_VOL_FILE: + case VIR_STORAGE_VOL_DIR: if (virFileUnlink(vol->target.path, vol->target.perms->uid, vol->target.perms->gid) < 0) { /* Silently ignore failures where the vol has already gone away */ if (errno != ENOENT) { - virReportSystemError(errno, - _("cannot unlink file '%s'"), - vol->target.path); + if (vol->type == VIR_STORAGE_VOL_FILE) + virReportSystemError(errno, + _("cannot unlink file '%s'"), + vol->target.path); + else + virReportSystemError(errno, + _("cannot remove directory '%s'"), + vol->target.path); return -1; } } break; - case VIR_STORAGE_VOL_DIR: - if (rmdir(vol->target.path) < 0) { - virReportSystemError(errno, - _("cannot remove directory '%s'"), - vol->target.path); - return -1; - } - break; case VIR_STORAGE_VOL_BLOCK: case VIR_STORAGE_VOL_NETWORK: case VIR_STORAGE_VOL_NETDIR: diff --git a/src/util/virfile.c b/src/util/virfile.c index fe9f8d4..7c285d9 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2316,7 +2316,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, /* virFileUnlink: - * @path: file to unlink + * @path: file to unlink or directory to remove * @uid: uid that was used to create the file (not required) * @gid: gid that was used to create the file (not required) * @@ -2341,8 +2341,12 @@ virFileUnlink(const char *path, * the file/volume, then use unlink directly */ if ((geteuid() != 0) || - ((uid == (uid_t) -1) && (gid == (gid_t) -1))) - return unlink(path); + ((uid == (uid_t) -1) && (gid == (gid_t) -1))) { + if (virFileIsDir(path)) + return rmdir(path); + else + return unlink(path); + } /* Otherwise, we have to deal with the NFS root-squash craziness * to run under the uid/gid that created the volume in order to @@ -2406,9 +2410,16 @@ virFileUnlink(const char *path, goto childerror; } - if (unlink(path) < 0) { - ret = errno; - goto childerror; + if (virFileIsDir(path)) { + if (rmdir(path) < 0) { + ret = errno; + goto childerror; + } + } else { + if (unlink(path) < 0) { + ret = errno; + goto childerror; + } } childerror: -- 2.1.0
On 18.09.2015 20:20, John Ferlan wrote:
Similar to commit id '35847860', it's possible to attempt to create a 'netfs' directory in an NFS root-squash environment which will cause the 'vol-delete' command to fail. It's also possible error paths from the 'vol-create' would result in an error to remove a created directory if the permissions were incorrect (and disallowed root access).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 20 +++++++++----------- src/util/virfile.c | 23 +++++++++++++++++------ 2 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index fe9f8d4..7c285d9 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2316,7 +2316,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
/* virFileUnlink: - * @path: file to unlink + * @path: file to unlink or directory to remove * @uid: uid that was used to create the file (not required) * @gid: gid that was used to create the file (not required) * @@ -2341,8 +2341,12 @@ virFileUnlink(const char *path, * the file/volume, then use unlink directly */ if ((geteuid() != 0) || - ((uid == (uid_t) -1) && (gid == (gid_t) -1))) - return unlink(path); + ((uid == (uid_t) -1) && (gid == (gid_t) -1))) { + if (virFileIsDir(path)) + return rmdir(path); + else + return unlink(path); + }
/* Otherwise, we have to deal with the NFS root-squash craziness * to run under the uid/gid that created the volume in order to @@ -2406,9 +2410,16 @@ virFileUnlink(const char *path, goto childerror; }
- if (unlink(path) < 0) { - ret = errno; - goto childerror; + if (virFileIsDir(path)) { + if (rmdir(path) < 0) { + ret = errno; + goto childerror; + } + } else { + if (unlink(path) < 0) { + ret = errno; + goto childerror; + } }
childerror:
Since this function is now able to work with dirs too, maybe it should be renamed to something like virFileDirRemove? But that can be saved for a follow up patch. Not a show stopper to me. Michal
On 09/21/2015 06:43 AM, Michal Privoznik wrote:
On 18.09.2015 20:20, John Ferlan wrote:
Similar to commit id '35847860', it's possible to attempt to create a 'netfs' directory in an NFS root-squash environment which will cause the 'vol-delete' command to fail. It's also possible error paths from the 'vol-create' would result in an error to remove a created directory if the permissions were incorrect (and disallowed root access).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 20 +++++++++----------- src/util/virfile.c | 23 +++++++++++++++++------ 2 files changed, 26 insertions(+), 17 deletions(-)
[...]
Since this function is now able to work with dirs too, maybe it should be renamed to something like virFileDirRemove? But that can be saved for a follow up patch. Not a show stopper to me.
OK, but rather than go through more churn - I've sent a v2 of patch 1. I've followed the C API then and named it 'virFileRemove' John
Similar to commit id '35847860', it's possible to attempt to create a 'netfs' directory in an NFS root-squash environment which will cause the 'vol-delete' command to fail. It's also possible error paths from the 'vol-create' would result in an error to remove a created directory if the permissions were incorrect (and disallowed root access). Thus rename the virFileUnlink to be virFileRemove to match the C API functionality, adjust the code to following using rmdir or unlink depending on the path type, and then use/call it for the VIR_STORAGE_VOL_DIR Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 +- src/storage/storage_backend_fs.c | 22 ++++++++++------------ src/util/virfile.c | 29 ++++++++++++++++++++--------- src/util/virfile.h | 2 +- 4 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1bb41f7..8c1f388 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1454,6 +1454,7 @@ virFileReadAllQuiet; virFileReadHeaderFD; virFileReadLimFD; virFileRelLinkPointsTo; +virFileRemove; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; @@ -1461,7 +1462,6 @@ virFileSanitizePath; virFileSkipRoot; virFileStripSuffix; virFileTouch; -virFileUnlink; virFileUnlock; virFileUpdatePerm; virFileWaitForDevices; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f41a41e..99ea394 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1203,25 +1203,23 @@ virStorageBackendFileSystemVolDelete(virConnectPtr conn ATTRIBUTE_UNUSED, switch ((virStorageVolType) vol->type) { case VIR_STORAGE_VOL_FILE: - if (virFileUnlink(vol->target.path, vol->target.perms->uid, + case VIR_STORAGE_VOL_DIR: + if (virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid) < 0) { /* Silently ignore failures where the vol has already gone away */ if (errno != ENOENT) { - virReportSystemError(errno, - _("cannot unlink file '%s'"), - vol->target.path); + if (vol->type == VIR_STORAGE_VOL_FILE) + virReportSystemError(errno, + _("cannot unlink file '%s'"), + vol->target.path); + else + virReportSystemError(errno, + _("cannot remove directory '%s'"), + vol->target.path); return -1; } } break; - case VIR_STORAGE_VOL_DIR: - if (rmdir(vol->target.path) < 0) { - virReportSystemError(errno, - _("cannot remove directory '%s'"), - vol->target.path); - return -1; - } - break; case VIR_STORAGE_VOL_BLOCK: case VIR_STORAGE_VOL_NETWORK: case VIR_STORAGE_VOL_NETDIR: diff --git a/src/util/virfile.c b/src/util/virfile.c index fe9f8d4..668daf8 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2315,8 +2315,8 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, } -/* virFileUnlink: - * @path: file to unlink +/* virFileRemove: + * @path: file to unlink or directory to remove * @uid: uid that was used to create the file (not required) * @gid: gid that was used to create the file (not required) * @@ -2327,7 +2327,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, * from the child. */ int -virFileUnlink(const char *path, +virFileRemove(const char *path, uid_t uid, gid_t gid) { @@ -2341,8 +2341,12 @@ virFileUnlink(const char *path, * the file/volume, then use unlink directly */ if ((geteuid() != 0) || - ((uid == (uid_t) -1) && (gid == (gid_t) -1))) - return unlink(path); + ((uid == (uid_t) -1) && (gid == (gid_t) -1))) { + if (virFileIsDir(path)) + return rmdir(path); + else + return unlink(path); + } /* Otherwise, we have to deal with the NFS root-squash craziness * to run under the uid/gid that created the volume in order to @@ -2406,9 +2410,16 @@ virFileUnlink(const char *path, goto childerror; } - if (unlink(path) < 0) { - ret = errno; - goto childerror; + if (virFileIsDir(path)) { + if (rmdir(path) < 0) { + ret = errno; + goto childerror; + } + } else { + if (unlink(path) < 0) { + ret = errno; + goto childerror; + } } childerror: @@ -2643,7 +2654,7 @@ virDirCreate(const char *path ATTRIBUTE_UNUSED, } int -virFileUnlink(const char *path, +virFileRemove(const char *path, uid_t uid ATTRIBUTE_UNUSED, gid_t gid ATTRIBUTE_UNUSED) { diff --git a/src/util/virfile.h b/src/util/virfile.h index 797ca65..312f226 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -219,7 +219,7 @@ int virFileOpenAs(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int virFileUnlink(const char *path, uid_t uid, gid_t gid); +int virFileRemove(const char *path, uid_t uid, gid_t gid); enum { VIR_DIR_CREATE_NONE = 0, -- 2.1.0
On 21.09.2015 14:08, John Ferlan wrote:
Similar to commit id '35847860', it's possible to attempt to create a 'netfs' directory in an NFS root-squash environment which will cause the 'vol-delete' command to fail. It's also possible error paths from the 'vol-create' would result in an error to remove a created directory if the permissions were incorrect (and disallowed root access).
Thus rename the virFileUnlink to be virFileRemove to match the C API functionality, adjust the code to following using rmdir or unlink depending on the path type, and then use/call it for the VIR_STORAGE_VOL_DIR
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 +- src/storage/storage_backend_fs.c | 22 ++++++++++------------ src/util/virfile.c | 29 ++++++++++++++++++++--------- src/util/virfile.h | 2 +- 4 files changed, 32 insertions(+), 23 deletions(-)
ACK Michal
Rather than inlining the code, use the common API Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 47 +++++++++++++---------------------------------- 1 file changed, 13 insertions(+), 34 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 7c285d9..a0c8f0c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2063,7 +2063,7 @@ 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 status = 0, ret = 0; int recvfd_errno = 0; int fd = -1; int pair[2] = { -1, -1 }; @@ -2163,42 +2163,21 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, if (fd < 0) recvfd_errno = errno; - /* wait for child to complete, and retrieve its exit code - * if waitpid fails, use that status - */ - 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 (virProcessWait(pid, &status, 0) < 0) { + /* virProcessWait() reports errno on waitpid failure, so we'll just + * set our return status to EINTR; otherwise, set status to EACCES + * since the original failure for the fork+setuid path would have + * been EACCES or EPERM by definition. + */ + if (virLastErrorIsSystemErrno(0)) + status = EINTR; + else if (!status) + status = EACCES; } - /* - * If waitpid succeeded, but if the child exited abnormally or - * reported non-zero status, report failure. - */ - if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { - char *msg = virProcessTranslateStatus(status); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("child failed to create '%s': %s"), - path, msg); + if (status) { VIR_FORCE_CLOSE(fd); - VIR_FREE(msg); - /* Use child exit status if possible; otherwise, - * just use -EACCES, since by our original failure in - * the non fork+setuid path would have been EACCES or - * EPERM by definition (see qemuOpenFileAs after the - * first virFileOpenAs failure), but EACCES is close enough. - * Besides -EPERM is like returning fd == -1. - */ - if (WIFEXITED(status)) - ret = -WEXITSTATUS(status); - else - ret = -EACCES; - return ret; + return -status; } /* if waitpid succeeded, but recvfd failed, report recvfd_errno */ -- 2.1.0
Rather than inlining the code, use the common API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index a0c8f0c..02d1a1f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,8 +2311,7 @@ virFileUnlink(const char *path, gid_t gid) { pid_t pid; - int waitret; - int status, ret = 0; + int status = 0, ret = 0; gid_t *groups; int ngroups; @@ -2352,32 +2351,21 @@ virFileUnlink(const char *path, /* wait for child to complete, and retrieve its exit code */ VIR_FREE(groups); - while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR); - if (waitret == -1) { - ret = -errno; - virReportSystemError(errno, - _("failed to wait for child unlinking '%s'"), - path); - goto parenterror; + if (virProcessWait(pid, &status, 0) < 0) { + /* virProcessWait() reports errno on waitpid failure, so we'll just + * set our return status to EINTR; otherwise, set status to EACCES + * since the original failure for the fork+setuid path would have + * been EACCES or EPERM by definition. + */ + if (virLastErrorIsSystemErrno(0)) + status = EINTR; + else if (!status) + status = EACCES; } - /* - * If waitpid succeeded, but if the child exited abnormally or - * reported non-zero status, report failure - */ - if (!WIFEXITED(status) || (WEXITSTATUS(status)) != 0) { - char *msg = virProcessTranslateStatus(status); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("child failed to unlink '%s': %s"), - path, msg); - VIR_FREE(msg); - if (WIFEXITED(status)) - ret = -WEXITSTATUS(status); - else - ret = -EACCES; - } + if (status) + ret = -status; - parenterror: return ret; } -- 2.1.0
Rather than inlining the code, use the common API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 45 +++++++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 02d1a1f..f8c4fec 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2448,8 +2448,7 @@ virDirCreate(const char *path, { struct stat st; pid_t pid; - int waitret; - int status, ret = 0; + int status = 0, ret = 0; gid_t *groups; int ngroups; @@ -2495,36 +2494,30 @@ virDirCreate(const char *path, /* wait for child to complete, and retrieve its exit code */ VIR_FREE(groups); - while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR); - if (waitret == -1) { - ret = -errno; - virReportSystemError(errno, - _("failed to wait for child creating '%s'"), - path); - goto parenterror; + if (virProcessWait(pid, &status, 0) < 0) { + /* virProcessWait() reports errno on waitpid failure, so we'll just + * set our return status to EINTR; otherwise, set status to EACCES + * since the original failure for the fork+setuid path would have + * been EACCES or EPERM by definition. + */ + if (virLastErrorIsSystemErrno(0)) + status = EINTR; + else if (!status) + status = EACCES; } /* - * If waitpid succeeded, but if the child exited abnormally or - * reported non-zero status, report failure, except for EACCES where - * we try to fall back to non-fork method as in the original logic - * introduced and explained by commit 98f6f381. + * If the child exited with EACCES, then fall back to non-fork method + * as in the original logic introduced and explained by commit 98f6f381. */ - if (!WIFEXITED(status) || (WEXITSTATUS(status)) != 0) { - if (WEXITSTATUS(status) == EACCES) - return virDirCreateNoFork(path, mode, uid, gid, flags); - char *msg = virProcessTranslateStatus(status); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("child failed to create '%s': %s"), - path, msg); - VIR_FREE(msg); - if (WIFEXITED(status)) - ret = -WEXITSTATUS(status); - else - ret = -EACCES; + if (status == EACCES) { + virResetLastError(); + return virDirCreateNoFork(path, mode, uid, gid, flags); } - parenterror: + if (status) + ret = -status; + return ret; } -- 2.1.0
On 18.09.2015 20:20, John Ferlan wrote:
A followup of sorts to recently pushed patches regarding NFS root-squash. During libvirt-security list review it was pointed out that the new code was essentially open coding what virProcessWait does. However, since the model being used also was open coded and there was a time element, the change was allowed as is with the expectation that a cleanup patch would follow. Which is what leads into this series....
The series started out purely as removing the open code and replacing with the call to virProcessWait, but during that exercise I also realized that it was possible to create a 'netdir' in a NFS root-squash environment (eg, virDirCreate); however, the corrollary to remove the directory using a fork/exec didn't exist - in fact all that was called was rmdir, which failed to delete in the NFS root-squash environment. Rather than having a whole new interface, the first patch reworks virFileUnlink to check whether the target is a directory or a file and either call rmdir or unlink appropriately.
The one common thread amongst the 3 API's changed here is they each looked to return an errno value, while typically virProcessWait consumers only return -1 and errno. Determining which failure in virProcessWait returns -1 is possible because one exit path uses virReportSystemError to report the error that caused the waitpid() to fail, while the other error path either receives the errno from the child process or if not present had already "assumed" EACCES, so these changes follow that model, except that if it's determined the waitpid failed, EINTR is returned similar to how virFileAccessibleAs sets errno and returns -1.
John Ferlan (4): storage: Use virFileUnlink instead of rmdir virfile: Use virProcessWait in virFileOpenForked virfile: Use virProcessWait in virFileUnlink virfile: Use virProcessWait in virDirCreate
src/storage/storage_backend_fs.c | 20 +++-- src/util/virfile.c | 153 ++++++++++++++++----------------------- 2 files changed, 71 insertions(+), 102 deletions(-)
ACK series. Looking forward to the follow up patch. Michal
participants (2)
-
John Ferlan -
Michal Privoznik