Minor improvements to ct_serialization.cc

net/cert/ct_serialization.cc

 // 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.
 
 #include "net/cert/ct_serialization.h"
 
 #include <stdint.h>
 
 #include <limits>
 
 #include "base/logging.h"
+#include "base/numerics/safe_math.h"
 
 namespace net {
 
 namespace ct {
 
 namespace {
 
 // Note: length is always specified in bytes.
 // Signed Certificate Timestamp (SCT) Version length
 const size_t kVersionLength = 1;
@@ skipping 37 matching lines @@
   T result = 0;
   for (size_t i = 0; i < length; ++i) {
     result = (result << 8) | static_cast<unsigned char>((*in)[i]);
   }
   in->remove_prefix(length);
   *out = result;
   return true;
 }
 
 // Reads a TLS-encoded field length from |in|.
-// The bytes read from |in| are discarded (i.e. |in|'s prefix removed)
-// |prefix_length| indicates the bytes needed to represent the length (e.g. 3)
+// The bytes read from |in| are discarded (i.e. |in|'s prefix removed).
+// |prefix_length| indicates the bytes needed to represent the length (e.g. 3).
+// Max |prefix_length| is 8.
 // success, returns true and stores the result in |*out|.
 bool ReadLength(size_t prefix_length, base::StringPiece* in, size_t* out) {
-  size_t length;
+  uint64_t length = 0;
   if (!ReadUint(prefix_length, in, &length))
     return false;
-  *out = length;
+  base::CheckedNumeric<size_t> checked_length = length;
+  if (!checked_length.IsValid())
+    return false;
+  *out = checked_length.ValueOrDie();
   return true;
 }
 
 // Reads |length| bytes from |*in|. If |*in| is too small, returns false.
 // The bytes read from |in| are discarded (i.e. |in|'s prefix removed)
 bool ReadFixedBytes(size_t length,
                     base::StringPiece* in,
                     base::StringPiece* out) {
   if (in->length() < length)
     return false;
   out->set(in->data(), length);
   in->remove_prefix(length);
   return true;
 }
 
 // Reads a length-prefixed variable amount of bytes from |in|, updating |out|
 // on success. |prefix_length| indicates the number of bytes needed to represent
 // the length.
 // The bytes read from |in| are discarded (i.e. |in|'s prefix removed)
 bool ReadVariableBytes(size_t prefix_length,
                        base::StringPiece* in,
                        base::StringPiece* out) {
-  size_t length;
+  size_t length = 0;
   if (!ReadLength(prefix_length, in, &length))
     return false;
   return ReadFixedBytes(length, in, out);
 }
 
 // Reads a variable-length list that has been TLS encoded.
 // The bytes read from |in| are discarded (i.e. |in|'s prefix removed)
 // |max_list_length| contains the overall length of the encoded list.
 // |max_item_length| contains the maximum length of a single item.
 // On success, returns true and updates |*out| with the encoded list.
@@ skipping 67 matching lines @@
 
 // Writes a TLS-encoded variable length unsigned integer to |output|.
 // |length| indicates the size (in bytes) of the integer.
 // |value| the value itself to be written.
 template <typename T>
 void WriteUint(size_t length, T value, std::string* output) {
   DCHECK_LE(length, sizeof(T));
   DCHECK(length == sizeof(T) || value >> (length * 8) == 0);
 
   for (; length > 0; --length) {
-    output->push_back((value >> ((length - 1)* 8)) & 0xFF);
+    output->push_back((value >> ((length - 1) * 8)) & 0xFF);
   }

[Ryan Sleevi, 2016/01/13 02:44:32]

What if we:
Delete both DCHECKs

DCHECK(length >= sizeof(T) || value >> (length * 8) == 0);

size_t bytes_to_write = std::min(length, sizeof(value));
output->insert(output.end(), length - bytes_to_write, 0);
for (; bytes_to_write > 0; --bytes_to_write) {
  output->push_back((value >> ((bytes_to_write - 1) * 8)) & 0xFF);
}

Let's cover possible permutations to sanity check:
length = 3, sizeof(T) == 4:
  - value >> (24) == 0 must be true (DCHECK)
  - bytes_to_write = 3 [length]
  - length - 3 = 0 (so no extra padding)
  - We write 3 bytes

length = 4, sizeof(T) == 4:
  - length == sizeof(T) is true (DCHECK)
  - bytes_to_write = 4 [length or sizeof(T)]
  - length - 4 = 0 (so no extra padding)
  - We write 4 bytes

length = 8, sizeof(T) == 4
  - length > sizeof(T) is true (DCHECK)
  - bytes_to_write = 4 [sizeof(T)]
  - length - 4 = 4 (so 4 bytes of leading zeroes are added to the end)
  - We write 4 additional bytes, the value of T

that should cover the <, =, and > case.

What are your thoughts on that approach? Too much complexity?

I mean, it's unclear to me whether the DCHECK_LE on line 188 is strictly
necessary. That said, I'm not fundamentally opposed to the uint64_t, but I mean,
there's weird stuff (for example, line 221 seems weird - we know
sizeof(input_size) - it's 8)

Alternatively, we could just change the template for WriteUint to not be
variable on type T, explicitly code it to take |value| as a uint64_t (and let
size extension Just Work), and instead just DCHECK that value >> (length * 8) ==
0

Either approach seems more... consistent. But it's late, perhaps I've missed
something.

 }
 
 // Writes an array to |output| from |input|.
 // Should be used in one of two cases:
 // * The length of |input| has already been encoded into the |output| stream.
 // * The length of |input| is fixed and the reader is expected to specify that
 // length when reading.
 // If the length of |input| is dynamic and data is expected to follow it,
 // WriteVariableBytes must be used.
-void WriteEncodedBytes(const base::StringPiece& input, std::string* output) {
+// Returns the number of bytes written (the length of |input|).
+size_t WriteEncodedBytes(const base::StringPiece& input, std::string* output) {
   input.AppendToString(output);
+  return input.size();
 }
 
 // Writes a variable-length array to |output|.
 // |prefix_length| indicates the number of bytes needed to represnt the length.

[Ryan Sleevi, 2016/01/12 00:10:15]

typo: represent

[Rob Percival, 2016/01/13 00:55:52]

Done.

 // |input| is the array itself.
-// If the size of |input| is less than 2^|prefix_length| - 1, encode the
-// length and data and return true. Otherwise, return false.
+// If 1 <= |prefix_length| <= 8 and the size of |input| is less than
+// 2^|prefix_length| - 1, encode the length and data and return true.
+// Otherwise, return false.
 bool WriteVariableBytes(size_t prefix_length,
                         const base::StringPiece& input,
                         std::string* output) {
-  size_t input_size = input.size();
-  size_t max_allowed_input_size =
-      static_cast<size_t>(((1 << (prefix_length * 8)) - 1));
+  // Use uint64_t instead of size_t to allow a |prefix_length| up to 8 bytes,
+  // even on 32-bit builds.
+  uint64_t input_size = input.size();
+  if (prefix_length > sizeof(input_size))
+    return false;
+
+  uint64_t max_allowed_input_size = ((1 << (prefix_length * 8)) - 1);
   if (input_size > max_allowed_input_size)
     return false;

[Rob Percival, 2016/01/20 20:20:33]

It's not clear to me why WriteVariableBytes returns false when prefix_length is
too small, but WriteUint DCHECKs. Any thoughts? Should I make them both either
DCHECK or return a bool?

[Ryan Sleevi, 2016/01/20 20:56:37]

I believe the logic was because WriteVariableBytes was presumed to be affected
more by 'user input' than WriteUint.

Even with the DCHECK, for unsigned types, WriteUint still *did the right thing*
(since value of a unsigned integer shifted than greater its size just
zero-extended and thus added zero padding)

I don't know if the perceived inconsistency is worth fixing, in that it results
in correct behaviour but guards against obvious badness. If you strongly feel it
is, that's really for a separate CL.

 
-  WriteUint(prefix_length, input.size(), output);
+  WriteUint(prefix_length, input_size, output);
   WriteEncodedBytes(input, output);
 
   return true;
 }
 
 // Writes a LogEntry of type X.509 cert to |output|.
 // |input| is the LogEntry containing the certificate.
 // Returns true if the leaf_certificate in the LogEntry does not exceed
 // kMaxAsn1CertificateLength and so can be written to |output|.
 bool EncodeAsn1CertLogEntry(const LogEntry& input, std::string* output) {
@@ skipping 57 matching lines @@
   WriteUint(kLogEntryTypeLength, input.type, output);
   switch (input.type) {
     case LogEntry::LOG_ENTRY_TYPE_X509:
       return EncodeAsn1CertLogEntry(input, output);
     case LogEntry::LOG_ENTRY_TYPE_PRECERT:
       return EncodePrecertLogEntry(input, output);
   }
   return false;
 }
 
+static bool ReadTimeSinceEpoch(base::StringPiece* input, base::Time* output) {
+  uint64_t time_since_epoch = 0;
+  if (!ReadUint(kTimestampLength, input, &time_since_epoch)) {
+    return false;
+  }
+
+  base::CheckedNumeric<int64_t> time_since_epoch_signed = time_since_epoch;
+
+  if (!time_since_epoch_signed.IsValid()) {
+    DVLOG(1) << "Timestamp value too big to cast to int64_t: "
+             << time_since_epoch;
+    return false;
+  }
+
+  *output =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(time_since_epoch_signed.ValueOrDie());
+
+  return true;
+}
+
 static void WriteTimeSinceEpoch(const base::Time& timestamp,
                                 std::string* output) {
   base::TimeDelta time_since_epoch = timestamp - base::Time::UnixEpoch();
   WriteUint(kTimestampLength, time_since_epoch.InMilliseconds(), output);
 }
 
 bool EncodeV1SCTSignedData(const base::Time& timestamp,
                            const std::string& serialized_log_entry,
                            const std::string& extensions,
                            std::string* output) {
@@ skipping 40 matching lines @@
       new SignedCertificateTimestamp());
   unsigned version;
   if (!ReadUint(kVersionLength, input, &version))
     return false;
   if (version != SignedCertificateTimestamp::SCT_VERSION_1) {
     DVLOG(1) << "Unsupported/invalid version " << version;
     return false;
   }
 
   result->version = SignedCertificateTimestamp::SCT_VERSION_1;
-  uint64_t timestamp;
   base::StringPiece log_id;
   base::StringPiece extensions;
   if (!ReadFixedBytes(kLogIdLength, input, &log_id) ||
-      !ReadUint(kTimestampLength, input, &timestamp) ||
-      !ReadVariableBytes(kExtensionsLengthBytes, input,
-                         &extensions) ||
+      !ReadTimeSinceEpoch(input, &result->timestamp) ||
+      !ReadVariableBytes(kExtensionsLengthBytes, input, &extensions) ||
       !DecodeDigitallySigned(input, &result->signature)) {
     return false;
   }
 
-  if (timestamp > static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
-    DVLOG(1) << "Timestamp value too big to cast to int64_t: " << timestamp;
-    return false;
-  }
-
   log_id.CopyToString(&result->log_id);
   extensions.CopyToString(&result->extensions);
-  result->timestamp =
-      base::Time::UnixEpoch() +
-      base::TimeDelta::FromMilliseconds(static_cast<int64_t>(timestamp));
-
   output->swap(result);
   return true;
 }
 
 bool EncodeSCTListForTesting(const base::StringPiece& sct,
                              std::string* output) {
   std::string encoded_sct;
   return WriteVariableBytes(kSerializedSCTLengthBytes, sct, &encoded_sct) &&
-      WriteVariableBytes(kSCTListLengthBytes, encoded_sct, output);
+         WriteVariableBytes(kSCTListLengthBytes, encoded_sct, output);
 }
 
 }  // namespace ct
 
 }  // namespace net