1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format (RFC 6962)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Sergei Chernov]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.111 Safari/537.36

Comment 1

[Sergei Chernov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6df2ee808d3e

Comment 2

[Sergei Chernov]

Attachment: MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review commit: https://reviewboard.mozilla.org/r/38411/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38411/

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

https://reviewboard.mozilla.org/r/38411/#review35453

Thanks, Sergei - this is impressive work, and it definitely gives me a sense of where this code is heading. That said, I think we need to change approaches somewhat. First off, part of why we wrote mozilla::pkix was to move away from using NSS for decoding things when possible. I think we should put the CT extracting code in mozilla::pkix itself and add a function to mozilla::pkix::TrustDomain like NoteCTInformation(CTType type, Input data). That way, when path building (and decoding OCSP responses), NSSCertDBTrustDomain can copy out any CT information given to it. After path building, CertVerifier can then call out to the CT implementation to actually verify the information.
Also, let's start landing this in pieces rather than as a whole. Our plan for CT has been to land code that detects (but doesn't verify) CT information and reports back telemetry on what mechanisms it's seeing and which logs are in use. Let's move more in this direction before we get too concerned with the verification aspects.

I think next steps would be:

* Add a function to mozilla::pkix::TrustDomain to note the presence of CT information in certificates and OCSP responses
* Call this function after CheckRevocation in pkixbuild.cpp (for end-entities only, I think) if the certificate has CT information
* Handle the appropriate extension in BackCert::RememberExtension (for CT in certificates)
* Similarly handle the appropriate extension in pkixocsp.cpp (currently no extensions are handled - this will have to change)
* Similarly call the new function in VerifyEncodedOCSPResponse after decoding the response
* Have NSSCertDBTrustDomain implement the new function - it should basically just note the information it received. If BuildCertChain succeeds, we can grab the noted information and use Telemetry::Accumulate on some new histograms to gather the information we want.

What do you think?

Comment 4

[Christian Stadelmann]

See also: bug #944175

Comment 5

[Sergei Chernov]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38411/diff/1-2/

Comment 6

[Sergei Chernov]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38411/diff/2-3/

Comment 7

[Sergei Chernov]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38411/diff/3-4/

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

https://reviewboard.mozilla.org/r/38411/#review43391

This is looking pretty solid. I noted some nits and have suggestions for some aspects of the general approach.

::: security/certverifier/CTSerialization.h:7
(Diff revision 4)
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_ct__CTSerialization_h

I think the include guard would actually be just "CTSerialization_h" since it isn't exported for inclusion in other modules by the build system (it depends on how other files include it, not where it is in the tree). (This applies to all of the include guards in this patch.)

::: security/certverifier/CTSerialization.cpp:54
(Diff revision 4)
> +// Reads a TLS-encoded variable length unsigned integer from |in|.
> +// The integer is expected to be in big-endian order, which is used by TLS.
> +// |length| indicates the size (in bytes) of the integer.
> +template <typename T>
> +static Result
> +ReadUint(size_t length, Reader& in, T& out)

Given my reading of the code in this patch, whenever this function is called, the length ultimately comes from a static constant, so a sufficiently advanced compiler should be able to handle static assertions here. If the compilers we're using aren't sufficiently advanced, though, we should have a code check in addition to the assertion (i.e. if (length > sizeof(T)) \{ return Result::FATAL_ERROR_LIBRARY_FAILURE; \} ) (Same with WriteUint, looks like.)

::: security/certverifier/CTSerialization.cpp:64
(Diff revision 4)
> +    uint8_t value;
> +    Result rv = in.Read(value);
> +    if (rv != Success) {
> +      return rv;
> +    }
> +    result = (result << 8) | value;

We should add something like 'static_assert(std::is_unsigned<T>::value, "T must be unsigned");' to prevent overflow.

::: security/certverifier/CTSerialization.cpp:129
(Diff revision 4)
> +    case DigitallySigned::SIG_ALGO_RSA:
> +    case DigitallySigned::SIG_ALGO_DSA:
> +    case DigitallySigned::SIG_ALGO_ECDSA:
> +      return Success;
> +  }
> +  return Result::ERROR_BAD_DER;  

nit: trailing whitespace

::: security/certverifier/CTSerialization.cpp:140
(Diff revision 4)
> +template <typename T>
> +static Result
> +WriteUint(size_t length, T value, Buffer& output)
> +{
> +  MOZ_ASSERT(length <= sizeof(T));
> +  MOZ_ASSERT(length == sizeof(T) || value >> (length * 8) == 0);

Let's add some documentation to this - it took me a bit to figure out the rationale here.

::: security/certverifier/CTSerialization.cpp:242
(Diff revision 4)
> +  rv = WriteUint(kSigAlgorithmLength, data.signatureAlgorithm, output);
> +  if (rv != Success) {
> +    return rv;
> +  }
> +  Input signatureData;
> +  rv = BufferToInput(data.signatureData, signatureData);

A common pattern in this file seems to be to convert a Buffer to an Input and then write that Input into a different Buffer. It seems like the intermediate conversion to an Input is unnecessary, unless I'm missing something.

::: security/certverifier/CTSerialization.cpp:256
(Diff revision 4)
> +{
> +  unsigned int hashAlgorithm;
> +  unsigned int signatureAlgorithm;
> +  DigitallySigned result;
> +
> +  Result rv = ReadUint(kHashAlgorithmLength, reader, hashAlgorithm);

Rather than reading things like hashAlgorithm and then (separately) validating that it's one of the expected values, I think we should define a 'ReadHashAlgorithm' that encapsulates the whole process (similarly with signatureAlgorithm, etc.). We should be able to avoid static casts as a result.

::: security/certverifier/CTSerialization.cpp:304
(Diff revision 4)
> +  }
> +  switch (entry.type) {
> +    case LogEntry::LOG_ENTRY_TYPE_X509:
> +      return EncodeAsn1CertLogEntry(entry, output);
> +    case LogEntry::LOG_ENTRY_TYPE_PRECERT:
> +      return EncodePrecertLogEntry(entry, output);

We should handle unreachable default cases similarly to MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM.

::: security/certverifier/SignedCertificateTimestamp.h:24
(Diff revision 4)
> +// LogEntry struct in RFC 6962, Section 3.1.
> +struct LogEntry
> +{
> +
> +  // LogEntryType enum in RFC 6962, Section 3.1.
> +  enum Type {

Let's use enum classes if possible.

::: security/certverifier/SignedCertificateTimestamp.h:62
(Diff revision 4)
> +    SIG_ALGO_RSA = 1,
> +    SIG_ALGO_DSA = 2,
> +    SIG_ALGO_ECDSA = 3
> +  };
> +
> +  // Returns true if |other_hash_algorithm| and |other_signature_algorithm|

nit: update documentation

::: security/certverifier/SignedCertificateTimestamp.h:88
(Diff revision 4)
> +  // or be re-assigned.
> +  enum Origin {
> +    SCT_EMBEDDED = 0,
> +    SCT_FROM_TLS_EXTENSION = 1,
> +    SCT_FROM_OCSP_RESPONSE = 2,
> +    SCT_ORIGIN_MAX,

We probably don't need SCT_ORIGIN_MAX.

::: security/certverifier/SignedCertificateTimestamp.cpp:21
(Diff revision 4)
> +  tbsCertificate.clear();
> +}
> +
> +bool
> +DigitallySigned::SignatureParametersMatch(HashAlgorithm aHashAlgorithm,
> +                            SignatureAlgorithm aSignatureAlgorithm) const

nit: for long lines where wrapping and indenting the second line to the usual place would still result in a long line, just indent the second line two spaces from the previous line.

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:41
(Diff revision 4)
> +  ASSERT_EQ(Success,
> +    DecodeDigitallySigned(digitallySignedReader, parsed));
> +  EXPECT_TRUE(digitallySignedReader.AtEnd());
> +
> +  EXPECT_EQ(
> +      DigitallySigned::HASH_ALGO_SHA256,

nit: I think it's more common in mozilla code to put this on the previous line

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:86
(Diff revision 4)
> +  ASSERT_EQ(Success, EncodeLogEntry(entry, encoded));
> +  EXPECT_EQ((718U + 5U), encoded.length());
> +  // First two bytes are log entry type. Next, length:
> +  // Length is 718 which is 512 + 206, which is 0x2ce
> +  Buffer expectedPrefix;
> +  MOZ_RELEASE_ASSERT(expectedPrefix.append("\0\0\0\x2\xCE", 5));

Why not ASSERT_TRUE?

::: security/certverifier/tests/gtest/CTTestUtils.cpp:138
(Diff revision 4)
> +  }
> +  return result;
> +}
> +
> +
> +void GetX509CertLogEntry(LogEntry& entry)

nit: return type on its own line

Comment 9

[Sergei Chernov]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38411/diff/4-5/

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

https://reviewboard.mozilla.org/r/38411/#review46145

This looks good. I think this should have another set of eyes on it, but after than and addressing review comments, let's go ahead with landing this piece so we can move forward incrementally.

::: security/certverifier/CTSerialization.cpp:56
(Diff revisions 4 - 5)
>  // The integer is expected to be in big-endian order, which is used by TLS.
> -// |length| indicates the size (in bytes) of the integer.
> +// Note: does not check if the output parameter overflows while reading.
> +// |length| indicates the size (in bytes) of the serialized integer.
>  template <typename T>
>  static Result
> -ReadUint(size_t length, Reader& in, T& out)
> +UncheckedReadUint(size_t length, Reader& in, T& out)

Is this function going to be reused elsewhere other than from ReadUint? Seems like we could just inline it into ReadUint without much loss of code legibility.

::: security/certverifier/CTSerialization.cpp:197
(Diff revisions 4 - 5)
> +// Performs a sanity check on the size of T and calls UncheckedWriteUint.
> +template <size_t length, typename T>
> +static inline Result
> +WriteUint(T value, Buffer& output)
> +{
> +  static_assert(sizeof(T) >= length, "T must be able to hold <length> bytes");

T should probably be unsigned here as well.

::: security/certverifier/CTSerialization.cpp:198
(Diff revisions 4 - 5)
> +template <size_t length, typename T>
> +static inline Result
> +WriteUint(T value, Buffer& output)
> +{
> +  static_assert(sizeof(T) >= length, "T must be able to hold <length> bytes");
> +  return UncheckedWriteUint(length, value, output);

Same question here as with UncheckedReadUint.

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:41
(Diff revisions 4 - 5)
>      DecodeDigitallySigned(digitallySignedReader, parsed));
>    EXPECT_TRUE(digitallySignedReader.AtEnd());
>  
> -  EXPECT_EQ(
> +  EXPECT_EQ(DigitallySigned::HashAlgorithm::HASH_ALGO_SHA256,
> -      DigitallySigned::HASH_ALGO_SHA256,
>        parsed.hashAlgorithm);

nit: indentation (align the 'p' with the 'D' from the previous line)

Comment 11

[:Cykesiopka]

Just as an update: I've been going through the changes, but I unfortunately haven't had much spare time in the last few days. Hopefully I'll be done in a day or two.

Comment 12

[Sergei Chernov]

https://reviewboard.mozilla.org/r/38411/#review43391

> Given my reading of the code in this patch, whenever this function is called, the length ultimately comes from a static constant, so a sufficiently advanced compiler should be able to handle static assertions here. If the compilers we're using aren't sufficiently advanced, though, we should have a code check in addition to the assertion (i.e. if (length > sizeof(T)) \{ return Result::FATAL_ERROR_LIBRARY_FAILURE; \} ) (Same with WriteUint, looks like.)

Moved the length to be a template parameter to allow static checks.

> We should add something like 'static_assert(std::is_unsigned<T>::value, "T must be unsigned");' to prevent overflow.

Used mozilla::IsUnsigned for this since type_traits is not available on my compiler (Mac's default).

> A common pattern in this file seems to be to convert a Buffer to an Input and then write that Input into a different Buffer. It seems like the intermediate conversion to an Input is unnecessary, unless I'm missing something.

Added overloaded functions for writing buffers. Note that we still need the original Input-based ones - see for example EncodeV1SCTSignedData function which is exported and so receives its source data via Inputs.

> Why not ASSERT_TRUE?

A failure here does not indicate a problem with the class being tested, but an out-of-memory error. But it's a bit theoretical case, so please reopen this if you prefer ASSERT_TRUE here.

Comment 13

[Sergei Chernov]

https://reviewboard.mozilla.org/r/38411/#review46145

> Is this function going to be reused elsewhere other than from ReadUint? Seems like we could just inline it into ReadUint without much loss of code legibility.

It's there because I wanted to spare some code bloat from template instantiations. ReadUint has <length, T> as template parameters, while UncheckedReadUint only has <T>. ReadUint only performs the static checks (so it's small and inline), while UncheckedReadUint does the actual read. Comparing to the existing code, making UncheckedReadUint<length, T> would generate additional template instantiations and I believe it's unnecessary in our case. On a second thought, maybe it would be even better to make UncheckedReadUint non-templated and always read the result into a 8 byte integer, and ReadUint convert it to T as needed. What do you think?

(reminder to myself - part of this logic is duplicated in tests, don't forget to update it there as well)

> T should probably be unsigned here as well.

Yes, I'll add a test to check that "value >= 0". Note that just as we allow T to have more bytes that is actually written (as long as the actual value is within the range), we can allow T to be signed (as long as its actual value is non-negative).

> Same question here as with UncheckedReadUint.

Let me know what you think of the comments on UncheckedReadUint above and I'll do the same here.

Comment 14

[:Cykesiopka]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

https://reviewboard.mozilla.org/r/38411/#review47141

Sorry for the delay. This mostly look good, but I would to take a second look just to be sure (it looks like you plan on making some other changes anyways).

::: security/certverifier/CTSerialization.h:10
(Diff revision 5)
> +#include "pkix/Result.h"
> +#include "pkix/Input.h"

Nit: Input.h should sort before Result.h.

::: security/certverifier/CTSerialization.cpp:45
(Diff revision 5)
> +enum class SignatureType {
> +  SIGNATURE_TYPE_CERTIFICATE_TIMESTAMP = 0,
> +  TREE_HASH = 1,
> +};

Nit: PSM enum class enumerator names typically use camel case.
Also, we probably don't need the |SIGNATURE_TYPE_| prefix either.
Same with all the other enum class declarations.

::: security/certverifier/CTSerialization.cpp:112
(Diff revision 5)
> +  DigitallySigned::HashAlgorithm algo =
> +      static_cast<DigitallySigned::HashAlgorithm>(value);

Nit: In these sorts of cases, we generally prefer indenting by two spaces instead of four.
Same thing for everywhere else a four space indent shows up.

::: security/certverifier/CTSerialization.cpp:186
(Diff revision 5)
> +    uint8_t next_byte = (value >> ((length - 1) * 8)) & 0xFF;
> +    output.infallibleAppend(next_byte);

Nit: Camel case.

::: security/certverifier/CTSerialization.cpp:221
(Diff revision 5)
> +
> +// Same as above, but the source data is in a Buffer.
> +static Result
> +WriteEncodedBytes(const Buffer& source, Buffer& output)
> +{
> +  if (!output.append(source.begin(), source.length())) {

Nit: Looks like appendAll() would work here.

::: security/certverifier/CTSerialization.cpp:274
(Diff revision 5)
> +}
> +
> +// Writes a LogEntry of type PreCertificate to |output|.
> +// |input| is the LogEntry containing the TBSCertificate and issuer key hash.
> +static Result
> +EncodePrecertLogEntry(const LogEntry& entry, Buffer& output)

There doesn't appear to be a test for this method.
GetPrecertLogEntry() also doesn't appear to be used anywhere.

::: security/certverifier/CTSerialization.cpp:371
(Diff revision 5)
> +  rv = WriteUint<kSignatureTypeLength>(static_cast<unsigned int>(
> +      SignatureType::SIGNATURE_TYPE_CERTIFICATE_TIMESTAMP), output);
> +  if (rv != Success) {
> +    return rv;
> +  }
> +  rv = WriteTimeSinceEpoch(timestamp, output);

|rv| should probably be checked here.

::: security/certverifier/CTSerialization.cpp:422
(Diff revision 5)
> +  }
> +  return listReader.Init(listData);
> +}
> +
> +Result
> +ReadSCTListItem(Reader& listReader, Input& result)

Optional: Rename |result| to |output| for consistency with the other methods.

::: security/certverifier/SignedCertificateTimestamp.h:11
(Diff revision 5)
> +#include "pkix/Result.h"
> +#include "pkix/Input.h"

Nit: Input.h should sort before Result.h.

::: security/certverifier/SignedCertificateTimestamp.h:95
(Diff revision 5)
> +  Version version;
> +  Buffer logId;
> +  uint64_t timestamp;
> +  Buffer extensions;
> +  DigitallySigned signature;
> +  Origin origin;

This doesn't appear to be set or read anywhere. I assume it will be used as part of future work?

::: security/certverifier/SignedCertificateTimestamp.cpp:12
(Diff revision 5)
> +LogEntry::Reset()
> +{
> +  type = LogEntry::Type::LOG_ENTRY_TYPE_X509;
> +  leafCertificate.clear();
> +  tbsCertificate.clear();
> +}

Looks like |issuerKeyHash| should be cleared here as well.

::: security/certverifier/SignedCertificateTimestamp.cpp:36
(Diff revision 5)
> +
> +bool
> +operator==(const ct::Buffer& a, const ct::Buffer& b)
> +{
> +  return (a.empty() && b.empty()) ||
> +    (a.length() == b.length() && 0 == memcmp(a.begin(), b.begin(), a.length()));

Nit: Non-Yoda |memcmp(...) == 0| is generally preferred in PSM.

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:53
(Diff revision 5)
> +{
> +  Input partial;
> +  ASSERT_EQ(Success,
> +    partial.Init(mTestDigitallySigned.begin(),
> +      mTestDigitallySigned.length() - 5));
> +  Reader partial_reader(partial);

Nit: Camel case.

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:86
(Diff revision 5)
> +  ASSERT_EQ(Success, EncodeLogEntry(entry, encoded));
> +  EXPECT_EQ((718U + 5U), encoded.length());
> +  // First two bytes are log entry type. Next, length:
> +  // Length is 718 which is 512 + 206, which is 0x2ce
> +  Buffer expectedPrefix;
> +  MOZ_RELEASE_ASSERT(expectedPrefix.append("\0\0\0\x2\xCE", 5));

Optional: I don't mind either way, but I don't think we really need to use this sort of assert for test code, even for OOM. Up to you.

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:95
(Diff revision 5)
> +  EXPECT_EQ(expectedPrefix, encodedPrefix);
> +}
> +
> +TEST_F(CTSerializationTest, EncodesV1SCTSignedData)
> +{
> +  uint64_t timestamp = 1348589665525;

Just to confirm: this is just some arbitrary timestamp right?

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:108
(Diff revision 5)
> +    0x61, 0x62, 0x63, 0x00, // log signature
> +    0x00 // extentions (empty)

Nit: s/extentions/extensions/. Also, the 0x00 in the line above should be part of the extensions line right?

::: security/certverifier/tests/gtest/CTTestUtils.h:7
(Diff revision 5)
> +#ifndef mozilla_ct__SCTestUtils_h
> +#define mozilla_ct__SCTestUtils_h

Nit: CTTestUtils_h.

::: security/certverifier/tests/gtest/CTTestUtils.h:19
(Diff revision 5)
> +// Note: unless specified otherwise, all test data is taken from Certificate
> +// Transparency test data repository

Optional: Providing a link to the repository like you do in CTTestUtils.cpp might be helpful.

::: security/certverifier/tests/gtest/CTTestUtils.h:27
(Diff revision 5)
> +// Fills |entry| with test data for an X.509 entry.
> +void GetX509CertLogEntry(LogEntry& entry);
> +
> +// Returns a DER-encoded X509 cert. The SCT provided by
> +// GetX509CertSCT is signed over this certificate.
> +Buffer GetDerEncodedX509Cert();

This and a few methods below appear to be unused for now. I assume they will be used in the future?

::: security/certverifier/tests/gtest/CTTestUtils.h:75
(Diff revision 5)
> +
> +// Returns Input for the data stored in the buffer, failing assertion on error.
> +pkix::Input InputForBuffer(const Buffer& buffer);
> +
> +// Returns Input for the data stored in the item, failing assertion on error.
> +pkix::Input InputForSECItem(const SECItem& item);

We can get rid of this and the seccomon.h include now that we don't use NSS right?

::: security/certverifier/tests/gtest/CTTestUtils.cpp:7
(Diff revision 5)
> +#include <stdint.h>
> +#include <iomanip>
> +
> +#include "mozilla/Vector.h"
> +#include "mozilla/Assertions.h"
> +#include "pkix/Result.h"
> +#include "pkix/Input.h"
> +#include "SignedCertificateTimestamp.h"
> +#include "SignedTreeHead.h"
> +#include "CTSerialization.h"
> +
> +#include "CTTestUtils.h"

Nit: Please sort these per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices :
> Includes are split into three blocks and are sorted alphabetically in each block:
>   1. The main header: Foo.h in Foo.cpp
>   2. Standard library includes: #include <map>
>   3. Mozilla includes: #include "mozilla/dom/Element.h"

::: security/certverifier/tests/gtest/CTTestUtils.cpp:27
(Diff revision 5)
> +using namespace mozilla::pkix;
> +
> +// The following test vectors are from
> +// https://github.com/google/certificate-transparency/tree/master/test/testdata
> +
> +const char kDefaultDerCert[] =

Optional: It might be helpful to add a comment for each of these noting the filename of the corresponding entry in the test case repo.

::: security/certverifier/tests/gtest/CTTestUtils.cpp:105
(Diff revision 5)
> +const char kSampleSTHTreeHeadSignature[] =
> +    "0403004730450220365a91a2a88f2b9332f41d8959fa7086da7e6d634b7b089bc9da066426"
> +    "6c7a20022100e38464f3c0fd066257b982074f7ac87655e0c8f714768a050b4be9a7b441cb"
> +    "d3";
> +const size_t kSampleSTHTreeSize = 21u;
> +const int64_t kSampleSTHTimestamp = 1396877277237u;

Looks like this should be uint64_t instead.

::: security/certverifier/tests/gtest/CTTestUtils.cpp:121
(Diff revision 5)
> +static Buffer
> +HexToBytes(const char* hex_data)
> +{
> +  size_t hex_len = strlen(hex_data);
> +  MOZ_RELEASE_ASSERT(hex_len > 0 && (hex_len % 2 == 0));
> +  size_t result_len = hex_len / 2;

Nit: Camel case.

::: security/certverifier/tests/gtest/CTTestUtils.cpp:263
(Diff revision 5)
> +GetSampleSTHTreeHeadDecodedSignature(DigitallySigned& signature)
> +{
> +  Buffer tree_head_signature = HexToBytes(kSampleSTHTreeHeadSignature);
> +  Input ths_input;
> +  MOZ_RELEASE_ASSERT(ths_input.Init(tree_head_signature.begin(),
> +    tree_head_signature.length()) == Success);
> +  Reader ths_reader(ths_input);

Nit: Camel case.

::: security/certverifier/tests/gtest/CTTestUtils.cpp:276
(Diff revision 5)
> +cloneBuffer(const Buffer& buffer)
> +{
> +  Buffer cloned_buffer;

Nit: Camel case.

Comment 15

[Sergei Chernov]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38411/diff/5-6/

Comment 16

[Sergei Chernov]

https://reviewboard.mozilla.org/r/38411/#review47141

> This doesn't appear to be set or read anywhere. I assume it will be used as part of future work?

Yes. Future code performing the actual deserialization will set this field according to the origin of the buffer containing the serialized SCT.

> Optional: I don't mind either way, but I don't think we really need to use this sort of assert for test code, even for OOM. Up to you.

Similar OOM checks are done in TestUtils (i.e. outside of test cases) where we can't use ASSERT\_\* and must use something else. I prefer to leave MOZ_RELEASE_ASSERT here for consistency.

> Just to confirm: this is just some arbitrary timestamp right?

Yes. Its serialized value is below in EXPECTED_BYTES. I've changed the format of the timestamp to hex since it's easier to see the correspondence.

> Nit: s/extentions/extensions/. Also, the 0x00 in the line above should be part of the extensions line right?

Yes, you are correct.

> This and a few methods below appear to be unused for now. I assume they will be used in the future?

Yes. Currently these methods are only provided for completeness. For example, GetX509CertLogEntry returns a sample log entry, and it makes sense to have GetDerEncodedX509Cert returning the corresponding certificate.

> We can get rid of this and the seccomon.h include now that we don't use NSS right?

Yes, removed it. We will probably need to bring it back at some later stage, but it is surely not needed now.

Comment 17

[Sergei Chernov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=403e1147b819

Comment 18

[:Cykesiopka]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

https://reviewboard.mozilla.org/r/38411/#review47893

Cool, almost there.

::: security/certverifier/CTSerialization.cpp:55
(Diff revision 6)
> +UncheckedReadUint(size_t length, Reader& in, uint_least64_t& out)
> +{
> +  uint_least64_t result = 0;

Nit: Can we just use uint64_t here?
We have a static assert that |length <= 8| in ReadUint() anyways, so I'm not really sure why uint_least64_t is better.

::: security/certverifier/CTSerialization.cpp:180
(Diff revision 6)
> +// Writes a TLS-encoded variable length unsigned integer to |output|.
> +// Note: range/overflow checks are not performed on the input parameters.
> +// |length| indicates the size (in bytes) of the integer to be written.
> +// |value| the value itself to be written.
> +static Result
> +UncheckedWriteUint(size_t length, uint_least64_t value, Buffer& output)

Same comment as the UncheckedReadUint() one.

::: security/certverifier/CTSerialization.cpp:199
(Diff revision 6)
> +  if (mozilla::IsSigned<T>::value && value < 0) {
> +    // We accept signed integer types assuming the actual value is non-negative.
> +    return Result::FATAL_ERROR_INVALID_ARGS;
> +  }
> +  if (sizeof(T) > length && (value >> (length * 8) != 0)) {
> +    // We allow the value variable to take more bytes than is written,
> +    // but the unwritten bytes must be zero.
> +    return Result::FATAL_ERROR_INVALID_ARGS;
> +  }

Looks like the right shift here is causing a compiler error (or warning as error).
Now that I've taken a second look at this, I'm also not sure why we need this sort of flexibility.
It feels like we can just assert that T is unsigned and that it's size doesn't exceed that of uint64_t.
Maybe I'm missing something.

::: security/certverifier/SignedCertificateTimestamp.h:20
(Diff revision 6)
> +// LogEntry struct in RFC 6962, Section 3.1.
> +struct LogEntry

Unless I'm misunderstanding (and this is quite possible), it looks like LogEntry here actually represents the following highlighted parts of SignedCertificateTimestamp:
> struct {
>    Version sct_version;
>    LogID id;
>    uint64 timestamp;
>    CtExtensions extensions;
>    digitally-signed struct {
>        Version sct_version;
>        SignatureType signature_type = certificate_timestamp;
>        uint64 timestamp;
>      ------------------------------------
>      | LogEntryType entry_type;         |
>      | select(entry_type) {             |
>      |     case x509_entry: ASN.1Cert;  |
>      |     case precert_entry: PreCert; |
>      | } signed_entry;                  |
>      ------------------------------------
>       CtExtensions extensions;
>    };
> } SignedCertificateTimestamp;

If this is the case, it might be a good idea to rename LogEntry here to something else to avoid confusing it with LogEntry from the spec.

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:104
(Diff revision 6)
> +  Buffer expectedPrefix;
> +  MOZ_RELEASE_ASSERT(expectedPrefix.append("\0\x1", 2));
> +  Buffer encodedPrefix;
> +  MOZ_RELEASE_ASSERT(encodedPrefix.
> +    append(encoded.begin(), encoded.begin() + 2));

It looks like we don't actually test that these two buffers are equal.

::: security/certverifier/tests/gtest/CTSerializationTest.cpp:192
(Diff revision 6)
> +  // Subtracting 4 bytes for signature data (hash & sig algs),
> +  // actual signature data should be 71 bytes.

Nit: Shouldn't this be 2 bytes for the CtExtensions length and 2 for (hash & sig algs)?

Comment 19

[Sergei Chernov]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38411/diff/6-7/

Comment 20

[Sergei Chernov]

https://reviewboard.mozilla.org/r/38411/#review46145

> It's there because I wanted to spare some code bloat from template instantiations. ReadUint has <length, T> as template parameters, while UncheckedReadUint only has <T>. ReadUint only performs the static checks (so it's small and inline), while UncheckedReadUint does the actual read. Comparing to the existing code, making UncheckedReadUint<length, T> would generate additional template instantiations and I believe it's unnecessary in our case. On a second thought, maybe it would be even better to make UncheckedReadUint non-templated and always read the result into a 8 byte integer, and ReadUint convert it to T as needed. What do you think?
> 
> (reminder to myself - part of this logic is duplicated in tests, don't forget to update it there as well)

The result is now always read into a 64bit integer. Marking this as fixed for now, please reopen if you have comments.

Comment 21

[Sergei Chernov]

https://reviewboard.mozilla.org/r/38411/#review47893

> Looks like the right shift here is causing a compiler error (or warning as error).
> Now that I've taken a second look at this, I'm also not sure why we need this sort of flexibility.
> It feels like we can just assert that T is unsigned and that it's size doesn't exceed that of uint64_t.
> Maybe I'm missing something.

For example, we have an integer which takes 3 bytes when serialized (see kSignatureLengthBytes). We can read it into 4-bytes integer, but when writing, it would be nice as a general principle to first check that the actual value of the 4-bytes integer does not exceed that of 3 bytes.
Regarding the compiler warning/error, I've added a workaround and explanation in the code. Note that the warning used to happen on some compilers (Windows/Android) for WriteUint<uint64_t, 8>.

> Unless I'm misunderstanding (and this is quite possible), it looks like LogEntry here actually represents the following highlighted parts of SignedCertificateTimestamp:
> > struct {
> >    Version sct_version;
> >    LogID id;
> >    uint64 timestamp;
> >    CtExtensions extensions;
> >    digitally-signed struct {
> >        Version sct_version;
> >        SignatureType signature_type = certificate_timestamp;
> >        uint64 timestamp;
> >      ------------------------------------
> >      | LogEntryType entry_type;         |
> >      | select(entry_type) {             |
> >      |     case x509_entry: ASN.1Cert;  |
> >      |     case precert_entry: PreCert; |
> >      | } signed_entry;                  |
> >      ------------------------------------
> >       CtExtensions extensions;
> >    };
> > } SignedCertificateTimestamp;
> 
> If this is the case, it might be a good idea to rename LogEntry here to something else to avoid confusing it with LogEntry from the spec.

You are correct, but note that LogEntry from the spec has exactly the same fields as the ones you've highlighted (don't know what was the reason for "inlining" it here).

Comment 22

[Sergei Chernov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be6dbf71f6d0

Comment 23

[:Cykesiopka]

https://reviewboard.mozilla.org/r/38411/#review47893

> For example, we have an integer which takes 3 bytes when serialized (see kSignatureLengthBytes). We can read it into 4-bytes integer, but when writing, it would be nice as a general principle to first check that the actual value of the 4-bytes integer does not exceed that of 3 bytes.
> Regarding the compiler warning/error, I've added a workaround and explanation in the code. Note that the warning used to happen on some compilers (Windows/Android) for WriteUint<uint64_t, 8>.

Ah yes, that's a perfectly good reason. Silly me.

> You are correct, but note that LogEntry from the spec has exactly the same fields as the ones you've highlighted (don't know what was the reason for "inlining" it here).

> struct {
>    LogEntryType entry_type;
>    select (entry_type) {
>        case x509_entry: X509ChainEntry;
>        case precert_entry: PrecertChainEntry;
>    } entry;
> } LogEntry;

Hmm, my interpretation is that the types of the fields are different, but maybe I'm reading the spec wrong.

Anyways, dropping this is fine with me as well.

Comment 24

[:Cykesiopka]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

https://reviewboard.mozilla.org/r/38411/#review48725

Looks good. r+ assuming you fix the remaining nit from my previous review.

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=be6dbf71f6d0
I guess you're on an old checkout? I see failures in that push that should not occur for a recent checkout. It'll probably be best to rebase to something recent before checking this in.

Comment 25

[Sergei Chernov]

Comment on attachment 8727294 [details]
MozReview Request: Bug 1241574 - Certificate Transparency - base definitions and serialization to/from TLS wire format; r?keeler, Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38411/diff/7-8/

Comment 26

[Sergei Chernov]

https://reviewboard.mozilla.org/r/38411/#review47893

> Nit: Shouldn't this be 2 bytes for the CtExtensions length and 2 for (hash & sig algs)?

You are right, 2 bytes for the length and 2 for the algo enums. But since we verify the expected parameters of a specific SCT, the comment really does not add any useful information - 75 is as good as 71. So I've deleted the comment.

Comment 27

[Sergei Chernov]

https://reviewboard.mozilla.org/r/38411/#review48725

Rebased and pushed.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35b845bf58f1

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35b845bf58f1