Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731603002:
Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for (Closed)
DescriptionCheck TBSCertificate.algorithm and Certificate.signatureAlgorithm for
consistency when verifying certificates.
The underlying platform verifiers don't do this, which can lead to
confusion when trying to enforce policy for SHA1 on the verified chain.
* If the two signature algorithms don't match will fail with
ERR_INVALID_CERT.
* If the chain contains a signature algorithm that we don't know how to
parse, will also fail with ERR_INVALID_CERT
BUG=690821
Review-Url: https://codereview.chromium.org/2731603002
Cr-Commit-Position: refs/heads/master@{#455682}
Committed: https://chromium.googlesource.com/chromium/src/+/a77953fe670968fe6728796b77cedf48f0954d78
Patch Set 1 #Patch Set 2 : fix #
Total comments: 27
Patch Set 3 : address mattm's comments #Patch Set 4 : moar #
Total comments: 4
Patch Set 5 : Address mattm's comments (use fixed serial numbers) #Patch Set 6 : rebase #Patch Set 7 : add a special case for iOS #Patch Set 8 : add tests for root #
Total comments: 12
Patch Set 9 : address rsleevi's comments #Patch Set 10 : Use rsleevi's background comment #
Messages
Total messages: 57 (39 generated)
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for consistency when verifying certificates. The underlying platform verifiers don't do this, which can lead to confusion when trying to enforce policy for SHA1 on the verified chain. * If the two signature algorithms don't match will fail with ERR_INVALID_CERT. * If the chain contains a signature algorithm that we don't know how to parse, will also fail with ERR_INVALID_CERT (whereas before BUG=690821 ========== to ========== Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for consistency when verifying certificates. The underlying platform verifiers don't do this, which can lead to confusion when trying to enforce policy for SHA1 on the verified chain. * If the two signature algorithms don't match will fail with ERR_INVALID_CERT. * If the chain contains a signature algorithm that we don't know how to parse, will also fail with ERR_INVALID_CERT BUG=690821 ==========
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eroman@chromium.org changed reviewers: + rsleevi@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
rsleevi@chromium.org changed reviewers: + mattm@chromium.org
I'm going to punt to Matt because I know I won't have a chance to get to this until ~Tues
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:404: // * Sets |*has_mismatched_signature_algorithms| to true if note that the has_mismatched_signature_algorithms and has_unknown_signature_algorithms aren't modified otherwise? https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:410: // whitelist. Maybe just me, but I was a bit confused by the comment at first, can read as: "could not be parsed or it did match against a known whitelist". vs "could not be parsed or could not be matched against a known whitelist" https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:439: return; I guess it doesn't matter with the current usage, but maybe this !algorithm return should be done before the IsEquivalent test? (otherwise you can set has_mismatched_signature_algorithms when really you just couldn't parse them) https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:449: NOTREACHED(); Is this reachable with malformed input data? Or does SignatureAlgorithm::Create guarantee it won't happen? https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:383: X509Certificate::FORMAT_AUTO); ASSERT_TRUE(cert) https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:609: X509Certificate::FORMAT_AUTO); ASSERT_TRUE(leaf) https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:637: // In the test setup, SHA384 is given special treatment as an unkown unknown https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:674: verify_result.verified_cert = chain; MockCertVerifyProc already sets verified_cert https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:680: flags, NULL, CertificateList(), &verify_result); Using the same CertVerifyResult object as was used to initialize the MockCertVerifyProc is a bit confusing, though I guess Verify does result->Reset() so it doesn't actually hurt.. (And the result isn't used here anyway) https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; what happens if you run the test under gtest_repeat? Does it eventually wrap around and start failing? https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:883: int rv = VerifyChain({CertParams::Sha256(), imo this is more confusing than just putting {DigestAlgorithm::Sha256, DigestAlgorithm::Sha256} https://codereview.chromium.org/2731603002/diff/20001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:170: if (!SignatureAlgorithm::IsEquivalent(alg1_tlv, alg2_tlv)) { is the ! here correct? Seems like that's backwards.
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:404: // * Sets |*has_mismatched_signature_algorithms| to true if On 2017/03/04 02:34:26, mattm wrote: > note that the has_mismatched_signature_algorithms and > has_unknown_signature_algorithms aren't modified otherwise? N/A -- I removed these out-parameters and combined them into a single boolean result. https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:410: // whitelist. On 2017/03/04 02:34:26, mattm wrote: > Maybe just me, but I was a bit confused by the comment at first, can read as: > "could not be parsed or it did match against a known whitelist". > vs > "could not be parsed or could not be matched against a known whitelist" > N/A -- replaced this with a hopefully clearer comment regarding the return value. https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:439: return; On 2017/03/04 02:34:26, mattm wrote: > I guess it doesn't matter with the current usage, but maybe this !algorithm > return should be done before the IsEquivalent test? (otherwise you can set > has_mismatched_signature_algorithms when really you just couldn't parse them) N/A -- I no longer distinguish between the two, and short-circuit for simplicity (INVALID_CERT will trump any weak signature error anyway). https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:449: NOTREACHED(); On 2017/03/04 02:34:26, mattm wrote: > Is this reachable with malformed input data? Or does SignatureAlgorithm::Create > guarantee it won't happen? I restructured this code so there is no longer a NOTREACHED(). To answer your question -- yes, it should only be possible for the RSASSA-PSS case to have an attached parameters object, and parsing will enforce this. https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:383: X509Certificate::FORMAT_AUTO); On 2017/03/04 02:34:26, mattm wrote: > ASSERT_TRUE(cert) Sorry for including these unrelated refactors -- I have removed these diffs from the patch (and will propose them separately). https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:609: X509Certificate::FORMAT_AUTO); On 2017/03/04 02:34:26, mattm wrote: > ASSERT_TRUE(leaf) Ditto. https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:637: // In the test setup, SHA384 is given special treatment as an unkown On 2017/03/04 02:34:26, mattm wrote: > unknown Done. https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:674: verify_result.verified_cert = chain; On 2017/03/04 02:34:27, mattm wrote: > MockCertVerifyProc already sets verified_cert Done -- thanks! https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:680: flags, NULL, CertificateList(), &verify_result); On 2017/03/04 02:34:26, mattm wrote: > Using the same CertVerifyResult object as was used to initialize the > MockCertVerifyProc is a bit confusing, though I guess Verify does > result->Reset() so it doesn't actually hurt.. (And the result isn't used here > anyway) Done. https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; On 2017/03/04 02:34:26, mattm wrote: > what happens if you run the test under gtest_repeat? Does it eventually wrap > around and start failing? I expect that once it has created around 255 certificates the serial number will wrap around and it will start to fail, but haven't tried. The base certificate in this case has a 1-byte-long serial number to tamper (technically high bit set would be a negative serial number, but NSS doesn't care about that). Do you think this is worth expanding on? https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:883: int rv = VerifyChain({CertParams::Sha256(), On 2017/03/04 02:34:26, mattm wrote: > imo this is more confusing than just putting {DigestAlgorithm::Sha256, > DigestAlgorithm::Sha256} Done. https://codereview.chromium.org/2731603002/diff/20001/net/cert/internal/verif... File net/cert/internal/verify_certificate_chain.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/internal/verif... net/cert/internal/verify_certificate_chain.cc:170: if (!SignatureAlgorithm::IsEquivalent(alg1_tlv, alg2_tlv)) { On 2017/03/04 02:34:27, mattm wrote: > is the ! here correct? Seems like that's backwards. Thanks for spotting that bug! Ugh. I will follow-up with a unit-test that covers this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; On 2017/03/07 23:43:00, eroman wrote: > On 2017/03/04 02:34:26, mattm wrote: > > what happens if you run the test under gtest_repeat? Does it eventually wrap > > around and start failing? > > I expect that once it has created around 255 certificates the serial number will > wrap around and it will start to fail, but haven't tried. > > The base certificate in this case has a 1-byte-long serial number to tamper > (technically high bit set would be a negative serial number, but NSS doesn't > care about that). > > Do you think this is worth expanding on? Hmm. I would say it's worth avoiding that. I guess gtest_repeat probably is rarely used but having a test that fails when run in a otherwise normal & supported way doesn't seem great. Would it be possible to just check in some cert files that have the required mismatches? Or I guess you could find a different base cert which has a longer serial number and randomize the whole serial.. https://codereview.chromium.org/2731603002/diff/60001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/2731603002/diff/60001/net/cert/asn1_util.h#ne... net/cert/asn1_util.h:55: // strict parsing or validate the resulting AlgorithmIdentifiers. extra space https://codereview.chromium.org/2731603002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:404: // Returns false if the signature algorithm was invalid (unknown algorithm). mention "unknown or mismatched"?
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; On 2017/03/08 01:41:34, mattm wrote: > On 2017/03/07 23:43:00, eroman wrote: > > On 2017/03/04 02:34:26, mattm wrote: > > > what happens if you run the test under gtest_repeat? Does it eventually wrap > > > around and start failing? > > > > I expect that once it has created around 255 certificates the serial number > will > > wrap around and it will start to fail, but haven't tried. > > > > The base certificate in this case has a 1-byte-long serial number to tamper > > (technically high bit set would be a negative serial number, but NSS doesn't > > care about that). > > > > Do you think this is worth expanding on? > > Hmm. I would say it's worth avoiding that. I guess gtest_repeat probably is > rarely used but having a test that fails when run in a otherwise normal & > supported way doesn't seem great. Would it be possible to just check in some > cert files that have the required mismatches? Or I guess you could find a > different base cert which has a longer serial number and randomize the whole > serial.. Here are two possible routes forward: (1) I can modify the cert generator to use a fixed amount of serial numbers rather than incrementing unboundedly (i.e. either generate the certificate DER only once, or map each permutation to a fixed serial number). This should address your concern of repeating tests and will be a small modification to what I already have. (2) I can move the creation of mismatched algorithm certificates to scripts external to the C++. While I agree this is an overall a cleaner design, I had avoided it because editing the shell scripts was trickier than doing the modifications in C++. (Also in earlier versions I was patching up the signature too, and I found it easier to just do from the C++). My preference would be to do (1) for this changelist, so I can try to land the security fix sooner rather than later. At that point I can follow-up with approach (2) which I may want anyway for regression testing that bug you found in verify_certificate_chain_unittest.cc. WDYT?
On 2017/03/08 01:59:29, eroman wrote: > https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... > File net/cert/cert_verify_proc_unittest.cc (right): > > https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... > net/cert/cert_verify_proc_unittest.cc:797: > *const_cast<char*>(serial_value.data()) = next_serial_number_++; > On 2017/03/08 01:41:34, mattm wrote: > > On 2017/03/07 23:43:00, eroman wrote: > > > On 2017/03/04 02:34:26, mattm wrote: > > > > what happens if you run the test under gtest_repeat? Does it eventually > wrap > > > > around and start failing? > > > > > > I expect that once it has created around 255 certificates the serial number > > will > > > wrap around and it will start to fail, but haven't tried. > > > > > > The base certificate in this case has a 1-byte-long serial number to tamper > > > (technically high bit set would be a negative serial number, but NSS doesn't > > > care about that). > > > > > > Do you think this is worth expanding on? > > > > Hmm. I would say it's worth avoiding that. I guess gtest_repeat probably is > > rarely used but having a test that fails when run in a otherwise normal & > > supported way doesn't seem great. Would it be possible to just check in some > > cert files that have the required mismatches? Or I guess you could find a > > different base cert which has a longer serial number and randomize the whole > > serial.. > > Here are two possible routes forward: > > (1) I can modify the cert generator to use a fixed amount of serial numbers > rather than incrementing unboundedly (i.e. either generate the certificate DER > only once, or map each permutation to a fixed serial number). This should > address your concern of repeating tests and will be a small modification to what > I already have. > > (2) I can move the creation of mismatched algorithm certificates to scripts > external to the C++. While I agree this is an overall a cleaner design, I had > avoided it because editing the shell scripts was trickier than doing the > modifications in C++. (Also in earlier versions I was patching up the signature > too, and I found it easier to just do from the C++). > > > My preference would be to do (1) for this changelist, so I can try to land the > security fix sooner rather than later. At that point I can follow-up with > approach (2) which I may want anyway for regression testing that bug you found > in verify_certificate_chain_unittest.cc. WDYT? Sounds good.
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; On 2017/03/08 01:59:28, eroman wrote: > On 2017/03/08 01:41:34, mattm wrote: > > On 2017/03/07 23:43:00, eroman wrote: > > > On 2017/03/04 02:34:26, mattm wrote: > > > > what happens if you run the test under gtest_repeat? Does it eventually > wrap > > > > around and start failing? > > > > > > I expect that once it has created around 255 certificates the serial number > > will > > > wrap around and it will start to fail, but haven't tried. > > > > > > The base certificate in this case has a 1-byte-long serial number to tamper > > > (technically high bit set would be a negative serial number, but NSS doesn't > > > care about that). > > > > > > Do you think this is worth expanding on? > > > > Hmm. I would say it's worth avoiding that. I guess gtest_repeat probably is > > rarely used but having a test that fails when run in a otherwise normal & > > supported way doesn't seem great. Would it be possible to just check in some > > cert files that have the required mismatches? Or I guess you could find a > > different base cert which has a longer serial number and randomize the whole > > serial.. > > Here are two possible routes forward: > > (1) I can modify the cert generator to use a fixed amount of serial numbers > rather than incrementing unboundedly (i.e. either generate the certificate DER > only once, or map each permutation to a fixed serial number). This should > address your concern of repeating tests and will be a small modification to what > I already have. > > (2) I can move the creation of mismatched algorithm certificates to scripts > external to the C++. While I agree this is an overall a cleaner design, I had > avoided it because editing the shell scripts was trickier than doing the > modifications in C++. (Also in earlier versions I was patching up the signature > too, and I found it easier to just do from the C++). > > > My preference would be to do (1) for this changelist, so I can try to land the > security fix sooner rather than later. At that point I can follow-up with > approach (2) which I may want anyway for regression testing that bug you found > in verify_certificate_chain_unittest.cc. WDYT? Done. I went with static serial numbers, and may switch to external certificate generation in a follow-up change. https://codereview.chromium.org/2731603002/diff/60001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/2731603002/diff/60001/net/cert/asn1_util.h#ne... net/cert/asn1_util.h:55: // strict parsing or validate the resulting AlgorithmIdentifiers. On 2017/03/08 01:41:35, mattm wrote: > extra space Done. https://codereview.chromium.org/2731603002/diff/60001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/60001/net/cert/cert_verify_pr... net/cert/cert_verify_proc.cc:404: // Returns false if the signature algorithm was invalid (unknown algorithm). On 2017/03/08 01:41:35, mattm wrote: > mention "unknown or mismatched"? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/08 22:59:58, mattm wrote: > lgtm patchset 8 still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM with a few minor comments and requests for changes (sorry to be auto-hating on you; normally I <3 it) https://codereview.chromium.org/2731603002/diff/140001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/asn1_util.h#n... net/cert/asn1_util.h:59: // with TBSCertificate.algorithm. Note lines 32, 25, and 19 all make explicit statements about the lifetime of the returned StringPiece's, but this does not. Probably worth rewording the comment: On successful return, |cert_signature_algorithm_sequence| points into the Certificate.signatureAlgorithm in |cert|, ... (or however you want to word it) https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... net/cert/cert_verify_proc.cc:424: auto algorithm = This is hiding the lifetime management of SignatureAlgorithm. I had to lookup what Create returns, given the line 426 below Even when it's a "safe pointer" (meaning unique || ref_counted), I would then have to read 429-443 to see if the lifetime could/would be 'extended' https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:633: // system verifiers don't check this. This can lead to confusion as the "in practice, system verifiers don't check this" - this goes back to the fact that RFC 5280 discourages verification libraries from enforcing conformance to the profile, so that they can be postel-friendly -_- https://codereview.chromium.org/2731603002/diff/140001/net/cert/internal/sign... File net/cert/internal/signature_algorithm.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:647: auto alg2 = Create(alg2_tlv, nullptr); I think this runs afoul of several of the principles in The type aids clarity to the reader - https://google.github.io/styleguide/cppguide.html#auto - The type is not clear from surrounding context (this isn't a new / make_unique style allocation) - The type matters because it's more than just 'simple' equality comparison https://codereview.chromium.org/2731603002/diff/140001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:664: const auto* params2 = alg2->ParamsForRsaPss(); ditto here
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2731603002/diff/140001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/asn1_util.h#n... net/cert/asn1_util.h:59: // with TBSCertificate.algorithm. On 2017/03/09 00:45:26, Ryan Sleevi wrote: > Note lines 32, 25, and 19 all make explicit statements about the lifetime of the > returned StringPiece's, but this does not. Probably worth rewording the comment: > > On successful return, |cert_signature_algorithm_sequence| points into the > Certificate.signatureAlgorithm in |cert|, ... > > > (or however you want to word it) Done. https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... net/cert/cert_verify_proc.cc:424: auto algorithm = On 2017/03/09 00:45:26, Ryan Sleevi wrote: > This is hiding the lifetime management of SignatureAlgorithm. I had to lookup > what Create returns, given the line 426 below > > Even when it's a "safe pointer" (meaning unique || ref_counted), I would then > have to read 429-443 to see if the lifetime could/would be 'extended' Done. https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:633: // system verifiers don't check this. This can lead to confusion as the On 2017/03/09 00:45:26, Ryan Sleevi wrote: > "in practice, system verifiers don't check this" - this goes back to the fact > that RFC 5280 discourages verification libraries from enforcing conformance to > the profile, so that they can be postel-friendly -_- Acknowledged. Was there a comment change you want as well, or just FYI? also, mumble-muble, Postel's law... https://codereview.chromium.org/2731603002/diff/140001/net/cert/internal/sign... File net/cert/internal/signature_algorithm.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:647: auto alg2 = Create(alg2_tlv, nullptr); On 2017/03/09 00:45:26, Ryan Sleevi wrote: > I think this runs afoul of several of the principles in The type aids clarity to > the reader - https://google.github.io/styleguide/cppguide.html#auto > > - The type is not clear from surrounding context (this isn't a new / make_unique > style allocation) > - The type matters because it's more than just 'simple' equality comparison Done. https://codereview.chromium.org/2731603002/diff/140001/net/cert/internal/sign... net/cert/internal/signature_algorithm.cc:664: const auto* params2 = alg2->ParamsForRsaPss(); On 2017/03/09 00:45:26, Ryan Sleevi wrote: > ditto here Done.
It wasn't meant to be a comment-nit, but since you mentioned it... :D https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:633: // system verifiers don't check this. This can lead to confusion as the On 2017/03/09 01:09:43, eroman wrote: > Acknowledged. > Was there a comment change you want as well, or just FYI? > also, mumble-muble, Postel's law... Wasn't requesting a comment change, but it was unclear if it was understood why the verifiers didn't do it (it wasn't obvious to me until a recent discussion with a CA reminded me of the stupidity of 5280 discouraging enforcing compliance to 5280 in verifiers) // X.509 certificates contain two redundant descriptors for the signature algorithm; one is covered by the // signature, but in order to verify the signature, the other signature algorithm is untrusted. RFC 5280 states // that the two should be equal, in order to mitigate risk of signature substitution attacks, but also discourages // verifiers from enforcing the profile of RFC 5280. System verifiers are inconsistent - some use the unsigned // signature, some use the signed signature, and they generally do not enforce that both match. This creates // confusion, as it's possible that the signature itself may be checked using algorithm A, but if subsequent // consumers report the certificate algorithm, they may end up reporting algorithm B, which was not used to // verify the certificate. CertVerifyProc enforces that the two signatures match in order to prevent this confusion. Is the VERY long way of wording it, but at least it has the context for future archaeology :)
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:633: // system verifiers don't check this. This can lead to confusion as the On 2017/03/09 01:19:46, Ryan Sleevi wrote: > On 2017/03/09 01:09:43, eroman wrote: > > Acknowledged. > > Was there a comment change you want as well, or just FYI? > > also, mumble-muble, Postel's law... > > Wasn't requesting a comment change, but it was unclear if it was understood why > the verifiers didn't do it (it wasn't obvious to me until a recent discussion > with a CA reminded me of the stupidity of 5280 discouraging enforcing compliance > to 5280 in verifiers) > > // X.509 certificates contain two redundant descriptors for the signature > algorithm; one is covered by the > // signature, but in order to verify the signature, the other signature > algorithm is untrusted. RFC 5280 states > // that the two should be equal, in order to mitigate risk of signature > substitution attacks, but also discourages > // verifiers from enforcing the profile of RFC 5280. System verifiers are > inconsistent - some use the unsigned > // signature, some use the signed signature, and they generally do not enforce > that both match. This creates > // confusion, as it's possible that the signature itself may be checked using > algorithm A, but if subsequent > // consumers report the certificate algorithm, they may end up reporting > algorithm B, which was not used to > // verify the certificate. CertVerifyProc enforces that the two signatures match > in order to prevent this confusion. > > > Is the VERY long way of wording it, but at least it has the context for future > archaeology :) Great comment! I have moved your comment to the implementation file (cert_verify_proc.cc).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, mattm@chromium.org Link to the patchset: https://codereview.chromium.org/2731603002/#ps180001 (title: "Use rsleevi's background comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489038604687420,
"parent_rev": "b66326f26629d4e161ad020ea34052c3f52598af", "commit_rev":
"a77953fe670968fe6728796b77cedf48f0954d78"}
Message was sent while issue was closed.
Description was changed from ========== Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for consistency when verifying certificates. The underlying platform verifiers don't do this, which can lead to confusion when trying to enforce policy for SHA1 on the verified chain. * If the two signature algorithms don't match will fail with ERR_INVALID_CERT. * If the chain contains a signature algorithm that we don't know how to parse, will also fail with ERR_INVALID_CERT BUG=690821 ========== to ========== Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for consistency when verifying certificates. The underlying platform verifiers don't do this, which can lead to confusion when trying to enforce policy for SHA1 on the verified chain. * If the two signature algorithms don't match will fail with ERR_INVALID_CERT. * If the chain contains a signature algorithm that we don't know how to parse, will also fail with ERR_INVALID_CERT BUG=690821 Review-Url: https://codereview.chromium.org/2731603002 Cr-Commit-Position: refs/heads/master@{#455682} Committed: https://chromium.googlesource.com/chromium/src/+/a77953fe670968fe6728796b77ce... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a77953fe670968fe6728796b77ce... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
