New issue
Advanced search Search tips

Issue 814591 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 803774



Sign in to add a comment

Review and clean up MerkleIntegritySourceStream class

Project Member Reported by horo@chromium.org, Feb 22 2018

Issue description

Comment 1 by horo@chromium.org, Feb 22 2018

Blocking: 803774
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3c6f1b76a58bf5d111af724d0a3eb2495b07278

commit d3c6f1b76a58bf5d111af724d0a3eb2495b07278
Author: David Benjamin <davidben@chromium.org>
Date: Thu Mar 08 00:23:08 2018

Fix missing bounds check in MerkleIntegritySourceStream.

Also use base::StringPiece rather than a std::string reference. It
avoids some unnecessary copies in the substrings.

Bug:  814591 
Change-Id: I7a1a0387b09038f7e084b2baf7b67716c729710a
Reviewed-on: https://chromium-review.googlesource.com/953744
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541652}
[modify] https://crrev.com/d3c6f1b76a58bf5d111af724d0a3eb2495b07278/content/browser/loader/merkle_integrity_source_stream.cc
[modify] https://crrev.com/d3c6f1b76a58bf5d111af724d0a3eb2495b07278/content/browser/loader/merkle_integrity_source_stream.h
[modify] https://crrev.com/d3c6f1b76a58bf5d111af724d0a3eb2495b07278/content/browser/loader/merkle_integrity_source_stream_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb8cafb55b5e2d4f1ebf1208fb976393a68b4465

commit fb8cafb55b5e2d4f1ebf1208fb976393a68b4465
Author: David Benjamin <davidben@chromium.org>
Date: Thu Mar 08 17:48:04 2018

Use a more standard record size limit for MerkleIntegritySourceStream.

5MiB is quite large, especially on mobile. The record size here has
analogous tradeoffs as in TLS and HTTP/2, which both use a generous
16KiB limit. Match those.

Bug:  814591 
Change-Id: I7238a8b544c92f99c1753d73654a7c04497dbd29
Reviewed-on: https://chromium-review.googlesource.com/953804
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541829}
[modify] https://crrev.com/fb8cafb55b5e2d4f1ebf1208fb976393a68b4465/content/browser/loader/merkle_integrity_source_stream.cc
[modify] https://crrev.com/fb8cafb55b5e2d4f1ebf1208fb976393a68b4465/content/browser/loader/merkle_integrity_source_stream_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98abd37767e4398aeb5ad199846fac075f757422

commit 98abd37767e4398aeb5ad199846fac075f757422
Author: David Benjamin <davidben@chromium.org>
Date: Tue Apr 03 23:56:07 2018

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/981798
Reviewed-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}
[modify] https://crrev.com/98abd37767e4398aeb5ad199846fac075f757422/content/browser/loader/merkle_integrity_source_stream.cc
[modify] https://crrev.com/98abd37767e4398aeb5ad199846fac075f757422/content/browser/loader/merkle_integrity_source_stream.h
[modify] https://crrev.com/98abd37767e4398aeb5ad199846fac075f757422/content/browser/loader/merkle_integrity_source_stream_unittest.cc
[add] https://crrev.com/98abd37767e4398aeb5ad199846fac075f757422/content/test/data/fuzzer_dictionaries/merkle_integrity_source_stream_fuzzer.dict
[modify] https://crrev.com/98abd37767e4398aeb5ad199846fac075f757422/content/test/fuzzer/BUILD.gn
[add] https://crrev.com/98abd37767e4398aeb5ad199846fac075f757422/content/test/fuzzer/merkle_integrity_source_stream_fuzzer.cc

Status: Fixed (was: Assigned)
Oops, forgot to close this.

Sign in to add a comment