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

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

David - some comments regarding the uploaded patch: 1. It's best to start the review from CTObjectsExtractor, then proceed to CTLogVerifier and then MultiLogCTVerifier. 2. The code in CTObjectsExtractor uses NSS and needs to call RegisterOid to initialize. The initialization is done implicitly during the first call to GetPrecertLogEntry, but it might be better to extract it to a separate function to be called explicitly. I am not sure though what would be the proper place to call this function in Firefox code. Could it be EnsureServerVerificationInitialized from SSLServerCertVerification.h?

Hi Sergei, it's taking me a bit longer to get to this review than I would like, so I thought I'd let you know I'm aware of it and that I'll finish it as soon as possible (hopefully tomorrow).

Hi David, thanks for the update!

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); https://reviewboard.mozilla.org/r/62676/#review60066 Overall this looks good - it's structured well, there's good documentation, etc.. I think the direct uses of NSS aren't necessary, though (see below for more details). I think we can rely more on mozilla::pkix code rather than using NSS code or reimplementing things (if this requires exposing more of mozilla::pkix's API, I think we can find a reasonable way to do that). For extracting the TBSCertificate without the SCT extension, I think we should rely more on mozilla::pkix than NSS. That is, let's make it possible for the CT implementation to use mozilla::pkix's parsing functions to pull the data out. Then, as far as I can tell, encoding is fairly straight-forward since there are only two length tags that actually change: the length of the extensions, and the length of the TBSCertificate data. We can basically write a one-off encoder that handles only this situation. Similarly, these APIs (particularly some of the test ones) make use of CERTCertificate where it doesn't seem entirely necessary (for example, a CERTCertificate is created, but then the only thing that is actually used is its encoding, which we already had). ::: security/certverifier/CTLogVerifier.h:33 (Diff revision 1) > + > + // Initializes the verifier with log-specific information. > + // |subjectPublicKeyInfoDer| is a DER-encoded SubjectPublicKeyInfo. > + // An error is returned if |subjectPublicKeyInfoDer| refers to an unsupported > + // public key. > + pkix::Result Init(pkix::Input subjectPublicKeyInfoDer); In every identifier name, "DER" is usually preferrable to "Der" (similarly with other acronyms). ::: security/certverifier/CTLogVerifier.h:61 (Diff revision 1) > + // Returns Success if passed verification, ERROR_BAD_SIGNATURE if failed > + // verification, or other result on error. > + pkix::Result VerifySignature(pkix::Input data, pkix::Input signature); > + pkix::Result VerifySignature(const Buffer& data, const Buffer& signature); > + > + UniqueSECKEYPublicKey mPublicKey; What's the lifetime of instances of this class? Are they guaranteed to be gone by the time NSS shuts down? (If not, we'll need to handle NSS shutdown here, which complicates things. Another solution would be to not hold NSS resources alive. I think this would actually be preferable.) ::: security/certverifier/CTLogVerifier.cpp:29 (Diff revision 1) > +GetNSSSigAlg(DigitallySigned::SignatureAlgorithm alg) > +{ > + switch (alg) { > + case DigitallySigned::SignatureAlgorithm::RSA: > + return SEC_OID_PKCS1_RSA_ENCRYPTION; > + case DigitallySigned::SignatureAlgorithm::DSA: Why have this if we don't support it? ::: security/certverifier/CTLogVerifier.cpp:44 (Diff revision 1) > + > +static SECOidTag > +GetNSSHashAlg(DigitallySigned::HashAlgorithm alg) > +{ > + switch (alg) { > + case DigitallySigned::HashAlgorithm::MD5: Again, we're not actually using most of these, right? ::: security/certverifier/CTLogVerifier.cpp:64 (Diff revision 1) > + return SEC_OID_UNKNOWN; > + } > +} > + > +CTLogVerifier::CTLogVerifier() : > + mHashAlgorithm(DigitallySigned::HashAlgorithm::None), nit: CTLogVerifier::CTLogVerifier() : mHashAlgorithm(DigitallySigned::HashAlgorithm::None) , mSignatureAlgorithm(DigitallySigned::SignatureAlgorithm::Anonymous) ::: security/certverifier/CTLogVerifier.cpp:188 (Diff revision 1) > + > +pkix::Result > +CTLogVerifier::VerifySignature(Input data, Input signature) > +{ > + SECItem signatureItem = UnsafeMapInputToSECItem(signature); > + SECStatus srv = VFY_VerifyDataDirect( Rather than re-implementing wrappers around NSS' verify functions, I think we should find a way to re-use the ones already present in pkixnss.cpp (this might also help with not holding NSS resources alive, since the mozilla::pkix implementation can do the spki decoding). ::: security/certverifier/CTObjectsExtractor.h:23 (Diff revision 1) > +// contains an X.509v3 extension with the OID 1.3.6.1.4.1.11129.2.4.2. On > +// success, fills |output| with the data for a PrecertChain log entry. > +// The filled |output| should be verified using ct::CTLogVerifier::Verify. > +// Note: if |leaf| does not contain the required extension, an error is > +// returned. > +pkix::Result GetPrecertLogEntry(CERTCertificate* leaf, For interfaces we define, we should pass around references to UniqueCERTCertificate rather than pointers, so ownership is clear. (Although, on that note, let's see if we can define these interfaces without using NSS types at all.) ::: security/certverifier/CTObjectsExtractor.cpp:116 (Diff revision 1) > + return MapPRErrorCodeToResult(PR_GetError()); > + } > + > + // To remove the extention, we break the certificate apart and > + // reconstruct it without the extention. > + CERTCertificate tempCert; I don't think it's safe to use CERTCertificate in this way (there may be reference-counting issues). ::: security/certverifier/CTObjectsExtractor.cpp:128 (Diff revision 1) > + size_t extCount = 0; > + for (CERTCertExtension** exts = cert->extensions; *exts; ++exts) { > + ++extCount; > + } > + > + CERTCertExtension** newExtensions = I'm concerned about this approach. Manually manipulating these pointers like this is fragile and difficult to verify for correctness and safety. ::: security/certverifier/CTObjectsExtractor.cpp:150 (Diff revision 1) > + } > + > + SECItem tbsData; > + tbsData.len = 0; > + tbsData.data = NULL; > + SECItem* result = SEC_ASN1EncodeItem(arena.get(), &tbsData, &tempCert, I think there may be compatibility concerns here. DER is *supposed* to have one and only one possible encoding, but in practice we've discovered that's not actually true (or, rather, for compatibility reasons we accept invalid encodings). As long as we accept some non-standard encodings, we're not guaranteed that the output of SEC_ASN1EncodeItem will match the original certificate. Consequently, verifying the SCTs will fail. ::: security/certverifier/MultiLogCTVerifier.h:39 (Diff revision 1) > + // Verification errors unrelated to invalid signatures (such as data decoding > + // errors) do not stop the verification process, so there is no easy way to > + // let the caller know of the error. The last encountered error code is stored > + // in the field below, and can be used as an indication that there were errors > + // during the verification. > + pkix::Result lastError; I'm concerned using this paradigm safely and correctly will be difficult. Let's have these functions return the success/failure of the overall operation (e.g. if given invalid arguments or if memory allocation fails, we should return an error). Then, if there's an error in decoding a specific SCT, we'll know this because of which bucket it's in in the CTVerifyResult structure. ::: security/certverifier/MultiLogCTVerifier.h:70 (Diff revision 1) > + // no extension was present. > + // |now| is the current time used to make sure SCTs are not in the future. > + // |result| will be filled with the SCTs present, divided into categories > + // based on the verification result. > + // The verification process does not stop on errors (but |result| > + // will indicate there were errors; see CTVerifyResult). nit: trailing whitespace ::: security/certverifier/tests/gtest/CTTestUtils.cpp:696 (Diff revision 1) > + return Success; > + } > + > + Result NetscapeStepUpMatchesServerAuth(Time, /*out*/ bool& matches) override > + { > + matches = true; This should probably be false. ::: security/certverifier/tests/gtest/CTTestUtils.cpp:707 (Diff revision 1) > + if (extension == AuxiliaryExtension::SCTListFromOCSPResponse) { > + if (InputToBuffer(data, signedCertificateTimestamps) == Success) { > + return; > + } > + } > + ADD_FAILURE(); Let's flip this - check for the exception conditions above and add the failure in those cases. ::: security/certverifier/tests/gtest/CTTestUtils.cpp:770 (Diff revision 1) > + MOZ_RELEASE_ASSERT(Success == > + input.Init(item.data, item.len)); > + return input; > +} > + > +// The serialization code below was copied as-is from CTSerialization.cpp Can we not just link against the original copy? ::: security/certverifier/tests/gtest/MultiLogCTVerifierTest.cpp:45 (Diff revision 1) > + // Set the current time making sure all test timestamps are in the past. > + mNow = UINT64_MAX; > + } > + > + void CheckForSingleVerifiedSCTInResult(const CTVerifyResult& result, > + SignedCertificateTimestamp::Origin origin) nit: alignment

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62676/diff/1-2/

https://reviewboard.mozilla.org/r/62676/#review60066 Thank you for the comments David. It will take me some time to complete all the necessary NSS-related changes. I've refactored the interfaces according to your comments, and isolated the NSS specific code. Since there were plenty of changes, I think we can actually proceed with the review, minus the localized NSS spots. Let me know if the uploaded refactored code is in line with what you have in mind. The NSS-specific code still left in the parch: 1. CTObjectsExtractor.cpp - please do not review this file. I will get back to you when I have a pkix-based implementation using DER manipulation (or questions on it). 2. ExtractSignatureParameters function in CTLogVerifier.cpp - It is used to extract the key length and type (RSA/EC). I am actually not sure about it. While the function currently uses NSS, it is self-contained and does not expose any NSS details. To use pkix for that purpose, we will need to add a similar function to pkix, probably modeled after CheckSubjectPublicKeyInfo in pkixcheck.cpp. But from looking at the pkixcheck code, I can't tell whether it handles all the cases we need and I will need help to properly implement the new function. Alternatively, we can leave the NSS based ExtractSignatureParameters without adding functionality to pkix. What do you think? > In every identifier name, "DER" is usually preferrable to "Der" (similarly with other acronyms). OK. In this particular case, I've removed the "DER" suffix since it is not used in similar cases in pkix and other code. > I'm concerned using this paradigm safely and correctly will be difficult. Let's have these functions return the success/failure of the overall operation (e.g. if given invalid arguments or if memory allocation fails, we should return an error). Then, if there's an error in decoding a specific SCT, we'll know this because of which bucket it's in in the CTVerifyResult structure. I think the field name and the current documentation is misleading. Let me explain - When verifying a certificate using SCTs, we need at least one valid SCT for the verification to pass. The verifier makes the best effort to extract the SCTs available from the provided binary sources. It does not stop on a malformed SCT (or, for that matter, on a SCT created by a future CT version). It also does not stop if a decoded SCT fails to verify because of, say, invalid signature data format. With the SCTs it has managed to extract, the verifier performs the verification, filling in the relevant fields of the CTVerifyResult struct. In release builds, there is not much to do with such errors besides silently discarding them. When debugging though, it could be nice to have some kind of information on the ignored errors. Hence the lastIgnoredError field, which is the easiest thing to implement. What do you think? > Can we not just link against the original copy? Extracted the relevant code from CTObjectsExtractor to CTObjectsExtractorInternal, shared by CTObjectsExtractor and the unit tests.

> In release builds, there is not much to do with *such errors* besides silently discarding them. Clarification - "such errors" are the currently ignored errors (such as when extracting SCTs from binary data).

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); https://reviewboard.mozilla.org/r/62676/#review61338 Cool - I think this looks better in terms of re-using mozilla::pkix code instead of NSS code. I didn't review CTObjectsExtractor.cpp - let me know if you want any input on how to use/refactor mozilla::pkix to do what we want there. I came up with a way of implementing ExtractSignatureParameters in terms of CheckSubjectPublicKeyInfo - let me know what you think. I think I understand the purpose of lastIgnoredError, but after some changes I don't think it will be necessary (see below). Also, with a fairly small change we can get away with not moving so much code around and creating new files, which is preferable in this case. ::: security/certverifier/CTLogVerifier.cpp:9 (Diff revision 2) > + * 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/. */ > + > +#include "CTLogVerifier.h" > + > +#include "keyhi.h" nit: #includes should be sorted alphabetically ::: security/certverifier/CTLogVerifier.cpp:23 (Diff revision 2) > +namespace mozilla { namespace ct { > + > +using namespace mozilla::pkix; > + > +static Result > +ExtractSignatureParameters(Input subjectPublicKeyInfo, We should be able to implement this by creating a TrustDomain implementation that returns Result::FATAL_ERROR_LIBRARY_FAILURE for everything except CheckECDSACurveIsAcceptable and CheckRSAPublicKeyModulusSizeInBits. Given that, we can just call CheckSubjectPublicKeyInfo and then see which type of key it turned out to be. ::: security/certverifier/MultiLogCTVerifier.h:38 (Diff revision 2) > + // For a certificate to pass Certificate Transparency verification, at least > + // one of the provided SCTs must validate. The verifier makes the best effort > + // to extract the available SCTs from the binary sources provided to it. > + // If some SCT cannot be extracted or verified due to encoding or format > + // errors, the verifier proceeds to the next available one. In other words, > + // such errors are effectively discarded. But we don't have to discard them, right? Let's have a 4th list in CTVerifyResult that collects the SCTs that had such errors. If it's empty, then the verifier must not have seen any invalid input. You're right that it's probably only useful for debugging and maybe tests, but we can also gather telemetry on how many invalid SCTs we're seeing (if the number is high, it might be an indication there's an error in some implementation of CT (either ours or a CA's, for example)). ::: security/certverifier/MultiLogCTVerifier.h:42 (Diff revision 2) > + // errors, the verifier proceeds to the next available one. In other words, > + // such errors are effectively discarded. > + // Note that a serialized SCT may fail to decode for a "legitimate" reason, > + // e.g. if the SCT is from a future version of the Certificate Transparency > + // standard. > + // |lastIgnoredError| field stores the last error of the above kind. It is lastIgnoredError seems to store other errors as well, such as the out of memory error in MultiLogCTVerifier.cpp:51. These are the kinds of errors that should be returned from MultiLogCTVerifier::Verify and related functions. Once that change is made and there's a 4th SCTList for SCTs that failed to decode, I don't think we need this any longer. ::: security/certverifier/MultiLogCTVerifier.h:72 (Diff revision 2) > + // in which case the embedded SCT list won't be verified. > + // |sctListFromOcspResponse| is the SCT list included in a stapled OCSP > + // response for |cert|. Empty if not available. > + // |sctListFromTlsExtension| is the SCT list from the TLS extension. Empty if > + // no extension was present. > + // |now| is the current time. Used to make sure SCTs are not in the future. The format of |now| is the same as in the SCT, right? Let's just have a note that says that (and maybe explicitly say it's milliseconds since the epoch ignoring leap seconds). Also, maybe |time| or |verificationTime| would be a better name. ::: security/certverifier/MultiLogCTVerifier.h:75 (Diff revision 2) > + // |sctListFromTlsExtension| is the SCT list from the TLS extension. Empty if > + // no extension was present. > + // |now| is the current time. Used to make sure SCTs are not in the future. > + // |result| will be filled with the SCTs present, divided into categories > + // based on the verification result. > + // The verification process does not stop on errors (but |result| Again, if there are fatal errors, the return value of this function should indicate that. ::: security/certverifier/MultiLogCTVerifier.cpp:20 (Diff revision 2) > + > +// The possible verification statuses for a Signed Certificate Timestamp. > +enum class SCTVerifyStatus { > + UnknownLog, // The SCT is from an unknown log and can not be verified. > + Invalid, // The SCT is from a known log, but the signature is invalid. > + OK // The SCT is from a known log, and the signature is valid. Should we add a "DecodingFailed" status? ::: security/certverifier/MultiLogCTVerifier.cpp:97 (Diff revision 2) > + sctListFromCert.GetLength() > 0) { > + VerifyEmbeddedSctList(cert, issuerSubjectPublicKeyInfo, > + sctListFromCert, now, result); > + } > + > + if (sctListFromOcspResponse.GetLength() == 0 && Since GetX509LogEntry isn't that slow or resource-intensive, we don't need this early return. ::: security/certverifier/tests/gtest/CTLogVerifierTest.cpp:95 (Diff revision 2) > + > +TEST_F(CTLogVerifierTest, DoesNotSetInvalidSTH) > +{ > + SignedTreeHead sth; > + GetSampleSignedTreeHead(sth); > + sth.sha256RootHash[0] = '\x0'; We should probably assert sth.sha256RootHash[0] isn't already 0. ::: security/certverifier/tests/gtest/CTTestUtils.h:71 (Diff revision 2) > > // The same signature as GetSampleSTHTreeHeadSignature, decoded. > void GetSampleSTHTreeHeadDecodedSignature(DigitallySigned& signature); > > +// Certificate with embedded SCT in an X509v3. > +Buffer GetDerEncodedTestEmbeddedCert(); Let's use "DER" instead of "Der" on all of these as well. ::: security/certverifier/tests/gtest/CTTestUtils.cpp:314 (Diff revision 2) > + "37586d73"; > + > +// A well-formed OCSP response with fake SCT contents. Does not come from > +// the CT test data repository, does not pertain to any of the test certs here, > +// and is only used to test extracting the extension contents from the response. > +// Source: https://cs.chromium.org/chromium/src/net/test/ct_test_util.cc We probably have to include the Chromium copyright block if these are more or less verbatim from Chromium source code. ::: security/certverifier/tests/gtest/CTTestUtils.cpp:636 (Diff revision 2) > +ExtractEmbeddedSCTList(const Buffer& cert, Buffer& result) > +{ > + ExtractEmbeddedSCTList(InputForBuffer(cert), result); > +} > + > +class OCSPExtensionTrustDomain : public TrustDomain We should determine which functions aren't expected to be called and just return Result::FATAL_ERROR_LIBRARY_FAILURE (e.g. NetscapeStepUpMatchesServerAuth will probably never be called). ::: security/certverifier/tests/gtest/CTTestUtils.cpp:781 (Diff revision 2) > + input.Init(item.data, item.len)); > + return input; > +} > + > +Result > +EncodeSCTListForTesting(Input sct, Buffer& output) Let's put this in CTSerialization.cpp and expose it in CTSerialization.h (maybe as "EncodeSCTList"). That way we don't have to add CTSerializationInternal and move all that code around. ::: security/certverifier/tests/gtest/MultiLogCTVerifierTest.cpp:60 (Diff revision 2) > + { > + result.clear(); > + Buffer sct(GetTestSignedCertificateTimestamp()); > + // Change a byte inside the Log ID part of the SCT so it does > + // not match the log used in the tests. > + sct[15] = 't'; Again we should probably just assert that sct[15] isn't already 't'. ::: security/certverifier/tests/gtest/MultiLogCTVerifierTest.cpp:176 (Diff revision 2) > + CTVerifyResult result; > + mVerifier.Verify(InputForBuffer(mTestCert), Input(), > + Input(), Input(), InputForBuffer(sctList), mNow, result); > + > + CheckForSingleVerifiedSCTInResult(result, > + SignedCertificateTimestamp::Origin::TLSExtension); nit: this can probably just be indented two spaces from the beginning of the first 'C' of the previous line

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62676/diff/2-3/

https://reviewboard.mozilla.org/r/62676/#review61338 Uploaded the updated patch - the open issues were addressed besides the copyright note (Eran is checking on the proper way to do that). The major changes are: 1. Reworked the error handling semantics of MultiLogCTVerifier. It now only ignores SCT decoding errors and fails on others. 2. CTObjectsExtractor now uses pkix. 3. CTLogVerifier now uses pkix to extract the key parameters (following your suggestion). Note that to implement 2, I had to link into pkix internals (pkixutil.h), mainly for DER parsing. The other option is to move the related code into pkix, but the code looks too ad hoc to me to be included in the infrastructure. It can be done though with some minor changes (CTObjectsExtractor currently uses mozilla::Vector internally, but we can remove that dependency without much effort). Let me know what you think. > We should be able to implement this by creating a TrustDomain implementation that returns Result::FATAL_ERROR_LIBRARY_FAILURE for everything except CheckECDSACurveIsAcceptable and CheckRSAPublicKeyModulusSizeInBits. Given that, we can just call CheckSubjectPublicKeyInfo and then see which type of key it turned out to be. Great, thanks for the suggestion! > But we don't have to discard them, right? Let's have a 4th list in CTVerifyResult that collects the SCTs that had such errors. If it's empty, then the verifier must not have seen any invalid input. > > You're right that it's probably only useful for debugging and maybe tests, but we can also gather telemetry on how many invalid SCTs we're seeing (if the number is high, it might be an indication there's an error in some implementation of CT (either ours or a CA's, for example)). Note that if SCT deserialization failed, we won't have an SCT to store in the 4th list. We will only have a binary block of data that failed to decode. That data block can represent a single SCT or a list of SCTs. I've added a field which counts the decoding errors (I don't think we need to keep the original data block). This counter can be used for the purposes you've mentioned. > lastIgnoredError seems to store other errors as well, such as the out of memory error in MultiLogCTVerifier.cpp:51. These are the kinds of errors that should be returned from MultiLogCTVerifier::Verify and related functions. Once that change is made and there's a 4th SCTList for SCTs that failed to decode, I don't think we need this any longer. Agreed, removed the lastIgnoredError field. > The format of |now| is the same as in the SCT, right? Let's just have a note that says that (and maybe explicitly say it's milliseconds since the epoch ignoring leap seconds). Also, maybe |time| or |verificationTime| would be a better name. Yes, documented the format and changed the name to |time| for consistency with CertVerifier. > Should we add a "DecodingFailed" status? Currently there seems to be no need - the status represents the result of the verification, but the decoding happens before the verification.

https://reviewboard.mozilla.org/r/62676/#review63236 This is looking great. I haven't quite finished reviewing everything, but I wanted to give you at least some feedback before the weekend. I'll finish up the review hopefully Monday. ::: security/certverifier/CTLogVerifier.h:63 (Diff revision 3) > + pkix::Result VerifySignature(pkix::Input data, pkix::Input signature); > + pkix::Result VerifySignature(const Buffer& data, const Buffer& signature); > + > + Buffer mSubjectPublicKeyInfo; > + Buffer mKeyId; > + DigitallySigned::HashAlgorithm mHashAlgorithm; I don't really understand why it's important to keep track of mHashAlgorithm if it can only be SHA-256 for the current specification of CT. ::: security/certverifier/CTLogVerifier.cpp:22 (Diff revision 3) > + > +using namespace mozilla::pkix; > + > +// A TrustDomain used to extract the SCT log signature parameters > +// given its subjectPublicKeyInfo. > +// Only RSASSA-PKCS1v15 with SHA-256 and ECDSA with SHA-256 are allowed. nit: let's add that only the NIST P-256 curve is supported ::: security/certverifier/CTLogVerifier.cpp:25 (Diff revision 3) > +// A TrustDomain used to extract the SCT log signature parameters > +// given its subjectPublicKeyInfo. > +// Only RSASSA-PKCS1v15 with SHA-256 and ECDSA with SHA-256 are allowed. > +// RSA keys must be at least 2048 bits. > +// See See RFC 6926, Section 2.1.4. > +class SignatureParamsTrustDomain : public TrustDomain Let's specify "final" for this class. ::: security/certverifier/CTLogVerifier.cpp:67 (Diff revision 3) > + Time) override > + { > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + > + Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override Since NIST P-256 (NamedCurve::secp256r1 in this codebase) is the only allowable curve at the moment, let's enforce that here (and return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE if we encounter a curve that isn't allowed). ::: security/certverifier/CTLogVerifier.cpp:85 (Diff revision 3) > + unsigned int modulusSizeInBits) > + override > + { > + // Require RSA keys of at least 2048 bits. See RFC 6926, Section 2.1.4. > + if (modulusSizeInBits < 2048) { > + return Result::FATAL_ERROR_INVALID_ARGS; Result::ERROR_INADEQUATE_KEY_SIZE would be a more useful error code here. ::: security/certverifier/CTLogVerifier.cpp:249 (Diff revision 3) > + return rv; > + } > + > + switch (mSignatureAlgorithm) { > + case DigitallySigned::SignatureAlgorithm::RSA: > + rv = VerifyRSAPKCS1SignedDigestNSS(signedDigest, spki, nullptr); nit: let's just return directly here instead of setting rv and then returning it later (same with the ECDSA case) ::: security/certverifier/CTObjectsExtractor.h:16 (Diff revision 3) > +#include "pkix/Input.h" > +#include "SignedCertificateTimestamp.h" > + > +namespace mozilla { namespace ct { > + > +// Obtains a PrecertChain log entry for |leafCertificate|, a DER-encoded We should note that the leafCertificate should already have been verified by BuildCertChain. ::: security/certverifier/CTObjectsExtractor.h:29 (Diff revision 3) > +// an error is returned. > +pkix::Result GetPrecertLogEntry(pkix::Input leafCertificate, > + pkix::Input issuerSubjectPublicKeyInfo, > + LogEntry& output); > + > +// Obtains an X509Chain log entry for |leafCertificate|, a DER-encoded (Similarly here) ::: security/certverifier/CTObjectsExtractor.cpp:31 (Diff revision 3) > +{ > +public: > + Output(uint8_t* buffer, size_t length) > + : begin(buffer) > + , current(buffer) > + , end(buffer + length) I think we have to check for pointer overflow here (or change the type of length so it's not possible to overflow a userspace pointer). ::: security/certverifier/CTObjectsExtractor.cpp:64 (Diff revision 3) > + size_t length = static_cast<size_t>(current - begin); > + return input.Init(begin, length); > + } > + > +private: > + uint8_t* begin; This might be a good application for a ranged pointer (see mfbt/RangedPtr.h). ::: security/certverifier/CTObjectsExtractor.cpp:203 (Diff revision 3) > + Result ExtractOptionalExtensionsExceptSCTs(Reader& tbsReader) > + { > + if (!tbsReader.Peek(EXTENSIONS_CONTEXT_TAG)) { > + return Success; > + } > + Result rv; nit: declare this when it's first assigned to ::: security/certverifier/CTObjectsExtractor.cpp:252 (Diff revision 3) > + { > + // What should be written here: > + // > + // TBSCertificate ::= SEQUENCE with header |tbsHeader| > + // dump of |mTLVsBeforeExtensions| > + // extensions [ 3 ] OPTIONAL with header |extensionsContextHeader| nit: [3] ? (i.e. I was thinking there would be no spaces before/after the "3") ::: security/certverifier/CTObjectsExtractor.cpp:268 (Diff revision 3) > + Input extensionsContextHeader; > + Input extensionsHeader; > + > + uint16_t extensionsValueLength = 0; > + for (auto& extensionTLV : mExtensionTLVs) { > + extensionsValueLength += extensionTLV.GetLength(); We should probably check for overflow here, even though currently we don't support certificates long enough to actually overflow a uint16_t. mfbt/CheckedInt.h might be of use here. ::: security/certverifier/CTObjectsExtractor.cpp:330 (Diff revision 3) > + output.Write(static_cast<uint8_t>(length)); > + } else { > + output.Write(0x82u); > + output.Write(static_cast<uint8_t>(length / 256)); > + output.Write(static_cast<uint8_t>(length % 256)); > + } Let's |static_assert(sizeof(length) == 2, "This only produces valid DER if length < 65k");| or similar ::: security/certverifier/CTObjectsExtractor.cpp:341 (Diff revision 3) > + Vector<Input, 16> mExtensionTLVs; > + Output mOutput; > + Input mPrecertTBS; > +}; > + > + nit: probably don't need a second blank line here ::: security/certverifier/CTSerialization.cpp:248 (Diff revision 3) > > -// Writes a variable-length array to |output|. > +// A variable-length byte array is prefixed by its length when serialized. > +// This writes the length prefix. > // |prefixLength| indicates the number of bytes needed to represent the length. > -// |input| is the array itself. > +// |dataLength| is the length of the byte array following the prefix. > // Fails if the size of |input| is more than 2^|prefixLength| - 1. nit: s/the size of |input|/|dataLength|/ ::: security/certverifier/MultiLogCTVerifier.h:14 (Diff revision 3) > + > +#include "mozilla/Vector.h" > +#include "pkix/Result.h" > +#include "pkix/Input.h" > +#include "SignedCertificateTimestamp.h" > +#include "CTLogVerifier.h" nit: remember to sort #include lists ::: security/certverifier/MultiLogCTVerifier.h:37 (Diff revision 3) > + > + // For a certificate to pass Certificate Transparency verification, at least > + // one of the provided SCTs must validate. The verifier makes the best effort > + // to extract the available SCTs from the binary sources provided to it. > + // If some SCT cannot be extracted due to encoding errors, the verifier > + // proceeds to the next available one. In other words,m decoding errors are typo: stray 'm' near the end of the line ::: security/certverifier/MultiLogCTVerifier.cpp:78 (Diff revision 3) > + > +Result > +MultiLogCTVerifier::Verify(Input cert, > + Input issuerSubjectPublicKeyInfo, > + Input sctListFromCert, > + Input sctListFromOcspResponse, nit: for non-leading acronyms, I think it's best to capitalize all of the letters (so |sctListFromOCSPResponse| here and |sctListFromTLSExtension| on the next line) ::: security/pkix/lib/pkixcheck.cpp:364 (Diff revision 3) > +CheckSubjectPublicKeyInfo(Input subjectPublicKeyInfo, TrustDomain& trustDomain, > + EndEntityOrCA endEntityOrCA) > +{ > + Reader spkiReader(subjectPublicKeyInfo); > + Result rv = der::Nested(spkiReader, der::SEQUENCE, [&](Reader& r) { > + return CheckSubjectPublicKeyInfo(r, trustDomain, endEntityOrCA); Let's rename the preexisting |CheckSubjectPublicKeyInfo| to |CheckSubjectPublicKeyInfoContents| for clarity and then have |CheckIssuerIndependentProperties| call this new function so we don't duplicate code.

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); https://reviewboard.mozilla.org/r/62676/#review63818 I guess I really only had the one additional comment. Clearing review for now while these items are addressed. ::: security/certverifier/tests/gtest/CTLogVerifierTest.cpp:63 (Diff revision 3) > + GetX509CertLogEntry(certEntry); > + > + SignedCertificateTimestamp certSct; > + GetX509CertSCT(certSct); > + > + // Mangle the timestamp, so that it should fail signature validation. We should probably also have a testcase where the sct signature has been modified, so the signature doesn't verify.

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62676/diff/3-4/

https://reviewboard.mozilla.org/r/62676/#review63236 > We should note that the leafCertificate should already have been verified by BuildCertChain. Added clarification that the function does not check the certificate for validity or well-formedness (I think that full verification is not strictly a requirement in the context of CTObjectsExtractor.h). > I think we have to check for pointer overflow here (or change the type of length so it's not possible to overflow a userspace pointer). I technically agree, but I think it is pretty common (for C code) to pass around a memory buffer as a raw pointer and a length (as size_t), assuming they represent a properly allocated block of memory. So assuming the buffer allocation was valid (which the receiver cannot ensure anyway), I think it's OK not to check for overflows and use size_t (as commonly done with array lengths). > We should probably check for overflow here, even though currently we don't support certificates long enough to actually overflow a uint16_t. mfbt/CheckedInt.h might be of use here. Changed the length variable type from uint16_t to Input::size_type. I think we can assume Input::size_type won't overflow in our case.

https://reviewboard.mozilla.org/r/62676/#review63818 > We should probably also have a testcase where the sct signature has been modified, so the signature doesn't verify. Excellent comment! Added the testcase, and changed the error handling in CTLogVerifier::VerifySignature following it. It now considers all non-fatal errors returned by VerifyXXXSignedDigestNSS as signature validation errors, and returns the appropriate error to the caller. Glad we caught this now.

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); https://reviewboard.mozilla.org/r/62676/#review64512 Great! Just a couple more small items. Also, I'd like to have another set of eyes on this, so I'll ask for review from Cykesiopka. ::: security/certverifier/CTLogVerifier.cpp:72 (Diff revisions 3 - 4) > - Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve) override > + Result CheckECDSACurveIsAcceptable(EndEntityOrCA, NamedCurve curve) override > { > - mHashAlgorithm = DigitallySigned::HashAlgorithm::SHA256; > + if (curve != NamedCurve::secp256r1) { > + return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE; > + } > mSignatureAlgorithm = DigitallySigned::SignatureAlgorithm::ECDSA; Just to be safe, we should probably assert mSignatureAlgorithm == DigitallySigned::SignatureAlgorithm::Anonymous at this point. ::: security/certverifier/CTLogVerifier.cpp:89 (Diff revisions 3 - 4) > // Require RSA keys of at least 2048 bits. See RFC 6926, Section 2.1.4. > if (modulusSizeInBits < 2048) { > - return Result::FATAL_ERROR_INVALID_ARGS; > + return Result::ERROR_INADEQUATE_KEY_SIZE; > } > - mHashAlgorithm = DigitallySigned::HashAlgorithm::SHA256; > mSignatureAlgorithm = DigitallySigned::SignatureAlgorithm::RSA; Same here. ::: security/certverifier/CTObjectsExtractor.cpp:267 (Diff revisions 3 - 4) > > Input tbsHeader; > Input extensionsContextHeader; > Input extensionsHeader; > > - uint16_t extensionsValueLength = 0; > + Input::size_type extensionsValueLength = 0; Let's add a comment that explains that since these extensions were all at one point in the same Input, there's no way their combined length could overflow Input::size_type. ::: security/certverifier/tests/gtest/CTLogVerifierTest.cpp:78 (Diff revisions 3 - 4) > + > + SignedCertificateTimestamp certSct; > + GetX509CertSCT(certSct); > + > + // Mangle the signature, so that it should fail validation. > + certSct.signature.signatureData[0] ^= '\xFF'; If I'm understanding the code correctly, this is mangling the encoding of an EC signature, which is maybe a good case to have, but I don't think is sufficient - I was thinking of modifying the value of the signature while keeping it syntactically valid.

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62676/diff/4-5/

As a heads up, I should be finished with my review within a day. Sorry for the delay.

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); https://reviewboard.mozilla.org/r/62676/#review66024 Cool - this looks good, but I would like to take another look after everything is addressed. Again, sorry for the delay. ::: security/certverifier/CTLogVerifier.h:10 (Diff revision 5) > +#include "pkix/pkix.h" > +#include "pkix/Input.h" > +#include "SignedCertificateTimestamp.h" > +#include "SignedTreeHead.h" Nit: Sort alphabetically. Same thing for a lot of the other include lists. ::: security/certverifier/CTLogVerifier.cpp:25 (Diff revision 5) > +// A TrustDomain used to extract the SCT log signature parameters > +// given its subjectPublicKeyInfo. > +// Only RSASSA-PKCS1v15 with SHA-256 and ECDSA (using the NIST P-256 curve) > +// with SHA-256 are allowed. > +// RSA keys must be at least 2048 bits. > +// See See RFC 6926, Section 2.1.4. Typo: s/6926/6962/. ::: security/certverifier/CTLogVerifier.cpp:89 (Diff revision 5) > + unsigned int modulusSizeInBits) > + override > + { > + MOZ_ASSERT(mSignatureAlgorithm == > + DigitallySigned::SignatureAlgorithm::Anonymous); > + // Require RSA keys of at least 2048 bits. See RFC 6926, Section 2.1.4. Typo: s/6926/6962/. ::: security/certverifier/CTLogVerifier.cpp:113 (Diff revision 5) > + Result NetscapeStepUpMatchesServerAuth(Time, bool&) override > + { > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + > + virtual void NoteAuxiliaryExtension(AuxiliaryExtension, Input) override Nit: It looks like |virtual| here is unnecessary. ::: security/certverifier/CTObjectsExtractor.cpp:29 (Diff revision 5) > +// overflows, an error flag is turned on and you won't be able to retrieve > +// the final data. > +class Output > +{ > +public: > + Output(uint8_t* buffer, size_t length) This is fine for now, but I think it would be better if we avoided this pattern as much as possible in the future. ::: security/certverifier/CTObjectsExtractor.cpp:60 (Diff revision 5) > + Result GetInput(/*out*/ Input& input) const > + { > + if (overflowed) { > + return Result::FATAL_ERROR_INVALID_STATE; > + } > + size_t length = static_cast<size_t>(current.get() - begin); Let's use AssertedCast from Casting.h instead for a bit more safety here and anywhere else new uses of static_cast are being added. ::: security/certverifier/CTObjectsExtractor.cpp:82 (Diff revision 5) > + overflowed = true; > + } > + if (overflowed) { > + return; > + } > + memcpy(current.get(), data, length); PodCopy() from PodOperations.h seems like it would be a tiny bit safer, but up to you. ::: security/certverifier/CTObjectsExtractor.cpp:170 (Diff revision 5) > + { > + return mPrecertTBS; > + } > + > +private: > + Nit: Extra blank line. ::: security/certverifier/CTObjectsExtractor.cpp:239 (Diff revision 5) > + Input extensionTLV; > + rv = extensionsReader.GetInput(extensionTLVBegin, extensionTLV); > + if (rv != Success) { > + return rv; > + } > + if (!mExtensionTLVs.append(Move(extensionTLV))) { This should probably have a corresponding explicit "mozilla/Move.h" include. Same everywhere else. ::: security/certverifier/CTSerialization.cpp:520 (Diff revision 5) > + size_t sctListLength = 0; > + for (auto& sct : scts) { > + sctListLength += > + /* data size */ sct.GetLength() + > + /* length prefix size */ kSerializedSCTLengthBytes; > + } Maybe I'm missing something - can't this theoretically overflow? ::: security/certverifier/MultiLogCTVerifier.h:58 (Diff revision 5) > + // Verifies SCTs embedded in the certificate itself, SCTs embedded in a > + // stapled OCSP response, and SCTs obtained via the > + // signed_certificate_timestamp TLS extension on the given |cert|. > + // A certificate is permitted but not required to use multiple sources for > + // SCTs. It is expected that most certificates will use only one source > + // (embedding, TLS extension or OCSP stapling). > + // |cert| - DER-encoded certificate to be validated using the provided SCTs. > + // |sctListFromCert| - SCT list embedded in |cert|, empty if not present. > + // |issuerSubjectPublicKeyInfo| - |cert| issuer's SPKI. Can be empty, > + // in which case the embedded SCT list won't be verified. > + // |sctListFromOCSPResponse| is the SCT list included in a stapled OCSP > + // response for |cert|. Empty if not available. > + // |sctListFromTLSExtension| is the SCT list from the TLS extension. Empty if > + // no extension was present. > + // |time| is the current time. Used to make sure SCTs are not in the future. > + // Measured in milliseconds since the epoch, ignoring leap seconds > + // (same format as the "timestamp" field of SignedCertificateTimestamp). > + // |result| will be filled with the SCTs present, divided into categories > + // based on the verification result. > + // The verifier stops on fatal errors (such as out of memory or invalid > + // DER encoding of |cert|), but it does not stop on SCT decoding errors. See > + // CTVerifyResult for more details. > + // The internal state of the verifier object is not modified > + // during the verification process. Comments like these would probably be easier to read as JavaDoc style comments (and this is what the style guide says we should do) but I leave it up to you. ::: security/certverifier/MultiLogCTVerifier.h:83 (Diff revision 5) > + pkix::Input issuerSubjectPublicKeyInfo, > + pkix::Input sctListFromCert, > + pkix::Input sctListFromOCSPResponse, > + pkix::Input sctListFromTLSExtension, Should we assert that these inputs are non-empty such that at least one of the if blocks below are entered, or are we just going to assume the caller just checks |result| as appropriate? ::: security/certverifier/MultiLogCTVerifier.h:93 (Diff revision 5) > + CTVerifyResult& result); > + > +private: > + // Verifies a list of SCTs from |encodedSctList| over |expectedEntry|, > + // placing the verification results in |result|. The SCTs in the list > + // come from |origin| (as will be indicated in the origin field of each SCT). Nit: Maybe s/indicated/reflected/? I initially thought this meant the origin field of each SCT was already set when this method gets called. Could just be me though. ::: security/certverifier/MultiLogCTVerifier.cpp:23 (Diff revision 5) > +// Note: this moves |sct| to the target list in |result|, invalidating |sct|. > +static Result > +StoreVerifiedSct(CTVerifyResult& result, > + SignedCertificateTimestamp& sct, This comment should probably be propagated upwards. Also, consider using |SignedCertificateTimestamp&&| so it's more clear at the call sites as well. ::: security/certverifier/MultiLogCTVerifier.cpp:50 (Diff revision 5) > + return Result::FATAL_ERROR_NO_MEMORY; > + } > + return Success; > +} > + > + Nit: Extra blank line. ::: security/certverifier/MultiLogCTVerifier.cpp:178 (Diff revision 5) > + const LogEntry& expectedEntry, > + uint64_t time, > + CTVerifyResult& result) > +{ > + CTLogVerifier* matchingLog = nullptr; > + for (CTLogVerifier* log = mLogs.begin(); log != mLogs.end(); ++log) { This might look nicer as a range based for loop, if preferred. ::: security/certverifier/tests/gtest/CTLogVerifierTest.cpp:104 (Diff revision 5) > + MOZ_RELEASE_ASSERT(certSct.logId.append('\x0')); > + > + EXPECT_EQ(Result::FATAL_ERROR_INVALID_ARGS, mLog.Verify(certEntry, certSct)); > +} > + > +TEST_F(CTLogVerifierTest, SetsValidSTH) Nit: "Sets" seems slightly weird to me, maybe "VerifiesValidSTH"? ::: security/certverifier/tests/gtest/CTLogVerifierTest.cpp:111 (Diff revision 5) > + SignedTreeHead sth; > + GetSampleSignedTreeHead(sth); > + EXPECT_EQ(Success, mLog.VerifySignedTreeHead(sth)); > +} > + > +TEST_F(CTLogVerifierTest, DoesNotSetInvalidSTH) Nit: Same "Set" comment as above. ::: security/certverifier/tests/gtest/CTObjectsExtractorTest.cpp:45 (Diff revision 5) > + Buffer mCaCert; > + Buffer mCaCertSPKI; > + CTLogVerifier mLog; > +}; > + > +TEST_F(CTObjectsExtractorTest, ExtractPrecert) We ought to have something that tests that |entry.tbsCertificate| is correct. However, it looks like there isn't anything in ct_test_util.cc or the CT test repo that makes this easy, so feel free to defer this to a follow up bug if needed and add a TODO comment here. ::: security/certverifier/tests/gtest/CTObjectsExtractorTest.cpp:66 (Diff revision 5) > + LogEntry entry; > + ASSERT_EQ(Success, GetX509LogEntry(InputForBuffer(mTestCert), entry)); > + > + EXPECT_EQ(LogEntry::Type::X509, entry.type); > + // Should have empty tbsCertificate for this log entry type. > + EXPECT_TRUE(entry.tbsCertificate.empty()); Maybe we should check |issuerKeyHash.empty()| as well? ::: security/certverifier/tests/gtest/CTTestUtils.h:70 (Diff revision 5) > Buffer GetSampleSTHTreeHeadSignature(); > > // The same signature as GetSampleSTHTreeHeadSignature, decoded. > void GetSampleSTHTreeHeadDecodedSignature(DigitallySigned& signature); > > +// Certificate with embedded SCT in an X509v3. Nit: s/X509v3/X509v3 extension/. ::: security/certverifier/tests/gtest/CTTestUtils.cpp:312 (Diff revision 5) > + "37586d73"; > + > +// A well-formed OCSP response with fake SCT contents. Does not come from > +// the CT test data repository, does not pertain to any of the test certs here, > +// and is only used to test extracting the extension contents from the response. > +// Source: https://cs.chromium.org/chromium/src/net/test/ct_test_util.cc I think being more explicit and using a link that includes the revision like https://chromium.googlesource.com/chromium/src/+/b38ec0a4aaf590b21c1344a5962bcfeb65f3e3d6/net/test/ct_test_util.cc#113 is nicer, but up to you. ::: security/certverifier/tests/gtest/CTTestUtils.cpp:360 (Diff revision 5) > +const Time kFakeOCSPThisUpdateTime = > + TimeFromElapsedSecondsAD(63397922400); // 20100101060000Z I find this format: > // Date.parse("2010-01-01T06:00:00Z") / 1000; > const Time kFakeOCSPThisUpdateTime = TimeFromEpochInSeconds(1262325600); ... easier to audit, but up to you. ::: security/certverifier/tests/gtest/MultiLogCTVerifierTest.cpp:23 (Diff revision 5) > + > +namespace mozilla { namespace ct { > + > +using namespace mozilla::pkix; > + > +class MultiLogCTVerifierTest : public ::testing::Test It looks like we also ought to have test for MultiLogCTVerifier::Verify() + OCSP, and a test for MultiLogCTVerifier::Verify() + multiple SCT sources. Again, feel free to defer and just add some TODOs for now.

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62676/diff/5-6/

https://reviewboard.mozilla.org/r/62676/#review66024 > Maybe I'm missing something - can't this theoretically overflow? Theoretically, if the total length of all Inputs in scts plus kSerializedSCTLengthBytes overhead per sct exceeds the maximum size of a possible object (size_t), sctListLength will overflow. But I think we can assume that it won't happen. > Comments like these would probably be easier to read as JavaDoc style comments (and this is what the style guide says we should do) but I leave it up to you. I've reformatted the text to be more readable. Didn't use the JavaDoc style for consistency with other comments. > Should we assert that these inputs are non-empty such that at least one of the if blocks below are entered, or are we just going to assume the caller just checks |result| as appropriate? The caller must check the SCTs in |result| to find out whether the certificate has passed the CT verification (the return value of the function does not indicate that). That being the case, I think we don't need to assert that the caller has passed us any SCTs in the Inputs.

https://reviewboard.mozilla.org/r/62676/#review66024 > Theoretically, if the total length of all Inputs in scts plus kSerializedSCTLengthBytes overhead per sct exceeds the maximum size of a possible object (size_t), sctListLength will overflow. But I think we can assume that it won't happen. Hmm, yeah, looks like the numbers would have to be pretty absurd to get this to overflow.

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); https://reviewboard.mozilla.org/r/62676/#review66696 Looks good, assuming the warnings-as-errors stuff from your try push is fixed and the license thing is sorted out. ::: security/certverifier/tests/gtest/CTTestUtils.cpp:382 (Diff revision 6) > +const Time kFakeOCSPThisUpdateTime = > + // Date.parse("2010-01-01T06:00:00Z") / 1000 > + TimeFromElapsedSecondsAD(63397922400); Sorry, I guess I wasn't clear - the Date.parse() comment is useful to explain the constant we would pass if we used TimeFromEpochInSeconds(), but is probably confusing if we use TimeFromElapsedSecondsAD(). Let's choose one of the below: 1. Date.parse() comment + TimeFromEpochInSeconds(). 2. Pure timestamp comment + TimeFromElapsedSecondsAD().

Comment on attachment 8768418 [details] Bug 1284256 - Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); https://reviewboard.mozilla.org/r/62676/#review66696 Fixed the warnings (see the last two try builds). Regarding the license issue - I've removed the offending OCSP data and the relevant unit test from MultiLogCTVerifierTest. Extraction of SCTs from OCSP responses is handled by pkix (see bug 1275238) and is tested as part of pkix unit tests. I thought it would be nice to have an additional test case somewhere which uses an independently generated block of raw data as a sanity check. Given the legal questions it's probably not worth the trouble, best to drop this test case altogether since it is covered by pkix tests. > Sorry, I guess I wasn't clear - the Date.parse() comment is useful to explain the constant we would pass if we used TimeFromEpochInSeconds(), but is probably confusing if we use TimeFromElapsedSecondsAD(). > Let's choose one of the below: > 1. Date.parse() comment + TimeFromEpochInSeconds(). > 2. Pure timestamp comment + TimeFromElapsedSecondsAD(). That was my problem actually, I didn't update the original constant. The idea of documenting integer date constants using JavaScript is very nice.

Pulled & rebased, all units tests pass (locally).

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a29eab248d20 Certificate Transparency - verification of Signed Certificate Timestamps (RFC 6962); r=keeler, r=Cykesiopka

What is the status of this? It doesn't seem to be operational in 55.0a1 (2017-06-01), as evidenced by test: https://invalid-expected-sct.badssl.com/ But, the comments seem to imply that it was implemented in 51.0 ?

This bug implemented the capability to decode and verify SCTs. Firefox has never enforced any kind of CT requirement. Currently it's disabled entirely - see bug 1355903.