From 6460e9d2d270cffe2074053af224af7ce94ce098 Mon Sep 17 00:00:00 2001
From: Jeffrey Griffin <geogriffin@fastmail.com>
Date: Tue, 16 May 2017 14:50:01 -0700
Subject: Fix infinite loop on incomplete multipart body

I have amended a lot of changes from the original commit
to make it behave as expected, including returning a 400
error. LH
---
 src/cowboy_req.erl | 26 +++++++++++++++++---------
 test/req_SUITE.erl | 31 +++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/src/cowboy_req.erl b/src/cowboy_req.erl
index 40aa652..43f7a44 100644
--- a/src/cowboy_req.erl
+++ b/src/cowboy_req.erl
@@ -506,7 +506,7 @@ read_part(Req) ->
 read_part(Req, Opts) ->
 	case maps:is_key(multipart, Req) of
 		true ->
-			{Data, Req2} = stream_multipart(Req, Opts),
+			{Data, Req2} = stream_multipart(Req, Opts, headers),
 			read_part(Data, Opts, Req2);
 		false ->
 			read_part(init_multipart(Req), Opts)
@@ -515,10 +515,10 @@ read_part(Req, Opts) ->
 read_part(Buffer, Opts, Req=#{multipart := {Boundary, _}}) ->
 	try cow_multipart:parse_headers(Buffer, Boundary) of
 		more ->
-			{Data, Req2} = stream_multipart(Req, Opts),
+			{Data, Req2} = stream_multipart(Req, Opts, headers),
 			read_part(<< Buffer/binary, Data/binary >>, Opts, Req2);
 		{more, Buffer2} ->
-			{Data, Req2} = stream_multipart(Req, Opts),
+			{Data, Req2} = stream_multipart(Req, Opts, headers),
 			read_part(<< Buffer2/binary, Data/binary >>, Opts, Req2);
 		{ok, Headers0, Rest} ->
 			Headers = maps:from_list(Headers0),
@@ -557,7 +557,7 @@ read_part_body(Buffer, Opts, Req=#{multipart := {Boundary, _}}, Acc) ->
 		true ->
 			{more, Acc, Req#{multipart => {Boundary, Buffer}}};
 		false ->
-			{Data, Req2} = stream_multipart(Req, Opts),
+			{Data, Req2} = stream_multipart(Req, Opts, body),
 			case cow_multipart:parse_body(<< Buffer/binary, Data/binary >>, Boundary) of
 				{ok, Body} ->
 					read_part_body(<<>>, Opts, Req2, << Acc/binary, Body/binary >>);
@@ -583,12 +583,20 @@ init_multipart(Req) ->
 				'Missing boundary parameter for multipart media type.'})
 	end.
 
-stream_multipart(Req=#{multipart := done}, _) ->
+stream_multipart(Req=#{multipart := done}, _, _) ->
 	{<<>>, Req};
-stream_multipart(Req=#{multipart := {_, <<>>}}, Opts) ->
-	{_, Data, Req2} = read_body(Req, Opts),
-	{Data, Req2};
-stream_multipart(Req=#{multipart := {Boundary, Buffer}}, _) ->
+stream_multipart(Req=#{multipart := {_, <<>>}}, Opts, Type) ->
+	case read_body(Req, Opts) of
+		{more, Data, Req2} ->
+			{Data, Req2};
+		%% We crash when the data ends unexpectedly.
+		{ok, <<>>, _} ->
+			exit({request_error, {multipart, Type},
+				'Malformed body; multipart expected.'});
+		{ok, Data, Req2} ->
+			{Data, Req2}
+	end;
+stream_multipart(Req=#{multipart := {Boundary, Buffer}}, _, _) ->
 	{Buffer, Req#{multipart => {Boundary, <<>>}}}.
 
 %% Response.
diff --git a/test/req_SUITE.erl b/test/req_SUITE.erl
index 66d42c9..3b32b3a 100644
--- a/test/req_SUITE.erl
+++ b/test/req_SUITE.erl
@@ -475,6 +475,26 @@ do_multipart(Path, Config) ->
 	} = LargeHeaders,
 	ok.
 
+multipart_error_empty(Config) ->
+	doc("Multipart request body is empty."),
+	%% We use an empty list as a body to make sure Gun knows
+	%% we want to send an empty body.
+	%% @todo This is a terrible hack. Improve Gun!
+	Body = [],
+	%% Ensure an empty body results in a 400 error.
+	{400, _} = do_body_error("POST", "/multipart", [
+		{<<"content-type">>, <<"multipart/mixed; boundary=deadbeef">>}
+	], Body, Config),
+	ok.
+
+multipart_error_preamble_only(Config) ->
+	doc("Multipart request body only contains a preamble."),
+	%% Ensure an empty body results in a 400 error.
+	{400, _} = do_body_error("POST", "/multipart", [
+		{<<"content-type">>, <<"multipart/mixed; boundary=deadbeef">>}
+	], <<"Preamble.">>, Config),
+	ok.
+
 multipart_error_headers(Config) ->
 	doc("Multipart request body with invalid part headers."),
 	ReqBody = [
@@ -490,6 +510,17 @@ multipart_error_headers(Config) ->
 %% The function to parse the multipart body currently does not crash,
 %% as far as I can tell. There is therefore no test for it.
 
+multipart_error_no_final_boundary(Config) ->
+	doc("Multipart request body with no final boundary."),
+	ReqBody = [
+		"--deadbeef\r\nContent-Type: text/plain\r\n\r\nCowboy is an HTTP server.\r\n"
+	],
+	%% Ensure parse errors result in a 400 response.
+	{400, _} = do_body_error("POST", "/multipart", [
+		{<<"content-type">>, <<"multipart/mixed; boundary=deadbeef">>}
+	], ReqBody, Config),
+	ok.
+
 multipart_missing_boundary(Config) ->
 	doc("Multipart request body without a boundary in the media type."),
 	ReqBody = [
-- 
cgit v1.2.3