Expose different reasons for Signed Certificate Timestamp invalidity |
||
Issue descriptionCurrently, Signed Certificate Timestamps (SCTs) can be marked as invalid by the MultiLogCTVerifier for two reasons: * The signature does not verify: https://cs.chromium.org/chromium/src/net/cert/multi_log_ct_verifier.cc?l=204&cl=GROK * The timestamp in the SCT is in the future: https://cs.chromium.org/chromium/src/net/cert/multi_log_ct_verifier.cc?l=212&cl=GROK That does not allow treating the SCT differently based on the reason it was marked invalid. For example, we do not get metrics on whether the signature over SCTs is invalid (indicating a log may be producing bad SCTs or SCTs are not configured correctly - generally an ecosystem problem) or clocks on clients are wrong. Another example is that Expect-CT reporting could treat invalid SCTs differently depending on the reason they were marked as invalid: SCTs with an invalid signature should not be reported back as they are arbitrary binary blobs that may be used to track/fingerprint clients. SCTs with a timestamp in the future have a valid signature from a log and it may be desirable to report them back in case the timestamp is too far into the future (indicating potential log misconfiguration or misbehaviour). The change that seem to have consensus is extending the SCTVerifyStatus enum to include two "invalid" states. That means making sure all client code treats both invalid states correctly. See the full discussion: https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/bFZQGnZrCkw
,
Aug 3 2016
Elaborating on the plan: As Eran noted, adding a new SCTVerifyStatus gets hairy, because there are places in content/ and chrome/ where we tally SCTs up by their status, ship those tallies around, and elsewhere convert those tallies back into vectors of net::ct::SCTVerifyStatuses. To clean this up, I proposed we do the following things: 1. Right now we ship tallies of net::ct::SCTVerifyStatuses to devtools, which used to be useful, but is now redundant with other information that we show about SCTs. Removing these tallies is https://codereview.chromium.org/2208803002, and it's not strictly necessary for this bug, but I think it's a cleanup we should do regardless. 2. That leaves WebsiteSettings as the consumer of the SCTVerifyStatus tallies that are currently stored on SSLStatus, via ChromeSecurityStateModelClient which converts the tallies back into a vector of SCTVerifyStatuses. It seems silly to convert an SCT list into tallies of statuses just to get converted to a vector of statuses, so https://codereview.chromium.org/2206093004/ changes SSLStatus to carry a vector of statuses instead of a tally for each possible SCTVerifyStatus. Like #1 I think this is a cleanup we should do regardless. 3. Now Eran's CL to add the new SCTVerifyStatus can be much simpler; there will be no more |num_blah_scts| fields scattered all over the place, and no reconstruction of SCTVerifyStatus lists from |num_blah_scts| fields. (where "blah" is every possible SCTVerifyStatus)
,
Aug 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/46add67f297acfab8c50a7b267a15740b70c1ae9 commit 46add67f297acfab8c50a7b267a15740b70c1ae9 Author: estark <estark@chromium.org> Date: Wed Aug 03 23:41:59 2016 Remove SCT counters from DevTools security panel This CL concerns the information that the DevTools security panel shows for the Signed Certificate Timestamps (SCTs) that were served on a request. (SCTs are part of the Certificate Transparency project.) Each SCT has a validation status, and initially (in https://codereview.chromium.org/1589703002), the security panel showed a count of how many SCTs were served with each status. Later, in https://codereview.chromium.org/1772603002, we added the full details of each SCT to the security panel. Thus the counters are now somewhat redundant: we show "X valid SCTs" followed by a summary of each SCT with its validation status. This CL removes the counters ("X valid SCTs, Y invalid SCTs, ..."). While the counters are a little more scannable at a glance, they clutters the UI with redundant information and present an extra burden for maintaining the plumbing needed to show the counters. This is relevant right now because we want to add an additional SCT status. We could rework the plumbing to accommodate this additional SCT status, but it seems to make more sense to just remove the redundant information from the UI. BUG= 591848 , 634006 Review-Url: https://codereview.chromium.org/2208803002 Cr-Commit-Position: refs/heads/master@{#409666} [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/content/child/web_url_loader_impl.cc [delete] https://crrev.com/4005d82163d916d4f3c9d062fafaf33d1831e438/third_party/WebKit/LayoutTests/http/tests/inspector/security/sct-summary-expected.txt [delete] https://crrev.com/4005d82163d916d4f3c9d062fafaf33d1831e438/third_party/WebKit/LayoutTests/http/tests/inspector/security/sct-summary.html [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/third_party/WebKit/Source/core/inspector/browser_protocol.json [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/third_party/WebKit/Source/platform/exported/WebURLResponse.cpp [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/third_party/WebKit/Source/platform/network/ResourceResponse.cpp [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/third_party/WebKit/Source/platform/network/ResourceResponse.h [modify] https://crrev.com/46add67f297acfab8c50a7b267a15740b70c1ae9/third_party/WebKit/public/platform/WebURLResponse.h
,
Aug 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41b454d9282c8dca0800b1d07dee9f38d99b3403 commit 41b454d9282c8dca0800b1d07dee9f38d99b3403 Author: estark <estark@chromium.org> Date: Thu Aug 04 05:10:06 2016 Change SSLStatus to carry a vector of SCT statuses instead of counters SSLStatus consumers only care about the number of SCTs with different statuses, so previously SSLStatus exposed counters: |num_invalid_scts|, |num_unknown_scts|, and |num_valid_scts|. However, in seeking to add a new type of SCT validation status, it became clear that this design is a bit messy: the layers between the net stack and the UI code that consumes the status have to know about all the possible validation statuses. Moreover, we take a list of SCTs and tally them up by status, only to convert those tallies back into a list of statuses. So instead, I've changed SSLStatus to hold a vector of SCTVerifyStatus enums, instead of counters for each possible status. Also note that connections typically have no more than 3 or 4 SCTs, so keeping a vector of SCTVerifyStatus instead of counters will not use a whole lot more memory. This change is based on top of https://codereview.chromium.org/2208803002/. BUG= 634006 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2206093004 Cr-Commit-Position: refs/heads/master@{#409721} [modify] https://crrev.com/41b454d9282c8dca0800b1d07dee9f38d99b3403/chrome/browser/ssl/chrome_security_state_model_client.cc [modify] https://crrev.com/41b454d9282c8dca0800b1d07dee9f38d99b3403/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc [modify] https://crrev.com/41b454d9282c8dca0800b1d07dee9f38d99b3403/content/browser/frame_host/navigation_controller_impl_unittest.cc [modify] https://crrev.com/41b454d9282c8dca0800b1d07dee9f38d99b3403/content/common/ssl_status_serialization.cc [modify] https://crrev.com/41b454d9282c8dca0800b1d07dee9f38d99b3403/content/common/ssl_status_serialization_unittest.cc [modify] https://crrev.com/41b454d9282c8dca0800b1d07dee9f38d99b3403/content/public/common/ssl_status.cc [modify] https://crrev.com/41b454d9282c8dca0800b1d07dee9f38d99b3403/content/public/common/ssl_status.h
,
Aug 4 2016
Now that estark has submitted the two CLs, the plan is as follows: - Migrate the CTVerifyResult to have a single list of SCTs and their statuses. Such a list exists under net/ssl, so it'll be moved to net/cert first, then CTVerifyResult modified to use it. - Then there will be no more call sites that distinguish between SCTs based on the list in CTVerifyResult they're in, and we could add the extra enum (and rename the histogram to).
,
Aug 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158 commit dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158 Author: eranm <eranm@chromium.org> Date: Mon Aug 08 12:08:15 2016 Move signed_certificate_timestamp_and_status from net to cert This is to enable using SignedCertificateTimestampAndStatusList in CTVerifyResult (to replace the three lists of SCTs with a single one). BUG= 634006 Review-Url: https://codereview.chromium.org/2206763005 Cr-Commit-Position: refs/heads/master@{#410332} [modify] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc [modify] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc [modify] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/content/common/resource_messages.h [modify] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/content/public/common/resource_response_info.h [rename] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/net/cert/signed_certificate_timestamp_and_status.cc [rename] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/net/cert/signed_certificate_timestamp_and_status.h [modify] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/net/net.gypi [modify] https://crrev.com/dec1c8a91cbefa9bfd7fcff2d8c9d1e8905ea158/net/ssl/ssl_info.h
,
Aug 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4bed0b57efe07c94d98836f5ae82fa6557e2b262 commit 4bed0b57efe07c94d98836f5ae82fa6557e2b262 Author: eranm <eranm@chromium.org> Date: Sun Aug 14 21:00:35 2016 Certificate Transparency: Change CTVerifyResult to have a single list CTVerifyResult currently has 3 lists of SCTs according to their verification status. As a result, adding a new SCT verification status that doesn't directly map into one of the existing lists loses information. This change switches the CTVerifyResult to have a single 'scts' list that contains SCTs and their statuses, making it easy to extend the SCT verify status enum and generally unifying handling of the SCTs list across the different layers of Chrome. BUG= 634006 Review-Url: https://codereview.chromium.org/2225223002 Cr-Commit-Position: refs/heads/master@{#411924} [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/cert/ct_signed_certificate_timestamp_log_param.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/cert/ct_verify_result.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/cert/ct_verify_result.h [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/cert/multi_log_ct_verifier.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/cert/multi_log_ct_verifier_unittest.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/quic/chromium/crypto/proof_verifier_chromium.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/quic/chromium/crypto/proof_verifier_chromium_test.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/socket/ssl_client_socket_impl.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/ssl/ssl_info.cc [modify] https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262/net/test/ct_test_util.cc
,
Aug 16 2016
(capturing some notes from discussion with estark@ on the matter): If we do end up finding out that the majority of invalid SCTs are invalid because of timestamps being in the future, we could: * Extend the Expect-CT report format to report SCTs that are invalid due to this reason. An SCT timestamp in the future is evidence of potential log misbehaviour, reporting it back to the domain owner is useful in that case (a log could date an SCT far enough into the future so that clients do not attempt to verify it is included in the log and so it can be kept out of the tree for a long time). * Use the later of wallclock time or timestamp from the STH received for that log. Since CT compliance status already affects some certificates (EV certs, Symantec-issued ones, etc) and may affect more in the future, we'd like to reduce the number of clients that incorrectly determine that a certificate is not CT-compliant to a minimum. The certificate validity period is a pretty large window usually (months / years), so a client with a clock that's slightly off may not encounter many SSL errors but could fail to validate SCTs that are particularly new (esp. for websites that rotate certificates + SCTs on a regular basis). Since Chrome clients receive STHs from CT logs via the component updater, clients can effectively rely on the timestamp from the STH for that log (i.e. the current time is definitely after the timestamp from the STH) and when comparing the timestamp in the SCT, compare it to the later of (wall-clock time, timestamp from STH). This is an opportunistic improvement: Not all clients may receive these updates in a timely manner and Chromium embedders will not have them (as a side note, exporting the STH is possible since the CTVerifier holds an instance of an object that has the STHs - the TreeStateTracker. However, that would break encapsulation quite badly and would probably merit a re-design of the CTVerifier / CTLogVerifier / TreeStateTracker).
,
Aug 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/194b45d575cf7168d3875ae1d06960151598093d commit 194b45d575cf7168d3875ae1d06960151598093d Author: eranm <eranm@chromium.org> Date: Thu Aug 18 10:00:33 2016 Distinguish between SCT invalidity reasons in UMA As detailed in the bug, Signed Certificate Timestamps (SCTs) can be marked as invalid for two reasons. This change splits the invalid SCT status into two: * Invalid timestamp. * Invalid signature. Since right now we'd only like to collect data on whether invalidity is due to an ecosystem problem (invalid signatures) or misconfigured clients (invalid timestamp), the change only affects the metrics we collect. All other consumers of the SCT verification status have been adjusted to treat both statuses the same. Per rsleevi's recommendation, the SCTVerifyStatus enum has been expanded and the value for the old invalid status is no longer used. BUG= 634006 Review-Url: https://codereview.chromium.org/2241213002 Cr-Commit-Position: refs/heads/master@{#412779} [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/chrome/browser/ssl/chrome_expect_ct_reporter.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/chrome/browser/ui/website_settings/website_settings.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/chrome/browser/ui/website_settings/website_settings_unittest.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/content/common/ssl_status_serialization.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/content/common/ssl_status_serialization_unittest.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/net/cert/ct_sct_to_string.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/net/cert/multi_log_ct_verifier.cc [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/net/cert/sct_status_flags.h [modify] https://crrev.com/194b45d575cf7168d3875ae1d06960151598093d/tools/metrics/histograms/histograms.xml
,
Aug 30 2016
I don't think there's any more work to be done here, is that right Eran? (Except issue 640689 of course.)
,
Sep 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f commit b082a9ffa25c1bcb6e884379f0f3cf4e0997931f Author: eranm <eranm@chromium.org> Date: Tue Sep 13 21:03:26 2016 Certificate Transparency: Remove the obsolete invalid sct status. Remove the obsolete enum value SCT_STATUS_INVALID which was replaced by two distinct enum values. To avoid crashing Chrome clients which have entries cached on disk with the obsolete enum value, fail to de-serialize such cache entries. This reverts commit 321ed2a53224c50af40387e8211726f8400ecad2. BUG=640296, 634006 , 640689 Review-Url: https://codereview.chromium.org/2294373002 Cr-Commit-Position: refs/heads/master@{#418367} [modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/chrome/browser/ssl/chrome_expect_ct_reporter.cc [modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/chrome/browser/ui/website_settings/website_settings.cc [modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/cert/ct_sct_to_string.cc [add] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/cert/sct_status_flags.cc [modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/cert/sct_status_flags.h [modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/http/http_response_info.cc [modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/http/http_response_info_unittest.cc [modify] https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f/net/net.gypi |
||
►
Sign in to add a comment |
||
Comment 1 by eranm@chromium.org
, Aug 3 2016