Hi Sasha,
On Mon, May 26, 2014 at 07:51:38AM -0600, Sasha Pachev wrote:
> 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
Thank you. I'm having some comments about the patch itself below :
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; }
+
Please avoid defining macros for such control blocks, especially when
involve implicit local variables and can cause returns, it's always
harder to follow and when checking for unrelated bugs, they tend to
be disturbing. I know we already do it with CHECK_HTTP_MESSAGE_FIRST(),
but it's used a lot more, something like 25 times or so. And even then
it's annoying when you have to single-step through it.
Another point is that nobody will ever see this alert, so in practice better
not even send it and just ensure that the function fails so that the rules
processing is aborted and an error is returned. I guess -1 already grants
this given that you added the control in your patch. So that could simply
result in adding this at the four places where you used CHECK_ACT_REPLACE :
if (trash.len < 0)
return -1;
@@ -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
(...)
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
Same here, please put these statements directly, it's really hard to follow
what test is being done on them when reading the code. Also, such constructs
with a hidden "if" are really dangerous because they can change the meaning
of a following else. For example, let's consider this totally made up example :
if (dst)
DST_CHECK;
else
dst = src;
It does not do what it seems, in fact it does this :
if (dst)
if (dst >= dst_end)
return -1;
else
dst = src;
That's why we usually employ the "do { } while (0)" construct in macros.
But here it's really not justified.
Another point concerning the code's efficiency, for the source I believe
you should stick to the *str test instead of str < str_end for 3 reasons :
- everywhere it's tested, the following instruction is a fetch of *str,
so the same register will be reused ;
- on machines with low number of registers (eg: x86), it will avoid
moving variables back and forth to the stack.
- it avoids having to run over the whole input string twice (once for
strlen() and a second time for the actual work).
These points aside, I think that your fix works correctly.
Best regards,
Willy
