[libvirt] [PATCH] storage: Sanitize pool target paths
Spurious / in a pool target path makes life difficult for apps using the GetVolByPath, and doing other path based comparisons with pools. This has caused a few issues for virt-manager users: https://bugzilla.redhat.com/show_bug.cgi?id=494005 https://bugzilla.redhat.com/show_bug.cgi?id=593565 Add a new util API which removes spurious /, virFileSanitizePath. Sanitize target paths when parsing pool XML, and for paths passed to GetVolByPath. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/storage_conf.c | 8 +++++++- src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 8 +++++++- src/util/util.c | 35 +++++++++++++++++++++++++++++++++++ src/util/util.h | 2 ++ 5 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9aad081..422e76a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -602,6 +602,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { xmlNodePtr source_node; char *type = NULL; char *uuid = NULL; + char *tmppath; if (VIR_ALLOC(ret) < 0) { virReportOOMError(); @@ -699,11 +700,16 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { } } - if ((ret->target.path = virXPathString("string(./target/path)", ctxt)) == NULL) { + if ((tmppath = virXPathString("string(./target/path)", ctxt)) == NULL) { virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool target path")); goto cleanup; } + ret->target.path = virFileSanitizePath(tmppath); + VIR_FREE(tmppath); + if (!ret->target.path) + goto cleanup; + if (virStorageDefParsePerms(ctxt, &ret->target.perms, "./target/permissions", 0700) < 0) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bdeab0f..8e1555c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -666,6 +666,7 @@ virFileReadLimFD; virFilePid; virFileReadPid; virFileLinkPointsTo; +virFileSanitizePath; virParseNumber; virParseVersionString; virPipeReadUntilEOF; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2c69ba9..1a10221 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1204,6 +1204,11 @@ storageVolumeLookupByPath(virConnectPtr conn, virStorageDriverStatePtr driver = conn->storagePrivateData; unsigned int i; virStorageVolPtr ret = NULL; + char *cleanpath; + + cleanpath = virFileSanitizePath(path); + if (!cleanpath) + return NULL; storageDriverLock(driver); for (i = 0 ; i < driver->pools.count && !ret ; i++) { @@ -1213,7 +1218,7 @@ storageVolumeLookupByPath(virConnectPtr conn, const char *stable_path; stable_path = virStorageBackendStablePath(driver->pools.objs[i], - path); + cleanpath); /* * virStorageBackendStablePath already does * virStorageReportError if it fails; we just need to keep @@ -1242,6 +1247,7 @@ storageVolumeLookupByPath(virConnectPtr conn, "%s", _("no storage vol with matching path")); cleanup: + VIR_FREE(cleanpath); storageDriverUnlock(driver); return ret; } diff --git a/src/util/util.c b/src/util/util.c index e937d39..8f86ed6 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1921,6 +1921,41 @@ int virFileAbsPath(const char *path, char **abspath) return 0; } +/* Remove spurious / characters from a path. The result must be freed */ +char * +virFileSanitizePath(const char *path) +{ + const char *cur = path; + char *cleanpath; + int idx = 0; + + cleanpath = strdup(path); + if (!cleanpath) { + virReportOOMError(); + return NULL; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + if (*cur == '/') { + /* Skip all extra / */ + while (*++cur == '/') + continue; + + /* Don't add a trailing / */ + if (*cur == '\0') + break; + + cleanpath[idx++] = '/'; + } + + cleanpath[idx++] = *cur++; + } + cleanpath[idx] = '\0'; + + return cleanpath; +} + /* Like strtol, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. diff --git a/src/util/util.h b/src/util/util.h index 6bf6bcc..abc2688 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -118,6 +118,8 @@ char *virFindFileInPath(const char *file); int virFileExists(const char *path); +char *virFileSanitizePath(const char *path); + enum { VIR_FILE_OP_NONE = 0, VIR_FILE_OP_AS_UID = (1 << 0), -- 1.6.6.1
On Thu, May 20, 2010 at 12:04:05PM -0400, Cole Robinson wrote:
Spurious / in a pool target path makes life difficult for apps using the GetVolByPath, and doing other path based comparisons with pools. This has caused a few issues for virt-manager users:
https://bugzilla.redhat.com/show_bug.cgi?id=494005 https://bugzilla.redhat.com/show_bug.cgi?id=593565
Add a new util API which removes spurious /, virFileSanitizePath. Sanitize target paths when parsing pool XML, and for paths passed to GetVolByPath.
If we're going todo this shouldn't we resolve symlinks, and canonicalize any '..' or '.' components in the path. IIRC gnulib had something for this ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 05/20/2010 12:19 PM, Daniel P. Berrange wrote:
On Thu, May 20, 2010 at 12:04:05PM -0400, Cole Robinson wrote:
Spurious / in a pool target path makes life difficult for apps using the GetVolByPath, and doing other path based comparisons with pools. This has caused a few issues for virt-manager users:
https://bugzilla.redhat.com/show_bug.cgi?id=494005 https://bugzilla.redhat.com/show_bug.cgi?id=593565
Add a new util API which removes spurious /, virFileSanitizePath. Sanitize target paths when parsing pool XML, and for paths passed to GetVolByPath.
If we're going todo this shouldn't we resolve symlinks, and canonicalize any '..' or '.' components in the path. IIRC gnulib had something for this ?
Daniel
Certainly '..' and '.', but maybe only resolve symlinks on path lookup? I don't think users would want their symlinks resolved away in the pool XML, they could use a symlink named /media to point to a horribly long complicated /mnt path. - Cole
On 05/20/2010 10:19 AM, Daniel P. Berrange wrote:
On Thu, May 20, 2010 at 12:04:05PM -0400, Cole Robinson wrote:
Spurious / in a pool target path makes life difficult for apps using the GetVolByPath, and doing other path based comparisons with pools. This has caused a few issues for virt-manager users:
https://bugzilla.redhat.com/show_bug.cgi?id=494005 https://bugzilla.redhat.com/show_bug.cgi?id=593565
Add a new util API which removes spurious /, virFileSanitizePath. Sanitize target paths when parsing pool XML, and for paths passed to GetVolByPath.
If we're going todo this shouldn't we resolve symlinks, and canonicalize any '..' or '.' components in the path. IIRC gnulib had something for this ?
Gnulib has canonicalize-lgpl, but that only succeeds on existing file names. If you want to canonicalize a name without regards to existence, there is canonicalize(), but it is GPL, so we can't use it. :( -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/20/2010 10:34 AM, Eric Blake wrote:
If we're going todo this shouldn't we resolve symlinks, and canonicalize any '..' or '.' components in the path. IIRC gnulib had something for this ?
Gnulib has canonicalize-lgpl, but that only succeeds on existing file names. If you want to canonicalize a name without regards to existence, there is canonicalize(), but it is GPL, so we can't use it. :(
Clarification - there is the 'canonicalize' module, which is GPL, and provides the canonicalize_filename_mode() interface which allows you to choose how to deal with non-existent files. Then there is the 'canonicalize-lgpl' module, which is LGPLv2+, and provides both the realpath() and canonicalize_file_name() functions, but both of those require the file to exist. Also, realpath() is broken by design unless you pass a NULL argument, at which point you might as well be using canonicalize_file_name() wrapper for one less argument. But both of those modules remove all symlinks - gnulib does not yet have any intermediate module that removes just redundant / and . while keeping symlinks intact. And since that operation is just textual, maybe it _does_ make sense to turn it into a gnulib module. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/20/2010 10:04 AM, Cole Robinson wrote:
Spurious / in a pool target path makes life difficult for apps using the GetVolByPath, and doing other path based comparisons with pools. This has caused a few issues for virt-manager users:
https://bugzilla.redhat.com/show_bug.cgi?id=494005 https://bugzilla.redhat.com/show_bug.cgi?id=593565
Add a new util API which removes spurious /, virFileSanitizePath. Sanitize target paths when parsing pool XML, and for paths passed to GetVolByPath.
In general, I like the idea, whether or not we should be moving the sanitization into a gnulib function. But I don't think it's ready for an ack quite yet.
index e937d39..8f86ed6 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1921,6 +1921,41 @@ int virFileAbsPath(const char *path, char **abspath) return 0; }
+/* Remove spurious / characters from a path. The result must be freed */ +char * +virFileSanitizePath(const char *path) +{ + const char *cur = path; + char *cleanpath; + int idx = 0; + + cleanpath = strdup(path); + if (!cleanpath) { + virReportOOMError(); + return NULL; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + if (*cur == '/') { + /* Skip all extra / */ + while (*++cur == '/') + continue;
POSIX says that you must be careful of "//file" - you cannot blindly simplify to "/file" (case in point - cygwin). But POSIX also says that "///file" sanitizes to "/file".
+ + /* Don't add a trailing / */ + if (*cur == '\0') + break; + + cleanpath[idx++] = '/'; + } + + cleanpath[idx++] = *cur++; + } + cleanpath[idx] = '\0'; + + return cleanpath;
Sanitizing "a/b/../c" to "a/c" might not always be the right thing (what if a is a symlink?), but it would also be safe to sanitize "a/./b" to "a/b".
+} + /* Like strtol, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. diff --git a/src/util/util.h b/src/util/util.h index 6bf6bcc..abc2688 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -118,6 +118,8 @@ char *virFindFileInPath(const char *file);
int virFileExists(const char *path);
+char *virFileSanitizePath(const char *path);
Mark this with ATTRIBUTE_NONNULL(1). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/20/2010 03:28 PM, Eric Blake wrote:
On 05/20/2010 10:04 AM, Cole Robinson wrote:
Spurious / in a pool target path makes life difficult for apps using the GetVolByPath, and doing other path based comparisons with pools. This has caused a few issues for virt-manager users:
https://bugzilla.redhat.com/show_bug.cgi?id=494005 https://bugzilla.redhat.com/show_bug.cgi?id=593565
Add a new util API which removes spurious /, virFileSanitizePath. Sanitize target paths when parsing pool XML, and for paths passed to GetVolByPath.
In general, I like the idea, whether or not we should be moving the sanitization into a gnulib function. But I don't think it's ready for an ack quite yet.
index e937d39..8f86ed6 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1921,6 +1921,41 @@ int virFileAbsPath(const char *path, char **abspath) return 0; }
+/* Remove spurious / characters from a path. The result must be freed */ +char * +virFileSanitizePath(const char *path) +{ + const char *cur = path; + char *cleanpath; + int idx = 0; + + cleanpath = strdup(path); + if (!cleanpath) { + virReportOOMError(); + return NULL; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + if (*cur == '/') { + /* Skip all extra / */ + while (*++cur == '/') + continue;
POSIX says that you must be careful of "//file" - you cannot blindly simplify to "/file" (case in point - cygwin). But POSIX also says that "///file" sanitizes to "/file".
+ + /* Don't add a trailing / */ + if (*cur == '\0') + break; + + cleanpath[idx++] = '/'; + } + + cleanpath[idx++] = *cur++; + } + cleanpath[idx] = '\0'; + + return cleanpath;
Sanitizing "a/b/../c" to "a/c" might not always be the right thing (what if a is a symlink?), but it would also be safe to sanitize "a/./b" to "a/b".
+} + /* Like strtol, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. diff --git a/src/util/util.h b/src/util/util.h index 6bf6bcc..abc2688 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -118,6 +118,8 @@ char *virFindFileInPath(const char *file);
int virFileExists(const char *path);
+char *virFileSanitizePath(const char *path);
Mark this with ATTRIBUTE_NONNULL(1).
Thanks for the review, updated patch sent. - Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake