[PATCH 0/7] Fixes for CVE-2025-12748
ACL checks were performed after parsing a user provided XML in its entirety which could be written in a way that would make libvirt allocate too much memory and crash. Instead parse just the identifiers out of which only name and UUID are needed for ACL checks, perform those and then parse the whole definition. In order not to pass bogus UUID to the ACL functions, rewrite any generated UUID in the first step with a nil UUID since the ACLs cannot be written to expect a particular generated UUID as that would not make sense. If squashing the patches is preferred, let me know. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Martin Kletzander (7): conf: Add virDomainDefIDsParseString bhyve: Check ACLs before parsing the whole domain XML libxl: Check ACLs before parsing the whole domain XML lxc: Check ACLs before parsing the whole domain XML vz: Check ACLs before parsing the whole domain XML ch: Check ACLs before parsing the whole domain XML qemu: Check ACLs before parsing the whole domain XML src/bhyve/bhyve_driver.c | 24 ++++++++--- src/ch/ch_driver.c | 76 +++++++++++++++++++++++---------- src/conf/domain_conf.c | 29 +++++++++++++ src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 20 ++++++--- src/lxc/lxc_driver.c | 22 +++++++--- src/qemu/qemu_driver.c | 90 ++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 21 ++++++++- src/qemu/qemu_migration.h | 4 +- src/qemu/qemu_saveimage.c | 25 +++++++++-- src/qemu/qemu_saveimage.h | 4 +- src/qemu/qemu_snapshot.c | 4 +- src/vz/vz_driver.c | 18 +++++--- 14 files changed, 243 insertions(+), 98 deletions(-) -- 2.51.2
From: Martin Kletzander <mkletzan@redhat.com> This function performs only parsing with the underlying virDomainDefParseIDs() function to get needed metadata for any ACL checks, but nothing else to avoid extraneous allocations and any parser-induced DoS over ACL-forbidden connections. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 33 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 396cd1c0dbc2..d2dea6952efc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20446,6 +20446,35 @@ virDomainDefParse(const char *xmlStr, return virDomainDefParseNode(ctxt, xmlopt, parseOpaque, flags); } +virDomainDef * +virDomainDefIDsParseString(const char *xmlStr, + virDomainXMLOption *xmlopt, + unsigned int flags) +{ + g_autoptr(virDomainDef) def = NULL; + g_autoptr(xmlDoc) xml = NULL; + g_autoptr(xmlXPathContext) ctxt = NULL; + bool uuid_generated = false; + + xml = virXMLParseWithIndent(NULL, xmlStr, _("(domain_definition)"), + "domain", &ctxt, "domain.rng", false); + + if (!xml) + return NULL; + + def = virDomainDefNew(xmlopt); + if (!def) + return NULL; + + if (virDomainDefParseIDs(def, ctxt, flags, &uuid_generated) < 0) + return NULL; + + if (uuid_generated) + memset(def->uuid, 0, VIR_UUID_BUFLEN); + + return g_steal_pointer(&def); +} + virDomainDef * virDomainDefParseString(const char *xmlStr, virDomainXMLOption *xmlopt, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 81e735993d47..11eb46ae5385 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3948,6 +3948,9 @@ virDomainDiskDef *virDomainDiskDefParse(const char *xmlStr, virStorageSource *virDomainDiskDefParseSource(const char *xmlStr, virDomainXMLOption *xmlopt, unsigned int flags); +virDomainDef * virDomainDefIDsParseString(const char *xmlStr, + virDomainXMLOption *xmlopt, + unsigned int flags); virDomainDef *virDomainDefParseString(const char *xmlStr, virDomainXMLOption *xmlopt, void *parseOpaque, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7269dd37862d..fb482fff40a5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -352,6 +352,7 @@ virDomainDefHasTimer; virDomainDefHasUSB; virDomainDefHasVcpusOffline; virDomainDefHasVDPANet; +virDomainDefIDsParseString; virDomainDefLifecycleActionAllowed; virDomainDefMaybeAddController; virDomainDefMaybeAddInput; -- 2.51.2
From: Martin Kletzander <mkletzan@redhat.com> Utilise the new virDomainDefIDsParseString() for that. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/bhyve/bhyve_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 00a484ae219c..72f1d7ace8e6 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -486,6 +486,15 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (!caps) return NULL; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, provconn->xmlopt, parse_flags))) + return NULL; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, g_object_unref); + if ((def = virDomainDefParseString(xml, privconn->xmlopt, NULL, parse_flags)) == NULL) goto cleanup; @@ -493,9 +502,6 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup; - if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) - goto cleanup; - if (bhyveDomainAssignAddresses(def, NULL) < 0) goto cleanup; @@ -889,11 +895,17 @@ bhyveDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_AUTODESTROY) start_flags |= VIR_BHYVE_PROCESS_START_AUTODESTROY; - if ((def = virDomainDefParseString(xml, privconn->xmlopt, - NULL, parse_flags)) == NULL) - goto cleanup; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, provconn->xmlopt, parse_flags))) + return NULL; if (virDomainCreateXMLEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, g_object_unref); + + if ((def = virDomainDefParseString(xml, privconn->xmlopt, + NULL, parse_flags)) == NULL) goto cleanup; if (bhyveDomainAssignAddresses(def, NULL) < 0) -- 2.51.2
On 11/7/25 12:26, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
Utilise the new virDomainDefIDsParseString() for that.
Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/bhyve/bhyve_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 00a484ae219c..72f1d7ace8e6 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -486,6 +486,15 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (!caps) return NULL;
+ /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, provconn->xmlopt, parse_flags))) + return NULL; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, g_object_unref);
Haven't checked other patches yet, but virDomainDef is NOT an virObject. This should have been: g_clear_pointer(&def, virDomainDefFree); Here and in the rest of patches.
+ if ((def = virDomainDefParseString(xml, privconn->xmlopt, NULL, parse_flags)) == NULL) goto cleanup; @@ -493,9 +502,6 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup;
So previously, illegal characters were checked for before ACL check and now they are checked for afterwards. But I think that's okay - the order shouldn't matter.
- if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) - goto cleanup; - if (bhyveDomainAssignAddresses(def, NULL) < 0) goto cleanup;
@@ -889,11 +895,17 @@ bhyveDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_AUTODESTROY) start_flags |= VIR_BHYVE_PROCESS_START_AUTODESTROY;
- if ((def = virDomainDefParseString(xml, privconn->xmlopt, - NULL, parse_flags)) == NULL) - goto cleanup; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, provconn->xmlopt, parse_flags))) + return NULL;
if (virDomainCreateXMLEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, g_object_unref); + + if ((def = virDomainDefParseString(xml, privconn->xmlopt, + NULL, parse_flags)) == NULL) goto cleanup;
if (bhyveDomainAssignAddresses(def, NULL) < 0)
Michal
On Tue, Nov 11, 2025 at 09:54:22AM +0100, Michal Prívozník wrote:
On 11/7/25 12:26, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
Utilise the new virDomainDefIDsParseString() for that.
Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/bhyve/bhyve_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 00a484ae219c..72f1d7ace8e6 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -486,6 +486,15 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (!caps) return NULL;
+ /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, provconn->xmlopt, parse_flags))) + return NULL; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, g_object_unref);
Haven't checked other patches yet, but virDomainDef is NOT an virObject. This should have been:
g_clear_pointer(&def, virDomainDefFree);
Here and in the rest of patches.
Well, that's a stupid mistake, thanks for catching that. Fixed locally.
From: Martin Kletzander <mkletzan@redhat.com> Utilise the new virDomainDefIDsParseString() for that. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libxl/libxl_driver.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 107477250ab8..0cdeec08bedc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1027,13 +1027,18 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, if (flags & VIR_DOMAIN_START_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; - if (!(def = virDomainDefParseString(xml, driver->xmlopt, - NULL, parse_flags))) + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) goto cleanup; if (virDomainCreateXMLEnsureACL(conn, def) < 0) goto cleanup; + g_clear_pointer(&def, virObjectUnref); + + if (!(def = virDomainDefParseString(xml, driver->xmlopt, + NULL, parse_flags))) + goto cleanup; + if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE | @@ -2813,6 +2818,14 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (flags & VIR_DOMAIN_DEFINE_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + goto cleanup; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + goto cleanup; + + g_clear_pointer(&def, virObjectUnref); + if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto cleanup; @@ -2820,9 +2833,6 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup; - if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) - goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, 0, -- 2.51.2
From: Martin Kletzander <mkletzan@redhat.com> Utilise the new virDomainDefIDsParseString() for that. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/lxc/lxc_driver.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 80cf07d2e5ff..0564f3e5332e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -409,6 +409,15 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + goto cleanup; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + goto cleanup; + + g_clear_pointer(&def, virObjectUnref); + if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto cleanup; @@ -416,9 +425,6 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup; - if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) - goto cleanup; - if (virSecurityManagerVerify(driver->securityManager, def) < 0) goto cleanup; @@ -1066,13 +1072,19 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; - if (!(def = virDomainDefParseString(xml, driver->xmlopt, - NULL, parse_flags))) + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) goto cleanup; if (virDomainCreateXMLWithFilesEnsureACL(conn, def) < 0) goto cleanup; + g_clear_pointer(&def, virObjectUnref); + + if (!(def = virDomainDefParseString(xml, driver->xmlopt, + NULL, parse_flags))) + goto cleanup; + if (virSecurityManagerVerify(driver->securityManager, def) < 0) goto cleanup; -- 2.51.2
From: Martin Kletzander <mkletzan@redhat.com> Utilise the new virDomainDefIDsParseString() for that. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/vz/vz_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 571735f94054..9fd9b199cd01 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -789,6 +789,15 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (flags & VIR_DOMAIN_DEFINE_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + return NULL; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, virObjectUnref); + if ((def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags)) == NULL) goto cleanup; @@ -796,9 +805,6 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup; - if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) - goto cleanup; - dom = virDomainObjListFindByUUID(driver->domains, def->uuid); if (dom == NULL) { virResetLastError(); @@ -2966,9 +2972,9 @@ vzDomainMigratePrepare3Params(virConnectPtr conn, | VZ_MIGRATION_COOKIE_DOMAIN_NAME) < 0) return -1; - if (!(def = virDomainDefParseString(dom_xml, driver->xmlopt, - NULL, - VIR_DOMAIN_DEF_PARSE_INACTIVE))) + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(dom_xml, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE))) return -1; if (dname) { -- 2.51.2
From: Martin Kletzander <mkletzan@redhat.com> Utilise the new virDomainDefIDsParseString() for that. This is one of the more complex ones since there is also a function that reads relevant metadata from a save image XML. In order not to extract the parsing out of the function (and make the function basically trivial and all callers more complex) add a callback to the function which will be used to check the ACLs. And since this function is called in APIs that perform ACL checks both with and without flags, add two of them for good measure. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/ch/ch_driver.c | 76 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 23 deletions(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index ad13306c4cfd..70653aeea7d3 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -216,14 +216,19 @@ chDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(vmdef = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + return NULL; + + if (virDomainCreateXMLEnsureACL(conn, vmdef) < 0) + return NULL; + + g_clear_pointer(&vmdef, virObjectUnref); if ((vmdef = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags)) == NULL) goto cleanup; - if (virDomainCreateXMLEnsureACL(conn, vmdef) < 0) - goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, &vmdef, driver->xmlopt, @@ -347,6 +352,15 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (flags & VIR_DOMAIN_START_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(vmdef = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + return NULL; + + if (virDomainDefineXMLFlagsEnsureACL(conn, vmdef) < 0) + return NULL; + + g_clear_pointer(&vmdef, virObjectUnref); + if ((vmdef = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags)) == NULL) goto cleanup; @@ -354,9 +368,6 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (virXMLCheckIllegalChars("name", vmdef->name, "\n") < 0) goto cleanup; - if (virDomainDefineXMLFlagsEnsureACL(conn, vmdef) < 0) - goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, &vmdef, driver->xmlopt, 0, &oldDef))) @@ -920,16 +931,24 @@ chDomainSaveXMLRead(int fd) return g_steal_pointer(&xml); } -static int chDomainSaveImageRead(virCHDriver *driver, +static int chDomainSaveImageRead(virConnectPtr conn, const char *path, - virDomainDef **ret_def) + virDomainDef **ret_def, + unsigned int flags, + int (*ensureACL)(virConnectPtr, virDomainDef *), + int (*ensureACLWithFlags)(virConnectPtr, + virDomainDef *, + unsigned int)) { + virCHDriver *driver = conn->privateData; g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); g_autoptr(virDomainDef) def = NULL; g_autofree char *from = NULL; g_autofree char *xml = NULL; VIR_AUTOCLOSE fd = -1; int ret = -1; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; from = g_strdup_printf("%s/%s", path, CH_SAVE_XML); if ((fd = virFileOpenAs(from, O_RDONLY, 0, cfg->user, cfg->group, 0)) < 0) { @@ -942,9 +961,23 @@ static int chDomainSaveImageRead(virCHDriver *driver, if (!(xml = chDomainSaveXMLRead(fd))) goto end; - if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + if (ensureACL || ensureACLWithFlags) { + /* Parse only the IDs for ACL checks */ + g_autoptr(virDomainDef) aclDef = virDomainDefIDsParseString(xml, + driver->xmlopt, + parse_flags); + + if (!aclDef) + goto end; + + if (ensureACL && ensureACL(conn, aclDef) < 0) + goto end; + + if (ensureACLWithFlags && ensureACLWithFlags(conn, aclDef, flags) < 0) + goto end; + } + + if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto end; *ret_def = g_steal_pointer(&def); @@ -965,10 +998,9 @@ chDomainSaveImageGetXMLDesc(virConnectPtr conn, virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); - if (chDomainSaveImageRead(driver, path, &def) < 0) - goto cleanup; - - if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0) + if (chDomainSaveImageRead(conn, path, &def, flags, + virDomainSaveImageGetXMLDescEnsureACL, + NULL) < 0) goto cleanup; ret = virDomainDefFormat(def, driver->xmlopt, @@ -1068,10 +1100,9 @@ chDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; path = chDomainManagedSavePath(driver, vm); - if (chDomainSaveImageRead(driver, path, &def) < 0) - goto cleanup; - - if (virDomainManagedSaveGetXMLDescEnsureACL(dom->conn, def, flags) < 0) + if (chDomainSaveImageRead(dom->conn, path, &def, flags, + NULL, + virDomainManagedSaveGetXMLDescEnsureACL) < 0) goto cleanup; ret = virDomainDefFormat(def, driver->xmlopt, @@ -1123,10 +1154,9 @@ chDomainRestoreFlags(virConnectPtr conn, return -1; } - if (chDomainSaveImageRead(driver, from, &def) < 0) - goto cleanup; - - if (virDomainRestoreFlagsEnsureACL(conn, def) < 0) + if (chDomainSaveImageRead(conn, from, &def, flags, + virDomainRestoreFlagsEnsureACL, + NULL) < 0) goto cleanup; if (chDomainSaveRestoreAdditionalValidation(driver, def) < 0) -- 2.51.2
On a Friday in 2025, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
Utilise the new virDomainDefIDsParseString() for that.
This is one of the more complex ones since there is also a function that reads relevant metadata from a save image XML. In order not to extract the parsing out of the function (and make the function basically trivial and all callers more complex) add a callback to the function which will be used to check the ACLs. And since this function is called in APIs that perform ACL checks both with and without flags, add two of them for good measure.
Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/ch/ch_driver.c | 76 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 23 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index ad13306c4cfd..70653aeea7d3 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c -static int chDomainSaveImageRead(virCHDriver *driver, +static int chDomainSaveImageRead(virConnectPtr conn, const char *path, - virDomainDef **ret_def) + virDomainDef **ret_def, + unsigned int flags, + int (*ensureACL)(virConnectPtr, virDomainDef *), + int (*ensureACLWithFlags)(virConnectPtr, + virDomainDef *, + unsigned int)) { + virCHDriver *driver = conn->privateData; g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); g_autoptr(virDomainDef) def = NULL; g_autofree char *from = NULL; g_autofree char *xml = NULL; VIR_AUTOCLOSE fd = -1; int ret = -1; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
from = g_strdup_printf("%s/%s", path, CH_SAVE_XML); if ((fd = virFileOpenAs(from, O_RDONLY, 0, cfg->user, cfg->group, 0)) < 0) { @@ -942,9 +961,23 @@ static int chDomainSaveImageRead(virCHDriver *driver, if (!(xml = chDomainSaveXMLRead(fd))) goto end;
- if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + if (ensureACL || ensureACLWithFlags) { + /* Parse only the IDs for ACL checks */ + g_autoptr(virDomainDef) aclDef = virDomainDefIDsParseString(xml, + driver->xmlopt, + parse_flags);
Two (possibly stupid) questions: 0. Would making the EnsureACL work on some kind of virDomainIDDef structure containing only the necessary elements make sure nobody adds an ACL check on a fully-parsed domain XML in the future?
+ + if (!aclDef) + goto end; + + if (ensureACL && ensureACL(conn, aclDef) < 0) + goto end; + + if (ensureACLWithFlags && ensureACLWithFlags(conn, aclDef, flags) < 0) + goto end;
1. Does using callbacks somehow evade the syntax check, or we don't check for the presence of those there? Jano
+ } + + if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto end;
*ret_def = g_steal_pointer(&def);
On Fri, Nov 07, 2025 at 03:11:22PM +0100, Ján Tomko wrote:
On a Friday in 2025, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
Utilise the new virDomainDefIDsParseString() for that.
This is one of the more complex ones since there is also a function that reads relevant metadata from a save image XML. In order not to extract the parsing out of the function (and make the function basically trivial and all callers more complex) add a callback to the function which will be used to check the ACLs. And since this function is called in APIs that perform ACL checks both with and without flags, add two of them for good measure.
Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/ch/ch_driver.c | 76 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 23 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index ad13306c4cfd..70653aeea7d3 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c -static int chDomainSaveImageRead(virCHDriver *driver, +static int chDomainSaveImageRead(virConnectPtr conn, const char *path, - virDomainDef **ret_def) + virDomainDef **ret_def, + unsigned int flags, + int (*ensureACL)(virConnectPtr, virDomainDef *), + int (*ensureACLWithFlags)(virConnectPtr, + virDomainDef *, + unsigned int)) { + virCHDriver *driver = conn->privateData; g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); g_autoptr(virDomainDef) def = NULL; g_autofree char *from = NULL; g_autofree char *xml = NULL; VIR_AUTOCLOSE fd = -1; int ret = -1; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
from = g_strdup_printf("%s/%s", path, CH_SAVE_XML); if ((fd = virFileOpenAs(from, O_RDONLY, 0, cfg->user, cfg->group, 0)) < 0) { @@ -942,9 +961,23 @@ static int chDomainSaveImageRead(virCHDriver *driver, if (!(xml = chDomainSaveXMLRead(fd))) goto end;
- if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + if (ensureACL || ensureACLWithFlags) { + /* Parse only the IDs for ACL checks */ + g_autoptr(virDomainDef) aclDef = virDomainDefIDsParseString(xml, + driver->xmlopt, + parse_flags);
Two (possibly stupid) questions:
0. Would making the EnsureACL work on some kind of virDomainIDDef structure containing only the necessary elements make sure nobody adds an ACL check on a fully-parsed domain XML in the future?
I was thinking of actually just passing the name, uuid and flags to all domains, but that does not really give us much future flexibility (not that I know whether that's needed or not). But also, just like passing flags to all *EnsureACL functions, it would make the backports gross. So I'm not against that, there are even more things that might be cleared up after this, but first we should deal with this issue.
+ + if (!aclDef) + goto end; + + if (ensureACL && ensureACL(conn, aclDef) < 0) + goto end; + + if (ensureACLWithFlags && ensureACLWithFlags(conn, aclDef, flags) < 0) + goto end;
1. Does using callbacks somehow evade the syntax check, or we don't check for the presence of those there?
I honestly did not check how the syntax-check actually checks things, but it did not fail for me *and* I already saw the ACL functions being passed as callbacks in QEMU driver before, so I figured that ought to be alright. Of course the double callback is not nice, but the two versions I had before I deleted because they were even uglier IMHO.
Jano
+ } + + if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto end;
*ret_def = g_steal_pointer(&def);
From: Martin Kletzander <mkletzan@redhat.com> Utilise the new virDomainDefIDsParseString() for that. This is one of the more complex ones since there is also a function that reads relevant metadata from a save image XML. In order _not_ to extract the parsing out of the function (and make the function basically trivial and all callers more complex) add a callback to the function which will be used to check the ACLs. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 90 ++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 21 ++++++++- src/qemu/qemu_migration.h | 4 +- src/qemu/qemu_saveimage.c | 25 +++++++++-- src/qemu/qemu_saveimage.h | 4 +- src/qemu/qemu_snapshot.c | 4 +- 6 files changed, 95 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a1b1edcbbf51..837935d524bc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1556,11 +1556,17 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_RESET_NVRAM) start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; - if (!(def = virDomainDefParseString(xml, driver->xmlopt, - NULL, parse_flags))) - goto cleanup; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + return NULL; if (virDomainCreateXMLEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, virObjectUnref); + + if (!(def = virDomainDefParseString(xml, driver->xmlopt, + NULL, parse_flags))) goto cleanup; if (!(vm = virDomainObjListAdd(driver->domains, &def, @@ -5780,7 +5786,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; - if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, ensureACL, conn, &def, &data) < 0) goto cleanup; sparse = data->header.format == QEMU_SAVE_FORMAT_SPARSE; @@ -5793,9 +5799,6 @@ qemuDomainRestoreInternal(virConnectPtr conn, if (fd < 0) goto cleanup; - if (ensureACL(conn, def) < 0) - goto cleanup; - if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { int hookret; @@ -5923,10 +5926,9 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); - if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0) - goto cleanup; - - if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, + virDomainSaveImageGetXMLDescEnsureACL, + conn, &def, &data) < 0) goto cleanup; ret = qemuDomainDefFormatXML(driver, NULL, def, flags); @@ -5956,7 +5958,9 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, else if (flags & VIR_DOMAIN_SAVE_PAUSED) state = 0; - if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, + virDomainSaveImageDefineXMLEnsureACL, + conn, &def, &data) < 0) goto cleanup; fd = qemuSaveImageOpen(driver, path, false, false, NULL, true); @@ -5964,9 +5968,6 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, if (fd < 0) goto cleanup; - if (virDomainSaveImageDefineXMLEnsureACL(conn, def) < 0) - goto cleanup; - if (STREQ(data->xml, dxml) && (state < 0 || state == data->header.was_running)) { /* no change to the XML */ @@ -6038,7 +6039,8 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; } - if (qemuSaveImageGetMetadata(driver, priv->qemuCaps, path, &def, &data) < 0) + if (qemuSaveImageGetMetadata(driver, priv->qemuCaps, path, + NULL, NULL, &def, &data) < 0) goto cleanup; ret = qemuDomainDefFormatXML(driver, priv->qemuCaps, def, flags); @@ -6102,7 +6104,7 @@ qemuDomainObjRestore(virConnectPtr conn, bool sparse = false; g_autoptr(qemuMigrationParams) restoreParams = NULL; - ret = qemuSaveImageGetMetadata(driver, NULL, path, &def, &data); + ret = qemuSaveImageGetMetadata(driver, NULL, path, NULL, NULL, &def, &data); if (ret < 0) { if (qemuSaveImageIsCorrupt(driver, path)) { if (unlink(path) < 0) { @@ -6464,6 +6466,15 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, if (flags & VIR_DOMAIN_DEFINE_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + return NULL; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, virObjectUnref); + if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) return NULL; @@ -6471,9 +6482,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup; - if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) - goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, 0, &oldDef))) @@ -10769,10 +10777,9 @@ qemuDomainMigratePrepareTunnel(virConnectPtr dconn, return -1; } - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepareTunnelEnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepareTunnelEnsureACL))) return -1; return qemuMigrationDstPrepareTunnel(driver, dconn, @@ -10822,10 +10829,9 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, return -1; } - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepare2EnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepare2EnsureACL))) return -1; /* Do not use cookies in v2 protocol, since the cookie @@ -11045,10 +11051,9 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepare3EnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepare3EnsureACL))) return -1; return qemuMigrationDstPrepareDirect(driver, dconn, @@ -11148,10 +11153,9 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, return -1; } - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepare3ParamsEnsureACL))) return -1; return qemuMigrationDstPrepareDirect(driver, dconn, @@ -11193,10 +11197,9 @@ qemuDomainMigratePrepareTunnel3(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepareTunnel3EnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepareTunnel3EnsureACL))) return -1; return qemuMigrationDstPrepareTunnel(driver, dconn, @@ -11245,10 +11248,9 @@ qemuDomainMigratePrepareTunnel3Params(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepareTunnel3ParamsEnsureACL))) return -1; return qemuMigrationDstPrepareTunnel(driver, dconn, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9109c4526db1..dcf9ea444ef9 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4030,7 +4030,9 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *dom_xml, const char *dname, - char **origname) + char **origname, + virConnectPtr sconn, + int (*ensureACL)(virConnectPtr, virDomainDef *)) { virDomainDef *def; char *name = NULL; @@ -4041,6 +4043,22 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, return NULL; } + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(dom_xml, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE))) + goto cleanup; + + if (dname) { + VIR_FREE(def->name); + def->name = g_strdup(dname); + } + + if (ensureACL && ensureACL(sconn, def) < 0) { + g_clear_pointer(&def, virObjectUnref); + goto cleanup; + } + g_clear_pointer(&def, virObjectUnref); + if (!(def = virDomainDefParseString(dom_xml, driver->xmlopt, qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE))) @@ -4969,6 +4987,7 @@ qemuMigrationSrcRun(virQEMUDriver *driver, if (!(persistDef = qemuMigrationAnyPrepareDef(driver, priv->qemuCaps, persist_xml, + NULL, NULL, NULL, NULL))) goto error; } else { diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 36865040dffc..50910ecb1f92 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -134,7 +134,9 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *dom_xml, const char *dname, - char **origname); + char **origname, + virConnectPtr sconn, + int (*ensureACL)(virConnectPtr, virDomainDef *)); int qemuMigrationDstPrepareTunnel(virQEMUDriver *driver, diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index aa030798ce19..145a0f483283 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -614,16 +614,21 @@ qemuSaveImageIsCorrupt(virQEMUDriver *driver, const char *path) * @driver: qemu driver data * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image + * @ensureACL: ACL callback to check against the definition or NULL + * @conn: parameter for the @ensureACL callback * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header * - * Open the save image file, read libvirt's save image metadata, and populate - * the @ret_def and @ret_data structures. Returns 0 on success and -1 on failure. + * Open the save image file, read libvirt's save image metadata, optionally + * check ACLs before parsing the whole domain definition and populate the + * @ret_def and @ret_data structures. Returns 0 on success and -1 on failure. */ int qemuSaveImageGetMetadata(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *path, + int (*ensureACL)(virConnectPtr, virDomainDef *), + virConnectPtr conn, virDomainDef **ret_def, virQEMUSaveData **ret_data) { @@ -631,6 +636,8 @@ qemuSaveImageGetMetadata(virQEMUDriver *driver, VIR_AUTOCLOSE fd = -1; virQEMUSaveData *data; g_autoptr(virDomainDef) def = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; int rc; if ((fd = qemuDomainOpenFile(cfg, NULL, path, O_RDONLY, NULL)) < 0) @@ -640,10 +647,20 @@ qemuSaveImageGetMetadata(virQEMUDriver *driver, return rc; data = *ret_data; + + if (ensureACL) { + /* Parse only the IDs for ACL checks */ + g_autoptr(virDomainDef) aclDef = virDomainDefIDsParseString(data->xml, + driver->xmlopt, + parse_flags); + + if (!aclDef || ensureACL(conn, aclDef) < 0) + return -1; + } + /* Create a domain from this XML */ if (!(def = virDomainDefParseString(data->xml, driver->xmlopt, qemuCaps, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + parse_flags))) return -1; *ret_def = g_steal_pointer(&def); diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 89c694138505..15b73eb39575 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -98,9 +98,11 @@ int qemuSaveImageGetMetadata(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *path, + int (*ensureACL)(virConnectPtr, virDomainDef *), + virConnectPtr conn, virDomainDef **ret_def, virQEMUSaveData **ret_data) - ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); + ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7); int qemuSaveImageOpen(virQEMUDriver *driver, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d4994dd54ed7..5aa7d1b3a79d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2486,8 +2486,8 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, g_autoptr(virDomainDef) savedef = NULL; memdata->path = snapdef->memorysnapshotfile; - if (qemuSaveImageGetMetadata(driver, NULL, memdata->path, &savedef, - &memdata->data) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, memdata->path, NULL, NULL, + &savedef, &memdata->data) < 0) return -1; memdata->fd = qemuSaveImageOpen(driver, memdata->path, -- 2.51.2
From: Martin Kletzander <mkletzan@redhat.com> Utilise the new virDomainDefIDsParseString() for that. This is one of the more complex ones since there is also a function that reads relevant metadata from a save image XML. In order _not_ to extract the parsing out of the function (and make the function basically trivial and all callers more complex) add a callback to the function which will be used to check the ACLs. Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 90 ++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 23 +++++++++- src/qemu/qemu_migration.h | 4 +- src/qemu/qemu_saveimage.c | 25 +++++++++-- src/qemu/qemu_saveimage.h | 4 +- src/qemu/qemu_snapshot.c | 4 +- 6 files changed, 97 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a1b1edcbbf51..1f7e587f61b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1556,11 +1556,17 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_RESET_NVRAM) start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; - if (!(def = virDomainDefParseString(xml, driver->xmlopt, - NULL, parse_flags))) - goto cleanup; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + return NULL; if (virDomainCreateXMLEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, virDomainDefFree); + + if (!(def = virDomainDefParseString(xml, driver->xmlopt, + NULL, parse_flags))) goto cleanup; if (!(vm = virDomainObjListAdd(driver->domains, &def, @@ -5780,7 +5786,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; - if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, ensureACL, conn, &def, &data) < 0) goto cleanup; sparse = data->header.format == QEMU_SAVE_FORMAT_SPARSE; @@ -5793,9 +5799,6 @@ qemuDomainRestoreInternal(virConnectPtr conn, if (fd < 0) goto cleanup; - if (ensureACL(conn, def) < 0) - goto cleanup; - if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { int hookret; @@ -5923,10 +5926,9 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); - if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0) - goto cleanup; - - if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, + virDomainSaveImageGetXMLDescEnsureACL, + conn, &def, &data) < 0) goto cleanup; ret = qemuDomainDefFormatXML(driver, NULL, def, flags); @@ -5956,7 +5958,9 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, else if (flags & VIR_DOMAIN_SAVE_PAUSED) state = 0; - if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, + virDomainSaveImageDefineXMLEnsureACL, + conn, &def, &data) < 0) goto cleanup; fd = qemuSaveImageOpen(driver, path, false, false, NULL, true); @@ -5964,9 +5968,6 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, if (fd < 0) goto cleanup; - if (virDomainSaveImageDefineXMLEnsureACL(conn, def) < 0) - goto cleanup; - if (STREQ(data->xml, dxml) && (state < 0 || state == data->header.was_running)) { /* no change to the XML */ @@ -6038,7 +6039,8 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; } - if (qemuSaveImageGetMetadata(driver, priv->qemuCaps, path, &def, &data) < 0) + if (qemuSaveImageGetMetadata(driver, priv->qemuCaps, path, + NULL, NULL, &def, &data) < 0) goto cleanup; ret = qemuDomainDefFormatXML(driver, priv->qemuCaps, def, flags); @@ -6102,7 +6104,7 @@ qemuDomainObjRestore(virConnectPtr conn, bool sparse = false; g_autoptr(qemuMigrationParams) restoreParams = NULL; - ret = qemuSaveImageGetMetadata(driver, NULL, path, &def, &data); + ret = qemuSaveImageGetMetadata(driver, NULL, path, NULL, NULL, &def, &data); if (ret < 0) { if (qemuSaveImageIsCorrupt(driver, path)) { if (unlink(path) < 0) { @@ -6464,6 +6466,15 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, if (flags & VIR_DOMAIN_DEFINE_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(def = virDomainDefIDsParseString(xml, driver->xmlopt, parse_flags))) + return NULL; + + if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) + return NULL; + + g_clear_pointer(&def, virDomainDefFree); + if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) return NULL; @@ -6471,9 +6482,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, if (virXMLCheckIllegalChars("name", def->name, "\n") < 0) goto cleanup; - if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) - goto cleanup; - if (!(vm = virDomainObjListAdd(driver->domains, &def, driver->xmlopt, 0, &oldDef))) @@ -10769,10 +10777,9 @@ qemuDomainMigratePrepareTunnel(virConnectPtr dconn, return -1; } - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepareTunnelEnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepareTunnelEnsureACL))) return -1; return qemuMigrationDstPrepareTunnel(driver, dconn, @@ -10822,10 +10829,9 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, return -1; } - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepare2EnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepare2EnsureACL))) return -1; /* Do not use cookies in v2 protocol, since the cookie @@ -11045,10 +11051,9 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepare3EnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepare3EnsureACL))) return -1; return qemuMigrationDstPrepareDirect(driver, dconn, @@ -11148,10 +11153,9 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, return -1; } - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepare3ParamsEnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepare3ParamsEnsureACL))) return -1; return qemuMigrationDstPrepareDirect(driver, dconn, @@ -11193,10 +11197,9 @@ qemuDomainMigratePrepareTunnel3(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepareTunnel3EnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepareTunnel3EnsureACL))) return -1; return qemuMigrationDstPrepareTunnel(driver, dconn, @@ -11245,10 +11248,9 @@ qemuDomainMigratePrepareTunnel3Params(virConnectPtr dconn, QEMU_MIGRATION_DESTINATION))) return -1; - if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname))) - return -1; - - if (virDomainMigratePrepareTunnel3ParamsEnsureACL(dconn, def) < 0) + if (!(def = qemuMigrationAnyPrepareDef(driver, NULL, dom_xml, dname, &origname, + dconn, + virDomainMigratePrepareTunnel3ParamsEnsureACL))) return -1; return qemuMigrationDstPrepareTunnel(driver, dconn, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9109c4526db1..9059f9aa3a6c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4030,7 +4030,9 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *dom_xml, const char *dname, - char **origname) + char **origname, + virConnectPtr sconn, + int (*ensureACL)(virConnectPtr, virDomainDef *)) { virDomainDef *def; char *name = NULL; @@ -4041,6 +4043,24 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, return NULL; } + if (ensureACL) { + g_autoptr(virDomainDef) aclDef = NULL; + + /* Avoid parsing the whole domain definition for ACL checks */ + if (!(aclDef = virDomainDefIDsParseString(dom_xml, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE))) + return NULL; + + if (dname) { + VIR_FREE(aclDef->name); + aclDef->name = g_strdup(dname); + } + + if (ensureACL(sconn, aclDef) < 0) { + return NULL; + } + } + if (!(def = virDomainDefParseString(dom_xml, driver->xmlopt, qemuCaps, VIR_DOMAIN_DEF_PARSE_INACTIVE))) @@ -4969,6 +4989,7 @@ qemuMigrationSrcRun(virQEMUDriver *driver, if (!(persistDef = qemuMigrationAnyPrepareDef(driver, priv->qemuCaps, persist_xml, + NULL, NULL, NULL, NULL))) goto error; } else { diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 36865040dffc..50910ecb1f92 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -134,7 +134,9 @@ qemuMigrationAnyPrepareDef(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *dom_xml, const char *dname, - char **origname); + char **origname, + virConnectPtr sconn, + int (*ensureACL)(virConnectPtr, virDomainDef *)); int qemuMigrationDstPrepareTunnel(virQEMUDriver *driver, diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index aa030798ce19..145a0f483283 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -614,16 +614,21 @@ qemuSaveImageIsCorrupt(virQEMUDriver *driver, const char *path) * @driver: qemu driver data * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image + * @ensureACL: ACL callback to check against the definition or NULL + * @conn: parameter for the @ensureACL callback * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header * - * Open the save image file, read libvirt's save image metadata, and populate - * the @ret_def and @ret_data structures. Returns 0 on success and -1 on failure. + * Open the save image file, read libvirt's save image metadata, optionally + * check ACLs before parsing the whole domain definition and populate the + * @ret_def and @ret_data structures. Returns 0 on success and -1 on failure. */ int qemuSaveImageGetMetadata(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *path, + int (*ensureACL)(virConnectPtr, virDomainDef *), + virConnectPtr conn, virDomainDef **ret_def, virQEMUSaveData **ret_data) { @@ -631,6 +636,8 @@ qemuSaveImageGetMetadata(virQEMUDriver *driver, VIR_AUTOCLOSE fd = -1; virQEMUSaveData *data; g_autoptr(virDomainDef) def = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; int rc; if ((fd = qemuDomainOpenFile(cfg, NULL, path, O_RDONLY, NULL)) < 0) @@ -640,10 +647,20 @@ qemuSaveImageGetMetadata(virQEMUDriver *driver, return rc; data = *ret_data; + + if (ensureACL) { + /* Parse only the IDs for ACL checks */ + g_autoptr(virDomainDef) aclDef = virDomainDefIDsParseString(data->xml, + driver->xmlopt, + parse_flags); + + if (!aclDef || ensureACL(conn, aclDef) < 0) + return -1; + } + /* Create a domain from this XML */ if (!(def = virDomainDefParseString(data->xml, driver->xmlopt, qemuCaps, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + parse_flags))) return -1; *ret_def = g_steal_pointer(&def); diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 89c694138505..15b73eb39575 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -98,9 +98,11 @@ int qemuSaveImageGetMetadata(virQEMUDriver *driver, virQEMUCaps *qemuCaps, const char *path, + int (*ensureACL)(virConnectPtr, virDomainDef *), + virConnectPtr conn, virDomainDef **ret_def, virQEMUSaveData **ret_data) - ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); + ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7); int qemuSaveImageOpen(virQEMUDriver *driver, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d4994dd54ed7..5aa7d1b3a79d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2486,8 +2486,8 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, g_autoptr(virDomainDef) savedef = NULL; memdata->path = snapdef->memorysnapshotfile; - if (qemuSaveImageGetMetadata(driver, NULL, memdata->path, &savedef, - &memdata->data) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, memdata->path, NULL, NULL, + &savedef, &memdata->data) < 0) return -1; memdata->fd = qemuSaveImageOpen(driver, memdata->path, -- 2.51.2
On 11/11/25 10:26, Martin Kletzander wrote:
From: Martin Kletzander <mkletzan@redhat.com>
Utilise the new virDomainDefIDsParseString() for that.
This is one of the more complex ones since there is also a function that reads relevant metadata from a save image XML. In order _not_ to extract the parsing out of the function (and make the function basically trivial and all callers more complex) add a callback to the function which will be used to check the ACLs.
Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 90 ++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 23 +++++++++- src/qemu/qemu_migration.h | 4 +- src/qemu/qemu_saveimage.c | 25 +++++++++-- src/qemu/qemu_saveimage.h | 4 +- src/qemu/qemu_snapshot.c | 4 +- 6 files changed, 97 insertions(+), 53 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On 11/7/25 12:26, Martin Kletzander via Devel wrote:
ACL checks were performed after parsing a user provided XML in its entirety which could be written in a way that would make libvirt allocate too much memory and crash.
Instead parse just the identifiers out of which only name and UUID are needed for ACL checks, perform those and then parse the whole definition. In order not to pass bogus UUID to the ACL functions, rewrite any generated UUID in the first step with a nil UUID since the ACLs cannot be written to expect a particular generated UUID as that would not make sense.
If squashing the patches is preferred, let me know.
Fixes: CVE-2025-12748 Reported-by: Святослав Терешин <s.tereshin@fobos-nt.ru>
Martin Kletzander (7): conf: Add virDomainDefIDsParseString bhyve: Check ACLs before parsing the whole domain XML libxl: Check ACLs before parsing the whole domain XML lxc: Check ACLs before parsing the whole domain XML vz: Check ACLs before parsing the whole domain XML ch: Check ACLs before parsing the whole domain XML qemu: Check ACLs before parsing the whole domain XML
src/bhyve/bhyve_driver.c | 24 ++++++++--- src/ch/ch_driver.c | 76 +++++++++++++++++++++++---------- src/conf/domain_conf.c | 29 +++++++++++++ src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 20 ++++++--- src/lxc/lxc_driver.c | 22 +++++++--- src/qemu/qemu_driver.c | 90 ++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 21 ++++++++- src/qemu/qemu_migration.h | 4 +- src/qemu/qemu_saveimage.c | 25 +++++++++-- src/qemu/qemu_saveimage.h | 4 +- src/qemu/qemu_snapshot.c | 4 +- src/vz/vz_driver.c | 18 +++++--- 14 files changed, 243 insertions(+), 98 deletions(-)
You get bonus points for fixing save image code in CH driver, but that's sooo broken anyways that basically we're unable to restore from a saved image anyway. But hey, at least we don't deplete memory :-D Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Ján Tomko -
Martin Kletzander -
Michal Prívozník