Rework MerkleIntegritySourceStream.

This cuts down on the number of copies and fixes some other bits:

- Add a fuzzer.

- Document a place where the original implementation did not match the
  specification. (The final record's size is a little iffy. We probably
  want a small spec tweak.)

- Use a streaming SHA-256 implementation, rather than making a copy to
  stick the 0 or 1 in the hash.

- If there is no more room in the output (the consumer may be issuing
  smaller reads), stop processing input. There is no need to make a copy
  of the entire input. MerkleIntegeritySourceStream only needs to buffer
  at most one record. (Ideally we wouldn't even do and instead
  coordinate with the base class's read buffer, but that would require
  tweaking the FilteredSourceStream interface. This CL addresses the
  easy stuff.)

- Fix O(N^2) behavior if the caller issues tiny reads in the buffered
  output.

- If the record is entirely in the input buffer (common case), don't
  make a copy to extract it.

- If the output fits entirely in the output buffer (common case), don't
  make a copy to return it.

- Flesh out missing tests, based on code coverage tools and important
  security checks (notably truncation).

  (For others trying to repeat the coverage bits: this file was
  unfortunately placed in //content rather than //net, so I wasn't able
  to get the coverage tools to work without hacking it into
  net_unittests locally. It seems the X server dependency is
  problematic for tools/code_coverage?? Also content_unittests is huge.)

- s/MI-256/MI-SHA256/. There are other 256-bit hashes.

Bug: 814591
Change-Id: If927d3f49085a5bec31939846c9a55f8903da34a
Reviewed-on: https://chromium-review.googlesource.com/981798Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547874}

content/browser/loader/merkle_integrity_source_stream.h

@@ -8,10 +8,12 @@
 #include <stdint.h>
 #include <string>
 
+#include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 #include "content/common/content_export.h"
 #include "net/filter/filter_source_stream.h"
+#include "third_party/boringssl/src/include/openssl/sha.h"
 
 namespace content {
 
@@ -36,14 +38,48 @@ class CONTENT_EXPORT MerkleIntegritySourceStream
   std::string GetTypeAsString() const override;
 
  private:
-  bool ProcessInput(bool upstream_eof_reached);
-
-  std::string input_;
-  std::string output_;
-  // SHA-256 hash for the next record, or empty if validation is completed.
-  std::string next_proof_;
-  uint64_t record_size_;
-  bool failed_;
+  // Processes as many bytes of |input| as are available or fit in
+  // |output|. Both |input| and |output| are advanced past any bytes consumed or
+  // written to, respectively. Returns true if all input processed, possibly
+  // none, was valid and false on fatal error.
+  bool FilterDataImpl(base::span<char>* output,
+                      base::span<const char>* input,
+                      bool upstream_eof_reached);
+
+  // Copies |partial_output_| to output, as much as fits and advances both
+  // buffers. Returns whether all output was copied.
+  bool CopyPartialOutput(base::span<char>* output);
+
+  // Consumes the next |len| bytes of data from |partial_input_| and |input|
+  // and, if available, points |result| to it and returns true. |result| will
+  // point into either |input| or data copied to |storage|. |input| is advanced
+  // past any consumed bytes. If |len| bytes are not available, returns false
+  // and fully consumes |input| |partial_input_| for a future call.
+  bool ConsumeBytes(base::span<const char>* input,
+                    size_t len,
+                    base::span<const char>* result,
+                    std::string* storage);
+
+  // Processes a record and returns whether it was valid. If valid, writes the
+  // contents into |output|, advancing past any bytes written. If |output| was
+  // not large enough, excess data will be copied into an internal buffer for a
+  // future call.
+  bool ProcessRecord(base::span<const char> record,
+                     bool is_final,
+                     base::span<char>* output);
+
+  // The partial input block, if the previous input buffer was too small.
+  std::string partial_input_;
+  // The partial output block, if the previous output buffer was too small.
+  std::string partial_output_;
+  // The index of |partial_output_| that has not been returned yet.
+  size_t partial_output_offset_ = 0;
+  // SHA-256 hash for the next record, if |final_record_done_| is false.
+  uint8_t next_proof_[SHA256_DIGEST_LENGTH];
+  size_t record_size_ = 0;
+  bool failed_ = false;
+  // Whether the final record has been processed.
+  bool final_record_done_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(MerkleIntegritySourceStream);
 };

content/test/data/fuzzer_dictionaries/merkle_integrity_source_stream_fuzzer.dict

+# Copyright 2018 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+# The fuzzer build fills all hashes with 0x42.
+"\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42\x42"
+
+# The header version of the hash.
+"mi-sha256=QkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkI\\ "
+
+# base::FuzzedDataProvider separator.
+"\\ "
+
+# Use a 16-byte record size if you don't have better ideas.
+"\x00\x00\x00\x00\x00\x00\x00\x10"

content/test/fuzzer/BUILD.gn

@@ -144,3 +144,24 @@ fuzzer_test("appcache_manifest_parser_fuzzer") {
   seed_corpus =
       "//third_party/WebKit/LayoutTests/http/tests/appcache/resources/"
 }
+
+fuzzer_test("merkle_integrity_source_stream_fuzzer") {
+  sources = [
+    "merkle_integrity_source_stream_fuzzer.cc",
+  ]
+
+  # This fuzzer depends on net::FuzzedSourceStream, in net_fuzzer_test_support,
+  # but both it and //content:test_support set up similar globals. As
+  # MerkleIntegritySourceStream does not depend on anything in //content and
+  # will ultimately live in //net, use the //net one instead of the //content
+  # one.
+  deps = [
+    "//content/browser:for_content_tests",
+    "//content/renderer:for_content_tests",
+    "//content/shell:content_shell_lib",
+    "//content/test:test_support",
+    "//net:net_fuzzer_test_support",
+    "//net:test_support",
+  ]
+  dict = "//content/test/data/fuzzer_dictionaries/merkle_integrity_source_stream_fuzzer.dict"
+}

content/test/fuzzer/merkle_integrity_source_stream_fuzzer.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/loader/merkle_integrity_source_stream.h"  // nogncheck
+
+#include <string>
+
+#include "base/logging.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/test/fuzzed_data_provider.h"
+#include "net/base/io_buffer.h"
+#include "net/base/test_completion_callback.h"
+#include "net/filter/fuzzed_source_stream.h"
+
+// Fuzzer for MerkleIntegritySourceStream
+//
+// |data| contains a header prefix, and then is used to build a
+// FuzzedSourceStream.
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
+  base::FuzzedDataProvider data_provider(data, size);
+  std::string header = data_provider.ConsumeRandomLengthString(256);
+
+  net::TestCompletionCallback callback;
+  auto fuzzed_source_stream =
+      std::make_unique<net::FuzzedSourceStream>(&data_provider);
+  auto mi_stream = std::make_unique<content::MerkleIntegritySourceStream>(
+      header, std::move(fuzzed_source_stream));
+  while (true) {
+    size_t read_size = data_provider.ConsumeUint32InRange(1, 1024);
+    auto io_buffer = base::MakeRefCounted<net::IOBufferWithSize>(read_size);
+    int result = mi_stream->Read(io_buffer.get(), io_buffer->size(),
+                                 callback.callback());
+    // Releasing the pointer to IOBuffer immediately is more likely to lead to a
+    // use-after-free.
+    io_buffer = nullptr;
+    if (callback.GetResult(result) <= 0)
+      break;
+  }
+
+  return 0;
+}