Hi developers,
The attached patch fixes a buffer overflow in at least one of the six
string replacement implementations in mod_proxy_html.
Unfortunately I don't remember anymore how to reproduce the issue
properly, but I know that some long time ago I fixed a segfault with
this patch.
The patch tries to address the buffer overflow by introducing a new
function preplace() dedicated to clean string replacement. This function
is now used on all six places where the error-prone string replacement
was previously implemented manually with memcpy() and memmove().
The patch is based on SVN trunk rev. 1579365.
I've filed it also in ASF bugzilla as issue #56289.
Regards,
Micha
Index: modules/filters/mod_proxy_html.c
===================================================================
--- modules/filters/mod_proxy_html.c (Revision 1579365)
+++ modules/filters/mod_proxy_html.c (Arbeitskopie)
@@ -29,6 +29,8 @@
#define VERBOSEB(x) if (verbose) {x}
#endif
+#include <assert.h>
+
/* libxml2 */
#include <libxml/HTMLparser.h>
@@ -191,6 +193,9 @@
}
}
+/**
+ * Appends the string buf with length len to ctx->buf at position ctx->offset
+ */
static void pappend(saxctxt *ctx, const char *buf, const size_t len)
{
preserve(ctx, len);
@@ -198,6 +203,34 @@
ctx->offset += len;
}
+/**
+ * In ctx->buf replaces the substring starting at offset start with length old_length
+ * by the string new with length new_length
+ */
+static void preplace(saxctxt* ctx, const size_t start, const size_t old_length, const char* new, const size_t new_length) {
+ // check arguments
+ assert(ctx->offset > start + old_length);
+ assert(new != NULL);
+
+ // do the string replacement
+ if (old_length < new_length) {
+ int old_buffer_length = ctx->offset;
+ preserve(ctx, new_length - old_length);
+ memmove(ctx->buf + start + new_length,
+ ctx->buf + start + old_length,
+ ctx->offset - (start + old_length));
+ ctx->offset += new_length - old_length;
+ }
+ memcpy(ctx->buf + start, new, new_length);
+ if (old_length > new_length) {
+ memmove(ctx->buf + start + new_length,
+ ctx->buf + start + old_length,
+ ctx->offset - (start + old_length));
+ assert(ctx->offset >= old_length - new_length);
+ ctx->offset -= old_length - new_length;
+ }
+}
+
static void dump_content(saxctxt *ctx)
{
urlmap *m;
@@ -236,17 +269,7 @@
ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, ctx->f->r,
"C/RX: match at %s, substituting %s", f, subs);
)
- if (s_to > s_from) {
- preserve(ctx, s_to - s_from);
- memmove(ctx->buf+offs+s_to, ctx->buf+offs+s_from,
- len + 1 - s_from - offs);
- memcpy(ctx->buf+offs, subs, s_to);
- }
- else {
- memcpy(ctx->buf + offs, subs, s_to);
- memmove(ctx->buf+offs+s_to, ctx->buf+offs+s_from,
- len + 1 - s_from - offs);
- }
+ preplace(ctx, offs, s_from, subs, s_to);
offs += s_to;
}
}
@@ -264,17 +287,7 @@
VERBOSE(ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, ctx->f->r,
"C: matched %s, substituting %s",
m->from.c, m->to));
- if (s_to > s_from) {
- preserve(ctx, s_to - s_from);
- memmove(ctx->buf+match+s_to, ctx->buf+match+s_from,
- len + 1 - s_from - match);
- memcpy(ctx->buf+match, m->to, s_to);
- }
- else {
- memcpy(ctx->buf+match, m->to, s_to);
- memmove(ctx->buf+match+s_to, ctx->buf+match+s_from,
- len + 1 - s_from - match);
- }
+ preplace(ctx, match, s_from, m->to, s_to);
}
}
}
@@ -481,19 +494,7 @@
})
s_to = strlen(subs);
len = strlen(ctx->buf);
- if (s_to > s_from) {
- preserve(ctx, s_to - s_from);
- memmove(ctx->buf+offs+s_to,
- ctx->buf+offs+s_from,
- len + 1 - s_from - offs);
- memcpy(ctx->buf+offs, subs, s_to);
- }
- else {
- memcpy(ctx->buf + offs, subs, s_to);
- memmove(ctx->buf+offs+s_to,
- ctx->buf+offs+s_from,
- len + 1 - s_from - offs);
- }
+ preplace(ctx, offs, s_from, subs, s_to);
}
} else {
s_from = strlen(m->from.c);
@@ -505,17 +506,7 @@
0, ctx->f->r,
"H: matched %s, substituting %s",
m->from.c, m->to));
- if (s_to > s_from) {
- preserve(ctx, s_to - s_from);
- memmove(ctx->buf+s_to, ctx->buf+s_from,
- len + 1 - s_from);
- memcpy(ctx->buf, m->to, s_to);
- }
- else { /* it fits in the existing space */
- memcpy(ctx->buf, m->to, s_to);
- memmove(ctx->buf+s_to, ctx->buf+s_from,
- len + 1 - s_from);
- }
+ preplace(ctx, 0, s_from, m->to, s_to);
break;
}
}
@@ -550,19 +541,7 @@
s_to = strlen(subs);
offs += match;
len = strlen(ctx->buf);
- if (s_to > s_from) {
- preserve(ctx, s_to - s_from);
- memmove(ctx->buf+offs+s_to,
- ctx->buf+offs+s_from,
- len + 1 - s_from - offs);
- memcpy(ctx->buf+offs, subs, s_to);
- }
- else {
- memcpy(ctx->buf + offs, subs, s_to);
- memmove(ctx->buf+offs+s_to,
- ctx->buf+offs+s_from,
- len + 1 - s_from - offs);
- }
+ preplace(ctx, offs, s_from, subs, s_to);
offs += s_to;
++num_match;
}
@@ -590,19 +569,7 @@
"E: matched %s, substituting %s",
m->from.c, m->to));
len = strlen(ctx->buf);
- if (s_to > s_from) {
- preserve(ctx, s_to - s_from);
- memmove(ctx->buf+match+s_to,
- ctx->buf+match+s_from,
- len + 1 - s_from - match);
- memcpy(ctx->buf+match, m->to, s_to);
- }
- else {
- memcpy(ctx->buf+match, m->to, s_to);
- memmove(ctx->buf+match+s_to,
- ctx->buf+match+s_from,
- len + 1 - s_from - match);
- }
+ preplace(ctx, match, s_from, m->to, s_to);
++num_match;
}
}
