Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(252)

Issue 2000503002: Remove the fingerprint and ca_fingerprint from X509Certificate (Closed)

Created:
4 years, 7 months ago by Ryan Sleevi
Modified:
4 years, 6 months ago
CC:
anandc+watch-blimp_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, grt+watch_chromium.org, jessicag+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@move_cache
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove the fingerprint and ca_fingerprint from X509Certificate X509Certificate provided a function to get the SHA-1 hash of the certificate data and the SHA-1 hash of the intermediates. This was largely for sorting optimizations, but was never intended to be a substitute for true equality checks (namely, IsSameOSCert()). However, because X509Certificate::LessThan used these, the comparison of two X509Certificates was less secure than desired. This removes the fingerprint members and the ability to publicly compute the SHA-1 hash of the certificate/intermediates. Callers can instead compute the SHA-256 fingerprint using X509Certificate::Calculate[CA]Fingerprint256 to obtain the equivalent SHA256HashValue fingerprint. This also optimizes CalculateCAFingerprint256 to avoid additional copies, by moving it to the platform-native implementation. BUG=613460 Committed: https://crrev.com/de70f5cff2a6e16cda89b15fadd75cba11694fc3 Cr-Commit-Position: refs/heads/master@{#400904}

Patch Set 1 #

Total comments: 17

Patch Set 2 : Rebased #

Patch Set 3 : Feedback #

Total comments: 14

Patch Set 4 : Feedback #

Patch Set 5 : I'm an idiot #

Total comments: 1

Patch Set 6 : More iOS & Style fixes #

Patch Set 7 : More iOS fixes #

Patch Set 8 : Rebased #

Patch Set 9 : Win & iOS fix #

Patch Set 10 : Rebased #

Patch Set 11 : Fix Win & iOS #

Total comments: 6

Patch Set 12 : Make iOS Great Again #

Patch Set 13 : iOS fix #

Total comments: 5

Patch Set 14 : Feedback #

Patch Set 15 : Rebased #

Patch Set 16 : Speculative iOS fix #

Patch Set 17 : Fix IDN test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+427 lines, -571 lines) Patch
M blimp/net/exact_match_cert_verifier.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/safe_browsing/download_protection_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/safe_browsing/download_protection_service_unittest.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -3 lines 0 comments Download
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -2 lines 0 comments Download
M chromeos/network/onc/onc_certificate_importer_impl_unittest.cc View 1 2 2 chunks +10 lines, -2 lines 0 comments Download
M content/browser/cert_store_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +26 lines, -1 line 0 comments Download
M content/browser/cert_store_impl.cc View 1 2 3 4 5 1 chunk +26 lines, -6 lines 0 comments Download
M ios/web/navigation/crw_session_certificate_policy_manager.mm View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +9 lines, -3 lines 0 comments Download
M ios/web/net/cert_host_pair.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +11 lines, -5 lines 0 comments Download
M ios/web/net/cert_host_pair.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +10 lines, -6 lines 0 comments Download
M ios/web/net/cert_host_pair_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +16 lines, -8 lines 0 comments Download
M ios/web/net/cert_policy.cc View 1 2 3 4 5 6 2 chunks +5 lines, -5 lines 0 comments Download
M ios/web/net/cert_store_impl.h View 1 2 3 4 5 1 chunk +22 lines, -1 line 0 comments Download
M ios/web/net/cert_store_impl.cc View 1 2 3 4 5 1 chunk +26 lines, -8 lines 0 comments Download
M ios/web/net/request_tracker_impl_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +4 lines, -4 lines 0 comments Download
M ios/web/public/cert_policy.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M net/cert/cert_verify_proc_nss.cc View 7 chunks +5 lines, -48 lines 0 comments Download
M net/cert/cert_verify_proc_unittest.cc View 1 3 chunks +6 lines, -11 lines 0 comments Download
M net/cert/cert_verify_proc_win.cc View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -3 lines 0 comments Download
M net/cert/nss_cert_database_unittest.cc View 1 2 3 4 5 6 7 2 chunks +9 lines, -1 line 0 comments Download
M net/cert/nss_profile_filter_chromeos_unittest.cc View 1 2 2 chunks +11 lines, -1 line 0 comments Download
M net/cert/x509_certificate.h View 1 2 4 chunks +0 lines, -43 lines 0 comments Download
M net/cert/x509_certificate.cc View 1 2 3 9 chunks +7 lines, -46 lines 0 comments Download
M net/cert/x509_certificate_ios.cc View 1 3 chunks +10 lines, -27 lines 0 comments Download
M net/cert/x509_certificate_mac.cc View 4 chunks +10 lines, -33 lines 0 comments Download
M net/cert/x509_certificate_nss.cc View 1 3 chunks +12 lines, -30 lines 0 comments Download
M net/cert/x509_certificate_openssl.cc View 1 3 chunks +9 lines, -21 lines 0 comments Download
M net/cert/x509_certificate_unittest.cc View 1 9 chunks +151 lines, -212 lines 0 comments Download
M net/cert/x509_certificate_win.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +10 lines, -30 lines 0 comments Download
M net/ssl/client_cert_store_mac.cc View 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 56 (17 generated)
Ryan Sleevi
eroman: Since you were poking me about it (and Thanks!) with respect to the CertVerifier::RequestParams ...
4 years, 7 months ago (2016-05-20 06:02:32 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000503002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000503002/1
4 years, 7 months ago (2016-05-20 06:03:03 UTC) #4
commit-bot: I haz the power
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/builds/9188) ios-simulator on ...
4 years, 7 months ago (2016-05-20 06:05:32 UTC) #6
Ryan Sleevi
Based on Emily's remarks, I'll update the CertStoreImpl to memoize, but would be good to ...
4 years, 7 months ago (2016-05-25 07:13:26 UTC) #7
eroman
+1 to removing the baked-in fingerprints. Providing them only where needed, and memoizing as part ...
4 years, 7 months ago (2016-05-26 01:46:26 UTC) #8
Ryan Sleevi
https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.cc File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.cc#newcode140 net/cert/x509_certificate.cc:140: X509Certificate::IsSameOSCert(*cert_handle, pos->second.cert_handle); On 2016/05/26 01:46:26, eroman wrote: > If ...
4 years, 7 months ago (2016-05-26 07:48:38 UTC) #9
eroman
https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.cc File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.cc#newcode140 net/cert/x509_certificate.cc:140: X509Certificate::IsSameOSCert(*cert_handle, pos->second.cert_handle); On 2016/05/26 07:48:38, Ryan Sleevi (OOO til ...
4 years, 7 months ago (2016-05-26 20:28:55 UTC) #10
Ryan Sleevi
On 2016/05/26 20:28:55, eroman wrote: > If that were computationally feasible, then we have bigger ...
4 years, 7 months ago (2016-05-26 20:49:44 UTC) #11
eroman
I haven't looked over CL 1991653002 yet (had back-burnered it due to WIP label), apologies ...
4 years, 7 months ago (2016-05-27 06:54:13 UTC) #12
eroman
I see, so your intent is to use a cryptographic hash function (SHA256) simply in ...
4 years, 6 months ago (2016-05-30 19:40:46 UTC) #13
eroman
lgtm https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.cc File net/cert/x509_certificate.cc (right): https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.cc#newcode246 net/cert/x509_certificate.cc:246: return std::lexicographical_compare( This is worth noting in the ...
4 years, 6 months ago (2016-05-30 20:20:30 UTC) #14
Ryan Sleevi
https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.h File net/cert/x509_certificate.h (left): https://codereview.chromium.org/2000503002/diff/1/net/cert/x509_certificate.h#oldcode392 net/cert/x509_certificate.h:392: // but callers should use IsSameOSCert() before assuming two ...
4 years, 6 months ago (2016-05-31 15:40:13 UTC) #15
Ryan Sleevi
Eric: I chatted with davidben while you're out, and I'm actually going to ask you ...
4 years, 6 months ago (2016-06-09 01:20:07 UTC) #16
eroman
lgtm after addressing https://codereview.chromium.org/2000503002/diff/40001/ios/web/navigation/crw_session_certificate_policy_manager.mm#newcode64 https://codereview.chromium.org/2000503002/diff/40001/content/browser/cert_store_impl.cc File content/browser/cert_store_impl.cc (right): https://codereview.chromium.org/2000503002/diff/40001/content/browser/cert_store_impl.cc#newcode35 content/browser/cert_store_impl.cc:35: hash_and_cert->chain_hash = I would say ...
4 years, 6 months ago (2016-06-09 22:18:25 UTC) #17
Ryan Sleevi
https://codereview.chromium.org/2000503002/diff/40001/content/browser/cert_store_impl.cc File content/browser/cert_store_impl.cc (right): https://codereview.chromium.org/2000503002/diff/40001/content/browser/cert_store_impl.cc#newcode35 content/browser/cert_store_impl.cc:35: hash_and_cert->chain_hash = On 2016/06/09 22:18:25, eroman wrote: > I ...
4 years, 6 months ago (2016-06-09 22:59:37 UTC) #18
Ryan Sleevi
Updated because I'm an idiot
4 years, 6 months ago (2016-06-09 23:08:01 UTC) #19
eroman
lgtm https://codereview.chromium.org/2000503002/diff/80001/ios/web/navigation/crw_session_certificate_policy_manager.mm File ios/web/navigation/crw_session_certificate_policy_manager.mm (right): https://codereview.chromium.org/2000503002/diff/80001/ios/web/navigation/crw_session_certificate_policy_manager.mm#newcode64 ios/web/navigation/crw_session_certificate_policy_manager.mm:64: return hashCompare_(lhs.certificateHash, rhs.certificateHash); Why is it that we ...
4 years, 6 months ago (2016-06-09 23:16:17 UTC) #20
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000503002/200001
4 years, 6 months ago (2016-06-16 01:00:56 UTC) #22
Ryan Sleevi
nparker: //chrome/browser/safe_browsing jochen: //content stevenjb: //chromeos eugenebut: //ios wez: //blimp Hopefully the CL description captures ...
4 years, 6 months ago (2016-06-16 01:02:46 UTC) #24
Eugene But (OOO till 7-30)
https://codereview.chromium.org/2000503002/diff/200001/ios/web/navigation/crw_session_certificate_policy_manager.mm File ios/web/navigation/crw_session_certificate_policy_manager.mm (right): https://codereview.chromium.org/2000503002/diff/200001/ios/web/navigation/crw_session_certificate_policy_manager.mm#newcode67 ios/web/navigation/crw_session_certificate_policy_manager.mm:67: net::SHA256HashValueLessThan hashCompare_; Optional NIT: Since you touching this could ...
4 years, 6 months ago (2016-06-16 01:14:34 UTC) #25
Ryan Sleevi
https://codereview.chromium.org/2000503002/diff/200001/ios/web/navigation/crw_session_certificate_policy_manager.mm File ios/web/navigation/crw_session_certificate_policy_manager.mm (right): https://codereview.chromium.org/2000503002/diff/200001/ios/web/navigation/crw_session_certificate_policy_manager.mm#newcode67 ios/web/navigation/crw_session_certificate_policy_manager.mm:67: net::SHA256HashValueLessThan hashCompare_; On 2016/06/16 01:14:34, Eugene But wrote: > ...
4 years, 6 months ago (2016-06-16 01:19:44 UTC) #26
Wez
blimp/ LGTM
4 years, 6 months ago (2016-06-16 01:28:35 UTC) #27
Ryan Sleevi
Eugene: PTAL. Now with more std::move()ing
4 years, 6 months ago (2016-06-16 01:36:09 UTC) #28
Eugene But (OOO till 7-30)
Thanks! ios lgtm https://codereview.chromium.org/2000503002/diff/200001/ios/web/navigation/crw_session_certificate_policy_manager.mm File ios/web/navigation/crw_session_certificate_policy_manager.mm (right): https://codereview.chromium.org/2000503002/diff/200001/ios/web/navigation/crw_session_certificate_policy_manager.mm#newcode67 ios/web/navigation/crw_session_certificate_policy_manager.mm:67: net::SHA256HashValueLessThan hashCompare_; On 2016/06/16 01:19:44, Ryan ...
4 years, 6 months ago (2016-06-16 01:36:23 UTC) #29
Eugene But (OOO till 7-30)
As you could guess I replied without seeing the latest changes. Latest ios changes lgtm!
4 years, 6 months ago (2016-06-16 01:39:29 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000503002/220001
4 years, 6 months ago (2016-06-16 01:47:14 UTC) #32
commit-bot: I haz the power
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/builds/21955)
4 years, 6 months ago (2016-06-16 02:03:55 UTC) #34
stevenjb
chromeos/ lgtm https://codereview.chromium.org/2000503002/diff/240001/chromeos/network/onc/onc_certificate_importer_impl_unittest.cc File chromeos/network/onc/onc_certificate_importer_impl_unittest.cc (right): https://codereview.chromium.org/2000503002/diff/240001/chromeos/network/onc/onc_certificate_importer_impl_unittest.cc#newcode194 chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:194: }); This seems potentially expensive since we ...
4 years, 6 months ago (2016-06-16 16:39:23 UTC) #35
Nathan Parker
lgtm for safe_browsing, with one question https://codereview.chromium.org/2000503002/diff/240001/chrome/browser/safe_browsing/download_protection_service.cc File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2000503002/diff/240001/chrome/browser/safe_browsing/download_protection_service.cc#newcode1675 chrome/browser/safe_browsing/download_protection_service.cc:1675: net::X509Certificate::GetDEREncoded(issuer.os_cert_handle(), &issuer_der); Can ...
4 years, 6 months ago (2016-06-16 18:32:24 UTC) #36
Ryan Sleevi
https://codereview.chromium.org/2000503002/diff/240001/chrome/browser/safe_browsing/download_protection_service.cc File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2000503002/diff/240001/chrome/browser/safe_browsing/download_protection_service.cc#newcode1675 chrome/browser/safe_browsing/download_protection_service.cc:1675: net::X509Certificate::GetDEREncoded(issuer.os_cert_handle(), &issuer_der); On 2016/06/16 18:32:23, Nathan Parker wrote: > ...
4 years, 6 months ago (2016-06-16 21:16:45 UTC) #37
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000503002/240001
4 years, 6 months ago (2016-06-16 22:29:37 UTC) #39
jochen (gone - plz use gerrit)
lgtm with comment https://codereview.chromium.org/2000503002/diff/240001/content/browser/cert_store_impl.h File content/browser/cert_store_impl.h (right): https://codereview.chromium.org/2000503002/diff/240001/content/browser/cert_store_impl.h#newcode56 content/browser/cert_store_impl.h:56: }; disallow copy & assign?
4 years, 6 months ago (2016-06-16 22:32:16 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000503002/260001
4 years, 6 months ago (2016-06-16 23:09:41 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/builds/22600)
4 years, 6 months ago (2016-06-16 23:59:31 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000503002/300001
4 years, 6 months ago (2016-06-21 02:02:16 UTC) #48
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/242162)
4 years, 6 months ago (2016-06-21 03:10:31 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000503002/320001
4 years, 6 months ago (2016-06-21 04:36:54 UTC) #53
commit-bot: I haz the power
Committed patchset #17 (id:320001)
4 years, 6 months ago (2016-06-21 06:00:42 UTC) #54
commit-bot: I haz the power
4 years, 6 months ago (2016-06-21 06:02:27 UTC) #56
Message was sent while issue was closed.
Patchset 17 (id:??) landed as
https://crrev.com/de70f5cff2a6e16cda89b15fadd75cba11694fc3
Cr-Commit-Position: refs/heads/master@{#400904}

Powered by Google App Engine
This is Rietveld 408576698