Jim Meyering wrote:

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.

I removed that since VIR_ALLOC_N fills the memory with zeros. Correct?

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 ":".

Ok.

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.

Good point. Suggestions on the limit? BTW, I pushed the patch after Eric's ACK. I'll role another to address these issues, once I get confirmation on the NUL-termination. Thanks, Jim

Jim Meyering wrote:

...

- if (VIR_ALLOC_N(*content, content_length) < 0 ) { + if (content_length > XEND_RCV_BUF_MAX_LEN) { + virXendError(VIR_ERR_INTERNAL_ERROR, + _("Xend returned HTTP Content-Length of %d, " + "which exceeds maximum of %d"), + content_length, + XEND_RCV_BUF_MAX_LEN); + return -1; + } +

This is subtle enough that a comment might avoid trouble later.

/* Allocate one byte beyond the end of the largest buffer we will read. Combined with the fact that VIR_ALLOC_N zeros the returned buffer, this guarantees that "content" will always be NUL-terminated. */

Good idea. I've added your wording verbatim.

...

+ if (VIR_ALLOC_N(*content, content_length + 1) < 0 ) { virReportOOMError(); return -1; } @@ -380,7 +390,7 @@ xend_get(virConnectPtr xend, const char *path, virXendError(VIR_ERR_GET_FAILED, _("%d status from xen daemon: %s:%s"), ret, path, - content ? *content: "NULL"); + content ? *content : "NULL");

Oh! I've just noticed that testing "content" is wrong, since it will always be non-NULL. BTW, the parameter should be marked as such via "ATTRIBUTE_NONNULL (3)".

I added ATTRIBUTE_NONNULL to xend_req() as well.

The test should examine "*content", i.e.,

*content ? *content : "NULL");

Then I remembered the NULLSTR macro. You can replace the above with this:

NULLSTR(*content));

FYI, here's its definition:

$ git grep -A2 ine.NULLSTR src/internal.h:# define NULLSTR(s) \ src/internal.h- ((void)verify_true(sizeof *(s) == sizeof (char)), \ src/internal.h- (s) ? (s) : "(null)")

Cool, that's handy. Updated patch attached. Regards, Jim