[libvirt] [PATCH] Allocate buffer to hold xend response

[Jim Meyering]

Jim Fehlig wrote:
...
> Subject: [PATCH] Allocate buffer to hold xend response
>
> There are cases when a response from xend can exceed 4096 bytes, in
> which case anything beyond 4096 is ignored. This patch changes the
> current fixed-size, stack-allocated buffer to a dynamically allocated
> buffer based on Content-Length in HTTP header.
> ---
>  src/xen/xend_internal.c |   80 ++++++++++++++++++++++++-----------------------
>  1 files changed, 41 insertions(+), 39 deletions(-)
>
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index e763bad..0c1a738 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -302,17 +302,19 @@ istartswith(const char *haystack, const char *needle)
>   * xend_req:
>   * @fd: the file descriptor
>   * @content: the buffer to store the content
> - * @n_content: the size of the buffer
>   *
>   * Read the HTTP response from a Xen Daemon request.
> + * If the response contains content, memory is allocated to
> + * hold the content.
>   *
> - * Returns the HTTP return code.
> + * Returns the HTTP return code and @content is set to the
> + * allocated memory containing HTTP content.
>   */
>  static int
> -xend_req(int fd, char *content, size_t n_content)
> +xend_req(int fd, char **content)
>  {
>      char buffer[4096];
> -    int content_length = -1;
> +    int content_length = 0;
>      int retcode = 0;
>
>      while (sreads(fd, buffer, sizeof(buffer)) > 0) {
> @@ -325,19 +327,17 @@ xend_req(int fd, char *content, size_t n_content)
>              retcode = atoi(buffer + 9);
>      }
>
> -    if (content_length > -1) {
> +    if (content_length > 0) {
>          ssize_t ret;
>
> -        if ((unsigned int) content_length > (n_content + 1))
> -            content_length = n_content - 1;
> +        if (VIR_ALLOC_N(*content, content_length) < 0 ) {
> +            virReportOOMError();
> +            return -1;
> +        }
>
> -        ret = sread(fd, content, content_length);
> +        ret = sread(fd, *content, content_length);
>          if (ret < 0)
>              return -1;
> -
> -        content[ret] = 0;
> -    } else {
> -        content[0] = 0;
>      }

Hi Jim,

The above removes the code that guarantees the content buffer is
NUL-terminated, yet I don't see where the requirement for NUL-termination
has been dropped.  Even if the content payload we receive typically ends
in a NUL, we cannot guarantee that (i.e., content may be corrupted or
malicious), so we have to verify the assumption or add a NUL byte of
our own.

While your change does adjust the xend_req caller to handle NULL
"content", here it will read beyond end of buffer when "*content" has
no trailing NUL byte:

         virXendError(VIR_ERR_GET_FAILED,
                      _("%d status from xen daemon: %s:%s"),
-                     ret, path, content);
+                     ret, path,
+                     content ? *content: "NULL");

BTW, please add a space before the ":".

Also, now that we'll get the buffer length from an untrusted
source, it would be good to ensure that it's not outrageously
large before using it as the size of the buffer we allocate,
assuming there is some reasonably low upper bound on the maximum
payload size.  That could save us from potential DoS abuse.