Re: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid

If the <sysinfo> system table 'uuid' field is improperly formatted, then qemu will fail to start the guest with the error: virsh start dom error: Failed to start domain dom error: internal error process exited while connecting to monitor: Invalid SMBIOS UUID string In this case the "system_uuid" field was a94b4335-6a14-8bc4-d6da-f7ea590b68-16 which passed the virUUIDParse() code because 32 hexadecimal digits were found and the extra hyphen in the last section was ignored. Add checks to not only parse the read field, but then use virUUIDFormat() to validate that what gets formatted matches what was read - if not, then fail the edit.

--- src/conf/domain_conf.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 43273f8..c1fd99b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11577,17 +11577,32 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (def->sysinfo->system_uuid != NULL) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; - if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + /* Ensure that what we convert to a uuidbuf is converted back to + * the same string when formatted as a UUID. This field may be + * used by the underlying hypervisor driver instead of the domain + * uuid field and must be properly formatted. The virUUIDParse() + * is designed to "skip" extra "-"'s in the values and only + * validate that there are 32 hexadecimal digits. virUUIDFormat() + * returns uuidstr formatted properly. + */ + if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0 || + memcmp(def->sysinfo->system_uuid, + virUUIDFormat(uuidbuf, uuidstr), + VIR_UUID_STRING_BUFLEN) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); + _("malformed <sysinfo> uuid element '%s' " + "found on '%s'"), + def->sysinfo->system_uuid, def->name); goto error; } if (uuid_generated) memcpy(def->uuid, uuidbuf, VIR_UUID_BUFLEN); else if (memcmp(def->uuid, uuidbuf, VIR_UUID_BUFLEN) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_INTERNAL_ERROR,

_("UUID mismatch between <uuid> and " - "<sysinfo>")); + "<sysinfo> 'uuid' <entry> on '%s'"), + def->name); goto error; } }