Skip to content

Commit

Permalink
Add error handling for Content-Length header
Browse files Browse the repository at this point in the history 
Return HTTP 400 if Content-Length header is invalid. The Content-Length
header is defined to be an integer 0 or greater.
See RFC 9110 Section 8.6. Content-Length.

Return HTTP 411 if Content-Length is empty for POST, if
Transfer-Encoding is not set. If however Transfer-Encoding is used for
POST, Content-Length is ignored.
See RFC 9112 Section 6.1 Transfer-Encoding.

This improves the previous behaviour when Content-Length is not an
integer where Yaws immediately cut the connection.

It is not totally clearly defined what to do when Content-Length is
invalid, but responding with HTTP 400 Bad Request at least notifies the
user of their error.

One implementation detail to note is that all headers needs to be
collected in order to know if Transfer-Encoding is set, if it is set
Content-Lenght is ignored. However, if Transfer-Encoding is not set, the
normal error handling for Content-Length is performed. This means that
the report of multiple content-length headers error is deferred to when
all headers are collected..
  • Loading branch information
@avtobiff @vinoski
avtobiff authored and vinoski committed Jul 22, 2023
1 parent cee0036 commit 0556128
Show file tree
Hide file tree
Showing 3 changed files with 199 additions and 6 deletions.
88 changes: 83 additions & 5 deletions src/yaws.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2669,7 +2669,38 @@ do_http_get_headers(CliSock, SSL) ->
{error, _}=Error ->
Error;
H ->
{R, H}
%% According to RFC 9112 Section 6.1 Transfer-Encoding we
%% select to process POST requests according to
%% Transfer-Encoding only while ignoring Content-Length if
%% both are supplied. It is also possible to reject these
%% requests, but that would probably introduce backward
%% incompatible behavior.
%%
%% If Transfer-Encoding is not set, we assert a valid
%% Content-Length header. Note that http_collect_headers/5
%% might have deferred the multiple_content_length_headers
%% error to here, since we need to collect all headers
%% before we can know if transfer encoding is used.

TE = yaws_api:headers_transfer_encoding(H),
CL = yaws_api:headers_content_length(H),

case {TE, assert_content_length(CL)} of
{TE, _} when is_list(TE) ->
%% Ignore Content-Length, see above.
{R, H#headers{content_length = undefined}};
{_, {error, _} = Err} ->
%% No transfer encoding so we return the deferred
%% error frow http_collect_headers/5.
Err;
{_, false} ->
{error, {invalid_content_length,
R#http_request{method = bad_request}}};
{_, true} ->
%% No transfer encoding and valid content length,
%% proceed as normal.
{R, H}
end
end
end.

Expand Down Expand Up @@ -2764,14 +2795,28 @@ http_collect_headers(CliSock, Req, H, SSL, Count) when Count < 1000 ->
case H#headers.content_length of
undefined ->
http_collect_headers(CliSock, Req,
H#headers{content_length = X},SSL,
Count+1);
H#headers{content_length = X},
SSL, Count+1);
X ->
%% duplicate header, ignore it
http_collect_headers(CliSock, Req, H, SSL, Count+1);


%% Defer multiple content-length header error until we know if
%% transfer-encoding is used. If transfer-encoding is used we
%% ignore content-length, see RFC 9112 Section 6.1.
%%
%% We store the error in #headers.content_length and when
%% return it if transfer-encoding is not used.
{error, _} ->
%% stored content-length error, continue
http_collect_headers(CliSock, Req, H, SSL, Count+1);
_ ->
{error, {multiple_content_length_headers,
Req#http_request{method=bad_request}}}
CLErr = {error, {multiple_content_length_headers,
Req#http_request{method=bad_request}}},
http_collect_headers(CliSock, Req,
H#headers{content_length = CLErr},
SSL, Count+1)
end;
{ok, {http_header, _Num, 'Content-Type', _, X}} ->
http_collect_headers(CliSock, Req,
Expand Down Expand Up @@ -2842,6 +2887,39 @@ http_collect_headers(_CliSock, Req, _H, _SSL, _Count) ->
{error, {too_many_headers, Req}}.


%% @doc Assert valid Content-Length header value.
%%
%% Must be empty or non negative integer.
%%
%% If HTTP method is POST Content-Length MUST be set, if transfer-encoding
%% is not chunked.
%%
%% Note that http_collect_headers/5 can have deferred the
%% multiple_content_length_values error because all headers need to be
%% collected before it can be known if transfer encoding is used.
%% See do_http_get_headers/2.
-spec assert_content_length(string() | Error) -> boolean() | Error when
Error :: {error, {atom(), #http_request{}}}.
assert_content_length(CL)
when CL == undefined orelse CL == "" ->
true;
assert_content_length({error, {multiple_content_length_headers,
#http_request{method = bad_request}}} = Err) ->
Err;
assert_content_length(X) ->
try
X2 = erlang:list_to_integer(X),
true = (0 =< X2)
catch
error:badarg ->
%% not a number
false;
error:{badmatch, false} ->
%% not non neg integer
false
end.


%% @doc Filter out empty accept headers from list.
-spec split_accept_hdrs([string()]) -> [string()].
split_accept_hdrs([]) ->
Expand Down
18 changes: 18 additions & 0 deletions src/yaws_server.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1202,6 +1202,17 @@ aloop(CliSock, {IP,Port}=IPPort, GS, Num) ->
deliver_431(CliSock, ReqTooMany, NoArg)
end,
{ok, Num+1};
{error, {invalid_content_length, ReqMultiCL}} ->
?Debug("Invalid Content-Length header content~n", []),
case pick_sconf(GS#gs.gconf, #headers{}, GS#gs.group) of
undefined ->
deliver_400(CliSock, ReqMultiCL, NoArg);
SC ->
put(sc, SC),
put(outh, #outh{}),
deliver_400(CliSock, ReqMultiCL, NoArg)
end,
{ok, Num+1};
{error, {multiple_content_length_headers, ReqMultiCL}} ->
%% RFC 7230 status code 400
?Debug("Multiple Content-Length headers~n", []),
Expand Down Expand Up @@ -1764,6 +1775,8 @@ body_method(CliSock, IPPort, Req, Head) ->
end;
{_, undefined} ->
<<>>;
{_, ""} ->
{error, yaws_api:code_to_phrase(411), fun deliver_411/3};
{_, Len} ->
Int_len = strip_list_to_integer(Len),
if
Expand Down Expand Up @@ -2796,6 +2809,9 @@ deliver_405(CliSock, Req, Arg, Methods) ->
"</p>"]),
deliver_xxx(CliSock, Req, Arg, 405, Methods_msg).

deliver_411(CliSock, Req, Arg) ->
deliver_xxx(CliSock, Req, Arg, 411). % Length Required

deliver_416(CliSock, _Req, Arg, Tot) ->
B = ["<html><h1>416 ", yaws_api:code_to_phrase(416), "</h1></html>"],
Sz = iolist_size(B),
Expand Down Expand Up @@ -4922,6 +4938,8 @@ flush(Sock, Pos, Sz, "chunked") ->
end;
flush(_Sock, Pos, undefined, _) ->
Pos;
flush(_Sock, Pos, "", _) ->
Pos;
flush(Sock, Pos, Sz, TE) when is_list(Sz) ->
flush(Sock, Pos, strip_list_to_integer(Sz), TE);
flush(Sock, Pos, Sz, _) ->
Expand Down
99 changes: 98 additions & 1 deletion testsuite/main_SUITE.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ all() ->
post_multi_same_content_length,
post_bogus_transfer_encoding,
post_bad_order_transfer_encoding,
post_content_length_transfer_encoding,
flush_small_get,
flush_large_get,
flush_chunked_get,
Expand Down Expand Up @@ -54,6 +55,7 @@ all() ->
exhtml,
accept_ranges,
status_and_content_length,
content_length_values,
encoded_url,
encoded_url_no_docroot_escape,
header_order,
Expand Down Expand Up @@ -388,6 +390,9 @@ post_multi_same_content_length(Config) ->
ok.

post_transfer_encoding(Config, TE, StatusCode) ->
post_transfer_encoding(Config, TE, "", StatusCode).

post_transfer_encoding(Config, TE, CL, StatusCode) ->
Port = testsuite:get_yaws_port(1, Config),
Url = testsuite:make_url(http, "127.0.0.1", Port, "/posttest/chunked/5"),
{ok, {Scheme, _, Host, Port, Path, _}} = yaws_dynopts:http_uri_parse(Url),
Expand All @@ -398,10 +403,12 @@ post_transfer_encoding(Config, TE, StatusCode) ->
"Host: " ++ Host ++ "\r\n",
"User-Agent: Yaws HTTP client\r\n",
"Transfer-Encoding: " ++ TE ++ "\r\n",
CL, %% Content-Length
"\r\n",
"5\r\n",
"hello\r\n",
"0\r\n"],
"0\r\n",
"\r\n"],
ok = gen_tcp:send(Sock, Req),
?assertMatch({ok,{{_,StatusCode,_},_,_}}, testsuite:receive_http_response(Sock))
after
Expand All @@ -426,6 +433,18 @@ post_bad_order_transfer_encoding(Config) ->
%% 400 (Bad Request) status code and then close the connection."
post_transfer_encoding(Config, "chunked, gzip", 400).

post_content_length_transfer_encoding(Config) ->
%% RFC 9112 Section 6.1 Transfer-Encoding "A server MAY reject
%% a request that contains both Content-Length and Transfer-Encoding
%% or process such a request in accordance with the Transfer-Encoding
%% alone. Regardless, the server MUST close the connection after
%% responding to such a request to avoid the potential attacks."
post_transfer_encoding(Config, "chunked", "Content-Length: bogus\r\n", 200),
post_transfer_encoding(Config, "chunked",
"Content-Length: bogus\r\n"
"Content-Length: bogus2\r\n",
200).

flush_small_get(Config) ->
File = filename:join(?tempdir(?MODULE), "www/1000.txt"),
{ok, FI} = file:read_file_info(File),
Expand Down Expand Up @@ -924,6 +943,84 @@ status_and_content_length(Config) ->
?assertNot(proplists:is_defined("content-length", Hdrs4)),
ok.

content_length_values(Config) ->
Port = testsuite:get_yaws_port(1, Config),
HostHdr = {"Host", "127.0.0.1:"++integer_to_list(Port)},

%% Positive tests

CLZeroHdr = {"Content-Length", "0"},
CLEmptyHdr1 = {"Content-Length", ""},
CLEmptyHdr2 = {"Content-Length", " "},

%% GET Content-Length: 0
{ok, Sock1} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock1, {get, "/", "HTTP/1.1"},
[HostHdr, CLZeroHdr])),
{ok, {{_,200,_}, _}} = testsuite:receive_http_headers(Sock1),
?assertEqual(ok, gen_tcp:close(Sock1)),

%% GET "Content-Length: "
{ok, Sock2} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock2, {get, "/", "HTTP/1.1"},
[HostHdr, CLEmptyHdr1])),
{ok, {{_,200,_}, _}} = testsuite:receive_http_headers(Sock2),
?assertEqual(ok, gen_tcp:close(Sock2)),

%% GET "Content-Length: "
{ok, Sock3} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock3, {get, "/", "HTTP/1.1"},
[HostHdr, CLEmptyHdr2])),
{ok, {{_,200,_}, _}} = testsuite:receive_http_headers(Sock3),
?assertEqual(ok, gen_tcp:close(Sock3)),

%% Negative tests

%% "Content-Length: "
%% "Content-Length: "
CLNegHdr = {"Content-Length", "-1"},
CLNanHdr = {"Content-Length", "abc"},

%% POST "Content-Length: "
{ok, Sock4} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock4, {post, "/", "HTTP/1.1"},
[HostHdr, CLEmptyHdr1])),
{ok, {{_,411,_}, _}} = testsuite:receive_http_headers(Sock4),
?assertEqual(ok, gen_tcp:close(Sock4)),

%% POST "Content-Length: "
{ok, Sock5} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock5, {post, "/", "HTTP/1.1"},
[HostHdr, CLEmptyHdr2])),
{ok, {{_,411,_}, _}} = testsuite:receive_http_headers(Sock5),
?assertEqual(ok, gen_tcp:close(Sock5)),

%% Content-Length: -1
{ok, Sock6} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock6, {get, "/", "HTTP/1.1"},
[HostHdr, CLNegHdr])),
{ok, {{_,400,_}, _}} = testsuite:receive_http_headers(Sock6),
?assertEqual(ok, gen_tcp:close(Sock6)),
{ok, Sock7} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock7, {post, "/", "HTTP/1.1"},
[HostHdr, CLNegHdr])),
{ok, {{_,400,_}, _}} = testsuite:receive_http_headers(Sock7),
?assertEqual(ok, gen_tcp:close(Sock7)),

%% Content-Length: abc
{ok, Sock8} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock8, {get, "/", "HTTP/1.1"},
[HostHdr, CLNanHdr])),
{ok, {{_,400,_}, _}} = testsuite:receive_http_headers(Sock8),
?assertEqual(ok, gen_tcp:close(Sock8)),
{ok, Sock9} = gen_tcp:connect("127.0.0.1", Port, [binary, {active, false}]),
?assertEqual(ok, testsuite:send_http_headers(Sock9, {post, "/", "HTTP/1.1"},
[HostHdr, CLNanHdr])),
{ok, {{_,400,_}, _}} = testsuite:receive_http_headers(Sock9),
?assertEqual(ok, gen_tcp:close(Sock9)),

ok.

encoded_url(Config) ->
PathInfo = "/%2F/HA",
UrlPath = "/policies" ++ PathInfo,
Expand Down

0 comments on commit 0556128

Please sign in to comment.