Distinguish between SCT invalidity reasons in UMA

net/cert/sct_status_flags.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_CERT_SCT_STATUS_FLAGS_H_
 #define NET_CERT_SCT_STATUS_FLAGS_H_
 
 namespace net {
 
 namespace ct {
 
 // The possible verification statuses for a SignedCertificateTimestamp.
 // Note: The numeric values are used within histograms and should not change
 // or be re-assigned.
 enum SCTVerifyStatus {
   // Not a real status, this just prevents a default int value from being
   // mis-interpreseted as a valid status.
   // Also used to count SCTs that cannot be decoded in the histogram.
   SCT_STATUS_NONE = 0,
 
   // The SCT is from an unknown log, so we cannot verify its signature.
   SCT_STATUS_LOG_UNKNOWN = 1,
 
-  // The SCT is from a known log, but the signature is invalid.
-  SCT_STATUS_INVALID = 2,
+  // SCTVerifyStatus=2 used to represent SCT_STATUS_INVALID, which has now been
+  // split into INVALID_SIGNATURE and INVALID_TIMESTAMP to represent the
+  // different reasons an SCT could be invalid.
 
   // The SCT is from a known log, and the signature is valid.
   SCT_STATUS_OK = 3,
 
+  // The SCT is from a known log, but the signature is invalid.
+  SCT_STATUS_INVALID_SIGNATURE = 4,
+
+  // The SCT is from a known log, but the timestamp is in the future.
+  SCT_STATUS_INVALID_TIMESTAMP = 5,
+
   // Used to bound the enum values.
   SCT_STATUS_MAX,

[Avi (use Gerrit), 2016/08/16 15:25:47]

That's not how MAX works; you assign MAX to the last valid value in the enum:

SCT_STATUS_MAX = SCT_STATUS_INVALID_TIMESTAMP,

(See https://cs.chromium.org/search/?q=FILTER_TYPE_LAST or
https://cs.chromium.org/search/?q=EVENT_MEDIA_TYPE_LAST or any number of other
enums.)

The issue is that SCT_STATUS_MAX is used in content/common/resource_messages.h
with:

IPC_ENUM_TRAITS_MAX_VALUE(net::ct::SCTVerifyStatus, net::ct::SCT_STATUS_MAX)

but if you look at the definition of IPC_ENUM_TRAITS_MAX_VALUE it says:

// Convenience macro for defining enumerated type traits for types which are
// range-checked by the IPC system to be in the range of 0..maxvalue inclusive.
// This macro should not need to be subsequently redefined.

so we're currently allowing the wrong range of enum values to pass over IPC.

We should fix this here in this CL.

[Eran Messeri, 2016/08/16 16:06:09]

Fixed.
The discrepancy comes, I believe, from using the same enum for UMA and IPC. The
convention when used for UMA is to have a _MAX value that's after the last valid
value (since UMA_HISTOGRAM_ENUMERATION  states it expects values that are
strictly less than the boundary value).

 };
 
 }  // namespace ct
 
 }  // namespace net
 
 #endif  // NET_CERT_SCT_STATUS_FLAGS_H_