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);