Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
412263003:
Add SHA-256 fingerprint functions to x509 certs. (Closed)
|
Created:
6 years, 5 months ago by jww Modified:
6 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd SHA-256 fingerprint functions to x509 certs.
Previously, the x509 cert implementation only had SHA-1 based fingerprinting for
quickly creating indexing appropriate hashes. Since SHA-1 is likely
cryptographically broken, these fingerprints should not be used for any security
decisions. This adds slow SHA-256 fingerprint support for when fingerprints are
needed for security decisions.
R=rsleevi@chromium.org
BUG=262615
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285851
Patch Set 1 #Patch Set 2 : Add comment to CalculateCAFingerprint256 #
Total comments: 5
Patch Set 3 : sleevi nit fix #Patch Set 4 : Small fix from trybot failures #
Total comments: 10
Messages
Total messages: 20 (0 generated)
Ryan, this is the CL we discussed adding SHA-256 fingerprints.
On 2014/07/24 20:56:43, jww wrote: > Ryan, this is the CL we discussed adding SHA-256 fingerprints. Hi wtc@, can you do this review? We need this for https://codereview.chromium.org/369703002/ to replace our use of SHA-1 for fingerprinting, and rsleevi@ suggested you'd be a good reviewer for it. Thanks!
https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... net/cert/x509_certificate.cc:721: if (!GetDEREncoded(intermediates[i], &der_encoded)) The only benefit to the platform implementation (and I think you'd still call crypto::SecureHash, FWIW) is that you could avoid the extra copy to get the cert data.
On 2014/07/25 00:21:34, Ryan Sleevi wrote: > https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... > File net/cert/x509_certificate.cc (right): > > https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... > net/cert/x509_certificate.cc:721: if (!GetDEREncoded(intermediates[i], > &der_encoded)) > The only benefit to the platform implementation (and I think you'd still call > crypto::SecureHash, FWIW) is that you could avoid the extra copy to get the cert > data. And I'm pretty sure for our initial use case, the extra complexity isn't worth it, right?
On 2014/07/25 00:25:22, jww wrote: > On 2014/07/25 00:21:34, Ryan Sleevi wrote: > > > https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... > > File net/cert/x509_certificate.cc (right): > > > > > https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... > > net/cert/x509_certificate.cc:721: if (!GetDEREncoded(intermediates[i], > > &der_encoded)) > > The only benefit to the platform implementation (and I think you'd still call > > crypto::SecureHash, FWIW) is that you could avoid the extra copy to get the > cert > > data. > > And I'm pretty sure for our initial use case, the extra complexity isn't worth > it, right? "Meh" :)
Review comments on patch set 2: https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... net/cert/x509_certificate.cc:711: SHA256HashValue X509Certificate::CalculateCAFingerprint256( You can move the code of this function to your code. Every function you use is public.
https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... net/cert/x509_certificate.cc:711: SHA256HashValue X509Certificate::CalculateCAFingerprint256( On 2014/07/26 01:25:12, wtc wrote: > > You can move the code of this function to your code. Every function you use is > public. sleevi suggested that we want this as a generic way, going forward, of having secure fingerprints for x509 certs. True enough that we can move this to our specific code for now, but it seems likely that we'll want secure fingerprints elsewhere in the future, no?
On 2014/07/26 01:30:47, jww wrote: > https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... > File net/cert/x509_certificate.cc (right): > > https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... > net/cert/x509_certificate.cc:711: SHA256HashValue > X509Certificate::CalculateCAFingerprint256( > On 2014/07/26 01:25:12, wtc wrote: > > > > You can move the code of this function to your code. Every function you use is > > public. > > sleevi suggested that we want this as a generic way, going forward, of having > secure fingerprints for x509 certs. True enough that we can move this to our > specific code for now, but it seems likely that we'll want secure fingerprints > elsewhere in the future, no? Yeah. palmer and I need to fix this for HPKP, which is going to permeate throughout //net. So let's keep it here.
LGTM (keeping it here in //net) Sorry for the many reviewers. I'll take it on myself to tweak it for performance when we add it to the hot-path. https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificate.h File net/cert/x509_certificate.h (right): https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... net/cert/x509_certificate.h:419: static SHA256HashValue CalculateFullChainFingerprint256( s/FullChain/Chain
https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificate.h File net/cert/x509_certificate.h (right): https://codereview.chromium.org/412263003/diff/20001/net/cert/x509_certificat... net/cert/x509_certificate.h:419: static SHA256HashValue CalculateFullChainFingerprint256( On 2014/07/26 01:56:21, Ryan Sleevi wrote: > s/FullChain/Chain Done.
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/412263003/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33714) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38725) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/412263003/60001
Message was sent while issue was closed.
Change committed as 285851
Message was sent while issue was closed.
Review comments on patch set 4: 1. These SHA-256 fingerprint functions don't seem to belong in a minimal API because they can be implemented using existing public functions. 2. The way OSCertHandles is hashed is just one possible definition -- another possible definition would include the number of certificates in the hash, similar to the code in X509Certificate::Persist. So it's not clear if the current definition is generally useful. I suggest some small changes below. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.cc:723: hash->Update(der_encoded.c_str(), der_encoded.length()); Nit: use data() instead of c_str() because we don't need a C string here. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificate.h File net/cert/x509_certificate.h (right): https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.h:408: // The implementation currently relies on the crypto::SecureHash utilities, Nit: it seems that the performance issue is caused by the need to copy the certificate bytes into local variables, rather than the use of crypto::SecureHash. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.h:419: static SHA256HashValue CalculateChainFingerprint256( For a minimal API, this method can be subsumed by the previous method, and the previous method can be renamed to avoid reference to CA certificates, for example, static SHA256HashValue CalculateCertArrayFingerprint256( const OSCertHandles& cert_handles); or static SHA256HashValue CalculateOSCertListFingerprint256( const OSCertHandles& cert_handles); https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.h:420: const OSCertHandle& leaf, Please pass a single OSCertHandle by value. It is a (platform-dependent) pointer type. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... File net/cert/x509_certificate_unittest.cc (right): https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate_unittest.cc:416: static const uint8 cert_chain1_full_chain_fingerprint_256[32] = { Nit: since you renamed the CalculateFullChainFingerprint256 function (s/FullChain/Chain), these variable names should also be renamed to match.
Message was sent while issue was closed.
I've uploaded a new CL to address the nits: https://codereview.chromium.org/421243003/ I explicitly didn't change anything regarding the two higher level points as I think they probably need more discussion, which we should probably have on the new CL. I'll respond on them there. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.cc:723: hash->Update(der_encoded.c_str(), der_encoded.length()); On 2014/07/28 17:29:52, wtc wrote: > > Nit: use data() instead of c_str() because we don't need a C string here. Done. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificate.h File net/cert/x509_certificate.h (right): https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.h:408: // The implementation currently relies on the crypto::SecureHash utilities, On 2014/07/28 17:29:52, wtc wrote: > > Nit: it seems that the performance issue is caused by the need to copy the > certificate bytes into local variables, rather than the use of > crypto::SecureHash. Done. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.h:419: static SHA256HashValue CalculateChainFingerprint256( On 2014/07/28 17:29:52, wtc wrote: > > For a minimal API, this method can be subsumed by the previous method, and the > previous method can be renamed to avoid reference to CA certificates, for > example, > > static SHA256HashValue CalculateCertArrayFingerprint256( > const OSCertHandles& cert_handles); > > or > > static SHA256HashValue CalculateOSCertListFingerprint256( > const OSCertHandles& cert_handles); This would make it inconsistent with the SHA1 API, so I'll let you and rsleevi duke it out on the new CL :-) https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate.h:420: const OSCertHandle& leaf, On 2014/07/28 17:29:52, wtc wrote: > > Please pass a single OSCertHandle by value. It is a (platform-dependent) pointer > type. Done. https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... File net/cert/x509_certificate_unittest.cc (right): https://codereview.chromium.org/412263003/diff/60001/net/cert/x509_certificat... net/cert/x509_certificate_unittest.cc:416: static const uint8 cert_chain1_full_chain_fingerprint_256[32] = { On 2014/07/28 17:29:52, wtc wrote: > > Nit: since you renamed the CalculateFullChainFingerprint256 function > (s/FullChain/Chain), these variable names should also be renamed to match. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
