[libvirt] [PATCH] virFindFileInPath: only find executable non-directory
Without this patch, at least tests/daemon-conf (which sticks $builddir/src in the PATH) tries to execute the directory $builddir/src/qemu rather than the real /usr/bin/qemu binary. That was fine when qemu_capabilities silently ignored execution failure, but not so good once it is converted to use virCommand. * src/util/util.h (virFileExists): Adjust prototype. (virFileIsExecutable): New prototype. * src/util/util.c (virFindFileInPath): Reject non-executables and directories. Avoid huge stack allocation. (virFileExists): Use lighter-weight syscall. (virFileIsExecutable): New function. * src/libvirt_private.syms (util.h): Export new function. --- Questions: Should I be requiring S_ISREG, rather than !S_ISDIR (that is, should we be rejecting devices and sockets as non-exectuable)? Should I import the gnulib module euidaccess (and/or faccessat) for the access check? Using access(F_OK) is okay regardless of uid/gid, but access(X_OK) may have different answers than euidaccess(X_OK) when the effective uid/gid do not match the current uid/gid. However, dragging in the gnulib module will require adding an extra link library for some platforms (for example, Solaris needs -lgen), which means the change is more invasive as it will also affect Makefiles. src/libvirt_private.syms | 1 + src/util/util.c | 53 ++++++++++++++++++++++++++++++--------------- src/util/util.h | 7 +++-- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 65911df..948bbe1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -850,6 +850,7 @@ virFileDeletePid; virFileExists; virFileFindMountPoint; virFileHasSuffix; +virFileIsExecutable; virFileLinkPointsTo; virFileMakePath; virFileMatchesNameSuffix; diff --git a/src/util/util.c b/src/util/util.c index 60feb79..25e6185 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1255,7 +1255,7 @@ int virFileResolveLink(const char *linkpath, } /* - * Finds a requested file in the PATH env. e.g.: + * Finds a requested executable file in the PATH env. e.g.: * "kvm-img" will return "/usr/bin/kvm-img" * * You must free the result @@ -1263,19 +1263,18 @@ int virFileResolveLink(const char *linkpath, char *virFindFileInPath(const char *file) { char *path; - char pathenv[PATH_MAX]; - char *penv = pathenv; + char *pathiter; char *pathseg; - char fullpath[PATH_MAX]; + char *fullpath = NULL; if (file == NULL) return NULL; /* if we are passed an absolute path (starting with /), return a - * copy of that path + * copy of that path, after validating that it is executable */ - if (file[0] == '/') { - if (virFileExists(file)) + if (IS_ABSOLUTE_FILE_NAME(file)) { + if (virFileIsExecutable(file)) return strdup(file); else return NULL; @@ -1284,27 +1283,45 @@ char *virFindFileInPath(const char *file) /* copy PATH env so we can tweak it */ path = getenv("PATH"); - if (path == NULL || virStrcpyStatic(pathenv, path) == NULL) + if (path == NULL || (path = strdup(path)) == NULL) return NULL; /* for each path segment, append the file to search for and test for * it. return it if found. */ - while ((pathseg = strsep(&penv, ":")) != NULL) { - snprintf(fullpath, PATH_MAX, "%s/%s", pathseg, file); - if (virFileExists(fullpath)) - return strdup(fullpath); + pathiter = path; + while ((pathseg = strsep(&pathiter, ":")) != NULL) { + if (virAsprintf(&fullpath, "%s/%s", pathseg, file) < 0 || + virFileIsExecutable(fullpath)) + break; + VIR_FREE(fullpath); } - return NULL; + VIR_FREE(path); + return fullpath; } -int virFileExists(const char *path) + +bool virFileExists(const char *path) { - struct stat st; + return access(path, F_OK) == 0; +} - if (stat(path, &st) >= 0) - return(1); - return(0); +/* Check that a file can be executed by the current user, and is not + * a directory. */ +bool +virFileIsExecutable(const char *file) +{ + struct stat sb; + + /* The existence of ACLs means that checking (sb.st_mode&0111) for + * executable bits may give false results; plus, access is + * lighter-weight than stat for a first pass filter. + * + * XXX: should this use gnulib's euidaccess module? + */ + return (access(file, X_OK) == 0 && + stat(file, &sb) == 0 && + !S_ISDIR(sb.st_mode)); } #ifndef WIN32 diff --git a/src/util/util.h b/src/util/util.h index 989962f..54c3058 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -1,8 +1,7 @@ - /* * utils.h: common, generic utility functions * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * @@ -32,6 +31,7 @@ # include <sys/select.h> # include <sys/types.h> # include <stdarg.h> +# include <stdbool.h> # ifndef MIN # define MIN(a, b) ((a) < (b) ? (a) : (b)) @@ -120,7 +120,8 @@ int virFileResolveLink(const char *linkpath, char *virFindFileInPath(const char *file); -int virFileExists(const char *path); +bool virFileExists(const char *file); +bool virFileIsExecutable(const char *file); char *virFileSanitizePath(const char *path); -- 1.7.3.4
On Wed, Jan 12, 2011 at 09:24:57AM -0700, Eric Blake wrote:
Without this patch, at least tests/daemon-conf (which sticks $builddir/src in the PATH) tries to execute the directory $builddir/src/qemu rather than the real /usr/bin/qemu binary.
Not looking at your patch, I have to question why on earth the script is adding $builddir/src to the $PATH ? There are no command line binaries in src/ anymore, only in tools/ and it clearly doesn't need any of them if it hasn't also added tools/ to $PATH.
Questions:
Should I be requiring S_ISREG, rather than !S_ISDIR (that is, should we be rejecting devices and sockets as non-exectuable)?
Should I import the gnulib module euidaccess (and/or faccessat) for the access check? Using access(F_OK) is okay regardless of uid/gid, but access(X_OK) may have different answers than euidaccess(X_OK) when the effective uid/gid do not match the current uid/gid. However, dragging in the gnulib module will require adding an extra link library for some platforms (for example, Solaris needs -lgen), which means the change is more invasive as it will also affect Makefiles.
+/* Check that a file can be executed by the current user, and is not + * a directory. */ +bool +virFileIsExecutable(const char *file) +{ + struct stat sb; + + /* The existence of ACLs means that checking (sb.st_mode&0111) for + * executable bits may give false results; plus, access is + * lighter-weight than stat for a first pass filter. + * + * XXX: should this use gnulib's euidaccess module? + */ + return (access(file, X_OK) == 0 && + stat(file, &sb) == 0 && + !S_ISDIR(sb.st_mode));
If we're doing stat() anyway, it could be better to kill the access() check and just check S_IXUSR on sb.sb_mode. Yes, that isn't the same as access, but I think we it can be argued that we should accept binaries which have any 'x' bit set even if we ourselves can't execute them. eg, if /usr/bin/qemu was -rwx-r--r-- then we should not skip it. We should pick that binary on the basis that the user will probably want to see the EACCESS message when we later try to exec() it, so they can see the installation bug & fix it. If we skipped the binary, they'd get a 'cannot find binary foo' error message from libvirt which would be somewhat misleading because it would clearly exist as far as the user can see. Regards, Daniel
On 01/12/2011 10:39 AM, Daniel P. Berrange wrote:
On Wed, Jan 12, 2011 at 09:24:57AM -0700, Eric Blake wrote:
Without this patch, at least tests/daemon-conf (which sticks $builddir/src in the PATH) tries to execute the directory $builddir/src/qemu rather than the real /usr/bin/qemu binary.
Not looking at your patch, I have to question why on earth the script is adding $builddir/src to the $PATH ? There are no command line binaries in src/ anymore, only in tools/ and it clearly doesn't need any of them if it hasn't also added tools/ to $PATH.
tests/Makefile.am is the culprit for that PATH setting: path_add = $$abs_top_builddir/src$(PATH_SEPARATOR)$$abs_top_builddir/daemon$(PATH_SEPARATOR)$$abs_top_builddir/tools TESTS_ENVIRONMENT = \ ... PATH="$(path_add)$(PATH_SEPARATOR)$$PATH" \ Probably worth a separate cleanup, now that you mention it.
+/* Check that a file can be executed by the current user, and is not + * a directory. */ +bool +virFileIsExecutable(const char *file) +{ + struct stat sb; + + /* The existence of ACLs means that checking (sb.st_mode&0111) for + * executable bits may give false results; plus, access is + * lighter-weight than stat for a first pass filter. + * + * XXX: should this use gnulib's euidaccess module? + */ + return (access(file, X_OK) == 0 && + stat(file, &sb) == 0 && + !S_ISDIR(sb.st_mode));
If we're doing stat() anyway, it could be better to kill the access() check and just check S_IXUSR on sb.sb_mode. Yes, that isn't the same as access, but I think we it can be argued that we should accept binaries which have any 'x' bit set even if we ourselves can't execute them.
Or, conversely, if the go+x bits are set ACLs include u-x (and thus deny the current user the right to execute them).
eg, if /usr/bin/qemu was -rwx-r--r-- then we should not skip it. We should pick that binary on the basis that the user will probably want to see the EACCESS message when we later try to exec() it, so they can see the installation bug & fix it.
Fair enough - I'll rewrite the patch to avoid access(X_OK), on the grounds that ACLs are corner case enough that we don't mind getting the occasional wrong answer due to ACL weirdness. Which means:
Questions:
Should I be requiring S_ISREG, rather than !S_ISDIR (that is, should we be rejecting devices and sockets as non-exectuable)?
Still not answered, but I'll go ahead and make that change for v2.
Should I import the gnulib module euidaccess (and/or faccessat) for the access check? Using access(F_OK) is okay regardless of uid/gid, but access(X_OK) may have different answers than euidaccess(X_OK) when the effective uid/gid do not match the current uid/gid. However, dragging in the gnulib module will require adding an extra link library for some platforms (for example, Solaris needs -lgen), which means the change is more invasive as it will also affect Makefiles.
Answered - avoid access (and thus euidaccess) altogether, and go with the naive but usually right stat() parsing instead. v2 coming shortly. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Without this patch, at least tests/daemon-conf (which sticks $builddir/src in the PATH) tries to execute the directory $builddir/src/qemu rather than a real qemu binary. * src/util/util.h (virFileExists): Adjust prototype. (virFileIsExecutable): New prototype. * src/util/util.c (virFindFileInPath): Reject non-executables and directories. Avoid huge stack allocation. (virFileExists): Use lighter-weight syscall. (virFileIsExecutable): New function. * src/libvirt_private.syms (util.h): Export new function. --- New in v2: Require S_ISREG, not !S_ISDIR; avoid access(X_OK) and instead document that ACLs are disregarded src/libvirt_private.syms | 1 + src/util/util.c | 53 ++++++++++++++++++++++++++++++--------------- src/util/util.h | 7 +++-- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e9b8cb7..a166bd9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -845,6 +845,7 @@ virFileDeletePid; virFileExists; virFileFindMountPoint; virFileHasSuffix; +virFileIsExecutable; virFileLinkPointsTo; virFileMakePath; virFileMatchesNameSuffix; diff --git a/src/util/util.c b/src/util/util.c index 60feb79..18b38f4 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1255,7 +1255,7 @@ int virFileResolveLink(const char *linkpath, } /* - * Finds a requested file in the PATH env. e.g.: + * Finds a requested executable file in the PATH env. e.g.: * "kvm-img" will return "/usr/bin/kvm-img" * * You must free the result @@ -1263,19 +1263,18 @@ int virFileResolveLink(const char *linkpath, char *virFindFileInPath(const char *file) { char *path; - char pathenv[PATH_MAX]; - char *penv = pathenv; + char *pathiter; char *pathseg; - char fullpath[PATH_MAX]; + char *fullpath = NULL; if (file == NULL) return NULL; /* if we are passed an absolute path (starting with /), return a - * copy of that path + * copy of that path, after validating that it is executable */ - if (file[0] == '/') { - if (virFileExists(file)) + if (IS_ABSOLUTE_FILE_NAME(file)) { + if (virFileIsExecutable(file)) return strdup(file); else return NULL; @@ -1284,27 +1283,45 @@ char *virFindFileInPath(const char *file) /* copy PATH env so we can tweak it */ path = getenv("PATH"); - if (path == NULL || virStrcpyStatic(pathenv, path) == NULL) + if (path == NULL || (path = strdup(path)) == NULL) return NULL; /* for each path segment, append the file to search for and test for * it. return it if found. */ - while ((pathseg = strsep(&penv, ":")) != NULL) { - snprintf(fullpath, PATH_MAX, "%s/%s", pathseg, file); - if (virFileExists(fullpath)) - return strdup(fullpath); + pathiter = path; + while ((pathseg = strsep(&pathiter, ":")) != NULL) { + if (virAsprintf(&fullpath, "%s/%s", pathseg, file) < 0 || + virFileIsExecutable(fullpath)) + break; + VIR_FREE(fullpath); } - return NULL; + VIR_FREE(path); + return fullpath; } -int virFileExists(const char *path) + +bool virFileExists(const char *path) { - struct stat st; + return access(path, F_OK) == 0; +} - if (stat(path, &st) >= 0) - return(1); - return(0); +/* Check that a file is regular and has executable bits. + * + * Note: In the presence of ACLs, this may return true for a file that + * would actually fail with EACCES for a given user, or false for a + * file that the user could actually execute, but setups with ACLs + * that weird are unusual. */ +bool +virFileIsExecutable(const char *file) +{ + struct stat sb; + + /* We would also want to check faccessat if we cared about ACLs, + * but we don't. */ + return (stat(file, &sb) == 0 && + S_ISREG(sb.st_mode) && + (sb.st_mode & 0111) != 0); } #ifndef WIN32 diff --git a/src/util/util.h b/src/util/util.h index 989962f..54c3058 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -1,8 +1,7 @@ - /* * utils.h: common, generic utility functions * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * @@ -32,6 +31,7 @@ # include <sys/select.h> # include <sys/types.h> # include <stdarg.h> +# include <stdbool.h> # ifndef MIN # define MIN(a, b) ((a) < (b) ? (a) : (b)) @@ -120,7 +120,8 @@ int virFileResolveLink(const char *linkpath, char *virFindFileInPath(const char *file); -int virFileExists(const char *path); +bool virFileExists(const char *file); +bool virFileIsExecutable(const char *file); char *virFileSanitizePath(const char *path); -- 1.7.3.4
On Wed, Jan 12, 2011 at 01:16:51PM -0700, Eric Blake wrote:
Without this patch, at least tests/daemon-conf (which sticks $builddir/src in the PATH) tries to execute the directory $builddir/src/qemu rather than a real qemu binary.
* src/util/util.h (virFileExists): Adjust prototype. (virFileIsExecutable): New prototype. * src/util/util.c (virFindFileInPath): Reject non-executables and directories. Avoid huge stack allocation. (virFileExists): Use lighter-weight syscall. (virFileIsExecutable): New function. * src/libvirt_private.syms (util.h): Export new function. ---
New in v2: Require S_ISREG, not !S_ISDIR; avoid access(X_OK) and instead document that ACLs are disregarded
src/libvirt_private.syms | 1 + src/util/util.c | 53 ++++++++++++++++++++++++++++++--------------- src/util/util.h | 7 +++-- 3 files changed, 40 insertions(+), 21 deletions(-)
ACK Daniel
Commit 870dba0 (Mar 2008) added builddir/src to PATH to pick up virsh. Later, virsh was moved to tools; commit db68d6b (Oct 2009) noticed this, but only added the new location rather than deleting the old location. * tests/Makefile.am (path_add): Drop now-useless directory. Suggested by Daniel P. Berrange. --- No v1 counterpart. tests/Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 697b401..345cf46 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -214,7 +214,7 @@ TESTS += interfacexml2xmltest TESTS += cputest -path_add = $$abs_top_builddir/src$(PATH_SEPARATOR)$$abs_top_builddir/daemon$(PATH_SEPARATOR)$$abs_top_builddir/tools +path_add = $$abs_top_builddir/daemon$(PATH_SEPARATOR)$$abs_top_builddir/tools # NB, automake < 1.10 does not provide the real # abs_top_{src/build}dir or builddir variables, so don't rely -- 1.7.3.4
On Wed, Jan 12, 2011 at 01:16:52PM -0700, Eric Blake wrote:
Commit 870dba0 (Mar 2008) added builddir/src to PATH to pick up virsh. Later, virsh was moved to tools; commit db68d6b (Oct 2009) noticed this, but only added the new location rather than deleting the old location.
* tests/Makefile.am (path_add): Drop now-useless directory. Suggested by Daniel P. Berrange. ---
No v1 counterpart.
tests/Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK Daniel
On 01/13/2011 06:42 AM, Daniel P. Berrange wrote:
On Wed, Jan 12, 2011 at 01:16:52PM -0700, Eric Blake wrote:
Commit 870dba0 (Mar 2008) added builddir/src to PATH to pick up virsh. Later, virsh was moved to tools; commit db68d6b (Oct 2009) noticed this, but only added the new location rather than deleting the old location.
* tests/Makefile.am (path_add): Drop now-useless directory. Suggested by Daniel P. Berrange. ---
No v1 counterpart.
tests/Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK
Thanks; both of these are now pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake