1:47 a.m.
Fixes a couple of minor issues in the Hyper-V driver found by code inspection.
v1->v2:
* Removed an "Invalid query" virReportError instead of adding a new one
because virBufferCheckError already reports a meaningful error (Daniel)
Ladi Prosek (3):
hyperv: Fix hypervInitConnection error reporting
hyperv: Escape WQL queries
hyperv: Map Limit to max_memory and VirtualQuantity to cur_balloon
src/hyperv/hyperv_driver.c | 104 ++++++++++++++++++++++-----------------------
src/hyperv/hyperv_wmi.c | 3 +-
src/util/virbuffer.c | 18 ++++++++
src/util/virbuffer.h | 3 ++
4 files changed, 74 insertions(+), 54 deletions(-)
--
2.13.5
1:47 a.m.
New subject: [libvirt] [PATCH v2 1/3] hyperv: Fix hypervInitConnection error reporting
"%s is not a Hyper-V server" is not a correct generalization of all possible
error conditions of hypervEnumAndPull. For example:
$ virsh --connect hyperv://localhost/?transport=http
Enter username for localhost [administrator]:
Enter administrator's password for localhost: <enters incorrect password>
error: failed to connect to the hypervisor
error: internal error: localhost is not a Hyper-V server
This commit removes the general virReportError from hypervInitConnection and
also the "Invalid query" virReportError from hypervSerializeEprParam, which
does not correctly describe the error either (virBufferCheckError has
already set a meaningful error message at that point).
The same scenario with the fix:
$ virsh --connect hyperv://localhost/?transport=http
Enter username for localhost [administrator]:
Enter administrator's password for localhost: <enters incorrect password>
error: failed to connect to the hypervisor
error: internal error: Transport error during enumeration: User, password or
similar was not accepted (26)
Signed-off-by: Ladi Prosek <lprosek(a)redhat.com>
---
src/hyperv/hyperv_driver.c | 2 --
src/hyperv/hyperv_wmi.c | 1 -
2 files changed, 3 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 09912610c..52274239c 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -105,8 +105,6 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate *priv,
priv->wmiVersion = HYPERV_WMI_VERSION_V1;
if (hypervEnumAndPull(priv, &wqlQuery, &computerSystem) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("%s is not a Hyper-V server"),
conn->uri->server);
goto cleanup;
}
}
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index 980a00e28..0b9431bfa 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -514,7 +514,6 @@ hypervSerializeEprParam(hypervParamPtr p, hypervPrivate *priv,
/* Get query and create filter based on it */
if (virBufferCheckError(p->epr.query) < 0) {
virBufferFreeAndReset(p->epr.query);
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid
query"));
goto cleanup;
}
query_string = virBufferContentAndReset(p->epr.query);
--
2.13.5
8:58 a.m.
New subject: [libvirt] [PATCH v2 1/3] hyperv: Fix hypervInitConnection error reporting
On 10/06/2017 02:47 AM, Ladi Prosek wrote:
"%s is not a Hyper-V server" is not a correct
generalization of all possible
error conditions of hypervEnumAndPull. For example:
$ virsh --connect hyperv://localhost/?transport=http
Enter username for localhost [administrator]:
Enter administrator's password for localhost: <enters incorrect password>
error: failed to connect to the hypervisor
error: internal error: localhost is not a Hyper-V server
This commit removes the general virReportError from hypervInitConnection and
also the "Invalid query" virReportError from hypervSerializeEprParam, which
does not correctly describe the error either (virBufferCheckError has
already set a meaningful error message at that point).
The same scenario with the fix:
$ virsh --connect hyperv://localhost/?transport=http
Enter username for localhost [administrator]:
Enter administrator's password for localhost: <enters incorrect password>
error: failed to connect to the hypervisor
error: internal error: Transport error during enumeration: User, password or
similar was not accepted (26)
Signed-off-by: Ladi Prosek <lprosek(a)redhat.com>
---
src/hyperv/hyperv_driver.c | 2 --
src/hyperv/hyperv_wmi.c | 1 -
2 files changed, 3 deletions(-)
Seems reasonable -
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 09912610c..52274239c 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -105,8 +105,6 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate *priv,
priv->wmiVersion = HYPERV_WMI_VERSION_V1;
if (hypervEnumAndPull(priv, &wqlQuery, &computerSystem) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("%s is not a Hyper-V server"),
conn->uri->server);
goto cleanup;
}
Before posting be sure to "make check syntax-check" which would have
told you that since there's only one line (cleanup;) in the condition
that the { } are not necessary.
I've removed them locally before pushing...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
}
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index 980a00e28..0b9431bfa 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -514,7 +514,6 @@ hypervSerializeEprParam(hypervParamPtr p, hypervPrivate *priv,
/* Get query and create filter based on it */
if (virBufferCheckError(p->epr.query) < 0) {
virBufferFreeAndReset(p->epr.query);
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid
query"));
goto cleanup;
}
query_string = virBufferContentAndReset(p->epr.query);
2:26 a.m.
New subject: [libvirt] [PATCH v2 1/3] hyperv: Fix hypervInitConnection error reporting
On Mon, Oct 16, 2017 at 3:58 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 10/06/2017 02:47 AM, Ladi Prosek wrote:
> "%s is not a Hyper-V server" is not a correct generalization of all
possible
> error conditions of hypervEnumAndPull. For example:
>
> $ virsh --connect hyperv://localhost/?transport=http
> Enter username for localhost [administrator]:
> Enter administrator's password for localhost: <enters incorrect
password>
> error: failed to connect to the hypervisor
> error: internal error: localhost is not a Hyper-V server
>
> This commit removes the general virReportError from hypervInitConnection and
> also the "Invalid query" virReportError from hypervSerializeEprParam,
which
> does not correctly describe the error either (virBufferCheckError has
> already set a meaningful error message at that point).
>
> The same scenario with the fix:
>
> $ virsh --connect hyperv://localhost/?transport=http
> Enter username for localhost [administrator]:
> Enter administrator's password for localhost: <enters incorrect
password>
> error: failed to connect to the hypervisor
> error: internal error: Transport error during enumeration: User, password or
> similar was not accepted (26)
>
> Signed-off-by: Ladi Prosek <lprosek(a)redhat.com>
> ---
> src/hyperv/hyperv_driver.c | 2 --
> src/hyperv/hyperv_wmi.c | 1 -
> 2 files changed, 3 deletions(-)
>
Seems reasonable -
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 09912610c..52274239c 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -105,8 +105,6 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate *priv,
> priv->wmiVersion = HYPERV_WMI_VERSION_V1;
>
> if (hypervEnumAndPull(priv, &wqlQuery, &computerSystem) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("%s is not a Hyper-V server"),
conn->uri->server);
> goto cleanup;
> }
Before posting be sure to "make check syntax-check" which would have
told you that since there's only one line (cleanup;) in the condition
that the { } are not necessary.
Will do, sorry about that.
I've removed them locally before pushing...
Thank you!
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
> }
> diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
> index 980a00e28..0b9431bfa 100644
> --- a/src/hyperv/hyperv_wmi.c
> +++ b/src/hyperv/hyperv_wmi.c
> @@ -514,7 +514,6 @@ hypervSerializeEprParam(hypervParamPtr p, hypervPrivate *priv,
> /* Get query and create filter based on it */
> if (virBufferCheckError(p->epr.query) < 0) {
> virBufferFreeAndReset(p->epr.query);
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid
query"));
> goto cleanup;
> }
> query_string = virBufferContentAndReset(p->epr.query);
>
1:47 a.m.
New subject: [libvirt] [PATCH v2 2/3] 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(a)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 0b9431bfa..5e2b3d7ed 100644
--- 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/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
8:58 a.m.
New subject: [libvirt] [PATCH v2 2/3] hyperv: Escape WQL queries
On 10/06/2017 02:47 AM, Ladi Prosek wrote:
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(a)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(-)
Surprised to a degree this worked correctly without adding
'virBufferEscapeSQL' to src/libvirt_private.syms
In any case, I'll add before pushing...
[...]
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,
}
FYI: More recently we've been using 2 blank lines between functions -
I'll adjust that here too.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
/**
+ * 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:32 a.m.
New subject: [libvirt] [PATCH v2 2/3] hyperv: Escape WQL queries
On Mon, Oct 16, 2017 at 3:58 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 10/06/2017 02:47 AM, Ladi Prosek wrote:
> 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(a)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(-)
>
Surprised to a degree this worked correctly without adding
'virBufferEscapeSQL' to src/libvirt_private.syms
Interesting, I followed instructions at
https://libvirt.org/compiling.html#building and didn't see any
warnings or indication that something was amiss.
In any case, I'll add before pushing...
Thank you!
1:47 a.m.
New subject: [libvirt] [PATCH v2 3/3] hyperv: Map Limit to max_memory and VirtualQuantity to cur_balloon
Hyper-V uses its own specific memory management so no mapping is going to
be perfect. However, it is more correct to map Limit to max_memory (it
really is the upper limit of what the VM may potentially use) and keep
cur_balloon equal to total_memory.
The typical value returned from Hyper-V in Limit is 1 TiB, which is not
really going to work if interpreted as "startup memory" to be ballooned
away later.
Signed-off-by: Ladi Prosek <lprosek(a)redhat.com>
---
src/hyperv/hyperv_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 998780b80..4565af3ed 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -920,8 +920,10 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
def->description = virBufferContentAndReset(&buf);
}
- virDomainDefSetMemoryTotal(def, memorySettingData->data.common->Limit * 1024);
/* megabyte to kilobyte */
- def->mem.cur_balloon = memorySettingData->data.common->VirtualQuantity *
1024; /* megabyte to kilobyte */
+ /* mebibytes to kibibytes */
+ def->mem.max_memory = memorySettingData->data.common->Limit * 1024;
+ def->mem.cur_balloon = memorySettingData->data.common->VirtualQuantity *
1024;
+ virDomainDefSetMemoryTotal(def, memorySettingData->data.common->VirtualQuantity
* 1024);
if (virDomainDefSetVcpusMax(def,
processorSettingData->data.common->VirtualQuantity,
--
2.13.5
9:25 a.m.
New subject: [libvirt] [PATCH v2 3/3] hyperv: Map Limit to max_memory and VirtualQuantity to cur_balloon
On 10/06/2017 02:47 AM, Ladi Prosek wrote:
Hyper-V uses its own specific memory management so no mapping is
going to
be perfect. However, it is more correct to map Limit to max_memory (it
really is the upper limit of what the VM may potentially use) and keep
cur_balloon equal to total_memory.
The typical value returned from Hyper-V in Limit is 1 TiB, which is not
really going to work if interpreted as "startup memory" to be ballooned
away later.
Signed-off-by: Ladi Prosek <lprosek(a)redhat.com>
---
src/hyperv/hyperv_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Seems reasonable... This was created this way by commit id '5e3b0f8b5'
before any of the interceding work to memory size adjustments
total_memory/max_balloon and max_memory. Still it would seem that the
initial code should have set max_memory to Limit and max_balloon to the
cur_balloon value
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2593
days inactive
2604
days old
8 comments
2 participants
participants (2)
-
John Ferlan -
Ladi Prosek