qemu: Check ACLs before parsing the whole domain XML

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>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a1b1edcbbf51b0e51e7769bb213d6073ff0fc8f1..1f7e587f61b97a6b4cc77666093d5e9b9d295f7d 100644 (file)
--- 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 9109c4526db1894461fa44191c04ad09a190b174..9059f9aa3a6cc0005aa4b32fb626288c68debf98 100644 (file)
--- 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 36865040dffc5f6e09921bac4cb5dd381ca04fea..50910ecb1f92f564a5bc27acf03d2a9de95b37b8 100644 (file)
--- 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 aa030798ce19f05d6993896d3a70a525519e53e0..145a0f483283f68f04fdc0809b4f5443b812a946 100644 (file)
--- 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 89c6941385050f92c42710b03055e26a474176a8..15b73eb395751d05fcd222d6a8ccf8c8ed4e779f 100644 (file)
--- 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 d4994dd54ed7a0a93a7f52ffdbb7fbad9186dd60..5aa7d1b3a79d5dbe0811f2b4d7961878fc1171e9 100644 (file)
--- 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,