From 5984409d1264871cbe61bfec875de53e51713efb Mon Sep 17 00:00:00 2001
From: Steve Vinoski <vinoski@ieee.org>
Date: Thu, 24 Nov 2011 21:03:33 -0500
Subject: honor packet_size for http packet parsing to fix OTP-9389
Allow applications to use a packet_size setting on a socket to control
acceptable HTTP header line length. This gives them the ability to
accept HTTP headers larger than the default settings allow, but also
lets them avoid DOS attacks by accepting header lines only up to
whatever length they wish to allow.
Without this change, if an HTTP request/response line or header
arrives on a socket in http, http_bin, httph, or httph_bin parsing
mode, and the request/response line or header is too long to fit into
a default inet_drv buffer of 1460 bytes, an unexpected error
occurs. These problems were described and discussed on
erlang-questions in June 2011 in this thread:
http://erlang.org/pipermail/erlang-questions/2011-June/059563.html
In the original code, no buffer reallocation occurs to enlarge the
buffer, even if packet_size or line_length are set in a way that
should allow the HTTP data to be parsed properly. The only available
workaround was to collect headers and parse them using
erlang:decode_packet, but that approach has drawbacks such as having
to collect all HTTP header data before it can be handed to
decode_packet for correct parsing, and also requiring each and every
Erlang web server developer/maintainer to add the workaround to his or
her web server.
Change the packet parser to honor the packet_size setting for HTTP
parsing. If packet_size is set, and an HTTP request/response or header
line exceeds the default 1460 byte TCP buffer limit, return an
indication to tcp_remain that it should realloc the buffer to enlarge
it to packet_size. Also fix the HTTP parsing code to properly honor
line_length by truncating any HTTP request/response or header lines
that exceed that setting.
For backward compatibility, default behavior is unchanged; if an
application wants to be able to accept long HTTP header lines, it must
set packet_size to an appropriate value. Buffer reallocation occurs
only when needed, so the original default buffer size in the code is
still the default.
Make the line mode parsing honor packet_size as well, for consistency.
Add new regression tests to the emulator decode_packet suite and also
to the kernel gen_tcp_misc suite.
The documentation for packet_size in inet:setopts/2 is already
sufficient.
Many thanks to Sverker Eriksson for his guidance on how to best fix
this bug and also for reviewing a number of patch attempts prior to
this one.
---
erts/emulator/beam/packet_parser.c | 43 +++++++++++++++++----
erts/emulator/drivers/common/inet_drv.c | 11 +++++-
erts/emulator/test/decode_packet_SUITE.erl | 62 +++++++++++++++++++++++++++++-
3 files changed, 105 insertions(+), 11 deletions(-)
(limited to 'erts')
diff --git a/erts/emulator/beam/packet_parser.c b/erts/emulator/beam/packet_parser.c
index a66d60aa22..8c1662dc6f 100644
--- a/erts/emulator/beam/packet_parser.c
+++ b/erts/emulator/beam/packet_parser.c
@@ -301,7 +301,11 @@ int packet_get_length(enum PacketParseType htype,
/* TCP_PB_LINE_LF: [Data ... \n] */
const char* ptr2;
if ((ptr2 = memchr(ptr, '\n', n)) == NULL) {
- if (n >= trunc_len && trunc_len!=0) { /* buffer full */
+ if (n > max_plen && max_plen != 0) { /* packet full */
+ DEBUGF((" => packet full (no NL)=%d\r\n", n));
+ goto error;
+ }
+ else if (n >= trunc_len && trunc_len!=0) { /* buffer full */
DEBUGF((" => line buffer full (no NL)=%d\r\n", n));
return trunc_len;
}
@@ -309,6 +313,10 @@ int packet_get_length(enum PacketParseType htype,
}
else {
int len = (ptr2 - ptr) + 1; /* including newline */
+ if (len > max_plen && max_plen!=0) {
+ DEBUGF((" => packet_size %d exceeded\r\n", max_plen));
+ goto error;
+ }
if (len > trunc_len && trunc_len!=0) {
DEBUGF((" => truncated line=%d\r\n", trunc_len));
return trunc_len;
@@ -401,7 +409,11 @@ int packet_get_length(enum PacketParseType htype,
const char* ptr2 = memchr(ptr1, '\n', len);
if (ptr2 == NULL) {
- if (n >= trunc_len && trunc_len!=0) { /* buffer full */
+ if (max_plen != 0) {
+ if (n > max_plen) /* packet full */
+ goto error;
+ }
+ else if (n >= trunc_len && trunc_len!=0) { /* buffer full */
plen = trunc_len;
goto done;
}
@@ -409,21 +421,38 @@ int packet_get_length(enum PacketParseType htype,
}
else {
plen = (ptr2 - ptr) + 1;
-
- if (*statep == 0)
+
+ if (*statep == 0) {
+ if (max_plen != 0 && plen > max_plen)
+ goto error;
+ if (plen >= trunc_len && trunc_len != 0)
+ plen = trunc_len;
goto done;
-
+ }
+
if (plen < n) {
if (SP(ptr2+1) && plen>2) {
/* header field value continue on next line */
ptr1 = ptr2+1;
len = n - plen;
}
- else
+ else {
+ if (max_plen != 0 && plen > max_plen)
+ goto error;
+ if (plen >= trunc_len && trunc_len != 0)
+ plen = trunc_len;
goto done;
+ }
}
- else
+ else {
+ if (max_plen != 0 && plen > max_plen)
+ goto error;
+ if (plen >= trunc_len && trunc_len != 0) {
+ plen = trunc_len;
+ goto done;
+ }
goto more;
+ }
}
}
}
diff --git a/erts/emulator/drivers/common/inet_drv.c b/erts/emulator/drivers/common/inet_drv.c
index e0d869f328..45089dcc2f 100644
--- a/erts/emulator/drivers/common/inet_drv.c
+++ b/erts/emulator/drivers/common/inet_drv.c
@@ -8729,8 +8729,15 @@ static int tcp_remain(tcp_descriptor* desc, int* len)
else if (tlen == 0) { /* need unknown more */
*len = 0;
if (nsz == 0) {
- if (nfill == n)
- goto error;
+ if (nfill == n) {
+ if (desc->inet.psize != 0 && desc->inet.psize > nfill) {
+ if (tcp_expand_buffer(desc, desc->inet.psize) < 0)
+ return -1;
+ return desc->inet.psize;
+ }
+ else
+ goto error;
+ }
DEBUGF((" => restart more=%d\r\n", nfill - n));
return nfill - n;
}
diff --git a/erts/emulator/test/decode_packet_SUITE.erl b/erts/emulator/test/decode_packet_SUITE.erl
index c0499554eb..55ef05079f 100644
--- a/erts/emulator/test/decode_packet_SUITE.erl
+++ b/erts/emulator/test/decode_packet_SUITE.erl
@@ -26,12 +26,14 @@
-export([all/0, suite/0,groups/0,init_per_suite/1, end_per_suite/1,
init_per_group/2,end_per_group/2,
init_per_testcase/2,end_per_testcase/2,
- basic/1, packet_size/1, neg/1, http/1, line/1, ssl/1, otp_8536/1]).
+ basic/1, packet_size/1, neg/1, http/1, line/1, ssl/1, otp_8536/1,
+ otp_9389/1, otp_9389_line/1]).
suite() -> [{ct_hooks,[ts_install_cth]}].
all() ->
- [basic, packet_size, neg, http, line, ssl, otp_8536].
+ [basic, packet_size, neg, http, line, ssl, otp_8536,
+ otp_9389, otp_9389_line].
groups() ->
[].
@@ -251,6 +253,28 @@ packet_size(Config) when is_list(Config) ->
?line {error,_} = decode_pkt(4,<<Size:32,Packet/binary>>)
end,
lists:seq(-10,-1)),
+
+ %% Test OTP-9389, long HTTP header lines.
+ Opts = [{packet_size, 128}],
+ Pkt = list_to_binary(["GET / HTTP/1.1\r\nHost: localhost\r\nLink: /",
+ string:chars($Y, 64), "\r\n\r\n"]),
+ <<Pkt1:50/binary, Pkt2/binary>> = Pkt,
+ ?line {ok, {http_request,'GET',{abs_path,"/"},{1,1}}, Rest1} =
+ erlang:decode_packet(http, Pkt1, Opts),
+ ?line {ok, {http_header,_,'Host',_,"localhost"}, Rest2} =
+ erlang:decode_packet(httph, Rest1, Opts),
+ ?line {more, undefined} = erlang:decode_packet(httph, Rest2, Opts),
+ ?line {ok, {http_header,_,"Link",_,_}, _} =
+ erlang:decode_packet(httph, list_to_binary([Rest2, Pkt2]), Opts),
+
+ Pkt3 = list_to_binary(["GET / HTTP/1.1\r\nHost: localhost\r\nLink: /",
+ string:chars($Y, 129), "\r\n\r\n"]),
+ ?line {ok, {http_request,'GET',{abs_path,"/"},{1,1}}, Rest3} =
+ erlang:decode_packet(http, Pkt3, Opts),
+ ?line {ok, {http_header,_,'Host',_,"localhost"}, Rest4} =
+ erlang:decode_packet(httph, Rest3, Opts),
+ ?line {error, invalid} = erlang:decode_packet(httph, Rest4, Opts),
+
ok.
@@ -557,3 +581,37 @@ decode_pkt(Type,Bin,Opts) ->
%%io:format(" -> ~p\n",[Res]),
Res.
+otp_9389(doc) -> ["Verify line_length works correctly for HTTP headers"];
+otp_9389(suite) -> [];
+otp_9389(Config) when is_list(Config) ->
+ Opts = [{packet_size, 16384}, {line_length, 3000}],
+ Pkt = list_to_binary(["GET / HTTP/1.1\r\nHost: localhost\r\nLink: /",
+ string:chars($X, 8192),
+ "\r\nContent-Length: 0\r\n\r\n"]),
+ <<Pkt1:5000/binary, Pkt2/binary>> = Pkt,
+ {ok, {http_request,'GET',{abs_path,"/"},{1,1}}, Rest1} =
+ erlang:decode_packet(http, Pkt1, Opts),
+ {ok, {http_header,_,'Host',_,"localhost"}, Rest2} =
+ erlang:decode_packet(httph, Rest1, Opts),
+ {more, undefined} = erlang:decode_packet(httph, Rest2, Opts),
+ {ok, {http_header,_,"Link",_,Link}, Rest3} =
+ erlang:decode_packet(httph, list_to_binary([Rest2, Pkt2]), Opts),
+ true = (length(Link) =< 3000),
+ {ok, {http_error, _}, Rest4} = erlang:decode_packet(httph, Rest3, Opts),
+ {ok, {http_error, _}, Rest5} = erlang:decode_packet(httph, Rest4, Opts),
+ {ok, {http_header,_,'Content-Length',_,"0"}, <<"\r\n">>} =
+ erlang:decode_packet(httph, Rest5, Opts),
+ ok.
+
+otp_9389_line(doc) -> ["Verify packet_size works correctly for line mode"];
+otp_9389_line(suite) -> [];
+otp_9389_line(Config) when is_list(Config) ->
+ Opts = [{packet_size, 20}],
+ Line1 = <<"0123456789012345678\n">>,
+ Line2 = <<"0123456789\n">>,
+ Line3 = <<"01234567890123456789\n">>,
+ Pkt = list_to_binary([Line1, Line2, Line3]),
+ ?line {ok, Line1, Rest1} = erlang:decode_packet(line, Pkt, Opts),
+ ?line {ok, Line2, Rest2} = erlang:decode_packet(line, Rest1, Opts),
+ ?line {error, invalid} = erlang:decode_packet(line, Rest2, Opts),
+ ok.
--
cgit v1.2.3
From dc5f7190f16cf4552db74fba3f4e0f2d654e2594 Mon Sep 17 00:00:00 2001
From: Sverker Eriksson <sverker@erlang.org>
Date: Mon, 28 Nov 2011 15:14:14 +0100
Subject: erts: Remove truncation of http packet parsing and return error
instead
This is a slight modification of previous commit by Steve Vinoski
For backward compatibility of old users of decode_packet, I think it's enough
to return error instead of keeping the old line truncation behaviour.
---
erts/doc/src/erlang.xml | 9 ++++++---
erts/emulator/beam/packet_parser.c | 22 +++++++++-------------
erts/emulator/test/decode_packet_SUITE.erl | 6 ++----
3 files changed, 17 insertions(+), 20 deletions(-)
(limited to 'erts')
diff --git a/erts/doc/src/erlang.xml b/erts/doc/src/erlang.xml
index 42a4e6a999..1ea88a185f 100644
--- a/erts/doc/src/erlang.xml
+++ b/erts/doc/src/erlang.xml
@@ -724,9 +724,12 @@ false</pre>
size limit.</p>
</item>
<tag><c>{line_length, integer()}</c></tag>
- <item><p>Applies only to line oriented protocols
- (<c>line</c>, <c>http</c>). Lines longer than this
- will be truncated.</p>
+ <item><p>For packet type <c>line</c>, truncate lines longer
+ than the indicated length.</p>
+ <p>Option <c>line_length</c> also applies to <c>http*</c>
+ packet types as an alias for option <c>packet_size</c> in the
+ case when <c>packet_size</c> itself is not set. This usage is
+ only intended for backward compatibility.</p>
</item>
</taglist>
<pre>
diff --git a/erts/emulator/beam/packet_parser.c b/erts/emulator/beam/packet_parser.c
index 8c1662dc6f..4d4b6ea196 100644
--- a/erts/emulator/beam/packet_parser.c
+++ b/erts/emulator/beam/packet_parser.c
@@ -405,18 +405,22 @@ int packet_get_length(enum PacketParseType htype,
const char* ptr1 = ptr;
int len = plen;
+ if (!max_plen) {
+ /* This is for backward compatibility with old user of decode_packet
+ * that might use option 'line_length' to limit accepted length of
+ * http lines.
+ */
+ max_plen = trunc_len;
+ }
+
while (1) {
const char* ptr2 = memchr(ptr1, '\n', len);
if (ptr2 == NULL) {
if (max_plen != 0) {
- if (n > max_plen) /* packet full */
+ if (n >= max_plen) /* packet full */
goto error;
}
- else if (n >= trunc_len && trunc_len!=0) { /* buffer full */
- plen = trunc_len;
- goto done;
- }
goto more;
}
else {
@@ -425,8 +429,6 @@ int packet_get_length(enum PacketParseType htype,
if (*statep == 0) {
if (max_plen != 0 && plen > max_plen)
goto error;
- if (plen >= trunc_len && trunc_len != 0)
- plen = trunc_len;
goto done;
}
@@ -439,18 +441,12 @@ int packet_get_length(enum PacketParseType htype,
else {
if (max_plen != 0 && plen > max_plen)
goto error;
- if (plen >= trunc_len && trunc_len != 0)
- plen = trunc_len;
goto done;
}
}
else {
if (max_plen != 0 && plen > max_plen)
goto error;
- if (plen >= trunc_len && trunc_len != 0) {
- plen = trunc_len;
- goto done;
- }
goto more;
}
}
diff --git a/erts/emulator/test/decode_packet_SUITE.erl b/erts/emulator/test/decode_packet_SUITE.erl
index 55ef05079f..4acbe8c6e0 100644
--- a/erts/emulator/test/decode_packet_SUITE.erl
+++ b/erts/emulator/test/decode_packet_SUITE.erl
@@ -596,11 +596,9 @@ otp_9389(Config) when is_list(Config) ->
{more, undefined} = erlang:decode_packet(httph, Rest2, Opts),
{ok, {http_header,_,"Link",_,Link}, Rest3} =
erlang:decode_packet(httph, list_to_binary([Rest2, Pkt2]), Opts),
- true = (length(Link) =< 3000),
- {ok, {http_error, _}, Rest4} = erlang:decode_packet(httph, Rest3, Opts),
- {ok, {http_error, _}, Rest5} = erlang:decode_packet(httph, Rest4, Opts),
+ true = (length(Link) > 8000),
{ok, {http_header,_,'Content-Length',_,"0"}, <<"\r\n">>} =
- erlang:decode_packet(httph, Rest5, Opts),
+ erlang:decode_packet(httph, Rest3, Opts),
ok.
otp_9389_line(doc) -> ["Verify packet_size works correctly for line mode"];
--
cgit v1.2.3