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() -- Sasha Pachev Fast Running Blog. http://fastrunningblog.com Run. Blog. Improve. Repeat.
haproxy-reg.cfg
Description: Binary data
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..e918c0a 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) {\
+ Alert("Buffer overrun or invalid expr processing ACT_REPLACE '%s'", exp->replace);\
+ break; }
+
#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
diff --git a/src/regex.c b/src/regex.c
index 7a76940..d0a3115 100644
--- a/src/regex.c
+++ b/src/regex.c
@@ -22,14 +22,25 @@
/* 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 0
+#define DST_CHECK if (dst >= dst_end) return 0
-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 +49,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_CHECK;
memcpy(dst, src + matches[num].rm_so, len);
dst += len;
}
@@ -46,23 +58,32 @@ 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
+
/* returns NULL if the replacement string <str> is valid, or the pointer to the first error */
const char *check_replace_string(const char *str)
{
