hyperv: Escape WQL queries

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 <lprosek@redhat.com>

diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index ed4a55da01b47ff313f40fd56d2ea1ba1cbc5edc..8ae87743da788e1631faf3bfe005f5f8d262db07 100644 (file)
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -301,12 +301,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;
@@ -493,7 +493,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;
@@ -525,7 +525,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;
@@ -673,13 +673,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) {
@@ -695,12 +695,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) {
@@ -716,12 +716,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) {
@@ -810,13 +810,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) {
@@ -832,12 +832,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) {
@@ -853,12 +853,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) {
@@ -1397,7 +1397,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\"} "
@@ -1534,7 +1534,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 0b9431bfa8a002323630efcac176fc5bdad36b90..5e2b3d7edf5b57eddc9fb09bcd4b33e7408984a6 100644 (file)
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -895,7 +895,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/libvirt_private.syms b/src/libvirt_private.syms
index 4c56f17e295b2c9d0ba2c410301c92ac171a4789..6eea6f103f1e49a77eb1c3ad7b592be40d904b2b 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1384,6 +1384,7 @@ virBufferEscapeN;
 virBufferEscapeRegex;
 virBufferEscapeSexpr;
 virBufferEscapeShell;
+virBufferEscapeSQL;
 virBufferEscapeString;
 virBufferFreeAndReset;
 virBufferGetIndent;

diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 28a291bb0f74acccf92992fea118d223323a1f4f..1a6bf122e021a1c8b514c4b8c4926f7148b84b96 100644 (file)
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -574,6 +574,26 @@ virBufferEscapeRegex(virBufferPtr buf,
     virBufferEscape(buf, '\\', "^$.|?*+()[]{}\\", format, str);
 }
 
+
+/**
+ * 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

diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index 3211c07b08b9480814bd45bd7d484e763b8d9388..4f5ed162fb083d23dc7aa8cd23463a00ff5488ad 100644 (file)
--- 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);