I found it and fixed it!
It was me again who added a bug in 1.7 with the optimizations for large
headers and large requests. If certain conditions are met, we could read
the \r from previous data (as we guessed) and complain that the next byte
was not an LF once the remaining part arrived.
I'm intending to merge the attached patch. "cas", I'm willing to add you
as the reporter here since you provided lots of very valuable information,
but for this it would be nice if you had a name :-)
I'll backport it to 1.7. Unfortunately there's no easy workaround on an
existing configuration, so I'll produce 1.7.2 soon I guess. I'll try to
add the remaining missing information to make dumps more accurate regarding
the expected state (this would definitely had helped here).
Cheers,
Willy
>From 5afd6967423caa2d55d4c5a5ae0e296b35e2292d Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Wed, 4 Jan 2017 14:44:46 +0100
Subject: BUG/MAJOR: http: fix risk of getting invalid reports of bad requests
X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4
Commits 5f10ea3 ("OPTIM: http: improve parsing performance of long URIs")
and 0431f9d ("OPTIM: http: improve parsing performance of long header lines")
introduced a bug in the HTTP parser : when a partial request is read, the
first part ends up on a 8-bytes boundary (or 4-byte on 32-bit machines), the
end lies in the header field value part, and the buffer used to contain a CR
character exactly after the last block, then the parser could be confused and
read this CR character as being part of the current request, then switch to a
new state waiting for an LF character. Then when the next part of the request
appeared, it would read the character following what was erroneously mistaken
for a CR, see that it is not an LF and fail on a bad request. The reason is
that there's no control of and of parsing just after breaking out of the loop.
One way to reproduce it is with this config :
global
stats socket /tmp/sock1 mode 666 level admin
stats timeout 1d
frontend px
bind :8001
mode http
timeout client 10s
redirect location /
And sending requests this way :
$ tcploop 8001 C P S:"$(dd if=/dev/zero bs=16384 count=1 2>/dev/null | tr
'\000' '\r')"
$ tcploop 8001 C P S:"$(dd if=/dev/zero bs=16384 count=1 2>/dev/null | tr
'\000' '\r')"
$ tcploop 8001 C P \
S:"GET / HTTP/1.0\r\nX-padding:
0123456789.123456789.123456789.123456789.123456789.123456789.1234567" P \
S:"89.123456789\r\n\r\n" P
Then a "show errors" on the socket will report :
$ echo "show errors" | socat - /tmp/sock1
Total events captured on [04/Jan/2017:15:09:15.755] : 32
[04/Jan/2017:15:09:13.050] frontend px (#2): invalid request
backend <NONE> (#-1), server <NONE> (#-1), event #31
src 127.0.0.1:59716, session #91, session flags 0x00000080
HTTP msg state 17, msg flags 0x00000000, tx flags 0x00000000
HTTP chunk len 0 bytes, HTTP body len 0 bytes
buffer flags 0x00808002, out 0 bytes, total 111 bytes
pending 111 bytes, wrapping at 16384, error at position 107:
00000 GET / HTTP/1.0\r\n
00016 X-padding:
0123456789.123456789.123456789.123456789.123456789.12345678
00086+ 9.123456789.123456789\r\n
00109 \r\n
This fix must be backported to 1.7.
---
src/proto_http.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/proto_http.c b/src/proto_http.c
index aa8d997..d894ef6 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -1557,6 +1557,10 @@ const char *http_parse_reqline(struct http_msg *msg,
ptr += sizeof(int);
}
#endif
+ if (ptr >= end) {
+ state = HTTP_MSG_RQURI;
+ goto http_msg_ood;
+ }
http_msg_rquri2:
if (likely((unsigned char)(*ptr - 33) <= 93)) /* 33 to 126
included */
EAT_AND_JUMP_OR_RETURN(http_msg_rquri2, HTTP_MSG_RQURI);
@@ -1983,6 +1987,10 @@ void http_msg_analyzer(struct http_msg *msg, struct
hdr_idx *idx)
ptr += sizeof(int);
}
#endif
+ if (ptr >= end) {
+ state = HTTP_MSG_HDR_VAL;
+ goto http_msg_ood;
+ }
http_msg_hdr_val2:
if (likely(!HTTP_IS_CRLF(*ptr)))
EAT_AND_JUMP_OR_RETURN(http_msg_hdr_val2,
HTTP_MSG_HDR_VAL);
--
1.7.12.1
