[libvirt] [PATCH 0/3] Restrict saved-state and core-dump files in controlled directories
This patch series tries to address the issue discussed in: https://www.redhat.com/archives/libvir-list/2011-September/msg01025.html In this series: . The filename parameter for virDomainSave[Flags], virDomainRestore[Flags], virDomainSaveImageGetXMLDesc, virDomainSaveImageDefineXML, and virDomainCoreDump, is interpreted as an ID; . The file ID is later translated to a real filesystem pathname by corresponding drivers; . The real file system paths are under controlled directories, different for saved-state-files and core-dumps, respectively; Hong Xiang (3): New util API virBase64EncodePathname/virBase64DecodePathname Remove virFileAbsPath() from virDomainSave*() and virDomainCoreDump() calls Encode input file id in qemuDomainSave/Restore and qemudDomainCoreDump src/libvirt.c | 84 +++------------------------------------ src/libvirt_private.syms | 2 + src/qemu/qemu_conf.h | 3 + src/qemu/qemu_driver.c | 98 +++++++++++++++++++++++++++++++++++++++++++-- src/util/util.c | 73 ++++++++++++++++++++++++++++++++++ src/util/util.h | 4 ++ 6 files changed, 182 insertions(+), 82 deletions(-)
Base64 en/decode a string and '/' in standard base64 is replaced with '_', so that encoded string can be safely used as a filesystem path component; * src/util/util.c: implementation * src/util/util.h: declaration * src/libvirt_private.syms: private symbols Signed-off-by: Hong Xiang <hxiang@linux.vnet.ibm.com> --- src/libvirt_private.syms | 2 + src/util/util.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 4 ++ 3 files changed, 79 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ae1157..5299f79 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1082,6 +1082,8 @@ safewrite; safezero; virArgvToString; virAsprintf; +virBase64EncodePathname; +virBase64DecodePathname; virBuildPathInternal; virDirCreate; virEmitXMLWarning; diff --git a/src/util/util.c b/src/util/util.c index af27344..3aa6f2e 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -67,6 +67,7 @@ #include "dirname.h" #include "virterror_internal.h" +#include "base64.h" #include "logging.h" #include "buf.h" #include "util.h" @@ -2497,3 +2498,75 @@ or other application using the libvirt API.\n\ return 0; } + +/** + * virBase64EncodePath: + * @src: source + * @dst: pointer to result + * + * Base64-encode @src, put pointer to dynamically allocated result into + * @dst. + * *And* make the result a filesystem safe pathname by replacing all '/' + * therein with '_'. + * + * Return 0 on success and -1 on failure. + */ +int +virBase64EncodePathname(const char * src, char ** dst) +{ + char *ptr; + + base64_encode_alloc(src, strlen(src), dst); + if(NULL == *dst) + return -1; + for (ptr = *dst; *ptr; ptr ++) { + if('/' == *ptr) + *ptr = '_'; + } + return 0; +} + +/** + * virBase64Decode: + * @src: source + * @dst: pointer to result + * + * Convert @src to a standard base64 string by replacing all '_' with '/', + * then base64-decode the intermediate result; pointer to dynamically + * allocated final result is put in @dst. + * + * Return 0 on success and -1 on memory failure and -2 if @src does not + * contain a valid base64 encoded string. + */ +int +virBase64DecodePathname(const char * src, char ** dst) +{ + char *i_dst; + int i; + size_t dst_len; + + *dst = NULL; + if(VIR_ALLOC_N(i_dst, strlen(src)) < 0) + return -1; + for (i = 0; src[i]; i ++) { + if('_' == src[i]) + i_dst[i] = '/'; + else + i_dst[i] = src[i]; + } + + dst_len = 3 * (i / 4) + 4; + if(VIR_ALLOC_N(*dst, dst_len) < 0) { + VIR_FREE(i_dst); + return -1; + } + if(!base64_decode_ctx(NULL, i_dst, i, *dst, &dst_len)) { + VIR_FREE(i_dst); + VIR_FREE(*dst); + return -2; + } + VIR_FREE(i_dst); + (*dst)[dst_len] = '\0'; + + return 0; +} diff --git a/src/util/util.h b/src/util/util.h index c55e852..7fb41c2 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -260,4 +260,8 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); int virEmitXMLWarning(int fd, const char *name, const char *cmd) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +int virBase64EncodePathname(const char * src, char ** dst); +int virBase64DecodePathname(const char * src, char ** dst); + #endif /* __VIR_UTIL_H__ */ -- 1.7.1
Change the semantics of '--file' parameter user provides in cmdline from a relative/absolute filesystem path to an id which is to be converted in driver to form a filesystem path. * src/libvirt.c: remove virFileAbsPath() calls Signed-off-by: Hong Xiang <hxiang@linux.vnet.ibm.com> --- src/libvirt.c | 84 +++++---------------------------------------------------- 1 files changed, 7 insertions(+), 77 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index a6bcee6..5de60c7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2470,18 +2470,8 @@ virDomainSave(virDomainPtr domain, const char *to) if (conn->driver->domainSave) { int ret; - char *absolute_to; - /* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("could not build absolute output file path")); - goto error; - } - - ret = conn->driver->domainSave(domain, absolute_to); - - VIR_FREE(absolute_to); + ret = conn->driver->domainSave(domain, to); if (ret < 0) goto error; @@ -2564,18 +2554,8 @@ virDomainSaveFlags(virDomainPtr domain, const char *to, if (conn->driver->domainSaveFlags) { int ret; - char *absolute_to; - - /* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("could not build absolute output file path")); - goto error; - } - ret = conn->driver->domainSaveFlags(domain, absolute_to, dxml, flags); - - VIR_FREE(absolute_to); + ret = conn->driver->domainSaveFlags(domain, to, dxml, flags); if (ret < 0) goto error; @@ -2623,18 +2603,8 @@ virDomainRestore(virConnectPtr conn, const char *from) if (conn->driver->domainRestore) { int ret; - char *absolute_from; - - /* We must absolutize the file path as the restore is done out of process */ - if (virFileAbsPath(from, &absolute_from) < 0) { - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("could not build absolute input file path")); - goto error; - } - ret = conn->driver->domainRestore(conn, absolute_from); - - VIR_FREE(absolute_from); + ret = conn->driver->domainRestore(conn, from); if (ret < 0) goto error; @@ -2708,20 +2678,10 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, if (conn->driver->domainRestoreFlags) { int ret; - char *absolute_from; - - /* We must absolutize the file path as the restore is done out of process */ - if (virFileAbsPath(from, &absolute_from) < 0) { - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("could not build absolute input file path")); - goto error; - } - ret = conn->driver->domainRestoreFlags(conn, absolute_from, dxml, + ret = conn->driver->domainRestoreFlags(conn, from, dxml, flags); - VIR_FREE(absolute_from); - if (ret < 0) goto error; return ret; @@ -2779,20 +2739,10 @@ virDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *file, if (conn->driver->domainSaveImageGetXMLDesc) { char *ret; - char *absolute_file; - /* We must absolutize the file path as the read is done out of process */ - if (virFileAbsPath(file, &absolute_file) < 0) { - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("could not build absolute input file path")); - goto error; - } - - ret = conn->driver->domainSaveImageGetXMLDesc(conn, absolute_file, + ret = conn->driver->domainSaveImageGetXMLDesc(conn, file, flags); - VIR_FREE(absolute_file); - if (!ret) goto error; return ret; @@ -2862,20 +2812,10 @@ virDomainSaveImageDefineXML(virConnectPtr conn, const char *file, if (conn->driver->domainSaveImageDefineXML) { int ret; - char *absolute_file; - - /* We must absolutize the file path as the read is done out of process */ - if (virFileAbsPath(file, &absolute_file) < 0) { - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("could not build absolute input file path")); - goto error; - } - ret = conn->driver->domainSaveImageDefineXML(conn, absolute_file, + ret = conn->driver->domainSaveImageDefineXML(conn, file, dxml, flags); - VIR_FREE(absolute_file); - if (ret < 0) goto error; return ret; @@ -2957,18 +2897,8 @@ virDomainCoreDump(virDomainPtr domain, const char *to, unsigned int flags) if (conn->driver->domainCoreDump) { int ret; - char *absolute_to; - - /* We must absolutize the file path as the save is done out of process */ - if (virFileAbsPath(to, &absolute_to) < 0) { - virLibConnError(VIR_ERR_INTERNAL_ERROR, - _("could not build absolute core file path")); - goto error; - } - - ret = conn->driver->domainCoreDump(domain, absolute_to, flags); - VIR_FREE(absolute_to); + ret = conn->driver->domainCoreDump(domain, to, flags); if (ret < 0) goto error; -- 1.7.1
On Tue, Oct 25, 2011 at 03:43:21PM +0800, Hong Xiang wrote:
Change the semantics of '--file' parameter user provides in cmdline from a relative/absolute filesystem path to an id which is to be converted in driver to form a filesystem path.
* src/libvirt.c: remove virFileAbsPath() calls
Signed-off-by: Hong Xiang <hxiang@linux.vnet.ibm.com> --- src/libvirt.c | 84 +++++---------------------------------------------------- 1 files changed, 7 insertions(+), 77 deletions(-)
NACK, you can't change the semantics of existing APIs in this way. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Base64 encode input file id in qemuDomainSaveFlags(), qemuDomainRestoreFlags(), qemuDomainSaveImageGetXMLDesc(), qemuDomainSaveImageDefineXML(), qemudDomainCoreDump(), and put generated files in controlled directories: qemu_driver->savedImageDir, and qemu_driver->coreDumpDir. * src/qemu/qemu_conf.h: add 2 fields to struct qemud_driver * src/qemu/qemu_driver.c: base64 encoding file id and id->path translation Signed-off-by: Hong Xiang <hxiang@linux.vnet.ibm.com> --- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_driver.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index ff5cf23..f7f12da 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -138,6 +138,9 @@ struct qemud_driver { * of guests which will be automatically killed * when the virConnectPtr is closed*/ virHashTablePtr autodestroy; + + char *savedImageDir; + char *coreDumpDir; }; typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6b65716..36d6284 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -472,6 +472,12 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->autoDumpPath, "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->savedImageDir, + "%s/lib/libvirt/qemu/saved-image", LOCALSTATEDIR) == -1) + goto out_of_memory; + if (virAsprintf(&qemu_driver->coreDumpDir, + "%s/lib/libvirt/qemu/core-dump", LOCALSTATEDIR) == -1) + goto out_of_memory; } else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -502,6 +508,10 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->savedImageDir, "%s/qemu/saved-image", base) == -1) + goto out_of_memory; + if (virAsprintf(&qemu_driver->coreDumpDir, "%s/qemu/core-dump", base) == -1) + goto out_of_memory; } if (virFileMakePath(qemu_driver->stateDir) < 0) { @@ -540,6 +550,18 @@ qemudStartup(int privileged) { qemu_driver->autoDumpPath, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } + if (virFileMakePath(qemu_driver->savedImageDir) < 0) { + char ebuf[1024]; + VIR_ERROR(_("Failed to create saved-image dir '%s': %s"), + qemu_driver->savedImageDir, virStrerror(errno, ebuf, sizeof ebuf)); + goto error; + } + if (virFileMakePath(qemu_driver->coreDumpDir) < 0) { + char ebuf[1024]; + VIR_ERROR(_("Failed to create core-dump dir '%s': %s"), + qemu_driver->coreDumpDir, virStrerror(errno, ebuf, sizeof ebuf)); + goto error; + } /* Configuration paths are either ~/.libvirt/qemu/... (session) or * /etc/libvirt/qemu/... (system). @@ -790,6 +812,8 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->snapshotDir); VIR_FREE(qemu_driver->qemuImgBinary); VIR_FREE(qemu_driver->autoDumpPath); + VIR_FREE(qemu_driver->savedImageDir); + VIR_FREE(qemu_driver->coreDumpDir); VIR_FREE(qemu_driver->vncTLSx509certdir); VIR_FREE(qemu_driver->vncListen); VIR_FREE(qemu_driver->vncPassword); @@ -2648,14 +2672,34 @@ static bool qemudCompressProgramAvailable(enum qemud_save_formats compress) return true; } +static char * +qemuSavedImagePath(struct qemud_driver *driver, const char *file) +{ + char *enc_file, *ret; + + if(0 > virBase64EncodePathname(file, &enc_file)) { + virReportOOMError(); + return NULL; + } + if (virAsprintf(&ret, "%s/%s", driver->savedImageDir, enc_file) < 0) { + virReportOOMError(); + VIR_FREE(enc_file); + return NULL; + } + VIR_FREE(enc_file); + + return(ret); +} + static int -qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, +qemuDomainSaveFlags(virDomainPtr dom, const char *file, const char *dxml, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int compressed; int ret = -1; virDomainObjPtr vm = NULL; + char *path = NULL; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | @@ -2696,11 +2740,15 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, goto cleanup; } + path = qemuSavedImagePath(driver, file); + if(!path) + goto cleanup; ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed, dxml, flags); vm = NULL; cleanup: + VIR_FREE(path); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -2933,8 +2981,27 @@ getCompressionType(struct qemud_driver *driver) return compress; } +static char * +qemuCoreDumpPath(struct qemud_driver *driver, const char *file) +{ + char *enc_file, *ret; + + if(0 > virBase64EncodePathname(file, &enc_file)) { + virReportOOMError(); + return NULL; + } + if (virAsprintf(&ret, "%s/%s", driver->coreDumpDir, enc_file) < 0) { + virReportOOMError(); + VIR_FREE(enc_file); + return NULL; + } + VIR_FREE(enc_file); + + return(ret); +} + static int qemudDomainCoreDump(virDomainPtr dom, - const char *path, + const char *file, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; @@ -2943,6 +3010,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, int resume = 0, paused = 0; int ret = -1; virDomainEventPtr event = NULL; + char *path = NULL; virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | VIR_DUMP_BYPASS_CACHE | VIR_DUMP_RESET, -1); @@ -2958,6 +3026,10 @@ static int qemudDomainCoreDump(virDomainPtr dom, goto cleanup; } + path = qemuCoreDumpPath(driver, file); + if(!path) + goto cleanup; + if (qemuDomainObjBeginAsyncJobWithDriver(driver, vm, QEMU_ASYNC_JOB_DUMP) < 0) goto cleanup; @@ -3032,6 +3104,7 @@ endjob: } cleanup: + VIR_FREE(path); if (vm) virDomainObjUnlock(vm); if (event) @@ -4178,7 +4251,7 @@ out: static int qemuDomainRestoreFlags(virConnectPtr conn, - const char *path, + const char *file, const char *dxml, unsigned int flags) { @@ -4190,6 +4263,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, struct qemud_save_header header; virFileDirectFdPtr directFd = NULL; int state = -1; + char *path = NULL; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | @@ -4202,6 +4276,9 @@ qemuDomainRestoreFlags(virConnectPtr conn, else if (flags & VIR_DOMAIN_SAVE_PAUSED) state = 0; + path = qemuSavedImagePath(driver, file); + if(!path) + goto cleanup; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, &directFd, dxml, state, false, false); @@ -4240,6 +4317,7 @@ cleanup: virFileDirectFdFree(directFd); if (vm) virDomainObjUnlock(vm); + VIR_FREE(path); qemuDriverUnlock(driver); return ret; } @@ -4252,7 +4330,7 @@ qemuDomainRestore(virConnectPtr conn, } static char * -qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, +qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *file, unsigned int flags) { struct qemud_driver *driver = conn->privateData; @@ -4260,12 +4338,16 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, virDomainDefPtr def = NULL; int fd = -1; struct qemud_save_header header; + char *path = NULL; /* We only take subset of virDomainDefFormat flags. */ virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); qemuDriverLock(driver); + path = qemuSavedImagePath(driver, file); + if(!path) + goto cleanup; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, NULL, -1, false, false); @@ -4277,12 +4359,13 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); + VIR_FREE(path); qemuDriverUnlock(driver); return ret; } static int -qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, +qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *file, const char *dxml, unsigned int flags) { struct qemud_driver *driver = conn->privateData; @@ -4293,6 +4376,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, char *xml = NULL; size_t len; int state = -1; + char *path = NULL; virCheckFlags(VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED, -1); @@ -4304,6 +4388,9 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, else if (flags & VIR_DOMAIN_SAVE_PAUSED) state = 0; + path = qemuSavedImagePath(driver, file); + if(!path) + goto cleanup; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, dxml, state, true, false); @@ -4347,6 +4434,7 @@ cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); VIR_FREE(xml); + VIR_FREE(path); qemuDriverUnlock(driver); return ret; } -- 1.7.1
On Tue, Oct 25, 2011 at 03:43:22PM +0800, Hong Xiang wrote:
Base64 encode input file id in qemuDomainSaveFlags(), qemuDomainRestoreFlags(), qemuDomainSaveImageGetXMLDesc(), qemuDomainSaveImageDefineXML(), qemudDomainCoreDump(), and put generated files in controlled directories: qemu_driver->savedImageDir, and qemu_driver->coreDumpDir.
* src/qemu/qemu_conf.h: add 2 fields to struct qemud_driver * src/qemu/qemu_driver.c: base64 encoding file id and id->path translation
Signed-off-by: Hong Xiang <hxiang@linux.vnet.ibm.com> --- src/qemu/qemu_conf.h | 3 + src/qemu/qemu_driver.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 5 deletions(-)
NACK, you can't change the semantics of existing APIs in this way. It will break any applications using the APIs already. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Hong Xiang