Review and clean up MerkleIntegritySourceStream class |
||
Issue description
,
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
,
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
,
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
,
Apr 30 2018
Oops, forgot to close this. |
||
►
Sign in to add a comment |
||
Comment 1 by horo@chromium.org
, Feb 22 2018