From: Christopher Faulet Date: Tue, 24 Nov 2020 08:49:01 +0000 (+0100) Subject: BUG/MAJOR: filters: Always keep all offsets up to date during data filtering X-Git-Tag: v2.4-dev2~34 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=401e6dbff3ee0b1932f6a16e3f280246752a7edf;p=thirdparty%2Fhaproxy.git BUG/MAJOR: filters: Always keep all offsets up to date during data filtering When at least one data filter is registered on a channel, the offsets of all filters must be kept up to date. For data filters but also for others. It is safer to do it in that way. Indirectly, this patch fixes 2 hidden bugs revealed by the commit 22fca1f2c ("BUG/MEDIUM: filters: Forward all filtered data at the end of http filtering"). The first one, the worst of both, happens at the end of http filtering when at least one data filtered is registered on the channel. We call the http_end() callback function on the filters, when defined, to finish the http filtering. But it is performed for all filters. Before the commit 22fca1f2c, the only risk was to call the http_end() callback function unexpectedly on a filter. Now, we may have an overflow on the offset variable, used at the end to forward all filtered data. Of course, from the moment we forward an arbitrary huge amount of data, all kinds of bad things may happen. So offset computation is performed for all filters and http_end() callback function is called only for data filters. The other one happens when a data filter alter the data of a channel, it must update the offsets of all previous filters. But the offset of non-data filters must be up to date, otherwise, here too we may have an integer overflow. Another way to fix these bugs is to always ignore non-data filters from the offsets computation. But this patch is safer and probably easier to maintain. This patch must be backported in all versions where the above commit is. So as far as 2.0. --- diff --git a/include/haproxy/filters.h b/include/haproxy/filters.h index 48d3c254e3..4a32c21cc8 100644 --- a/include/haproxy/filters.h +++ b/include/haproxy/filters.h @@ -166,11 +166,10 @@ unregister_data_filter(struct stream *s, struct channel *chn, struct filter *fil } /* This function must be called when a filter alter payload data. It updates - * offsets of all previous filters and the offset of the stream. Do not call - * this function when a filter change the size of payload data leads to an - * undefined behavior. + * offsets of all previous filters. Do not call this function when a filter + * change the size of payload data leads to an undefined behavior. * - * This is the filter's responsiblitiy to update data itself.. + * This is the filter's responsiblitiy to update data itself. */ static inline void flt_update_offsets(struct filter *filter, struct channel *chn, int len) @@ -181,8 +180,7 @@ flt_update_offsets(struct filter *filter, struct channel *chn, int len) list_for_each_entry(f, &strm_flt(s)->filters, list) { if (f == filter) break; - if (IS_DATA_FILTER(filter, chn)) - FLT_OFF(f, chn) += len; + FLT_OFF(f, chn) += len; } } diff --git a/src/filters.c b/src/filters.c index eddfb70d2c..226546649a 100644 --- a/src/filters.c +++ b/src/filters.c @@ -555,6 +555,12 @@ flt_http_end(struct stream *s, struct http_msg *msg) unsigned long long flt_off = FLT_OFF(filter, msg->chn); offset = flt_off - *strm_off; + /* Call http_end for data filters only. But the filter offset is + * still valid for all filters + . */ + if (!IS_DATA_FILTER(filter, msg->chn)) + continue; + if (FLT_OPS(filter)->http_end) { DBG_TRACE_DEVEL(FLT_ID(filter), STRM_EV_HTTP_ANA|STRM_EV_FLT_ANA, s); ret = FLT_OPS(filter)->http_end(s, filter, msg); @@ -629,13 +635,18 @@ flt_http_payload(struct stream *s, struct http_msg *msg, unsigned int len) ret = data = len - out; DBG_TRACE_ENTER(STRM_EV_STRM_ANA|STRM_EV_HTTP_ANA|STRM_EV_FLT_ANA, s, s->txn, msg); list_for_each_entry(filter, &strm_flt(s)->filters, list) { - /* Call "data" filters only */ - if (!IS_DATA_FILTER(filter, msg->chn)) + unsigned long long *flt_off = &FLT_OFF(filter, msg->chn); + unsigned int offset = *flt_off - *strm_off; + + /* Call http_payload for filters only. Forward all data for + * others and update the filter offset + */ + if (!IS_DATA_FILTER(filter, msg->chn)) { + *flt_off += data - offset; continue; - if (FLT_OPS(filter)->http_payload) { - unsigned long long *flt_off = &FLT_OFF(filter, msg->chn); - unsigned int offset = *flt_off - *strm_off; + } + if (FLT_OPS(filter)->http_payload) { DBG_TRACE_DEVEL(FLT_ID(filter), STRM_EV_HTTP_ANA|STRM_EV_FLT_ANA, s); ret = FLT_OPS(filter)->http_payload(s, filter, msg, out + offset, data - offset); if (ret < 0) @@ -793,10 +804,8 @@ flt_analyze_http_headers(struct stream *s, struct channel *chn, unsigned int an_ size_t data = http_get_hdrs_size(htxbuf(&chn->buf)); struct filter *f; - list_for_each_entry(f, &strm_flt(s)->filters, list) { - if (IS_DATA_FILTER(f, chn)) - FLT_OFF(f, chn) = data; - } + list_for_each_entry(f, &strm_flt(s)->filters, list) + FLT_OFF(f, chn) = data; } check_result: @@ -894,12 +903,18 @@ flt_tcp_payload(struct stream *s, struct channel *chn, unsigned int len) ret = data = len - out; DBG_TRACE_ENTER(STRM_EV_TCP_ANA|STRM_EV_FLT_ANA, s); list_for_each_entry(filter, &strm_flt(s)->filters, list) { - /* Call "data" filters only */ - if (!IS_DATA_FILTER(filter, chn)) + unsigned long long *flt_off = &FLT_OFF(filter, chn); + unsigned int offset = *flt_off - *strm_off; + + /* Call tcp_payload for filters only. Forward all data for + * others and update the filter offset + */ + if (!IS_DATA_FILTER(filter, chn)) { + *flt_off += data - offset; continue; + } + if (FLT_OPS(filter)->tcp_payload) { - unsigned long long *flt_off = &FLT_OFF(filter, chn); - unsigned int offset = *flt_off - *strm_off; DBG_TRACE_DEVEL(FLT_ID(filter), STRM_EV_TCP_ANA|STRM_EV_FLT_ANA, s); ret = FLT_OPS(filter)->tcp_payload(s, filter, chn, out + offset, data - offset);