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

2 files changed, 218 insertions, 17 deletions

diff --git a/include/multipart_parser.hpp b/include/multipart_parser.hpp
index 2fe3679452..a2f63bd167 100644
--- a/include/multipart_parser.hpp
+++ b/include/multipart_parser.hpp
@@ -16,7 +16,10 @@ enum class ParserError
     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 @@ class MultipartParser
 
         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 @@ class MultipartParser
                     {
                         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 @@ class MultipartParser
                     partDataMark = i;
                     [[fallthrough]];
                 case State::PART_DATA:
+                {
                     if (index == 0)
                     {
                         skipNonBoundary(buffer, len, boundary.size() - 1, i);
@@ -196,12 +203,23 @@ class MultipartParser
                         // 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 @@ class MultipartParser
         }
     }
 
-    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 @@ class MultipartParser
                     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 @@ class MultipartParser
                 {
                     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 @@ class MultipartParser
             // 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 7ad7d98f89..f29bcfe5c8 100644
--- a/test/include/multipart_test.cpp
+++ b/test/include/multipart_test.cpp
@@ -83,11 +83,8 @@ TEST_F(MultipartTest, TestBadMultipartParser1)
 
     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 @@ TEST_F(MultipartTest, TestBadMultipartParser2)
 
     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 @@ TEST_F(MultipartTest, TestErrorHeaderEnding)
     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