Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2004843002:
Adds a MerkleAuditProof struct to net/cert. (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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds a MerkleAuditProof struct to net/cert.
This holds a Merkle audit path, as defined in RFC6962.
BUG=612439
Committed: https://crrev.com/c96ecf9d0e6f78ea6a1ef8c1dadb9446146cc665
Cr-Commit-Position: refs/heads/master@{#396302}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addresses review comments from eranm #
Total comments: 2
Patch Set 3 : Addresses final comment from eranm #Patch Set 4 : Only test DCHECK in debug builds where GTEST_HAS_DEATH_TEST #Patch Set 5 : Use DCHECK_IS_ON() rather than EXPECT_DEBUG_DEATH #
Total comments: 3
Patch Set 6 : Addresses review comment from eroman #
Messages
Total messages: 35 (13 generated)
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/2004843002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004843002/1
robpercival@chromium.org changed reviewers: + eranm@chromium.org
https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof.cc File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof.cc:11: // As described in RFC6962, section 4.5 The length calculation is not described in this section... https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof.cc:13: uint64_t ln = tree_size - 1; Can you think of more informative names to describe the values held in these variables? https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof.cc:17: if ((li & 1) || li < ln) (1) Better documentation (copy from the DNS I-D if possible). (2) li > ln should never happen (should be checked as a precondition probably), so I'd suggest replacing the li < ln check with !(li == ln) for clarity. https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... File net/cert/merkle_audit_proof_unittest.cc (right): https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof_unittest.cc:15: EXPECT_EQ(20u, CalculateAuditPathLength(123456, 999999)); Add a few more test cases, particularly relating to where the node in the tree is?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof.cc File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof.cc:11: // As described in RFC6962, section 4.5 On 2016/05/23 13:10:51, Eran Messeri wrote: > The length calculation is not described in this section... Fixed to point to the CT over DNS draft RFC. https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof.cc:13: uint64_t ln = tree_size - 1; On 2016/05/23 13:10:51, Eran Messeri wrote: > Can you think of more informative names to describe the values held in these > variables? Done. https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof.cc:17: if ((li & 1) || li < ln) On 2016/05/23 13:10:51, Eran Messeri wrote: > (1) Better documentation (copy from the DNS I-D if possible). > (2) li > ln should never happen (should be checked as a precondition probably), > so I'd suggest replacing the li < ln check with !(li == ln) for clarity. 1. The DNS I-D doesn't really document this, it just describes the steps the algorithm takes in pseudo-code (but doesn't explain the reasoning behind it). To usefully document it here, I'd probably have to explain Merkle trees and audit proofs, so is just linking to the appropriate section of RFC6962 ok? 2. Done. By the way, our Python code in GitHub also uses < rather than != (https://github.com/google/certificate-transparency/blob/af98904302724c29aa665...). https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... File net/cert/merkle_audit_proof_unittest.cc (right): https://codereview.chromium.org/2004843002/diff/1/net/cert/merkle_audit_proof... net/cert/merkle_audit_proof_unittest.cc:15: EXPECT_EQ(20u, CalculateAuditPathLength(123456, 999999)); On 2016/05/23 13:10:52, Eran Messeri wrote: > Add a few more test cases, particularly relating to where the node in the tree > is? Done.
lgtm https://codereview.chromium.org/2004843002/diff/20001/net/cert/merkle_audit_p... File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2004843002/diff/20001/net/cert/merkle_audit_p... net/cert/merkle_audit_proof.cc:14: // https://github.com/google/certificate-transparency-rfcs/blob/c8844de6bd0b5d3d.... Nit: The ct-over-dns is what you may call a living document. I'd swap this comment line with the next one (15) as the RFC is the authoritative source.
robpercival@chromium.org changed reviewers: + eroman@chromium.org
PTAL Eric https://codereview.chromium.org/2004843002/diff/20001/net/cert/merkle_audit_p... File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2004843002/diff/20001/net/cert/merkle_audit_p... net/cert/merkle_audit_proof.cc:14: // https://github.com/google/certificate-transparency-rfcs/blob/c8844de6bd0b5d3d.... On 2016/05/25 09:59:55, Eran Messeri wrote: > Nit: The ct-over-dns is what you may call a living document. > I'd swap this comment line with the next one (15) as the RFC is the > authoritative source. Done.
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/2004843002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004843002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
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/2004843002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004843002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
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/2004843002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004843002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
https://codereview.chromium.org/2004843002/diff/80001/net/cert/merkle_audit_p... File net/cert/merkle_audit_proof_unittest.cc (right): https://codereview.chromium.org/2004843002/diff/80001/net/cert/merkle_audit_p... net/cert/merkle_audit_proof_unittest.cc:40: TEST(MerkleAuditProofDeathTest, DiesIfLeafIndexIsGreaterThanOrEqualToTreeSize) { This test sounds fishy. (a) if it is reachable code (i.e. leaf_index and tree_size are attacker controlled), then it should be checking in production code and not using a DCHECK() (b) if it really is unreachable (and a programmer error for the caller of this function) then testing that DCHECK is reached doesn't seem very valuable. I don't particular care if we have a test for this or not, but want to make sure it isn't situation (a).
https://codereview.chromium.org/2004843002/diff/80001/net/cert/merkle_audit_p... File net/cert/merkle_audit_proof_unittest.cc (right): https://codereview.chromium.org/2004843002/diff/80001/net/cert/merkle_audit_p... net/cert/merkle_audit_proof_unittest.cc:40: TEST(MerkleAuditProofDeathTest, DiesIfLeafIndexIsGreaterThanOrEqualToTreeSize) { On 2016/05/26 00:39:43, eroman wrote: > This test sounds fishy. > > (a) if it is reachable code (i.e. leaf_index and tree_size are attacker > controlled), then it should be checking in production code and not using a > DCHECK() > > (b) if it really is unreachable (and a programmer error for the caller of this > function) then testing that DCHECK is reached doesn't seem very valuable. > > I don't particular care if we have a test for this or not, but want to make sure > it isn't situation (a). The tree size will come from a signed, verified data structure delivered by the component updater, so that's highly unlikely to be attacker controlled. The leaf index will have been received over DNS, so that could, conceivably, be attacker controlled. I'll upgrade the check to CHECK(). I can't imagine to what ends an attacker would abuse this function but I'm sure that's just down to a lack of imagination on my part!
Thanks. Apologies for not being more familiar with CT. I should probably take some time to read though the specs at some point :) In a nutshell, if there is any possible way for a DCHECK() to be reached in a production binary (be it by invalid input, something weird, or an adversarially constructed input) then the right thing to do is handle it and fail. A CHECK() would be appropriate for something which there is no possible execution path for (that we can think of), and offers an extra level of protection and future proofing if something messes things up. A DCHECK() I find to be mostly useful for documenting invariants (which if violated represent a programming mistake).
https://codereview.chromium.org/2004843002/diff/80001/net/cert/merkle_audit_p... File net/cert/merkle_audit_proof_unittest.cc (right): https://codereview.chromium.org/2004843002/diff/80001/net/cert/merkle_audit_p... net/cert/merkle_audit_proof_unittest.cc:40: TEST(MerkleAuditProofDeathTest, DiesIfLeafIndexIsGreaterThanOrEqualToTreeSize) { On 2016/05/26 03:17:13, Rob Percival wrote: > On 2016/05/26 00:39:43, eroman wrote: > > This test sounds fishy. > > > > (a) if it is reachable code (i.e. leaf_index and tree_size are attacker > > controlled), then it should be checking in production code and not using a > > DCHECK() > > > > (b) if it really is unreachable (and a programmer error for the caller of this > > function) then testing that DCHECK is reached doesn't seem very valuable. > > > > I don't particular care if we have a test for this or not, but want to make > sure > > it isn't situation (a). > > The tree size will come from a signed, verified data structure delivered by the > component updater, so that's highly unlikely to be attacker controlled. The leaf > index will have been received over DNS, so that could, conceivably, be attacker > controlled. I'll upgrade the check to CHECK(). I can't imagine to what ends an > attacker would abuse this function but I'm sure that's just down to a lack of > imagination on my part! Done.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2004843002/#ps100001 (title: "Addresses review comment from eroman")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004843002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004843002/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 a MerkleAuditProof struct to net/cert. This holds a Merkle audit path, as defined in RFC6962. BUG=612439 ========== to ========== Adds a MerkleAuditProof struct to net/cert. This holds a Merkle audit path, as defined in RFC6962. BUG=612439 Committed: https://crrev.com/c96ecf9d0e6f78ea6a1ef8c1dadb9446146cc665 Cr-Commit-Position: refs/heads/master@{#396302} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c96ecf9d0e6f78ea6a1ef8c1dadb9446146cc665 Cr-Commit-Position: refs/heads/master@{#396302} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
