[PATCH] BUG/MAJOR: h1-htx: report an error on a too long chunk-size line

When a chunked message starts with a chunk-size line longer than the input
buffer, the parser was looping forever: the line was never complete, no data
could be consumed, and the connection remained stuck until the client
timeout because the buffer was full and could not be extended.

The full-buffer detection used for too large headers and trailers is now
also applied to the chunk-size line: if nothing was parsed and the input
buffer is full, the line cannot possibly fit and a parsing error is reported
instead of waiting for more data that will never fit.

A workaround consists to reject chunk-encoded request:

    http-request reject if { req.hdr(transfer-encoding) -i chunked }

This should be backported at least to 3.0. But possibly to all stable
versions with some modifications. However, no loop can be experienced on 2.8
and 2.6.

Many thanks to Adam Crawford for the report and the fix.

(cherry picked from commit 6e495dbb2a4850578439cf795966c068c1bdffe3)
[cf: VTC slightly adapted for 2.6]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
Gbp-Pq: Name 0001-BUG-MAJOR-h1-htx-report-an-error-on-a-too-long-chunk.patch

diff --git a/reg-tests/http-messaging/h1_big_chunk_size_line.vtc b/reg-tests/http-messaging/h1_big_chunk_size_line.vtc
new file mode 100644 (file)
index 0000000..4c7340a
--- /dev/null
+++ b/reg-tests/http-messaging/h1_big_chunk_size_line.vtc
@@ -0,0 +1,65 @@
+varnishtest "Test a chunk-size line which fills the whole buffer"
+
+# A chunked request whose first chunk-size line is longer than the
+# buffer used to cause an endless parsing loop: the parser could not
+# progress on the incomplete line, the buffer was full so no more data
+# could be received, and the connection remained stuck until the client
+# timeout. Such a line is now reported as a parsing error.
+#
+# The first client below sends a request whose first chunk-size line,
+# including its final LF, is longer than tune.bufsize, so it cannot
+# possibly fit in any buffer. The proxy must reply with a 400 at once
+# instead of remaining stuck until the client timeout. The error is
+# checked in the log because the response may be lost when the proxy
+# closes the connection with the request not fully drained.
+#
+# The second client sends a valid request to check that the proxy
+# remains alive and behaves normally.
+
+feature ignore_unknown_macro
+
+syslog S -level info {
+    recv
+    expect ~ ".* li1 li1/s1 [0-9]*/[0-9]*/-1/-1/[0-9]* 400 .* PH-- .* \"POST /big HTTP/1.1\""
+} -start
+
+haproxy h1 -conf {
+    global
+    .if feature(THREAD)
+        thread-groups 1
+    .endif
+        tune.bufsize 1024
+
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    listen li1
+        bind "fd@${li1}"
+        option httplog
+        option dontlog-normal
+        log "${S_addr}:${S_port}" local0 info
+        server s1 "${h1_li2_addr}:${h1_li2_port}"
+
+    listen li2
+        bind "fd@${li2}"
+        http-request return status 200
+} -start
+
+# 42 + 984 + 2 = 1028 bytes: too long for a 1024-byte buffer
+client c1 -connect ${h1_li1_sock} {
+    txreq -req POST -url /big -hdr "Transfer-Encoding: chunked" -nolen
+    send_n 984 "0"
+    send "\r\n"
+} -run
+
+# a valid request must still work
+client c2 -connect ${h1_li1_sock} {
+    txreq -url /ok
+    rxresp
+    expect resp.status == 200
+} -run
+
+syslog S -wait

diff --git a/src/h1_htx.c b/src/h1_htx.c
index cb4144883024ad38851c1ca7c765f917a6822880..1a602af960f2fd2eb5dc9519e4a3e65943fed8a8 100644 (file)
--- a/src/h1_htx.c
+++ b/src/h1_htx.c
@@ -852,7 +852,22 @@ size_t h1_parse_msg_data(struct h1m *h1m, struct htx **dsthtx,
        }
        else if (h1m->flags & H1_MF_CHNK) {
                /* te:chunked : parse chunks */
-               total += h1_parse_msg_chunks(h1m, dsthtx, srcbuf, ofs, max, htxbuf);
+               sz = h1_parse_msg_chunks(h1m, dsthtx, srcbuf, ofs, max, htxbuf);
+
+               /* Incomplete chunk-size line. If the input buffer only
+                * contains it and is full, which is detected by it being full
+                * and the offset to be zero, it's an error because the line is
+                * too large to be handled by the parser. Report a parsing
+                * error rather than retrying forever on the same data.
+                */
+               if (!sz && !ofs && h1m->state == H1_MSG_CHUNK_SIZE &&
+                   !buf_room_for_htx_data(srcbuf)) {
+                       (*dsthtx)->flags |= HTX_FL_PARSING_ERROR;
+                       h1m->err_state = h1m->state;
+                       h1m->err_pos = ofs;
+                       return 0;
+               }
+               total += sz;
        }
        else if (h1m->flags & H1_MF_XFER_LEN) {
                /* XFER_LEN is set but not CLEN nor CHNK, it means there is no