[#ERL-407]: Fix httpc misbehaviour based on RFC7230, section 3.3.3

If a message is received with both a Transfer-Encoding and a
Content-Length header field, it might indicate an attempt to
perform request smuggling or response splitting and must be
handled as an error in default mode (not relaxed mode).

Bug report: https://bugs.erlang.org/browse/ERL-407

2 files changed, 59 insertions, 5 deletions

diff --git a/lib/inets/src/http_client/httpc_response.erl b/lib/inets/src/http_client/httpc_response.erl
index b3b11b74ab..91638f5d2e 100644
--- a/lib/inets/src/http_client/httpc_response.erl
+++ b/lib/inets/src/http_client/httpc_response.erl
@@ -269,7 +269,7 @@ parse_headers(<<?LF,?LF,Body/binary>>, Header, Headers,
 		  MaxHeaderSize, Result, Relaxed);
 
 parse_headers(<<?CR,?LF,?CR,?LF,Body/binary>>, Header, Headers,
-	      MaxHeaderSize, Result, _) ->
+	      MaxHeaderSize, Result, Relaxed) ->
     HTTPHeaders = [lists:reverse(Header) | Headers],
     Length = lists:foldl(fun(H, Acc) -> length(H) + Acc end,
 			   0, HTTPHeaders),
@@ -277,8 +277,42 @@ parse_headers(<<?CR,?LF,?CR,?LF,Body/binary>>, Header, Headers,
  	true ->   
 	    ResponseHeaderRcord = 
 		http_response:headers(HTTPHeaders, #http_response_h{}),
-	    {ok, list_to_tuple(
-		   lists:reverse([Body, ResponseHeaderRcord | Result]))};
+
+            %% RFC7230, Section 3.3.3
+            %% If a message is received with both a Transfer-Encoding and a
+            %% Content-Length header field, the Transfer-Encoding overrides the
+            %% Content-Length. Such a message might indicate an attempt to
+            %% perform request smuggling (Section 9.5) or response splitting
+            %% (Section 9.4) and ought to be handled as an error. A sender MUST
+            %% remove the received Content-Length field prior to forwarding such
+            %% a message downstream.
+            case ResponseHeaderRcord#http_response_h.'transfer-encoding' of
+                undefined ->
+                    {ok, list_to_tuple(
+                           lists:reverse([Body, ResponseHeaderRcord | Result]))};
+                Value ->
+                    TransferEncoding = string:lowercase(Value),
+                    ContentLength = ResponseHeaderRcord#http_response_h.'content-length',
+                    if
+                        %% Respond without error but remove Content-Length field in relaxed mode
+                        (Relaxed =:= true)
+                        andalso (TransferEncoding =:= "chunked")
+                        andalso (ContentLength =/= "-1") ->
+                            ResponseHeaderRcordFixed =
+                                ResponseHeaderRcord#http_response_h{'content-length' = "-1"},
+                            {ok, list_to_tuple(
+                                   lists:reverse([Body, ResponseHeaderRcordFixed | Result]))};
+                        %% Respond with error in default (not relaxed) mode
+                        (Relaxed =:= false)
+                        andalso (TransferEncoding =:= "chunked")
+                        andalso (ContentLength =/= "-1") ->
+                            throw({error, {headers_conflict, {'content-length',
+                                                              'transfer-encoding'}}});
+                        true  ->
+                            {ok, list_to_tuple(
+                                   lists:reverse([Body, ResponseHeaderRcord | Result]))}
+                    end
+            end;
  	false ->
 	    throw({error, {header_too_long, MaxHeaderSize, 
 			   MaxHeaderSize-Length}})
diff --git a/lib/inets/test/httpc_SUITE.erl b/lib/inets/test/httpc_SUITE.erl
index a39e786c79..cc166d522e 100644
--- a/lib/inets/test/httpc_SUITE.erl
+++ b/lib/inets/test/httpc_SUITE.erl
@@ -115,6 +115,7 @@ only_simulated() ->
      invalid_chunk_size,
      headers_dummy,
      headers_with_obs_fold,
+     headers_conflict_chunked_with_length,
      empty_response_header,
      remote_socket_close,
      remote_socket_close_async,
@@ -978,7 +979,6 @@ headers_dummy(Config) when is_list(Config) ->
 		       {"If-Range", "Sat, 29 Oct 1994 19:43:31 GMT"},
 		       {"If-Match", "*"},
 		       {"Content-Type", "text/plain"},
-		       {"Content-Encoding", "chunked"},
 		       {"Content-Length", "6"},
 		       {"Content-Language", "en"},
 		       {"Content-Location", "http://www.foobar.se"},
@@ -1004,6 +1004,18 @@ headers_with_obs_fold(Config) when is_list(Config) ->
 
 %%-------------------------------------------------------------------------
 
+headers_conflict_chunked_with_length(doc) ->
+    ["Test the code for handling headers with both Transfer-Encoding"
+     "and Content-Length which must receive error in default (not relaxed) mode"
+     "and must receive successful response in relaxed mode"];
+headers_conflict_chunked_with_length(Config) when is_list(Config) ->
+    Request = {url(group_name(Config), "/headers_conflict_chunked_with_length.html", Config), []},
+    {error, {could_not_parse_as_http, _}} = httpc:request(get, Request, [{relaxed, false}], []),
+    {ok,{{_,200,_},_,_}} = httpc:request(get, Request, [{relaxed, true}], []),
+    ok.
+
+%%-------------------------------------------------------------------------
+
 invalid_headers(Config) ->
     Request  = {url(group_name(Config), "/dummy.html", Config), [{"cookie", undefined}]},
     {error, _} = httpc:request(get, Request, [], []).
@@ -1869,7 +1881,6 @@ handle_uri(_,"/dummy_headers.html",_,_,Socket,_) ->
     %% user to evaluate. This is not a valid response
     %% it only tests that the header handling code works.
     Head = "HTTP/1.1 200 ok\r\n" ++
-	"Content-Length:32\r\n" ++
 	"Pragma:1#no-cache\r\n"  ++
 	"Via:1.0 fred, 1.1 nowhere.com (Apache/1.1)\r\n"  ++
 	"Warning:1#pseudonym foobar\r\n"  ++
@@ -1899,6 +1910,15 @@ handle_uri(_,"/obs_folded_headers.html",_,_,_,_) ->
     " b\r\n\r\n"
     "Hello";
 
+handle_uri(_,"/headers_conflict_chunked_with_length.html",_,_,Socket,_) ->
+    Head =  "HTTP/1.1 200 ok\r\n"
+        "Content-Length:32\r\n"
+	"Transfer-Encoding:Chunked\r\n\r\n",
+    send(Socket, Head),
+    send(Socket, http_chunk:encode("<HTML><BODY>fo")),
+    send(Socket, http_chunk:encode("obar</BODY></HTML>")),
+    http_chunk:encode_last();
+
 handle_uri(_,"/capital_transfer_encoding.html",_,_,Socket,_) ->
     Head =  "HTTP/1.1 200 ok\r\n" ++
 	"Transfer-Encoding:Chunked\r\n\r\n",