Fixed issues with multipart parser
- Index was not checked against size before dereference. Which cased to
override memory.
- Header without colon could put parser into invalid state. Now it will
return with error.
- Content after boundary was not correctly discarded.
- Parser did not check body for final boudary. Now missing final
boundary will return with error.
Tested:
- Tested that payload with header without colon doesn't cause memory
corruption anymore.
Signed-off-by: Krzysztof Grobelny <krzysztof.grobelny@intel.com>
Change-Id: I12f496ab5f53e6c088cdfdf2e96be636d66f7c7f
diff --git a/include/multipart_parser.hpp b/include/multipart_parser.hpp
index 2fe3679..a2f63bd 100644
--- a/include/multipart_parser.hpp
+++ b/include/multipart_parser.hpp
@@ -16,7 +16,10 @@
ERROR_EMPTY_HEADER,
ERROR_HEADER_NAME,
ERROR_HEADER_VALUE,
- ERROR_HEADER_ENDING
+ ERROR_HEADER_ENDING,
+ ERROR_UNEXPECTED_END_OF_HEADER,
+ ERROR_UNEXPECTED_END_OF_INPUT,
+ ERROR_OUT_OF_RANGE
};
enum class State
@@ -72,7 +75,6 @@
const char* buffer = req.body.data();
size_t len = req.body.size();
- size_t prevIndex = index;
char cl = 0;
for (size_t i = 0; i < len; i++)
@@ -182,6 +184,10 @@
{
return ParserError::ERROR_HEADER_ENDING;
}
+ if (index > 0)
+ {
+ return ParserError::ERROR_UNEXPECTED_END_OF_HEADER;
+ }
state = State::PART_DATA_START;
break;
case State::PART_DATA_START:
@@ -189,6 +195,7 @@
partDataMark = i;
[[fallthrough]];
case State::PART_DATA:
+ {
if (index == 0)
{
skipNonBoundary(buffer, len, boundary.size() - 1, i);
@@ -196,12 +203,23 @@
// NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
c = buffer[i];
}
- processPartData(prevIndex, buffer, i, c);
+ const ParserError ec = processPartData(buffer, i, c);
+ if (ec != ParserError::PARSER_SUCCESS)
+ {
+ return ec;
+ }
break;
+ }
case State::END:
break;
}
}
+
+ if (state != State::END)
+ {
+ return ParserError::ERROR_UNEXPECTED_END_OF_INPUT;
+ }
+
return ParserError::PARSER_SUCCESS;
}
std::vector<FormPart> mime_fields;
@@ -242,10 +260,9 @@
}
}
- void processPartData(size_t& prevIndex, const char* buffer, size_t& i,
- char c)
+ ParserError processPartData(const char* buffer, size_t& i, char c)
{
- prevIndex = index;
+ size_t prevIndex = index;
if (index < boundary.size())
{
@@ -295,7 +312,7 @@
flags = Boundary::NON_BOUNDARY;
mime_fields.push_back({});
state = State::HEADER_FIELD_START;
- return;
+ return ParserError::PARSER_SUCCESS;
}
}
if (flags == Boundary::END_BOUNDARY)
@@ -304,11 +321,21 @@
{
state = State::END;
}
+ else
+ {
+ flags = Boundary::NON_BOUNDARY;
+ index = 0;
+ }
}
}
if (index > 0)
{
+ if ((index - 1) >= lookbehind.size())
+ {
+ // Should never happen, but when it does it won't cause crash
+ return ParserError::ERROR_OUT_OF_RANGE;
+ }
lookbehind[index - 1] = c;
}
else if (prevIndex > 0)
@@ -317,13 +344,13 @@
// lookbehind belongs to partData
mime_fields.rbegin()->content += lookbehind.substr(0, prevIndex);
- prevIndex = 0;
partDataMark = i;
// reconsider the current character even so it interrupted
// the sequence it could be the beginning of a new sequence
i--;
}
+ return ParserError::PARSER_SUCCESS;
}
std::string currentHeaderName;
diff --git a/test/include/multipart_test.cpp b/test/include/multipart_test.cpp
index 7ad7d98..f29bcfe 100644
--- a/test/include/multipart_test.cpp
+++ b/test/include/multipart_test.cpp
@@ -83,11 +83,8 @@
crow::Request reqIn(req, ec);
ParserError rc = parser.parse(reqIn);
- ASSERT_EQ(rc, ParserError::PARSER_SUCCESS);
- EXPECT_EQ(parser.boundary,
- "\r\n-----------------------------d74496d66958873e");
- EXPECT_EQ(parser.mime_fields.size(), 1);
+ EXPECT_EQ(rc, ParserError::ERROR_UNEXPECTED_END_OF_INPUT);
}
TEST_F(MultipartTest, TestBadMultipartParser2)
@@ -103,11 +100,8 @@
crow::Request reqIn(req, ec);
ParserError rc = parser.parse(reqIn);
- ASSERT_EQ(rc, ParserError::PARSER_SUCCESS);
- EXPECT_EQ(parser.boundary,
- "\r\n-----------------------------d74496d66958873e");
- EXPECT_EQ(parser.mime_fields.size(), 1);
+ EXPECT_EQ(rc, ParserError::ERROR_UNEXPECTED_END_OF_INPUT);
}
TEST_F(MultipartTest, TestErrorBoundaryFormat)
@@ -253,4 +247,184 @@
crow::Request reqIn(req, ec);
EXPECT_EQ(parser.parse(reqIn), ParserError::ERROR_HEADER_ENDING);
}
-} // namespace
\ No newline at end of file
+
+TEST_F(MultipartTest, TestGoodMultipartParserMultipleHeaders)
+{
+ req.set("Content-Type",
+ "multipart/form-data; "
+ "boundary=---------------------------d74496d66958873e");
+
+ req.body() = "-----------------------------d74496d66958873e\r\n"
+ "Content-Disposition: form-data; name=\"Test1\"\r\n"
+ "Other-Header: value=\"v1\"\r\n"
+ "\r\n"
+ "Data1\r\n"
+ "-----------------------------d74496d66958873e--";
+
+ crow::Request reqIn(req, ec);
+ ParserError rc = parser.parse(reqIn);
+ ASSERT_EQ(rc, ParserError::PARSER_SUCCESS);
+
+ EXPECT_EQ(parser.boundary,
+ "\r\n-----------------------------d74496d66958873e");
+ ASSERT_EQ(parser.mime_fields.size(), 1);
+
+ EXPECT_EQ(parser.mime_fields[0].fields.at("Content-Disposition"),
+ "form-data; name=\"Test1\"");
+ EXPECT_EQ(parser.mime_fields[0].fields.at("Other-Header"), "value=\"v1\"");
+ EXPECT_EQ(parser.mime_fields[0].content, "Data1");
+}
+
+TEST_F(MultipartTest, TestErrorHeaderWithoutColon)
+{
+ req.set("Content-Type", "multipart/form-data; "
+ "boundary=--end");
+
+ req.body() = "----end\r\n"
+ "abc\r\n"
+ "\r\n"
+ "Data1\r\n"
+ "----end--\r\n";
+
+ crow::Request reqIn(req, ec);
+ EXPECT_EQ(parser.parse(reqIn), ParserError::ERROR_UNEXPECTED_END_OF_HEADER);
+}
+
+TEST_F(MultipartTest, TestUnknownHeaderIsCorrectlyParsed)
+{
+ req.set("Content-Type", "multipart/form-data; "
+ "boundary=--end");
+
+ req.body() =
+ "----end\r\n"
+ "t-DiPpcccc:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccgcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccaaaaaa\r\n"
+ "\r\n"
+ "Data1\r\n"
+ "----end--\r\n";
+
+ crow::Request reqIn(req, ec);
+ ParserError rc = parser.parse(reqIn);
+
+ ASSERT_EQ(rc, ParserError::PARSER_SUCCESS);
+
+ EXPECT_EQ(parser.boundary, "\r\n----end");
+ ASSERT_EQ(parser.mime_fields.size(), 1);
+
+ EXPECT_EQ(
+ parser.mime_fields[0].fields.at("t-DiPpcccc"),
+ "cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccgcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccaaaaaa");
+ EXPECT_EQ(parser.mime_fields[0].content, "Data1");
+}
+
+TEST_F(MultipartTest, TestErrorMissingSeparatorBetweenMimeFieldsAndData)
+{
+ req.set(
+ "Content-Type",
+ "multipart/form-data; boundary=---------------------------d74496d66958873e");
+
+ req.body() =
+ "-----------------------------d74496d66958873e\r\n"
+ "t-DiPpcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccgcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccaaaaaa\r\n"
+ "Data1"
+ "-----------------------------d74496d66958873e--";
+
+ crow::Request reqIn(req, ec);
+ ParserError rc = parser.parse(reqIn);
+
+ EXPECT_EQ(rc, ParserError::ERROR_UNEXPECTED_END_OF_HEADER);
+}
+
+TEST_F(MultipartTest, TestDataWithoutMimeFields)
+{
+ req.set(
+ "Content-Type",
+ "multipart/form-data; boundary=---------------------------d74496d66958873e");
+
+ req.body() = "-----------------------------d74496d66958873e\r\n"
+ "\r\n"
+ "Data1\r\n"
+ "-----------------------------d74496d66958873e--";
+
+ crow::Request reqIn(req, ec);
+ ParserError rc = parser.parse(reqIn);
+
+ ASSERT_EQ(rc, ParserError::PARSER_SUCCESS);
+
+ EXPECT_EQ(parser.boundary,
+ "\r\n-----------------------------d74496d66958873e");
+ ASSERT_EQ(parser.mime_fields.size(), 1);
+
+ EXPECT_EQ(std::distance(parser.mime_fields[0].fields.begin(),
+ parser.mime_fields[0].fields.end()),
+ 0);
+ EXPECT_EQ(parser.mime_fields[0].content, "Data1");
+}
+
+TEST_F(MultipartTest, TestErrorMissingFinalBoundry)
+{
+ req.set("Content-Type", "multipart/form-data; boundary=--XX");
+
+ req.body() =
+ "----XX\r\n"
+ "Content-Disposition: form-data; name=\"Test2\"\r\n\r\n"
+ "t-DiPpccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccAAAAAAAAAAAAAAABCDz\r\n"
+ "\335\r\n\r\n";
+
+ crow::Request reqIn(req, ec);
+ ParserError rc = parser.parse(reqIn);
+
+ EXPECT_EQ(rc, ParserError::ERROR_UNEXPECTED_END_OF_INPUT);
+}
+
+TEST_F(MultipartTest, TestIgnoreDataAfterFinalBoundary)
+{
+ req.set("Content-Type", "multipart/form-data; boundary=--XX");
+
+ req.body() = "----XX\r\n"
+ "Content-Disposition: form-data; name=\"Test1\"\r\n\r\n"
+ "Data1\r\n"
+ "----XX--\r\n"
+ "Content-Disposition: form-data; name=\"Test2\"\r\n\r\n"
+ "Data2\r\n"
+ "----XX--\r\n";
+
+ crow::Request reqIn(req, ec);
+ ParserError rc = parser.parse(reqIn);
+
+ ASSERT_EQ(rc, ParserError::PARSER_SUCCESS);
+
+ EXPECT_EQ(parser.boundary, "\r\n----XX");
+ EXPECT_EQ(parser.mime_fields.size(), 1);
+
+ EXPECT_EQ(parser.mime_fields[0].fields.at("Content-Disposition"),
+ "form-data; name=\"Test1\"");
+ EXPECT_EQ(parser.mime_fields[0].content, "Data1");
+}
+
+TEST_F(MultipartTest, TestFinalBoundaryIsCorrectlyRecognized)
+{
+ req.set("Content-Type", "multipart/form-data; boundary=--XX");
+
+ req.body() = "----XX\r\n"
+ "Content-Disposition: form-data; name=\"Test1\"\r\n\r\n"
+ "Data1\r\n"
+ "----XX-abc-\r\n"
+ "StillData1\r\n"
+ "----XX--\r\n";
+
+ crow::Request reqIn(req, ec);
+ ParserError rc = parser.parse(reqIn);
+
+ ASSERT_EQ(rc, ParserError::PARSER_SUCCESS);
+
+ EXPECT_EQ(parser.boundary, "\r\n----XX");
+ EXPECT_EQ(parser.mime_fields.size(), 1);
+
+ EXPECT_EQ(parser.mime_fields[0].fields.at("Content-Disposition"),
+ "form-data; name=\"Test1\"");
+ EXPECT_EQ(parser.mime_fields[0].content, "Data1\r\n"
+ "----XX-abc-\r\n"
+ "StillData1");
+}
+
+} // namespace