[libvirt] [PATCH v2 2/3] hyperv: Escape WQL queries

[John Ferlan]

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 at 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 at 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);
>  
>