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 <cstdint>

[Ryan Sleevi, 2016/01/08 19:46:31]

Chromium code recommends stdint.h (and, in general, the C headers over the C++
ones due to the std:: namespacing concerns). They're not entirely isomorphic 

Was there a specific reason for this change?

[Rob Percival, 2016/01/10 03:48:56]

No, I just wasn't aware of that recommendation - I'll revert this change.

 #include <limits>
 
 #include "base/logging.h"
 
 namespace net {
 
 namespace ct {
 
 namespace {
 
@@ skipping 40 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;
+  if (length > std::numeric_limits<size_t>::max()) {
+    return false;
+  }

[Ryan Sleevi, 2016/01/08 19:46:31]

CONSISTENCY: No braces for one-line conditionals

[Rob Percival, 2016/01/10 03:48:56]

Done.

+  *out = static_cast<size_t>(length);
   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;

[Ryan Sleevi, 2016/01/08 19:46:31]

Why? (I mean, on a mere pedantry that adds an extra instruction or three for a
case that you also seem to handle above)

[Rob Percival, 2016/01/10 03:48:56]

It's a little more defensive against bugs than trusting that ReadLength will
definitely have assigned to length when it returns true. This avoids a buffer
over-read in ReadFixedBytes. Admittedly, it's almost certainly unnecessary,
since the odds of ReadLength ever changing and introducing a bug like that are
very slim, but there seems no harm in it either and it's a good practice in
general.

   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);
   }
 }
 
 // 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.
 // |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.

[Ryan Sleevi, 2016/01/08 19:46:31]

Why? input.size() will never exceed 4 bytes on 32-bit builds

[Rob Percival, 2016/01/10 03:48:56]

The catch here is that WriteUint (below) is a template function that checks that
sizeof(input_size) >= prefix_length. On a 32-bit build, size_t doesn't exceed 4
bytes, as you say, but prefix_length is sometimes 8, causing a crash.

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

Isn't this moreso a bug in line 186/187 having bad preconditions? The
explanation was a little confusing, but now it's clearer that you're talking
about cases where sizeof(T) == 4, when T = size_t on 32-bit, but prefix_length
== 8. This triggers the DCHECK on line 186.

Put differently, I'm not sure whether it's "correct" to fix this here or whether
to fix WriteUint - my gut is that WriteUint should be robust enough to handle
the size_t mismatch case for all possible inputs.

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

How about we replace the preconditions of WriteUint with either:
a) DCHECK_EQ(value >> (length * 8), 0);
b) DCHECK_GE(length, sizeof(T)); 

I prefer (b), since it's simpler and requires fewer tests, whereas (a) really
requires all calling code to have boundary tests for the values they can pass
in. On the other hand, (b) does require more strictness with the size of
variable you use, e.g. if length == 1, you have to use (u)int8_t or cast to it,
rather than being able to pass in an int or an enum as you can now. Perhaps
that's an improvement though?

+  uint64_t input_size = input.size();
+  if (prefix_length > sizeof(input_size)) {
+    return false;
+  }

[Ryan Sleevi, 2016/01/08 19:46:31]

Consistency: Braces

[Rob Percival, 2016/01/10 03:48:56]

Done.

+
+  uint64_t max_allowed_input_size = ((1 << (prefix_length * 8)) - 1);
   if (input_size > max_allowed_input_size)
     return false;
 
-  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;
+  }
+
+  if (time_since_epoch >
+      static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {

[Ryan Sleevi, 2016/01/08 19:46:31]

There's stuff in base/safe_numerics to help with this sort of cast+cast+convert

[Rob Percival, 2016/01/10 03:48:56]

Done.

+    DVLOG(1) << "Timestamp value too big to cast to int64_t: "
+             << time_since_epoch;
+    return false;
+  }
+
+  *output =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(static_cast<int64_t>(time_since_epoch));
+
+  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