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

Signed Certificate Timestamps (SCTs, see RFC 6962) are embedded in certificates and OCSP response data. We will add support for extracting serialized SCT data to mozilla::pkix. Further parsing of the raw data is handled externally (see bug 1241574). Implementation outline (as proposed by David Keeler in bug 1241574): * 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.

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

The submitted code has two parts: 1. pkix and related - these changes are ready for the code review. 2. NSSCertDBTrustDomain / CertVerifier - just a "preview" of how the pkix changes can actually be integrated into CertVerifier during the later stage of the project. Those changes are not to be committed.

Comment on attachment 8763064 [details] Bug 1275238 - Certificate Transparency support in mozilla::pkix; https://reviewboard.mozilla.org/r/59396/#review56960 Cool - this looks really good. I just have a collection of small suggestions for changes. I think I've answered all outstanding questions from the meeting, but let me know if I've overlooked anything. r- for now since I would like to see an interdiff after these changes are made. ::: security/certverifier/CertVerifier.cpp:159 (Diff revision 1) > } > } > if (ocspStaplingStatus) { > *ocspStaplingStatus = trustDomain.GetOCSPStaplingStatus(); > } > + if (rv == Success) { I think we should put this in the certificateUsageSSLServer section of CertVerifier::VerifyCert (which is where we'll eventually surface this to telemetry and later actually make a trust and/or ui decision based on it). ::: security/certverifier/NSSCertDBTrustDomain.h:193 (Diff revision 1) > const char* mHostname; // non-owning - only used for pinning checks > nsCOMPtr<nsICertBlocklist> mCertBlocklist; > CertVerifier::OCSPStaplingStatus mOCSPStaplingStatus; > + // Certificate Transparency data extracted during certificate verification > + SECItem mSCTListFromCertificate; // non-owning > + SECItem mSCTListFromOCSPStapling; // non-owning I would compare these to mBuiltChain. In NSSCertDBTrustDomain::IsChainValid, we (dynamically) construct a CERTCertList from the data passed in and ensure its ownership outlives that of the call to BuildCertChain. For this SCT data, we have to either copy it or be careful that all references to it don't outlive the certDER Input here: https://dxr.mozilla.org/mozilla-central/rev/51377a64158941f89ed73f388ae437cfa494c030/security/certverifier/CertVerifier.cpp#237 It think copying would be the easiest/most clear to express in C++ (given that we don't have lifetimes like in rust). (I think UniqueSECItem and SECITEM_DupItem(UnsafeMapInputToSECItem(...)) should work.) ::: security/certverifier/NSSCertDBTrustDomain.cpp:826 (Diff revision 1) > > mBuiltChain = Move(certList); > > + // If CT extensions are available and chain building succeeds, > + // mSCTListFromCertificate will be set later by NoteAuxiliaryExtension > + // callback. The buffer will point to the same data as mBuiltChain, so the nit: trailing whitespace ::: security/certverifier/NSSCertDBTrustDomain.cpp:989 (Diff revision 1) > + case AuxiliaryExtension::EmbeddedSCTList: > + out = &mSCTListFromCertificate; > + break; > + case AuxiliaryExtension::SCTListFromOCSPResponse: > + out = &mSCTListFromOCSPStapling; > + break; Let's have a default case with MOZ_ASSERT_UNREACHABLE(...) in case we ever add new types. ::: security/certverifier/OCSPVerificationTrustDomain.cpp:114 (Diff revision 1) > } > > +void > +OCSPVerificationTrustDomain::NoteAuxiliaryExtension( > + AuxiliaryExtension /* extension */, Input /* extensionData */) > +{ Doesn't this need to call mCertDBTrustDomain.NoteAuxiliaryExtension to pass the data along? ::: security/pkix/include/pkix/pkixtypes.h:359 (Diff revision 1) > > + // Some certificate or OCSP response extensions do not directly participate > + // in the verification flow, but might still be of interest to the clients > + // (notably Certificate Transparency data, RFC 6962). Such extensions are > + // extracted and passed to this function for further processing. > + virtual void NoteAuxiliaryExtension(AuxiliaryExtension extension, This should probably have a Result return value so TrustDomains can signal catastropic errors or for when tests aren't expecting this to be called, for example. ::: security/pkix/lib/pkixbuild.cpp:252 (Diff revision 1) > + > + if (subject.endEntityOrCA == EndEntityOrCA::MustBeEndEntity) { > + const Input* sctExtension = subject.GetSignedCertificateTimestamps(); > + if (sctExtension) { > + trustDomain.NoteAuxiliaryExtension(AuxiliaryExtension::EmbeddedSCTList, > + *sctExtension); nit: indent this to line up with the A in AuxiliaryExtension::EmbeddedSCTList on the previous line. ::: security/pkix/lib/pkixocsp.cpp:667 (Diff revision 1) > } > > rv = der::OptionalExtensions(input, > - der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1, > + der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1, > - ExtensionNotUnderstood); > + [&context](Reader& extnID, const Input& extnValue, bool critical, > + /*out*/ bool& understood) { nit: indent this line one more space ::: security/pkix/lib/pkixocsp.cpp:668 (Diff revision 1) > > rv = der::OptionalExtensions(input, > - der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1, > + der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1, > - ExtensionNotUnderstood); > + [&context](Reader& extnID, const Input& extnValue, bool critical, > + /*out*/ bool& understood) { > + return RememberSingleExtension(context, extnID, extnValue, critical, Hmmm - the CT spec seems to be ambiguous as to which OCSP response extension the data is supposed to go in - the SingleResponse extensions or the ResponseData extensions. Can it be either? Are we expecting implementations to use one and not the other? ::: security/pkix/lib/pkixocsp.cpp:846 (Diff revision 1) > return Success; > } > > +Result > +RememberSingleExtension(Context& context, Reader& extnID, Input extnValue, > + bool /* critical */, /*out*/ bool& understood) nit: for consistency, no spaces after/before /\* and \*/ ::: security/pkix/lib/pkixocsp.cpp:859 (Diff revision 1) > + static const uint8_t id_ocsp_singleExtensionSctList[] = { > + 0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79, 0x02, 0x04, 0x05 > + }; > + > + Input* out = nullptr; > + if (extnID.MatchRest(id_ocsp_singleExtensionSctList)) { Rather than breaking this function up into two sections (matching and then validation/saving), let's just do it all at once: if (extnID.MatchRest(...)) { if (extnValue.GetLength() == 0) { return Result::ERROR_... } if (context.signedCertificateTimestamps.Init(extnValue) != Success) { return Result::ERROR_... } understood = true; } ::: security/pkix/lib/pkixocsp.cpp:877 (Diff revision 1) > + return Result::ERROR_EXTENSION_VALUE_INVALID; > + } > + understood = true; > + } > + > + return Success; nit: watch out for trailing whitespace ::: security/pkix/test/gtest/pkixbuild_tests.cpp:488 (Diff revision 1) > INSTANTIATE_TEST_CASE_P(pkixbuild_IssuerNameCheck, pkixbuild_IssuerNameCheck, > testing::ValuesIn(ISSUER_NAME_CHECK_PARAMS)); > + > + > +// Records auxiliary extensions for later examination. > +// Note: reuses the "single root" chain builder implemented in Maybe it would be best if ExpiredCertTrustDomain and AuxiliaryExtensionTestTrustDomain subclassed from a TrustDomain that implemented the common functionality, rather than having AuxiliaryExtensionTestTrustDomain extend ExpiredCertTrustDomain. ::: security/pkix/test/gtest/pkixbuild_tests.cpp:511 (Diff revision 1) > + virtual void NoteAuxiliaryExtension(AuxiliaryExtension extension, > + Input extensionData) override > + { > + if (extension == AuxiliaryExtension::EmbeddedSCTList) { > + signedCertificateTimestamps = InputToByteString(extensionData); > + } If the extension isn't of the expected type, this should probably return an error. ::: security/pkix/test/gtest/pkixgtest.h:183 (Diff revision 1) > return NotReached("NetscapeStepUpMatchesServerAuth should not be called", > Result::FATAL_ERROR_LIBRARY_FAILURE); > } > + > + virtual void NoteAuxiliaryExtension(AuxiliaryExtension, Input) override > + { This should fail if called. ::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:50 (Diff revision 1) > } > + > + virtual void NoteAuxiliaryExtension(AuxiliaryExtension extension, > + Input extensionData) override > + { > + if (extension == AuxiliaryExtension::SCTListFromOCSPResponse) { Again, if given unexpected data, this should fail. ::: security/pkix/test/lib/pkixtestutil.h:417 (Diff revision 1) > // form; otherwise responderID will use the > // byKeyHash form. > > std::time_t producedAt; > > - OCSPResponseExtension* extensions; > + OCSPResponseExtension* singleExtensions; Let's add a comment explaining that singleExtensions refers to extensions that will appear in the SingleResponse, whereas responseExtensions refers to extensions that will appear in the ResponseData.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #5) <snip> > Hmmm - the CT spec seems to be ambiguous as to which OCSP response extension > the data is supposed to go in - the SingleResponse extensions or the > ResponseData extensions. Can it be either? Are we expecting implementations > to use one and not the other? I think you should expect implementations to use SingleResponse and not the other. (FWIW, this is clarified in 6962-bis).

(In reply to Rob Stradling from comment #6) > I think you should expect implementations to use SingleResponse and not the > other. (FWIW, this is clarified in 6962-bis). Thanks Rob! The relevant section in 6962-bis: https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-16#section-9.1.1

Comment on attachment 8763064 [details] Bug 1275238 - Certificate Transparency support in mozilla::pkix; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59396/diff/1-2/

https://reviewboard.mozilla.org/r/59396/#review56960 Great! I've removed all the irrelevant code from CertVerifier / NSSCertDBTrustDomain, better diff against the base version. I will fix the issues in these files before we begin reviewing them later. > I think we should put this in the certificateUsageSSLServer section of CertVerifier::VerifyCert (which is where we'll eventually surface this to telemetry and later actually make a trust and/or ui decision based on it). Will do. For now, removing the file from the patch since it is not supposed to be included in the final commit. > I would compare these to mBuiltChain. In NSSCertDBTrustDomain::IsChainValid, we (dynamically) construct a CERTCertList from the data passed in and ensure its ownership outlives that of the call to BuildCertChain. For this SCT data, we have to either copy it or be careful that all references to it don't outlive the certDER Input here: https://dxr.mozilla.org/mozilla-central/rev/51377a64158941f89ed73f388ae437cfa494c030/security/certverifier/CertVerifier.cpp#237 > It think copying would be the easiest/most clear to express in C++ (given that we don't have lifetimes like in rust). (I think UniqueSECItem and SECITEM_DupItem(UnsafeMapInputToSECItem(...)) should work.) Will do, for now removing these changes from the patch. > nit: trailing whitespace Same here - removing the changes from the patch for now. > Let's have a default case with MOZ_ASSERT_UNREACHABLE(...) in case we ever add new types. Removing this change too for now, but I think we should not have MOZ_ASSERT_UNREACHABLE here (unlike other places). Theoretically speaking, I think adding code for extracting a new kind of extension reported during the verification should not break this code. > This should probably have a Result return value so TrustDomains can signal catastropic errors or for when tests aren't expecting this to be called, for example. Currently, NoteAuxiliaryExtension is designed to be purely informational, it is not supposed to affect the validation flow. Therefore it can't fail validation by returning an error. As for handling errors in the data NoteAuxiliaryExtension passes, it can be done similarly to the normal flow - storing a member variable to be checked later, after BuildCertChain / VerifyEncodedOCSPResponse returns. Unit tests can use the ADD_FAILURE macro (I've added it to the tests code). What do you think? > Hmmm - the CT spec seems to be ambiguous as to which OCSP response extension the data is supposed to go in - the SingleResponse extensions or the ResponseData extensions. Can it be either? Are we expecting implementations to use one and not the other? The Chromium CT implementation uses SingleResponse only, probably since SingleResponse references a specific certificate. I am assuming we can use Chromium code to resolve the ambiguity :) (but see also Rob's answer above).

(In reply to Rob Stradling from comment #6) > (In reply to David Keeler [:keeler] (use needinfo?) from comment #5) > <snip> > > Hmmm - the CT spec seems to be ambiguous as to which OCSP response extension > > the data is supposed to go in - the SingleResponse extensions or the > > ResponseData extensions. Can it be either? Are we expecting implementations > > to use one and not the other? > > I think you should expect implementations to use SingleResponse and not the > other. (FWIW, this is clarified in 6962-bis). Ok - great. Thanks!

Comment on attachment 8763064 [details] Bug 1275238 - Certificate Transparency support in mozilla::pkix; https://reviewboard.mozilla.org/r/59396/#review57138 Awesome. Just a few nits and some slight touch-ups to the tests, and I think we're good to go for this patch. ::: security/apps/AppTrustDomain.cpp:382 (Diff revision 2) > matches = false; > return Success; > } > > +void > +AppTrustDomain::NoteAuxiliaryExtension(AuxiliaryExtension /* extension */, nit: I think in this code it's more common to not have spaces after/before /\* and \*/ in these situations ::: security/certverifier/NSSCertDBTrustDomain.cpp:962 (Diff revision 2) > } > return Result::FATAL_ERROR_LIBRARY_FAILURE; > } > > +void > +NSSCertDBTrustDomain::NoteAuxiliaryExtension(AuxiliaryExtension /* extension */, (same here) ::: security/manager/ssl/CSTrustDomain.cpp:219 (Diff revision 2) > matches = false; > return Success; > } > > +void > +CSTrustDomain::NoteAuxiliaryExtension(AuxiliaryExtension /* extension */, (same here) ::: security/pkix/lib/pkixocsp.cpp:664 (Diff revision 2) > } > if (context.time > notAfterPlusSlop) { > context.expired = true; > } > > rv = der::OptionalExtensions(input, nit: put |input| on the next line as well ::: security/pkix/test/gtest/pkixbuild_tests.cpp:311 (Diff revision 2) > private: > ByteString rootDER; > }; > > +// A TrustDomain that explicitly fails if CheckRevocation is called. > +class ExpiredCertTrustDomain : public SingleRootTrustDomain This class should remain final. ::: security/pkix/test/gtest/pkixbuild_tests.cpp:514 (Diff revision 2) > INSTANTIATE_TEST_CASE_P(pkixbuild_IssuerNameCheck, pkixbuild_IssuerNameCheck, > testing::ValuesIn(ISSUER_NAME_CHECK_PARAMS)); > + > + > +// Records the embedded SCT list extension for later examination. > +class EmbeddedSCTListTestTrustDomain : public SingleRootTrustDomain Missed this earlier - this class should be final as well. ::: security/pkix/test/gtest/pkixgtest.h:188 (Diff revision 2) > + { > + ADD_FAILURE(); > + } > }; > > class DefaultCryptoTrustDomain : public EverythingFailsByDefaultTrustDomain This class should implement NoteAuxiliaryExtension with a non-failing implementation. ::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:438 (Diff revision 2) > + CreateEncodedOCSPSuccessfulResponse( > + OCSPResponseContext::good, *endEntityCertID, byKey, > + *rootKeyPair, oneDayBeforeNow, > + oneDayBeforeNow, &oneDayAfterNow, > + sha256WithRSAEncryption(), > + /* certs */ nullptr, nit: /*certs*/

Comment on attachment 8763064 [details] Bug 1275238 - Certificate Transparency support in mozilla::pkix; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59396/diff/2-3/

Comment on attachment 8763064 [details] Bug 1275238 - Certificate Transparency support in mozilla::pkix; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59396/diff/3-4/

I've tried to feed the extension extraction code with some test data from the CT test repository, and found a problem - the SCT list data should be encoded as DER octet string before placing it in the certificate/OCSP extension (see RFC 6962 section 3.3). I've fixed the unit tests to produce valid test data, and added code to pkix to remove the encoding (since DER manipulation is internal to pkix). Take a look?

https://reviewboard.mozilla.org/r/59396/#review57738 ::: security/pkix/lib/pkixbuild.cpp:251 (Diff revision 4) > } > + > + if (subject.endEntityOrCA == EndEntityOrCA::MustBeEndEntity) { > + const Input* sctExtension = subject.GetSignedCertificateTimestamps(); > + if (sctExtension) { > + trustDomain.NoteAuxiliaryExtension(AuxiliaryExtension::EmbeddedSCTList, I think we should actually do the OCTET STRING unwrapping here... ::: security/pkix/lib/pkixocsp.cpp:339 (Diff revision 4) > case CertStatus::Good: > if (expired) { > return Result::ERROR_OCSP_OLD_RESPONSE; > } > + if (context.signedCertificateTimestamps.GetLength()) { > + context.trustDomain.NoteAuxiliaryExtension( ... and here (we could add a small helper function to handle it)

Comment on attachment 8763064 [details] Bug 1275238 - Certificate Transparency support in mozilla::pkix; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59396/diff/4-5/

Uploaded the fixes. Take a look if it makes sense?

https://reviewboard.mozilla.org/r/59396/#review58194 This is good. Just a couple of suggestions for improvements. ::: security/pkix/lib/pkixbuild.cpp:252 (Diff revisions 3 - 5) > > if (subject.endEntityOrCA == EndEntityOrCA::MustBeEndEntity) { > const Input* sctExtension = subject.GetSignedCertificateTimestamps(); > if (sctExtension) { > - trustDomain.NoteAuxiliaryExtension(AuxiliaryExtension::EmbeddedSCTList, > - *sctExtension); > + Input sctList; > + if (ExtractSignedCertificateTimestampListFromExtension(*sctExtension, If ExtractSignedCertificateTimestampListFromExtension fails, we should return an error (I guarantee that implementations will forget the extra OCTET STRING wrapping, and it would be best to forbid this from the start rather than silently ignoring it). ::: security/pkix/lib/pkixcert.cpp:315 (Diff revisions 3 - 5) > +Result > +ExtractSignedCertificateTimestampListFromExtension(Input extnValue, > + Input& sctList) > +{ > + Reader sctListReader(extnValue); > + Input decodedValue; This works, but I think if we use ExpectTagAndGetValueAtEnd, we can simplify this a fair bit. Unfortunately, that function takes a Reader as its output value, but it looks like you can call SkipToEnd and pass it sctList to essentially convert it to what we want. ::: security/pkix/lib/pkixocsp.cpp:340 (Diff revisions 3 - 5) > if (expired) { > return Result::ERROR_OCSP_OLD_RESPONSE; > } > if (context.signedCertificateTimestamps.GetLength()) { > + Input sctList; > + if (ExtractSignedCertificateTimestampListFromExtension( Similarly, this error should be propagated to the caller.

Comment on attachment 8763064 [details] Bug 1275238 - Certificate Transparency support in mozilla::pkix; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59396/diff/5-6/

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/990aca9e4d11 Certificate Transparency support in mozilla::pkix; r=keeler