Fix parsing form data binary data.

FormDataParserMultipart contract says that "*OCTET" part of body is read
 and passed as string of bytes. In case header has filename, octet part
 is ignored - store filename instead.
Actual code doesn't implement it. It led to situation when octet data
 were read and were put as string to base::Value. That led to DCHECK as far
 as string value must be utf-8 string.
The same problem occurs with FormDataParserUrlEncoded, where encoded part
 can be as utf-8 string or binary data.

Bug: 813561
Change-Id: I453e5767e44334535253d1bd3fa4e857c2d3a3ff
Reviewed-on: https://chromium-review.googlesource.com/910848
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539907}

chrome/browser/extensions/api/web_request/web_request_api_unittest.cc

@@ -139,6 +139,19 @@ void GetPartOfMessageArguments(IPC::Message* message,
   ASSERT_TRUE(list.GetDictionary(0, out));
 }
 
+base::Value FormBinaryValue(base::StringPiece str) {
+  base::Value list(base::Value::Type::LIST);
+  list.GetList().emplace_back(base::Value(
+      base::Value::BlobStorage(str.data(), str.data() + str.size())));
+  return list;
+}
+
+base::Value FormStringValue(base::StringPiece str) {
+  base::Value list(base::Value::Type::LIST);
+  list.GetList().emplace_back(base::Value(str));
+  return list;
+}
+
 }  // namespace
 
 // A mock event router that responds to events with a pre-arranged queue of
@@ -592,17 +605,26 @@ TEST_F(ExtensionWebRequestTest, AccessRequestBodyData) {
   const size_t kPlainBlock2Length = sizeof(kPlainBlock2) - 1;
   std::vector<char> plain_2(kPlainBlock2, kPlainBlock2 + kPlainBlock2Length);
 #define kBoundary "THIS_IS_A_BOUNDARY"
-  const char kFormBlock1[] = "--" kBoundary "\r\n"
+  const char kFormBlock1[] =
+      "--" kBoundary
+      "\r\n"
       "Content-Disposition: form-data; name=\"A\"\r\n"
       "\r\n"
       "test text\r\n"
-      "--" kBoundary "\r\n"
+      "--" kBoundary
+      "\r\n"
       "Content-Disposition: form-data; name=\"B\"; filename=\"\"\r\n"
       "Content-Type: application/octet-stream\r\n"
-      "\r\n";
+      "\r\n"
+      "--" kBoundary
+      "\r\n"
+      "Content-Disposition: form-data; name=\"B_content\"\r\n"
+      "Content-Type: application/octet-stream\r\n"
+      "\r\n"
+      "\uffff\uffff\uffff\uffff\r\n"
+      "--" kBoundary "\r\n";
   std::vector<char> form_1(kFormBlock1, kFormBlock1 + sizeof(kFormBlock1) - 1);
-  const char kFormBlock2[] = "\r\n"
-      "--" kBoundary "\r\n"
+  const char kFormBlock2[] =
       "Content-Disposition: form-data; name=\"C\"\r\n"
       "\r\n"
       "test password\r\n"
@@ -623,10 +645,21 @@ TEST_F(ExtensionWebRequestTest, AccessRequestBodyData) {
     &kRawPath
   };
   // Contents of formData.
-  const char kFormData[] =
-      "{\"A\":[\"test text\"],\"B\":[\"\"],\"C\":[\"test password\"]}";
-  std::unique_ptr<const base::Value> form_data =
-      base::JSONReader::Read(kFormData);
+  struct KeyValuePairs {
+    const char* key;
+    base::Value value;
+  };
+  KeyValuePairs kFormDataPairs[] = {
+      {"A", FormStringValue("test text")},
+      {"B", FormStringValue("")},
+      {"B_content", FormBinaryValue("\uffff\uffff\uffff\uffff")},
+      {"C", FormStringValue("test password")}};
+  std::unique_ptr<base::Value> form_data =
+      base::MakeUnique<base::Value>(base::Value::Type::DICTIONARY);
+  for (auto& pair : kFormDataPairs) {
+    form_data->SetKey(pair.key, std::move(pair.value));
+  }
+
   ASSERT_TRUE(form_data.get() != NULL);
   ASSERT_TRUE(form_data->type() == base::Value::Type::DICTIONARY);
   // Contents of raw.

extensions/browser/api/web_request/form_data_parser.cc

@@ -52,6 +52,7 @@ struct Patterns {
   const RE2 value_pattern;
   const RE2 unquote_pattern;
   const RE2 url_encoded_pattern;
+  const RE2 content_type_octet_stream;
 };
 
 Patterns::Patterns()
@@ -68,9 +69,9 @@ Patterns::Patterns()
       value_pattern("\\bfilename=\"([^\"]*)\""),
       unquote_pattern(kEscapeClosingQuote),
       url_encoded_pattern(std::string("(") + kCharacterPattern + "*)=(" +
-                          kCharacterPattern +
-                          "*)") {
-}
+                          kCharacterPattern + "*)"),
+      content_type_octet_stream(
+          "Content-Type: application\\/octet-stream\\r\\n") {}
 
 base::LazyInstance<Patterns>::Leaky g_patterns = LAZY_INSTANCE_INITIALIZER;
 
@@ -226,10 +227,12 @@ class FormDataParserMultipart : public FormDataParser {
   // possibly |value| from "filename=" fields of that header. Only if the
   // "name" or "filename" fields are found, then |name| or |value| are touched.
   // Returns true iff |source_| is seeked forward. Sets |value_assigned|
-  // to true iff |value| has been assigned to.
+  // to true iff |value| has been assigned to. Sets |value_is_binary| to true if
+  // header has content-type: application/octet-stream.
   bool TryReadHeader(base::StringPiece* name,
                      base::StringPiece* value,
-                     bool* value_assigned);
+                     bool* value_assigned,
+                     bool* value_is_binary);
 
   // Helper to GetNextNameValue. Expects that the input starts with a data
   // portion of a body part. An attempt is made to read the input until the end
@@ -269,6 +272,11 @@ class FormDataParserMultipart : public FormDataParser {
   const RE2& value_pattern() const {
     return patterns_->value_pattern;
   }
+
+  const RE2& content_type_octet_stream() const {
+    return patterns_->content_type_octet_stream;
+  }
+
   // However, this is used in a static method so it needs to be static.
   static const RE2& unquote_pattern() {
     return g_patterns.Get().unquote_pattern;  // No caching g_patterns here.
@@ -293,6 +301,15 @@ class FormDataParserMultipart : public FormDataParser {
 FormDataParser::Result::Result() {}
 FormDataParser::Result::~Result() {}
 
+void FormDataParser::Result::SetBinaryValue(base::StringPiece str) {
+  value_ = base::Value(
+      base::Value::BlobStorage(str.data(), str.data() + str.size()));
+}
+
+void FormDataParser::Result::SetStringValue(std::string str) {
+  value_ = base::Value(std::move(str));
+}
+
 FormDataParser::~FormDataParser() {}
 
 // static
@@ -383,7 +400,15 @@ bool FormDataParserUrlEncoded::GetNextNameValue(Result* result) {
   bool success = RE2::ConsumeN(&source_, pattern(), args_, args_size_);
   if (success) {
     result->set_name(net::UnescapeURLComponent(name_, unescape_rules_));
-    result->set_value(net::UnescapeURLComponent(value_, unescape_rules_));
+    const std::string unescaped_value =
+        net::UnescapeURLComponent(value_, unescape_rules_);
+    const base::StringPiece unescaped_data(unescaped_value.data(),
+                                           unescaped_value.length());
+    if (base::IsStringUTF8(unescaped_data)) {
+      result->SetStringValue(std::move(unescaped_value));
+    } else {
+      result->SetBinaryValue(unescaped_data);
+    }
   }
   if (source_.length() > 0) {
     if (source_[0] == '&')
@@ -496,9 +521,14 @@ bool FormDataParserMultipart::GetNextNameValue(Result* result) {
   base::StringPiece name;
   base::StringPiece value;
   bool value_assigned = false;
+  bool value_is_binary = false;
   bool value_assigned_temp;
-  while (TryReadHeader(&name, &value, &value_assigned_temp))
+  bool value_is_binary_temp;
+  while (TryReadHeader(&name, &value, &value_assigned_temp,
+                       &value_is_binary_temp)) {
+    value_is_binary |= value_is_binary_temp;
     value_assigned |= value_assigned_temp;
+  }
   if (name.empty() || state_ == STATE_ERROR) {
     state_ = STATE_ERROR;
     return false;
@@ -517,7 +547,7 @@ bool FormDataParserMultipart::GetNextNameValue(Result* result) {
     return_value = true;
     state_ = STATE_SUSPEND;
   } else {
-    return_value = FinishReadingPart(value_assigned ? NULL : &value);
+    return_value = FinishReadingPart(value_assigned ? nullptr : &value);
   }
 
   std::string unescaped_name = net::UnescapeURLComponent(
@@ -526,7 +556,14 @@ bool FormDataParserMultipart::GetNextNameValue(Result* result) {
           net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS |
           net::UnescapeRule::SPOOFING_AND_CONTROL_CHARS);
   result->set_name(unescaped_name);
-  result->set_value(value);
+  if (value_assigned) {
+    // Hold filename as value.
+    result->SetStringValue(value.as_string());
+  } else if (value_is_binary) {
+    result->SetBinaryValue(value);
+  } else {
+    result->SetStringValue(value.as_string());
+  }
 
   return return_value;
 }
@@ -557,7 +594,7 @@ bool FormDataParserMultipart::SetSource(base::StringPiece source) {
     case STATE_READY:  // Nothing to do.
       break;
     case STATE_SUSPEND:
-      state_ = FinishReadingPart(NULL) ? STATE_READY : STATE_ERROR;
+      state_ = FinishReadingPart(nullptr) ? STATE_READY : STATE_ERROR;
       break;
     default:
       state_ = STATE_ERROR;
@@ -567,8 +604,16 @@ bool FormDataParserMultipart::SetSource(base::StringPiece source) {
 
 bool FormDataParserMultipart::TryReadHeader(base::StringPiece* name,
                                             base::StringPiece* value,
-                                            bool* value_assigned) {
+                                            bool* value_assigned,
+                                            bool* value_is_binary) {
   *value_assigned = false;
+  *value_is_binary = false;
+  // Support Content-Type: application/octet-stream.
+  // Form data with this content type is represented as string of bytes.
+  if (RE2::Consume(&source_, content_type_octet_stream())) {
+    *value_is_binary = true;
+    return true;
+  }
   const char* header_start = source_.data();
   if (!RE2::Consume(&source_, header_pattern()))
     return false;

extensions/browser/api/web_request/form_data_parser.h

@@ -7,10 +7,12 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "base/macros.h"
 // Cannot forward declare StringPiece because it is a typedef.
 #include "base/strings/string_piece.h"
+#include "base/values.h"
 
 namespace net {
 class URLRequest;
@@ -22,20 +24,23 @@ namespace extensions {
 class FormDataParser {
  public:
   // Result encapsulates name-value pairs returned by GetNextNameValue.
+  // Value stored as base::Value, which is string if data is UTF-8 string and
+  // binary blob if value represents form data binary data.
   class Result {
    public:
     Result();
     ~Result();
 
     const std::string& name() const { return name_; }
-    const std::string& value() const { return value_; }
+    base::Value take_value() { return std::move(value_); }
 
     void set_name(base::StringPiece str) { str.CopyToString(&name_); }
-    void set_value(base::StringPiece str) { str.CopyToString(&value_); }
+    void SetBinaryValue(base::StringPiece str);
+    void SetStringValue(std::string str);
 
    private:
     std::string name_;
-    std::string value_;
+    base::Value value_;
 
     DISALLOW_COPY_AND_ASSIGN(Result);
   };

extensions/browser/api/web_request/form_data_parser_unittest.cc

@@ -43,7 +43,13 @@ bool RunParser(const std::string& content_type_header,
       return false;
     while (parser->GetNextNameValue(&result)) {
       output->push_back(result.name());
-      output->push_back(result.value());
+      base::Value value = result.take_value();
+      if (value.is_string()) {
+        output->push_back(value.GetString());
+      } else {
+        const auto& blob = value.GetBlob();
+        output->push_back(std::string(blob.begin(), blob.end()));
+      }
     }
   }
   return parser->AllDataReadOK();
@@ -64,7 +70,13 @@ bool CheckParserFails(const std::string& content_type_header,
       break;
     while (parser->GetNextNameValue(&result)) {
       output.push_back(result.name());
-      output.push_back(result.value());
+      base::Value value = result.take_value();
+      if (value.is_string()) {
+        output.push_back(value.GetString());
+      } else {
+        const auto& blob = value.GetBlob();
+        output.push_back(std::string(blob.begin(), blob.end()));
+      }
     }
   }
   return !parser->AllDataReadOK();
@@ -127,16 +139,28 @@ TEST(WebRequestFormDataParserTest, Parsing) {
       "\r\n"
       "one\r\n"
       "--" +
+      kBoundary + "\r\n";
+  const std::string kBlockStr3 =
+      "Content-Disposition: form-data; name=\"binary\"\r\n"
+      "Content-Type: application/octet-stream\r\n"
+      "\r\n"
+      "\u0420\u043e\u0434\u0436\u0435\u0440 "
+      "\u0416\u0435\u043b\u044f\u0437\u043d\u044b"
+      "\r\n"
+      "--" +
       kBoundary + "--";
   // POST data input.
-  const std::string kBigBlock = kBlockStr1 + kBlockStr2;
+  const std::string kBigBlock = kBlockStr1 + kBlockStr2 + kBlockStr3;
   const std::string kUrlEncodedBlock =
       "text=test%0Dtext%0Awith+non-CRLF+line+breaks"
       "&file=test&password=test+password&radio=Yes&check=option+A"
-      "&check=option+B&txtarea=Some+text.%0D%0AOther.%0D%0A&select=one";
+      "&check=option+B&txtarea=Some+text.%0D%0AOther.%0D%0A&select=one"
+      "&binary=%D0%A0%D0%BE%D0%B4%D0%B6%D0%B5%D1%80%20%D0%96%D0%B5%D0%BB%D1%8F%"
+      "D0%B7%D0%BD%D1%8B";
   const base::StringPiece kMultipartBytes(kBigBlock);
   const base::StringPiece kMultipartBytesSplit1(kBlockStr1);
   const base::StringPiece kMultipartBytesSplit2(kBlockStr2);
+  const base::StringPiece kMultipartBytesSplit3(kBlockStr3);
   const base::StringPiece kUrlEncodedBytes(kUrlEncodedBlock);
   const std::string kPlainBlock = "abc";
   const base::StringPiece kTextPlainBytes(kPlainBlock);
@@ -146,16 +170,25 @@ TEST(WebRequestFormDataParserTest, Parsing) {
   const std::string kMultipart =
       std::string("multipart/form-data; boundary=") + kBoundary;
   // Expected output.
-  const char* kPairs[] = {
-    "text", "test\rtext\nwith non-CRLF line breaks",
-    "file", "test",
-    "password", "test password",
-    "radio", "Yes",
-    "check", "option A",
-    "check", "option B",
-    "txtarea", "Some text.\r\nOther.\r\n",
-    "select", "one"
-  };
+  const char* kPairs[] = {"text",
+                          "test\rtext\nwith non-CRLF line breaks",
+                          "file",
+                          "test",
+                          "password",
+                          "test password",
+                          "radio",
+                          "Yes",
+                          "check",
+                          "option A",
+                          "check",
+                          "option B",
+                          "txtarea",
+                          "Some text.\r\nOther.\r\n",
+                          "select",
+                          "one",
+                          "binary",
+                          "\u0420\u043e\u0434\u0436\u0435\u0440 "
+                          "\u0416\u0435\u043b\u044f\u0437\u043d\u044b"};
   const std::vector<std::string> kExpected(kPairs, kPairs + arraysize(kPairs));
 
   std::vector<const base::StringPiece*> input;
@@ -170,6 +203,7 @@ TEST(WebRequestFormDataParserTest, Parsing) {
   input.clear();
   input.push_back(&kMultipartBytesSplit1);
   input.push_back(&kMultipartBytesSplit2);
+  input.push_back(&kMultipartBytesSplit3);
   EXPECT_TRUE(RunParser(kMultipart, input, &output));
   EXPECT_EQ(kExpected, output);
 

extensions/browser/api/web_request/upload_data_presenter.cc

@@ -123,7 +123,7 @@ void ParsedDataPresenter::FeedNext(const net::UploadElementReader& reader) {
   FormDataParser::Result result;
   while (parser_->GetNextNameValue(&result)) {
     base::Value* list = GetOrCreateList(dictionary_.get(), result.name());
-    list->GetList().emplace_back(result.value());
+    list->GetList().emplace_back(result.take_value());
   }
 }