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 48d3c254e3ee06eaafcff37ceb96774ed0ba6032..4a32c21cc89a8f573990487efb72b08fd8b4175a 100644 (file)
--- 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 eddfb70d2c040cb66f38896c375c494e98fbfab7..226546649a98d6f6ad407ab340c42b413a080f66 100644 (file)
--- 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);