Minor improvements to ct_serialization.cc

net/cert/ct_serialization.cc

Index: net/cert/ct_serialization.cc
diff --git a/net/cert/ct_serialization.cc b/net/cert/ct_serialization.cc
index 235d37e65ad1fc8106a05d602c3c9f04b4e61a48..9608cb3ac2bcd9f3a00ad4dc55d7ec3b62f63ee2 100644
--- a/net/cert/ct_serialization.cc
+++ b/net/cert/ct_serialization.cc
@@ -9,6 +9,7 @@
 #include <limits>
 
 #include "base/logging.h"
+#include "base/numerics/safe_math.h"
 
 namespace net {
 
@@ -66,14 +67,18 @@ bool ReadUint(size_t length, base::StringPiece* in, T* out) {
 }
 
 // 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;
 }
 
@@ -96,7 +101,7 @@ bool ReadFixedBytes(size_t length,
 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);
@@ -184,7 +189,7 @@ void WriteUint(size_t length, T value, std::string* output) {
   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.

 }
 
@@ -195,25 +200,32 @@ void WriteUint(size_t length, T value, std::string* output) {
 // 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;
@@ -291,6 +303,27 @@ bool EncodeLogEntry(const LogEntry& input, std::string* 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();
@@ -351,28 +384,17 @@ bool DecodeSignedCertificateTimestamp(
   }
 
   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;
 }
@@ -381,7 +403,7 @@ 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