From: Martin Kletzander <[email protected]> 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: Святослав Терешин <[email protected]> Signed-off-by: Martin Kletzander <[email protected]> --- 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
