The code was vulnerable to SQL injection. Likely not a security issue due to
WMI SQL and other constraints but still lame. For example:
virsh # dominfo \"
error: failed to get domain '"'
error: internal error: SOAP fault during enumeration: code 's:Sender', subcode
'n:CannotProcessFilter', reason 'The data source could not process the filter.
The filter might be missing or it might be invalid. Change the filter and try
the request again. ', detail 'The WS-Management service cannot process the
request. The WQL query is invalid. '
This commit fixes the Hyper-V driver by escaping all WMI SQL string parameters.
The same command with the fix:
virsh # dominfo \"
error: failed to get domain '"'
error: Domain not found: No domain with name "
Signed-off-by: Ladi Prosek <lpro...@redhat.com>
---
src/hyperv/hyperv_driver.c | 96 +++++++++++++++++++++++-----------------------
src/hyperv/hyperv_wmi.c | 2 +-
src/util/virbuffer.c | 18 +++++++++
src/util/virbuffer.h | 3 ++
4 files changed, 70 insertions(+), 49 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 52274239c..998780b80 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -302,12 +302,12 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
}
/* Get Win32_Processor list */
- virBufferAsprintf(&query,
- "associators of "
- "{Win32_ComputerSystem.Name=\"%s\"} "
- "where AssocClass = Win32_ComputerSystemProcessor "
- "ResultClass = Win32_Processor",
- computerSystem->data.common->Name);
+ virBufferEscapeSQL(&query,
+ "associators of "
+ "{Win32_ComputerSystem.Name=\"%s\"} "
+ "where AssocClass = Win32_ComputerSystemProcessor "
+ "ResultClass = Win32_Processor",
+ computerSystem->data.common->Name);
if (hypervGetWin32ProcessorList(priv, &query, &processorList) < 0)
goto cleanup;
@@ -494,7 +494,7 @@ hypervDomainLookupByUUID(virConnectPtr conn, const unsigned
char *uuid)
virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
virBufferAddLit(&query, "where ");
virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
- virBufferAsprintf(&query, "and Name = \"%s\"", uuid_string);
+ virBufferEscapeSQL(&query, "and Name = \"%s\"", uuid_string);
if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0)
goto cleanup;
@@ -526,7 +526,7 @@ hypervDomainLookupByName(virConnectPtr conn, const char
*name)
virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
virBufferAddLit(&query, "where ");
virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
- virBufferAsprintf(&query, "and ElementName = \"%s\"", name);
+ virBufferEscapeSQL(&query, "and ElementName = \"%s\"", name);
if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0)
goto cleanup;
@@ -674,13 +674,13 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr
info)
goto cleanup;
/* Get Msvm_VirtualSystemSettingData */
- virBufferAsprintf(&query,
- "associators of "
-
"{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
- "Name=\"%s\"} "
- "where AssocClass = Msvm_SettingsDefineState "
- "ResultClass = Msvm_VirtualSystemSettingData",
- uuid_string);
+ virBufferEscapeSQL(&query,
+ "associators of "
+
"{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
+ "Name=\"%s\"} "
+ "where AssocClass = Msvm_SettingsDefineState "
+ "ResultClass = Msvm_VirtualSystemSettingData",
+ uuid_string);
if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query,
&virtualSystemSettingData) <
0) {
@@ -696,12 +696,12 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr
info)
}
/* Get Msvm_ProcessorSettingData */
- virBufferAsprintf(&query,
- "associators of "
- "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
- "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
- "ResultClass = Msvm_ProcessorSettingData",
- virtualSystemSettingData->data.common->InstanceID);
+ virBufferEscapeSQL(&query,
+ "associators of "
+ "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
+ "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
+ "ResultClass = Msvm_ProcessorSettingData",
+ virtualSystemSettingData->data.common->InstanceID);
if (hypervGetMsvmProcessorSettingDataList(priv, &query,
&processorSettingData) < 0) {
@@ -717,12 +717,12 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr
info)
}
/* Get Msvm_MemorySettingData */
- virBufferAsprintf(&query,
- "associators of "
- "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
- "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
- "ResultClass = Msvm_MemorySettingData",
- virtualSystemSettingData->data.common->InstanceID);
+ virBufferEscapeSQL(&query,
+ "associators of "
+ "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
+ "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
+ "ResultClass = Msvm_MemorySettingData",
+ virtualSystemSettingData->data.common->InstanceID);
if (hypervGetMsvmMemorySettingDataList(priv, &query,
&memorySettingData) < 0) {
@@ -811,13 +811,13 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int
flags)
goto cleanup;
/* Get Msvm_VirtualSystemSettingData */
- virBufferAsprintf(&query,
- "associators of "
-
"{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
- "Name=\"%s\"} "
- "where AssocClass = Msvm_SettingsDefineState "
- "ResultClass = Msvm_VirtualSystemSettingData",
- uuid_string);
+ virBufferEscapeSQL(&query,
+ "associators of "
+
"{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
+ "Name=\"%s\"} "
+ "where AssocClass = Msvm_SettingsDefineState "
+ "ResultClass = Msvm_VirtualSystemSettingData",
+ uuid_string);
if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query,
&virtualSystemSettingData) <
0) {
@@ -833,12 +833,12 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int
flags)
}
/* Get Msvm_ProcessorSettingData */
- virBufferAsprintf(&query,
- "associators of "
- "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
- "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
- "ResultClass = Msvm_ProcessorSettingData",
- virtualSystemSettingData->data.common->InstanceID);
+ virBufferEscapeSQL(&query,
+ "associators of "
+ "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
+ "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
+ "ResultClass = Msvm_ProcessorSettingData",
+ virtualSystemSettingData->data.common->InstanceID);
if (hypervGetMsvmProcessorSettingDataList(priv, &query,
&processorSettingData) < 0) {
@@ -854,12 +854,12 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int
flags)
}
/* Get Msvm_MemorySettingData */
- virBufferAsprintf(&query,
- "associators of "
- "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
- "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
- "ResultClass = Msvm_MemorySettingData",
- virtualSystemSettingData->data.common->InstanceID);
+ virBufferEscapeSQL(&query,
+ "associators of "
+ "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
+ "where AssocClass =
Msvm_VirtualSystemSettingDataComponent "
+ "ResultClass = Msvm_MemorySettingData",
+ virtualSystemSettingData->data.common->InstanceID);
if (hypervGetMsvmMemorySettingDataList(priv, &query,
&memorySettingData) < 0) {
@@ -1398,7 +1398,7 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int
codeset,
if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0)
goto cleanup;
- virBufferAsprintf(&query,
+ virBufferEscapeSQL(&query,
"associators of "
"{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\","
"Name=\"%s\"} "
@@ -1535,7 +1535,7 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned
long memory,
}
virBufferAddLit(&eprQuery, MSVM_COMPUTERSYSTEM_WQL_SELECT);
- virBufferAsprintf(&eprQuery, "where Name = \"%s\"", uuid_string);
+ virBufferEscapeSQL(&eprQuery, "where Name = \"%s\"", uuid_string);
if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery,
Msvm_ComputerSystem_WmiInfo) < 0)
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index 6a51eff20..21913f352 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -896,7 +896,7 @@ hypervInvokeMethod(hypervPrivate *priv,
hypervInvokeParamsListPtr params,
*/
while (!completed && timeout >= 0) {
virBufferAddLit(&query, MSVM_CONCRETEJOB_WQL_SELECT);
- virBufferAsprintf(&query, "where InstanceID = \"%s\"", instanceID);
+ virBufferEscapeSQL(&query, "where InstanceID = \"%s\"",
instanceID);
if (hypervGetMsvmConcreteJobList(priv, &query, &job) < 0
|| job == NULL)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 28a291bb0..15f6e21fb 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -575,6 +575,24 @@ virBufferEscapeRegex(virBufferPtr buf,
}
/**
+ * virBufferEscapeSQL:
+ * @buf: the buffer to append to
+ * @format: a printf like format string but with only one %s parameter
+ * @str: the string argument which needs to be escaped
+ *
+ * Do a formatted print with a single string to a buffer. The @str is
+ * escaped to prevent SQL injection (format is expected to contain \"%s\").
+ * Auto indentation may be applied.
+ */
+void
+virBufferEscapeSQL(virBufferPtr buf,
+ const char *format,
+ const char *str)
+{
+ virBufferEscape(buf, '\\', "'\"\\", format, str);
+}
+
+/**
* virBufferEscape:
* @buf: the buffer to append to
* @escape: the escape character to inject
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index 3211c07b0..4f5ed162f 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -93,6 +93,9 @@ void virBufferEscapeSexpr(virBufferPtr buf, const char
*format,
void virBufferEscapeRegex(virBufferPtr buf,
const char *format,
const char *str);
+void virBufferEscapeSQL(virBufferPtr buf,
+ const char *format,
+ const char *str);
void virBufferEscapeShell(virBufferPtr buf, const char *str);
void virBufferURIEncodeString(virBufferPtr buf, const char *str);
--
2.13.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
