On a Friday in 2025, Martin Kletzander via Devel wrote:
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. 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: Святослав Терешин <[email protected]> Signed-off-by: Martin Kletzander <[email protected]> --- 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);
signature.asc
Description: PGP signature
