Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1945183005:
Adds function for generating MerkleTreeLeaf hash. (Closed)
|
Created:
4 years, 7 months ago by Rob Percival Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ct_merkle_tree_leaf_encode Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds function for generating MerkleTreeLeaf hash.
This will be used when requesting audit proofs from Certificate Transparency
logs.
BUG=608868
Committed: https://crrev.com/34ef59d7bcd81a747334eadb0aac0dbac54ef1a5
Cr-Commit-Position: refs/heads/master@{#392703}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes Hash function from method to free function #Patch Set 3 : Addresses review comments #
Total comments: 9
Patch Set 4 : Addresses eroman review comments #
Total comments: 8
Patch Set 5 : Addresses final eroman review comments #Patch Set 6 : Rebase #
Depends on Patchset: Messages
Total messages: 25 (9 generated)
robpercival@chromium.org changed reviewers: + eranm@chromium.org
Two minor comments, otherwise LGTM. https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf.cc File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf.c... net/cert/merkle_tree_leaf.cc:25: *out = crypto::SHA256HashString("\x00" + leaf_in_tls_format); Nit: Link to the section of RFC6962 detailing the 0 byte prefix for leaf hashes. https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf_u... File net/cert/merkle_tree_leaf_unittest.cc (right): https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf_u... net/cert/merkle_tree_leaf_unittest.cc:28: if (base::HexStringToBytes(hexStr, &bytes)) { Return early by flipping the condition of the if statement and returning.
https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf.cc File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf.c... net/cert/merkle_tree_leaf.cc:25: *out = crypto::SHA256HashString("\x00" + leaf_in_tls_format); On 2016/05/05 16:03:26, Eran Messeri wrote: > Nit: Link to the section of RFC6962 detailing the 0 byte prefix for leaf hashes. Done. https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf_u... File net/cert/merkle_tree_leaf_unittest.cc (right): https://codereview.chromium.org/1945183005/diff/1/net/cert/merkle_tree_leaf_u... net/cert/merkle_tree_leaf_unittest.cc:28: if (base::HexStringToBytes(hexStr, &bytes)) { On 2016/05/05 16:03:26, Eran Messeri wrote: > Return early by flipping the condition of the if statement and returning. Done.
robpercival@chromium.org changed reviewers: + eroman@chromium.org
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1945183005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945183005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.cc:26: *out = crypto::SHA256HashString("\x00" + leaf_in_tls_format); This does not behave as described -- no 0 byte is prepended. "\x00" is interpreted as a 0-length c-string, since its first character is NUL. Would be simpler to call string::insert() with '\0' to avoid this. https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.h (right): https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:45: // Sets |*out| to the hash of this Merkle |tree_leaf|, as defined in RFC6962. nit: "this Merkle" --> "the Merkle" ? https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf_unittest.cc (right): https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:40: return memcmp(arg.data(), bytes.data(), bytes.size()) == 0; technically this is incorrect in the case where the input is empty (since memcmp() is undefined when called with NULL pointers) https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:108: EXPECT_THAT(hash, HexEq("d306c83ef45ac97bba7a0d99331f23433e5fa97cf3a08ed90b24" Where does this data come from? Are there officially sanctioned test vectors you can use? Given that I think the implementation has a bug, would be good to understand why this works and whether it should.
https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.cc:26: *out = crypto::SHA256HashString("\x00" + leaf_in_tls_format); On 2016/05/06 23:51:22, eroman wrote: > This does not behave as described -- no 0 byte is prepended. > > "\x00" is interpreted as a 0-length c-string, since its first character is NUL. > > Would be simpler to call string::insert() with '\0' to avoid this. Done. https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.h (right): https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:45: // Sets |*out| to the hash of this Merkle |tree_leaf|, as defined in RFC6962. On 2016/05/06 23:51:22, eroman wrote: > nit: "this Merkle" --> "the Merkle" ? Done. https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf_unittest.cc (right): https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:40: return memcmp(arg.data(), bytes.data(), bytes.size()) == 0; On 2016/05/06 23:51:22, eroman wrote: > technically this is incorrect in the case where the input is empty (since > memcmp() is undefined when called with NULL pointers) Fixed. Surprisingly, that is only mentioned in the C++ spec, and not specifically with regard to memcmp. All of the other documentation I could find didn't mention that and, in fact, claimed that if the 3rd argument is 0, memcmp was guaranteed to return 0. I suspect the latter is reality, and the former is just unfortunate wording on the part of the spec, but I've fixed this nonetheless. https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:108: EXPECT_THAT(hash, HexEq("d306c83ef45ac97bba7a0d99331f23433e5fa97cf3a08ed90b24" On 2016/05/06 23:51:22, eroman wrote: > Where does this data come from? > Are there officially sanctioned test vectors you can use? > > Given that I think the implementation has a bug, would be good to understand why > this works and whether it should. I independently generated this, by taking the serialized tree leaf produced in the CtSerialization unit tests, writing to to a file using std::fstream (attempting to prepend a zero byte in the process, but again using "\x00", which got ignored as an empty string) and then passing it through sha256sum. I've updated the expected hash values to the correct hashes now (I've added the zero byte to the files by hand). There are tests for this sort of hashing in https://github.com/google/certificate-transparency/blob/master/python/ct/cryp... that I could borrow test data from, but no officially sanctioned test vectors that I'm aware of.
https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf_unittest.cc (right): https://codereview.chromium.org/1945183005/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:40: return memcmp(arg.data(), bytes.data(), bytes.size()) == 0; On 2016/05/09 12:36:05, Rob Percival wrote: > On 2016/05/06 23:51:22, eroman wrote: > > technically this is incorrect in the case where the input is empty (since > > memcmp() is undefined when called with NULL pointers) > > Fixed. Surprisingly, that is only mentioned in the C++ spec, and not > specifically with regard to memcmp. All of the other documentation I could find > didn't mention that and, in fact, claimed that if the 3rd argument is 0, memcmp > was guaranteed to return 0. I suspect the latter is reality, and the former is > just unfortunate wording on the part of the spec, but I've fixed this > nonetheless. Yeah, the C++ spec defers to the C spec for definition of memcmp. And the the C spec implies that the pointers to memcmp/memcpy/memmove must be non-NULL in 7.24.1: """Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4.""" While this does seem like a spec bug to me (and completely idiotic behavior), compilers *do* unfortunately exploit this undefined behavior :( See for instance the "Null pointer checks may be optimized away more aggressively" section in the GCC 4.9 release notes: https://gcc.gnu.org/gcc-4.9/porting_to.html This isn't just a theoretical problem; it impacted OpenSSL.
lgtm https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.cc:26: *out = crypto::SHA256HashString('\0' + leaf_in_tls_format); Note: since EncodeTreeLeaf() already does an append, you could avoid this concatenation. i.e something like: leaf_in_tls_format.push_back('\0'); if (!EncodeTreeLeaf(tree_leaf, &leaf_in_tls_format)) return false; *out = crypto::SHA256HashString(leaf_in_tls_format); https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf_unittest.cc (right): https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:40: return true; formatting. https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:112: EXPECT_THAT(hash, HexEq("452da788b3b8d15872ff0bb0777354b2a7f1c1887b5633201e76" Please document in the test how this was generated. https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:122: EXPECT_THAT(hash, HexEq("257ae85f08810445511e35e33f7aee99ee19407971e35e95822b" Same
https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.cc:26: *out = crypto::SHA256HashString('\0' + leaf_in_tls_format); On 2016/05/09 19:01:22, eroman wrote: > Note: since EncodeTreeLeaf() already does an append, you could avoid this > concatenation. i.e something like: > > leaf_in_tls_format.push_back('\0'); > if (!EncodeTreeLeaf(tree_leaf, &leaf_in_tls_format)) > return false; > > *out = crypto::SHA256HashString(leaf_in_tls_format); Done. https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf_unittest.cc (right): https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:40: return true; On 2016/05/09 19:01:22, eroman wrote: > formatting. Done. https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:112: EXPECT_THAT(hash, HexEq("452da788b3b8d15872ff0bb0777354b2a7f1c1887b5633201e76" On 2016/05/09 19:01:22, eroman wrote: > Please document in the test how this was generated. Done. https://codereview.chromium.org/1945183005/diff/60001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf_unittest.cc:122: EXPECT_THAT(hash, HexEq("257ae85f08810445511e35e33f7aee99ee19407971e35e95822b" On 2016/05/09 19:01:22, eroman wrote: > Same Done.
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1945183005/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1945183005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945183005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by robpercival@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1945183005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945183005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adds function for generating MerkleTreeLeaf hash. This will be used when requesting audit proofs from Certificate Transparency logs. BUG=608868 ========== to ========== Adds function for generating MerkleTreeLeaf hash. This will be used when requesting audit proofs from Certificate Transparency logs. BUG=608868 Committed: https://crrev.com/34ef59d7bcd81a747334eadb0aac0dbac54ef1a5 Cr-Commit-Position: refs/heads/master@{#392703} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/34ef59d7bcd81a747334eadb0aac0dbac54ef1a5 Cr-Commit-Position: refs/heads/master@{#392703} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
