On Sat, May 24, 2014 at 10:37 PM, Sasha Pachev <[email protected]> wrote:
> Currently exp_replace() (which is used in reqrep/reqirep) is
> vulnerable to a buffer overrun. I have been able to reproduce it using
> the attached configuration file and issuing the following command:
>
> wget -O - -S -q http://localhost:8000/`perl -e 'print "a"x4000'`/cookie.php
>
> The other attachment contains the proposed fix. Since I was fixing up
> exp_replace() I also added two improvements/fixes:
>
> - str was being checked only in in while (str) and it was possible to
> read past that when more than one character was being accessed in the
> loop
> - I need exp_replace() to support str not necessarily being
> NULL-terminated for the replace-header/modify-header feature, so I
> added str_len argument which can be set to 0, in which case it is
> determined with a call to strlen()
Attached is an improved patch. The following errors are corrected:
- not properly checking for buffer overrun before memcpy() in ext_replace
- when the buffer overrun was detected, the filtering of the headers
continued normally. The patch terminates the filtering by returning -1
from apply_filter_* functions
- the original patch did not distinguish between replacing with an
empty string and a buffer overrun
--
Sasha Pachev
Fast Running Blog.
http://fastrunningblog.com
Run. Blog. Improve. Repeat.
diff --git a/include/common/regex.h b/include/common/regex.h
index 9789ec3..89e6506 100644
--- a/include/common/regex.h
+++ b/include/common/regex.h
@@ -79,7 +79,9 @@ extern regmatch_t pmatch[MAX_MATCH];
* The function return 1 is succes case, else return 0 and err is filled.
*/
int regex_comp(const char *str, struct my_regex *regex, int cs, int cap, char **err);
-int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches);
+int exp_replace(char *dst, uint dst_size, char *src, const char *str, uint str_len,
+ const regmatch_t *matches);
+
const char *check_replace_string(const char *str);
const char *chain_regex(struct hdr_exp **head, const regex_t *preg,
int action, const char *replace, void *cond);
diff --git a/src/proto_http.c b/src/proto_http.c
index bd6d024..7e6e79f 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -235,6 +235,10 @@ struct chunk http_err_chunks[HTTP_ERR_SIZE];
/* this struct is used between calls to smp_fetch_hdr() or smp_fetch_cookie() */
static struct hdr_ctx static_hdr_ctx;
+#define CHECK_ACT_REPLACE if (trash.len == -1) {\
+ Alert("Buffer overrun or invalid expr processing ACT_REPLACE '%s'", exp->replace);\
+ return -1; }
+
#define FD_SETS_ARE_BITFIELDS
#ifdef FD_SETS_ARE_BITFIELDS
/*
@@ -6817,7 +6821,8 @@ int apply_filter_to_req_headers(struct session *s, struct channel *req, struct h
break;
case ACT_REPLACE:
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, 0, pmatch);
+ CHECK_ACT_REPLACE;
delta = buffer_replace2(req->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -6927,7 +6932,8 @@ int apply_filter_to_req_line(struct session *s, struct channel *req, struct hdr_
case ACT_REPLACE:
*cur_end = term; /* restore the string terminator */
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, 0, pmatch);
+ CHECK_ACT_REPLACE;
delta = buffer_replace2(req->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -7676,7 +7682,8 @@ int apply_filter_to_resp_headers(struct session *s, struct channel *rtr, struct
break;
case ACT_REPLACE:
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, 0, pmatch);
+ CHECK_ACT_REPLACE;
delta = buffer_replace2(rtr->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -7766,7 +7773,8 @@ int apply_filter_to_sts_line(struct session *s, struct channel *rtr, struct hdr_
case ACT_REPLACE:
*cur_end = term; /* restore the string terminator */
- trash.len = exp_replace(trash.str, cur_ptr, exp->replace, pmatch);
+ trash.len = exp_replace(trash.str, trash.size, cur_ptr, exp->replace, 0, pmatch);
+ CHECK_ACT_REPLACE;
delta = buffer_replace2(rtr->buf, cur_ptr, cur_end, trash.str, trash.len);
/* FIXME: if the user adds a newline in the replacement, the
* index will not be recalculated for now, and the new line
@@ -7847,7 +7855,8 @@ int apply_filters_to_response(struct session *s, struct channel *rtr, struct pro
/* The filter did not match the response, it can be
* iterated through all headers.
*/
- apply_filter_to_resp_headers(s, rtr, exp);
+ if (unlikely(apply_filter_to_resp_headers(s, rtr, exp) < 0))
+ return -1;
}
}
return 0;
diff --git a/src/regex.c b/src/regex.c
index 7a76940..b93c61d 100644
--- a/src/regex.c
+++ b/src/regex.c
@@ -22,14 +22,26 @@
/* regex trash buffer used by various regex tests */
regmatch_t pmatch[MAX_MATCH]; /* rm_so, rm_eo for regular expressions */
+#define STR_CHECK if (str >= str_end) return -1
+#define DST_CHECK if (dst >= dst_end) return -1
+#define DST_LEN_CHECK(l) if (dst + l >= dst_end) return -1
-int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches)
+int exp_replace(char *dst, uint dst_size, char *src, const char *str, uint str_len, const regmatch_t *matches)
{
char *old_dst = dst;
+ char* dst_end = dst + dst_size;
+ const char* str_end;
- while (*str) {
+ if (!str_len)
+ str_len = strlen(str);
+
+ str_end = str + str_len;
+
+ while (str < str_end) {
if (*str == '\\') {
str++;
+ STR_CHECK;
+
if (isdigit((unsigned char)*str)) {
int len, num;
@@ -38,6 +50,7 @@ int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches
if (matches[num].rm_eo > -1 && matches[num].rm_so > -1) {
len = matches[num].rm_eo - matches[num].rm_so;
+ DST_LEN_CHECK(len);
memcpy(dst, src + matches[num].rm_so, len);
dst += len;
}
@@ -46,23 +59,33 @@ int exp_replace(char *dst, char *src, const char *str, const regmatch_t *matches
unsigned char hex1, hex2;
str++;
+ STR_CHECK;
hex1 = toupper(*str++) - '0';
+ STR_CHECK;
hex2 = toupper(*str++) - '0';
if (hex1 > 9) hex1 -= 'A' - '9' - 1;
if (hex2 > 9) hex2 -= 'A' - '9' - 1;
+ DST_CHECK;
*dst++ = (hex1<<4) + hex2;
} else {
+ DST_CHECK;
*dst++ = *str++;
}
} else {
+ DST_CHECK;
*dst++ = *str++;
}
}
+ DST_CHECK;
*dst = '\0';
return dst - old_dst;
}
+#undef STR_CHECK
+#undef DST_CHECK
+#undef DST_LEN_CHECK
+
/* returns NULL if the replacement string <str> is valid, or the pointer to the first error */
const char *check_replace_string(const char *str)
{
