[libvirt] [PATCH v3] 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. v2: Leading // must be preserved, properly sanitize path=/, sanitize away /./ -> / v3: Properly handle starting ./ and ending /. 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 | 75 ++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 5 files changed, 93 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 1e4bfd0..8b397bf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -675,6 +675,7 @@ virFileReadLimFD; virFilePid; virFileReadPid; virFileLinkPointsTo; +virFileSanitizePath; virParseNumber; virParseVersionString; virPipeReadUntilEOF; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b148e39..0870f74 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 92b9a0f..ca10444 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1921,6 +1921,81 @@ 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; + } + + /* Need to sanitize: + * // -> // + * /// -> / + * /../foo -> /../foo + * /.//foo -> /foo + * /foo///bar/ -> /foo/bar + * ./foo/./. -> /foo + */ + + /* Starting with // is valid posix, but ///foo == /foo */ + if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') { + idx = 2; + cur += 2; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + int offset = cur - path; + + /* Copy all dirname characters */ + if ((cur[0] != '/' && cur[0] != '.') || + (cur[0] == '.' && cur[1] != '/' && cur[1] != '\0')) { + cleanpath[idx++] = *cur++; + continue; + } + + /* Sanitize away / and single . */ + do { + bool slash_follow = (cur[1] == '/'); + bool slash_before = (offset != 0 && cur[-1] == '/'); + + /* Skip all extra / */ + if (*cur == '/') { + cur++; + continue; + } + + /* Handle starting ./ ending /. //./ . */ + if ((slash_follow && slash_before) || + (offset == 0 && slash_follow) || + (slash_before && cur[1] == '\0') || + (offset == 0 && cur[1] == '\0')) { + cur++; + continue; + } + + /* '.' was part of a dirname */ + break; + } while (*cur == '/' || *cur == '.'); + + /* Don't add trailing / */ + if (idx > 0 && (cleanpath[idx-1] == '/' || *cur == '\0')) + continue; + + cleanpath[idx++] = '/'; + } + 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..302bddf 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -118,6 +118,9 @@ char *virFindFileInPath(const char *file); int virFileExists(const char *path); +char *virFileSanitizePath(const char *path) +ATTRIBUTE_NONNULL(1); + enum { VIR_FILE_OP_NONE = 0, VIR_FILE_OP_AS_UID = (1 << 0), -- 1.6.6.1
On 05/24/2010 12:52 PM, 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.
v2: Leading // must be preserved, properly sanitize path=/, sanitize away /./ -> /
v3: Properly handle starting ./ and ending /.
+ + /* Need to sanitize: + * // -> // + * /// -> / + * /../foo -> /../foo + * /.//foo -> /foo + * /foo///bar/ -> /foo/bar + * ./foo/./. -> /foo
comment typo: ./foo/./. -> foo (not absolute)
+ */ + + /* Starting with // is valid posix, but ///foo == /foo */ + if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') { + idx = 2; + cur += 2; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + int offset = cur - path; + + /* Copy all dirname characters */ + if ((cur[0] != '/' && cur[0] != '.') || + (cur[0] == '.' && cur[1] != '/' && cur[1] != '\0')) { + cleanpath[idx++] = *cur++; + continue; + } + + /* Sanitize away / and single . */ + do { + bool slash_follow = (cur[1] == '/');
Phooey. Need a v4; this can fault. If you have "///" ending on a page boundary, then...
+ bool slash_before = (offset != 0 && cur[-1] == '/'); + + /* Skip all extra / */ + if (*cur == '/') { + cur++; + continue; + }
...this advances cur to the '\0', and the next iteration of the nested do-while accesses past the trailing NUL when computing slash_follow. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/24/2010 04:55 PM, Eric Blake wrote:
Phooey. Need a v4; this can fault. If you have "///" ending on a page boundary, then...
+ bool slash_before = (offset != 0 && cur[-1] == '/'); + + /* Skip all extra / */ + if (*cur == '/') { + cur++; + continue; + }
...this advances cur to the '\0', and the next iteration of the nested do-while accesses past the trailing NUL when computing slash_follow.
I spoke too soon. I keep forgetting that with a do-while, the continue still checks the loop condition, rather than blindly jumping to the loop start. I'm re-reading the patch in that context, and you may be clean with v3 after all. Sorry for the poor review... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/24/2010 12:52 PM, Cole Robinson wrote:
+ + /* Need to sanitize: + * // -> // + * /// -> / + * /../foo -> /../foo + * /.//foo -> /foo + * /foo///bar/ -> /foo/bar + * ./foo/./. -> /foo + */ +
For my second attempt at a valid review, I actually compiled the function, and threw the above inputs at it. /../foo -> /./foo (oops, didn't match documentation), and ./foo/./. -> /foo (oops, matched documentation, but turned a relative path into absolute), so we do need a v4, but not for the original reasons in my first NAK where I mis-read the do-while loop. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/25/2010 10:42 AM, Eric Blake wrote:
On 05/24/2010 12:52 PM, Cole Robinson wrote:
+ + /* Need to sanitize: + * // -> // + * /// -> / + * /../foo -> /../foo + * /.//foo -> /foo + * /foo///bar/ -> /foo/bar + * ./foo/./. -> /foo + */ +
For my second attempt at a valid review, I actually compiled the function, and threw the above inputs at it. /../foo -> /./foo (oops, didn't match documentation), and ./foo/./. -> /foo (oops, matched documentation, but turned a relative path into absolute), so we do need a v4, but not for the original reasons in my first NAK where I mis-read the do-while loop.
Thanks for the review, I'll fix that case and repost. - Cole
On Tue, May 25, 2010 at 08:42:31AM -0600, Eric Blake wrote:
On 05/24/2010 12:52 PM, Cole Robinson wrote:
+ + /* Need to sanitize: + * // -> // + * /// -> / + * /../foo -> /../foo + * /.//foo -> /foo + * /foo///bar/ -> /foo/bar + * ./foo/./. -> /foo + */ +
For my second attempt at a valid review, I actually compiled the function, and threw the above inputs at it. /../foo -> /./foo (oops, didn't match documentation), and ./foo/./. -> /foo (oops, matched documentation, but turned a relative path into absolute), so we do need a v4, but not for the original reasons in my first NAK where I mis-read the do-while loop.
This function is crying out for a real test case to be written and put under tests/, feeding it all sorts of evil input. 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/25/2010 10:54 AM, Daniel P. Berrange wrote:
On Tue, May 25, 2010 at 08:42:31AM -0600, Eric Blake wrote:
On 05/24/2010 12:52 PM, Cole Robinson wrote:
+ + /* Need to sanitize: + * // -> // + * /// -> / + * /../foo -> /../foo + * /.//foo -> /foo + * /foo///bar/ -> /foo/bar + * ./foo/./. -> /foo + */ +
For my second attempt at a valid review, I actually compiled the function, and threw the above inputs at it. /../foo -> /./foo (oops, didn't match documentation), and ./foo/./. -> /foo (oops, matched documentation, but turned a relative path into absolute), so we do need a v4, but not for the original reasons in my first NAK where I mis-read the do-while loop.
This function is crying out for a real test case to be written and put under tests/, feeding it all sorts of evil input.
Agreed, however I'm backtracking a bit on this, my next post is only going to sanitize spurious /, like my original posting (but with the original comments addressed). A user can accidentally add an extra slash to a pool target, and its reasonable to expect it won't cause cause problems, but the same can't be said for relative path characters. This function is becoming impossible to review (the current posting goes into infinite loop with a path like /foo./bar), and most of the logic is not helping solve a real world problem. This patch is going to be backported to a few places: less complex the better. Oh, and I'm lazy :) - Cole
On 05/25/2010 09:49 AM, Cole Robinson wrote:
This function is crying out for a real test case to be written and put under tests/, feeding it all sorts of evil input.
Agreed, however I'm backtracking a bit on this, my next post is only going to sanitize spurious /, like my original posting (but with the original comments addressed).
Which is all the more argument for a gnulib module that does text-based sanitization on the user's behalf. I'll see about adding that, and in the meantime,...
A user can accidentally add an extra slash to a pool target, and its reasonable to expect it won't cause cause problems, but the same can't be said for relative path characters. This function is becoming impossible to review (the current posting goes into infinite loop with a path like /foo./bar), and most of the logic is not helping solve a real world problem.
This patch is going to be backported to a few places: less complex the better. Oh, and I'm lazy :)
I agree with Cole's approach of minimizing this particular fix to the particular problem raised. A fancy gnulib solution is not as important as a duplicated slash solution. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake