New issue
Advanced search Search tips

Issue 634006 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Expose different reasons for Signed Certificate Timestamp invalidity

Project Member Reported by eranm@chromium.org, Aug 3 2016

Issue description

Currently, 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

 

Comment 1 by eranm@chromium.org, Aug 3 2016

I've attempted implementing this change in a single CL:
https://codereview.chromium.org/2208073002
That proved to be a perfect example of the shotgun surgery pattern (https://sourcemaking.com/refactoring/smells/shotgun-surgery).

From an out of bad discussion with estark, it seems we could solve this bug with only three CLs:
- Remove the SCT count from DevTools, because we have more detailed information now.
- Rather than have counts of various SCT states scattered around the code, have a single vector with SCT validation statuses.
- Once there's a single vector, extend the enum with the additional status.
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)
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Comment 5 by eranm@chromium.org, 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).
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 8 2016

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Comment 8 by eranm@chromium.org, 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).
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Labels: M-54
Status: Fixed (was: Assigned)
I don't think there's any more work to be done here, is that right Eran? (Except  issue 640689  of course.)
Project Member

Comment 11 by bugdroid1@chromium.org, 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