[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [Libevent-users] patch for evhttp_htmlescape() in http.c
On Mon, May 23, 2011 at 5:54 PM, Mansour Moufid <mansourmoufid@xxxxxxxxx> wrote:
> A couple changes in the file `http.c'.
>
> Removed the `scratch_space' variable from the `evhttp_htmlescape'
> function since it wasn't actually used; also removed the `buf'
> variable from the `evhttp_htmlescape' function since it was only used
> by `scratch_space'.
>
> Modified the `html_replace' function so that it returns the length of
> the replacement string instead of the string itself. This is used to
> easily check for overflows of the `new_size' variable in the first for
> loop of the `evhttp_htmlescape' function, and thus potential out of
> bounds writes in the second for loop (if an overflow occurs in
> new_size, then new_size < old_size). Also check that new_size + 1
> doesn't overflow in mm_malloc(new_size + 1).
I like it, except for all the 'if (escaped != NULL)' checks:
assignments are much cheaper than branches, so let's just always pass
in a pointer for "escaped".
Alternative patch attached: looks ok?
--
Nick
From 06c51cdf9302967be1547a00fa449d9889ab4666 Mon Sep 17 00:00:00 2001
From: Mansour Moufid <mansourmoufid@xxxxxxxxx>
Date: Mon, 23 May 2011 18:01:24 -0400
Subject: [PATCH] Prevent size_t overflow in evhttp_htmlescape.
Modified the `html_replace' function so that it returns the length of
the replacement string instead of the string itself. This is used to
easily check for overflows of the `new_size' variable in the first for
loop of the `evhttp_htmlescape' function, and thus potential out of
bounds writes in the second for loop (if an overflow occurs in
new_size, then new_size < old_size). Also check that new_size + 1
doesn't overflow in mm_malloc(new_size + 1).
Removed the `scratch_space' variable from the `evhttp_htmlescape'
function since it wasn't actually used; also removed the `buf'
variable from the `evhttp_htmlescape' function since it was only used
by `scratch_space'.
---
http.c | 52 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 33 insertions(+), 19 deletions(-)
diff --git a/http.c b/http.c
index c878a57..13a0c97 100644
--- a/http.c
+++ b/http.c
@@ -219,29 +219,30 @@ strsep(char **s, const char *del)
}
#endif
-static const char *
-html_replace(char ch, char *buf)
+static size_t
+html_replace(const char ch, const char **escaped)
{
switch (ch) {
case '<':
- return "<";
+ *escaped = "<";
+ return 4;
case '>':
- return ">";
+ *escaped = ">";
+ return 4;
case '"':
- return """;
+ *escaped = """;
+ return 6;
case '\'':
- return "'";
+ *escaped = "'";
+ return 6;
case '&':
- return "&";
+ *escaped = "&";
+ return 5;
default:
break;
}
- /* Echo the character back */
- buf[0] = ch;
- buf[1] = '\0';
-
- return buf;
+ return 1;
}
/*
@@ -255,21 +256,34 @@ char *
evhttp_htmlescape(const char *html)
{
size_t i;
- size_t new_size = 0, old_size = strlen(html);
+ size_t new_size = 0, old_size = 0;
char *escaped_html, *p;
- char scratch_space[2];
- for (i = 0; i < old_size; ++i)
- new_size += strlen(html_replace(html[i], scratch_space));
+ if (html == NULL)
+ return (NULL);
+
+ old_size = strlen(html);
+ for (i = 0; i < old_size; ++i) {
+ const char *replaced = NULL;
+ const size_t replace_size = html_replace(html[i], &replaced);
+ if (replace_size > EV_SIZE_MAX - new_size) {
+ event_warn("%s: html_replace overflow", __func__);
+ return (NULL);
+ }
+ new_size += replace_size;
+ }
+ if (new_size == EV_SIZE_MAX)
+ return (NULL);
p = escaped_html = mm_malloc(new_size + 1);
if (escaped_html == NULL) {
- event_warn("%s: malloc(%ld)", __func__, (long)(new_size + 1));
+ event_warn("%s: malloc(%lu)", __func__,
+ (unsigned long)(new_size + 1));
return (NULL);
}
for (i = 0; i < old_size; ++i) {
- const char *replaced = html_replace(html[i], scratch_space);
- size_t len = strlen(replaced);
+ const char *replaced = &html[i];
+ const size_t len = html_replace(html[i], &replaced);
memcpy(p, replaced, len);
p += len;
}
--
1.7.4.4