Implement a content-signature service to verify content-signatures.

There are some loose ends. In particular: 1) I'm not (yet) checking EKUs based on context 2) We seem unsure of whether to cause the call to VerifyContentSignature to fail with NS_ERROR_INVALID_SIGNATURE or just return false. The latter seems right, IMO. 3) I haven't prepended the Content-Signature:\00 to the header yet (I'll fix that when I add tests) 4) Tests are needed 5) I'm unsure what, exactly, I'm meant to do with GetVerificationKey - is this a remnant of the other content signing stuff or am I supposed to do something with it?

Comment on attachment 8730694 [details] [diff] [review] Verify content signatures with a certificate chain Review of attachment 8730694 [details] [diff] [review]: ----------------------------------------------------------------- looking good already. My main complains are about the bits where you're not using Unique types. Now some tests. Can you make the next version include my patches for the signature verification (such that the patch in the bug can be used on its own without local changes) and use mozreview? :) Let me know if you need help with rebasing. ::: security/content-signature/CSSignatureVerifier.cpp @@ +39,5 @@ > bool* _retval) > { > if (NS_FAILED(CreateContext(aData, aCSHeader, aCertChain, aContext))) { > + // Maybe the right thing to do here is to set _retval to false instead of > + // returning a failure condition? yeah, we should probably set _retval=false AND return NS_ERROR_INVALID_SIGNATURE @@ +47,5 @@ > return End(_retval); > } > > +SECStatus > +collect_chain(void *arg, SECItem **certs, int numcerts) is this C? :P @@ +50,5 @@ > +SECStatus > +collect_chain(void *arg, SECItem **certs, int numcerts) > +{ > + CERTDERCerts *collectArgs; > + SECItem *cert; please use only Scoped (or better Unique versions of NSS types) @@ +51,5 @@ > +collect_chain(void *arg, SECItem **certs, int numcerts) > +{ > + CERTDERCerts *collectArgs; > + SECItem *cert; > + SECStatus rv; you want to use MapSECStatus to map the NSS SECStatus to nsresult @@ +56,5 @@ > + > + collectArgs = (CERTDERCerts *)arg; > + > + collectArgs->numcerts = numcerts; > + collectArgs->rawCerts = (SECItem *) PORT_ArenaZAlloc(collectArgs->arena, shouldn't be necessary with UniqueCERTDERCerts @@ +92,5 @@ > } > > + // TODO: Prepend the Content-Signature bit to the start of the header > + > + UniquePLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE)); hm, not sure why we need an arena. The certChain we can fit in a nsAutoCString (or any other of our strings if you prefer). The CERTDERCerts should be a UniqueCERTDERCerts, then we should be all good. @@ +116,5 @@ > + > + // We assume that aCertChain number of PEM certificates. Since NSS provides no > + // way of dealing what that kind of package currently (see bug 1255014), we > + // divide the certificate chain string into different PEM sections and call > + // CERT_DecodeCertPackage on each of these in turn. could you put this in an extra function? then it's easier to replace it when bug 1255014 gets fixed (and it's better to read). @@ +231,5 @@ > + > + CERTCertListNode* node = CERT_LIST_HEAD(certCertList.get()); > + ScopedCERTCertificate signerCert(CERT_DupCertificate(node->cert)); > + > + SECItem* certSecItem = &signerCert.get()->derCert; Unique @@ +276,5 @@ > // We have a raw ecdsa signature r||s so we have to DER-encode it first > // Note that we have to check rawSignatureItem->len % 2 here as > // DSAU_EncodeDerSigWithLen asserts this > if (rawSignatureItem->len == 0 || rawSignatureItem->len % 2 != 0) { > + printf("Signature length is bad: %i\n", rawSignatureItem->len); we should keep something like this. make it a debug output. @@ +281,5 @@ > return NS_ERROR_INVALID_SIGNATURE; > } > if (DSAU_EncodeDerSigWithLen(signatureItem.get(), rawSignatureItem.get(), > rawSignatureItem->len) != SECSuccess) { > + printf("problem encoding the signature.\n"); same here ::: security/content-signature/CSTrustDomain.cpp @@ +43,5 @@ > + if (!policy.IsAnyPolicy()) { > + return Result::FATAL_ERROR_INVALID_ARGS; > + } > + > + SECItem candidateCertDERSECItem = UnsafeMapInputToSECItem(candidateCertDER); hm, not sure if I like this. It should be fine because the result contains only a pointer to the candidateCertDER data but it looks like a lot got go wrong with this. @@ +67,5 @@ > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + > + if (isCertRevoked) { > + // TODO: some logging might be useful yep @@ +86,5 @@ > return Success; > } > > Result > CSTrustDomain::FindIssuer(Input encodedIssuerName, IssuerChecker& checker, this allows certs in our chain to be random, right? that's nice, but not necessary if we require the list to be ordered. But nice to have anyway :) @@ +206,5 @@ > CSTrustDomain::CheckValidityIsAcceptable(Time notBefore, Time notAfter, > EndEntityOrCA endEntityOrCA, > KeyPurposeId keyPurpose) > { > + // TODO: Do we want to enforce the short life of content signers? we maybe should eventually. But I think we can live without for the beginning. @@ +250,5 @@ > +CSPrefsHelper::PreferenceChanged(const char* aPref, void* aClosure) > +{ > + MutexAutoLock lock(gMutex); > + > + if(strcmp(aPref, PREF_CONTENT_SIGNING_ROOT) == 0) { I always prefer strncmp (paranoid I know) @@ +259,5 @@ > + nsAutoCString encodedHash; > + encodedHash.Assign(ToNewCString(prefValue)); > + nsCString decodedHash; > + > + nsresult rv = Base64Decode(encodedHash, decodedHash); you don't use rv anywhere else, so just put the Base64Decode() in the if @@ +261,5 @@ > + nsCString decodedHash; > + > + nsresult rv = Base64Decode(encodedHash, decodedHash); > + if (NS_FAILED(rv)) { > + return; // TODO: Maybe log? yeah, log @@ +265,5 @@ > + return; // TODO: Maybe log? > + } > + // ensure it's 32 bytes > + if (decodedHash.Length() != CERTIFICATE_HASH_LENGTH) { > + return; // TODO: Maybe log? dito

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

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

Keeler, as discussed on IRC, this is feedback? rather than r? - changes I plan on making are: 1) More tests. In particular, malformed data (bad signature lengths, bad root hashes, etc). 2) Removal of the default value for gSignerRoot - the value there was for testing. I guess just initial value of {0} would be fine?

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer https://reviewboard.mozilla.org/r/43313/#review39931 This seems like a good approach in general. ::: security/manager/ssl/tests/unit/pycert.py:526 (Diff revision 1) > return univ.ObjectIdentifier('2.16.840.1.113730.4.1') > if keyPurpose == 'OCSPSigning': > return univ.ObjectIdentifier('1.3.6.1.5.5.7.3.9') > if keyPurpose == 'timeStamping': > return rfc2459.id_kp_timeStamping > + if keyPurpose == 'remoteNewTabSigning': I think instead of relying on some new EKU OIDs that we come up with (and then hacking around the fact that the extension isn't required when doing path building), we should just use CheckCertHostname and say that the new tab data should only come from "new-tab-signer.mozilla.org" and the oneCRL data should only come from "oneCRL-signer.mozilla.org" (use subjectAlternativeName). To prevent those certificates from being used as TLS server certificates, we can stick in an EKU with id-kp-codeSigning or something. ::: security/manager/ssl/tests/unit/test_content_signing.js:18 (Diff revision 1) > +// Is the signature rejected when the supplied certificate chain is in the > +// wrong order. > + > +// First, we need to set up some data > +const contentSigner = Cc["@mozilla.org/security/cssignatureverifier;1"] > + .getService(Ci.nsICSSignatureVerifier); The implementation only keeps per-instance state, right? Seems like this should really be 'createInstance' instead. ::: security/manager/ssl/tests/unit/test_content_signing/README.txt:1 (Diff revision 1) > +The certificates for the content signature tests make use of pycert. The test Can pysign.py just have this documentation? ::: security/manager/ssl/tests/unit/test_content_signing/content_signing_good_int.pem.certspec:1 (Diff revision 1) > +issuer:ca Even though the automatic generation of certificates is disabled for the moment, it would be nice to check in a moz.build file that contains what we would say (see https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_sha1/moz.build )

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6) > I think instead of relying on some new EKU OIDs that we come up with (and > then hacking around the fact that the extension isn't required when doing > path building), we should just use CheckCertHostname and say that the new > tab data should only come from "new-tab-signer.mozilla.org" and the oneCRL > data should only come from "oneCRL-signer.mozilla.org" (use > subjectAlternativeName). To prevent those certificates from being used as > TLS server certificates, we can stick in an EKU with id-kp-codeSigning or > something. > EKUs were originally rbarnes' idea - we've spoken and he's OK with the suggested approach.

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43313/diff/1-2/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/1-2/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/2-3/

Hi Julien, Given that what's discussed in comment 7 (and 6) is what you originally wanted, I have assumed you're OK with the use of SANs instead of EKUs. If you're not, let's discuss that here.

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka https://reviewboard.mozilla.org/r/43315/#review39933 This looks good in general, but I think there are some details that either aren't documented or haven't been worked out entirely. Is an instance of the signature verifier intended to be reusable? If so, what about concurrency issues? Perhaps we should just prevent re-use here. ::: security/certverifier/NSSCertDBTrustDomain.cpp:24 (Diff revision 3) > #include "NSSErrorsService.h" > #include "nsServiceManagerUtils.h" > #include "pk11pub.h" > #include "pkix/pkix.h" > #include "pkix/pkixnss.h" > +#include "pkix/Result.h" I'm surprised this needed to be added? ::: security/certverifier/NSSCertDBTrustDomain.cpp:869 (Diff revision 3) > return VerifyRSAPKCS1SignedDigestNSS(signedDigest, subjectPublicKeyInfo, > mPinArg); > } > > Result > + Unnecessary blank line? ::: security/manager/ssl/CSSignatureVerifier.h:15 (Diff revision 3) > + > +#include "CSTrustDomain.h" > +#include "mozilla/Attributes.h" > +#include "nsICSSignatureVerifier.h" > +#include "ScopedNSSTypes.h" > +#include "nsNSSShutDown.h" nit: I think this sorts earlier ::: security/manager/ssl/CSSignatureVerifier.h:17 (Diff revision 3) > +#include "mozilla/Attributes.h" > +#include "nsICSSignatureVerifier.h" > +#include "ScopedNSSTypes.h" > +#include "nsNSSShutDown.h" > + > +typedef struct CERTCertificateStr CERTCertificate; I'm surprised this typedef is necessary. Just #include "cert.h" where it's needed? ::: security/manager/ssl/CSSignatureVerifier.h:26 (Diff revision 3) > + { 0x45a5fe2f, 0xc350, 0x4b86, \ > + { 0x96, 0x2d, 0x02, 0xd5, 0xaa, 0xaa, 0x95, 0x5a } } > +#define NS_CSSIGNATUREVERIFIER_CONTRACTID \ > + "@mozilla.org/security/cssignatureverifier;1" > + > +class CSSignatureVerifier final : public nsICSSignatureVerifier Why "CSSignatureVerifier"? Doesn't "CS" stand for "ContentSignature"? (making this "ContentSignatureSignatureVerifier"?) It's long, but I think "ContentSignatureVerifier" would be a reasonable name. ::: security/manager/ssl/CSSignatureVerifier.h:65 (Diff revision 3) > + nsresult ParseContentSignatureHeader(const nsACString& aContentSignatureHeader); > + > + // verifier context for incremental verifications > + mozilla::UniqueVFYContext mCx; > + // signature to verify > + nsCString mSignature; It doesn't seem like this actually needs to be kept as state (same with mKey). ::: security/manager/ssl/CSSignatureVerifier.cpp:59 (Diff revision 3) > + > + return End(_retval); > +} > + > +SECStatus > +CollectChain(void *arg, SECItem **certs, int numcerts) nit: style issues (* to the left) ::: security/manager/ssl/CSSignatureVerifier.cpp:89 (Diff revision 3) > + // PEM sections and call CERT_DecodeCertPackage on each of these in turn. > + > + // This code is a fixed version of the PEM parsing in CERT_DecodeCertPackage. > + // Once we have a version of NSS with 1255014 fixed in tree, the contents of > + // this function can be replaced with a single call to CERT_DecodeCertPackage. > + const char *certbegin; CERT_DecodeCertPackage is a bit of a convoluted and indirect API. All it really does is strip out the cert headers/footers and newlines and then base64-decodes the results (which are only available via a callback). We don't even need NSS to do that, so let's just not. I think if we use the gecko string/string tokenizer APIs, this function can be about 10 lines long. ::: security/manager/ssl/CSSignatureVerifier.cpp:208 (Diff revision 3) > + if (NS_FAILED(rv)) { > + return rv; > + } > + > + CERTCertListNode* node = CERT_LIST_HEAD(certCertList.get()); > + ScopedCERTCertificate signerCert(CERT_DupCertificate(node->cert)); Probably don't need a ScopedCERTCertificate here, since the lifetime of the certificate we're interested in is enclosed by the lifetime of the ScopedCERTCertList that owns it. ::: security/manager/ssl/CSSignatureVerifier.cpp:221 (Diff revision 3) > + // Check the signerCert chain is good > + CSTrustDomain trustDomain(certCertList, nullptr); > + Result result = BuildCertChain(trustDomain, certDER, Now(), > + EndEntityOrCA::MustBeEndEntity, > + KeyUsage::noParticularKeyUsageRequired, > + KeyPurposeId::anyExtendedKeyUsage, Should we specify KeyPurposeId::id_kp_codeSigning here? ::: security/manager/ssl/CSSignatureVerifier.cpp:235 (Diff revision 3) > + // Check the SAN > + nsAutoCString hostname; > + > + Input hostnameInput; > + > + switch(aContext) { nit: space after switch ::: security/manager/ssl/CSSignatureVerifier.cpp:244 (Diff revision 3) > + case ONECRL: > + hostname = "oneCRL-signer.mozilla.org"; > + break; > + default: > + CSVerifier_LOG(("ContentVerifier: bad context\n")); > + return NS_ERROR_FAILURE; NS_ERROR_INVALID_ARG? ::: security/manager/ssl/CSSignatureVerifier.cpp:248 (Diff revision 3) > + CSVerifier_LOG(("ContentVerifier: bad context\n")); > + return NS_ERROR_FAILURE; > + } > + > + result = hostnameInput.Init(uint8_t_ptr_cast(hostname.BeginReading()), > + hostname.Length()); nit: indentation ::: security/manager/ssl/CSSignatureVerifier.cpp:323 (Diff revision 3) > + > +/** > + * Add data to the context that shold be verified. > + */ > +NS_IMETHODIMP > +CSSignatureVerifier::Update(const nsACString& aData) Pretty much all of these public functions need the NSS shutdown checks. ::: security/manager/ssl/CSSignatureVerifier.cpp:347 (Diff revision 3) > + > + return NS_OK; > +} > + > +nsresult > +CSSignatureVerifier::ParseContentSignatureHeader( This is very similar to the other content signature parsing code. Can we unify them? Alternatively, I feel like you could just use string.Find since the expected format is so simple in this case. ::: security/manager/ssl/CSSignatureVerifier.cpp:374 (Diff revision 3) > + CSVerifier_LOG(("ContentVerifier: found two ContentSignatures\n")); > + return NS_ERROR_INVALID_SIGNATURE; > + } > + > + CSVerifier_LOG(("ContentVerifier: found a ContentSignature directive\n")); > + contentSignature = NS_ConvertUTF8toUTF16(directive->mValue); contentSignature seems superfluous here if we have mSignature ::: security/manager/ssl/CSTrustDomain.h:72 (Diff revision 3) > + > +private: > + /*out*/ ScopedCERTCertList& mCertChain; > + void* mPinArg; // non-owning! > + ScopedCERTCertificate mTrustedRoot; > + unsigned int mMinRSABits; Is this configurable? Seems like we could just hard-code it. ::: security/manager/ssl/CSTrustDomain.h:74 (Diff revision 3) > + /*out*/ ScopedCERTCertList& mCertChain; > + void* mPinArg; // non-owning! > + ScopedCERTCertificate mTrustedRoot; > + unsigned int mMinRSABits; > + > + static StaticMutex sMutex; Doesn't look like this is actually used? ::: security/manager/ssl/CSTrustDomain.cpp:105 (Diff revision 3) > + } > + > + // if the subject does not match, try the next certificate > + Input subjectDER; > + rv = subjectDER.Init(n->cert->derSubject.data, > + n->cert->derSubject.len); nit: indentation ::: security/manager/ssl/CSTrustDomain.cpp:109 (Diff revision 3) > + rv = subjectDER.Init(n->cert->derSubject.data, > + n->cert->derSubject.len); > + if (rv != Success) { > + continue; // just try the next one > + } > + if (!InputsAreEqual(subjectDER, encodedIssuerName)) { Seems like we should do this even before creating certDER. ::: security/manager/ssl/CSTrustDomain.cpp:147 (Diff revision 3) > +Result > +CSTrustDomain::IsChainValid(const DERArray& certChain, Time time) > +{ > + // Check that we have a chain - and that it's not empty > + ScopedCERTCertList certList; > + SECStatus srv = ConstructCERTCertListFromReversedDERArray(certChain, Seems like you could just check certChain.GetLength() > 0, and in any case mozilla::pkix won't pass a bogus certChain to this callback. ::: security/manager/ssl/CSTrustDomain.cpp:231 (Diff revision 3) > +} > + > +nsresult > +CSPrefsHelper::Init() > +{ > + if (!NS_IsMainThread()) { What's the threading model here? Is the signature verifier supposed to be main-thread-only as well? (If that's the case, it seems like we could do without the CSPrefsHelper entirely as long as each signature verifier is created as-needed, rather than recycled each time. ::: security/manager/ssl/CSTrustDomain.cpp:255 (Diff revision 3) > +void > +CSPrefsHelper::PreferenceChanged(const char* aPref, void* aClosure) > +{ > + MutexAutoLock lock(gMutex); > + > + if (strncmp(aPref, PREF_CONTENT_SIGNING_ROOT, strcmp should be sufficient here ::: security/manager/ssl/nsICSSignatureVerifier.idl:16 (Diff revision 3) > +[scriptable, uuid(45a5fe2f-c350-4b86-962d-02d5aaaa955a)] > +interface nsICSSignatureVerifier : nsISupports > +{ > + > + /** > + * Verification contexts I found the term "context" confusing here. Maybe we could use "source"? Or "payload type"? (Neither of those are great replacements, though, really...) ::: security/manager/ssl/nsICSSignatureVerifier.idl:35 (Diff revision 3) > + * @param aContext The context this verification should be > + * performed in (one of the values defined above). > + * @returns true if the signature matches the data and aCertificateChain is > + * valid within aContext, false if not. > + */ > + boolean verifyContentSignature(in ACString aData, in ACString aSignature, It seems unnecessary to have both a streaming interface and an all-at-once interface. ::: security/manager/ssl/nsNSSCertificate.cpp:17 (Diff revision 3) > #include "ExtendedValidation.h" > #include "mozilla/UniquePtr.h" > #include "mozilla/unused.h" > #include "pkix/pkixnss.h" > #include "pkix/pkixtypes.h" > +#include "pkix/Result.h" The changes to this file are surprising - is this because the added files changed the chunking on unified builds?

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer https://reviewboard.mozilla.org/r/43313/#review40107 Looks good. I have some suggestions for additional tests and whatnot. ::: security/manager/ssl/tests/unit/test_content_signing.js:7 (Diff revision 2) > +/* 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/. */ > + > +// This test checks a number of things: > +// Is a signature accepted when signed correctly with a certificate that We probably don't need to list out each testcase here. ::: security/manager/ssl/tests/unit/test_content_signing.js:18 (Diff revision 2) > +// Is the signature rejected when the supplied certificate chain is in the > +// wrong order. > + > +// First, we need to set up some data > +const contentSigner = Cc["@mozilla.org/security/cssignatureverifier;1"] > + .createInstance(Ci.nsICSSignatureVerifier); nit: commonly the '.' is aligned with the '[' from the previous line Also, looks like this one instance is re-used. Is this what we want? (see review on other patch) ::: security/manager/ssl/tests/unit/test_content_signing.js:22 (Diff revision 2) > +const contentSigner = Cc["@mozilla.org/security/cssignatureverifier;1"] > + .createInstance(Ci.nsICSSignatureVerifier); > + > +const PREF_SIGNATURE_ROOT = "security.content.signature.root"; > + > +const test_data_dir= "test_content_signing/"; It would be nice if these consts were consistent in their naming schemes (upper-case?) ::: security/manager/ssl/tests/unit/test_content_signing.js:48 (Diff revision 2) > + encodedCert += line.trim(); > + } > + } > + > + let value = atob(encodedCert); > + let encodedHash = sha256Hash(value); Any particular reason you're not constructing an nsIX509Cert and using the sha256Fingerprint property? (use constructCertFromFile) ::: security/manager/ssl/tests/unit/test_content_signing.js:51 (Diff revision 2) > + > + let value = atob(encodedCert); > + let encodedHash = sha256Hash(value); > + > + > + Services.prefs.setCharPref(PREF_SIGNATURE_ROOT, nit: looks like this fits on one line ::: security/manager/ssl/tests/unit/test_content_signing.js:74 (Diff revision 2) > + readFile(do_get_file(test_data_dir + 'test.txt.signature')) > + .trim(); > + > +const badSignature = "p384ecdsa = WqRXFQ7tnlVufpg7A-ZavXvWd2Zln0o4woHBy26C2rUWM4GJke4pE8ecHiXoi-7KnZXty6Pe3s4o3yAIyKDP9jUC52Ek1Gq25j_X703nP5rk5gM1qz5Fe-qCWakPPl6L"; > + > +let remoteNewTabChain = loadChain(test_data_dir + "content_signing_good", 'let' at the top level is not encouraged, I believe - maybe wrap all this up in run_test? Also, this is all synchronous, so I'm not sure why add_task is particularly helpful in this case. ::: security/manager/ssl/tests/unit/test_content_signing.js:78 (Diff revision 2) > + > +let remoteNewTabChain = loadChain(test_data_dir + "content_signing_good", > + ["remote_newtab_ee", "int", "root"]); > + > +let oneCRLChain = loadChain(test_data_dir + "content_signing_good", > + ["onecrl_ee", "int", "root"]); nit: indentation ::: security/manager/ssl/tests/unit/test_content_signing.js:155 (Diff revision 2) > + "-----BEGIN CERTIFICATE-----\nBSsPRlYp5+gaFMRIczwUzaioRfteCjr94xyz0g==\n", > + "-----BEGIN CERTIFICATE-----\nBSsPRlYp5+gaFMRIczwUzaioRfteCjr94xyz0g==\n-----END CERTIFICATE-----", > + ]; > + for (let badChain of badChains) { > + try { > + contentSigner.verifyContentSignature(data, signature, badChain, Use throws: https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_sts_fqdn.js#47 ::: security/manager/ssl/tests/unit/test_content_signing.js:171 (Diff revision 2) > + // Ensure the signature verifies at the start of the test > + ok(contentSigner.verifyContentSignature(data, signature, chain1, > + contentSigner.ONECRL)); > + > + // Set the root to be some other hash > + Services.prefs.setCharPref(PREF_SIGNATURE_ROOT, sha256Hash("blah")); Let's test with what could be a valid hash (i.e. it's the right length and everything) ::: security/manager/ssl/tests/unit/test_content_signing/content_signing_good_onecrl_ee.pem.certspec:4 (Diff revision 2) > +issuer:int-CA > +subject:ee-int-CA > +subjectKey:secp384r1 > +extension:extKeyUsage:codeSigning Creating a hierarchy where the EKUs don't match is a little strange. ::: security/manager/ssl/tests/unit/test_content_signing/content_signing_good_remote_newtab_ee.pem.certspec:3 (Diff revision 2) > +issuer:int-CA > +subject:ee-int-CA > +subjectKey:secp384r1 We should probably also have a certificate with the right SAN but a different secp384r1 key (or even a secp256r1 key or rsa or something)

> Given that what's discussed in comment 7 (and 6) is what you originally wanted, I have assumed you're OK with the use of SANs instead of EKUs. If you're not, let's discuss that here. Yes, I'm OK with using SANs instead of EKUs. Are we going to allow wildcards?

https://reviewboard.mozilla.org/r/43313/#review40301 ::: security/manager/ssl/tests/unit/test_content_signing.js:5 (Diff revision 2) > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* 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/. */ > + Just as a heads up, this file will likely fail ESLint checks. I haven't run ESLint against the changes here myself, but by inspection, this file is missing "use strict";, so that's at least one failure.

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43313/diff/2-3/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/3-4/

https://reviewboard.mozilla.org/r/43315/#review39933 > I'm surprised this needed to be added? The addition of new files broke a number of things. I suspect this is something to do with unified build stuff. > It doesn't seem like this actually needs to be kept as state (same with mKey). We need to keep state here because we need to support a streaming interface (see below) > This is very similar to the other content signature parsing code. Can we unify them? Alternatively, I feel like you could just use string.Find since the expected format is so simple in this case. Yes. The plan is to unify the content signature code here (and remove the existing stuff once this has landed - see below). > What's the threading model here? Is the signature verifier supposed to be main-thread-only as well? (If that's the case, it seems like we could do without the CSPrefsHelper entirely as long as each signature verifier is created as-needed, rather than recycled each time. Unfortunately, the signature verifier will be used off the main thread (when used in streaming mode). I'll make changes to prevent concurrent use and re-use of verifier instances. > I found the term "context" confusing here. Maybe we could use "source"? Or "payload type"? (Neither of those are great replacements, though, really...) I'm not a fan of "context" either - or "source", or "payload type" for that matter. I've set it to "source" for now. I remain open to other ideas. > It seems unnecessary to have both a streaming interface and an all-at-once interface. Yes. Either we must support both streaming and all-at-once interfaces to the signature verifier, or we have to implement near-identical signature verification code for content signatures and collection signatures.

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43313/diff/3-4/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/4-5/

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer https://reviewboard.mozilla.org/r/43313/#review41069 lgtm. But I think we should add some more tests for the streaming interface (see comments) ::: security/manager/ssl/tests/unit/test_content_signing.js:6 (Diff revision 4) > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* 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/. */ > + > +// This tests ensure content signatures are working correctly. nit: These ::: security/manager/ssl/tests/unit/test_content_signing.js:35 (Diff revision 4) > +} > + > +function run_test() { > + // set up some data > + const DATA = readFile(do_get_file(TEST_DATA_DIR + 'test.txt')); > + const GOOD_SIGNATURE = "p384ecdsa = " + spaces? see next comment ::: security/manager/ssl/tests/unit/test_content_signing.js:39 (Diff revision 4) > + const DATA = readFile(do_get_file(TEST_DATA_DIR + 'test.txt')); > + const GOOD_SIGNATURE = "p384ecdsa = " + > + readFile(do_get_file(TEST_DATA_DIR + 'test.txt.signature')) > + .trim(); > + > + const BAD_SIGNATURE = "p384ecdsa = WqRXFQ7tnlVufpg7A-ZavXvWd2Zln0o4woHBy26C2rUWM4GJke4pE8ecHiXoi-7KnZXty6Pe3s4o3yAIyKDP9jUC52Ek1Gq25j_X703nP5rk5gM1qz5Fe-qCWakPPl6L"; Is this signature actually bad or is the representation wrong? I don't think there should be spaces in here. ::: security/manager/ssl/tests/unit/test_content_signing.js:71 (Diff revision 4) > + verifier = getSignatureVerifier(); > + ok(!verifier.verifyContentSignature(DATA, BAD_SIGNATURE, chain1, > + verifier.ONECRL), > + "A bad signature should not verify"); > + > + // Check a good signature from cert with good SAN but the wrong signing key what's a wrong signing key? Is the provided signature done with a key that is not the one from the provided chain (which is correct)? ::: security/manager/ssl/tests/unit/test_content_signing.js:146 (Diff revision 4) > + > + // Check the streaming interface works OK when a good chain / data > + // combination is provided > + chain1 = oneCRLChain.join("\n"); > + verifier = getSignatureVerifier(); > + verifier.createContext("", GOOD_SIGNATURE, chain1, verifier.ONECRL); do we have a test that also adds data in createContext? and multiple update calls?

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka https://reviewboard.mozilla.org/r/43315/#review41087 lgtm wtih those small comments below. ::: security/manager/ssl/CSTrustDomain.cpp:75 (Diff revision 5) > + bool isRoot = false; > + nsCOMPtr<nsINSSComponent> component(do_GetService(PSM_COMPONENT_CONTRACTID)); > + if (!component) { > + return Result::FATAL_ERROR_LIBRARY_FAILURE; > + } > + nsrv = component ->IsCertContentSigningRoot(candidateCert.get(), isRoot); * component -> has space * you should probably check nsrv is an error ::: security/manager/ssl/CSTrustDomain.cpp:103 (Diff revision 5) > + } > + > + // if the subject does not match, try the next certificate > + Input subjectDER; > + rv = subjectDER.Init(n->cert->derSubject.data, > + n->cert->derSubject.len); indentation ::: security/manager/ssl/ContentSignatureVerifier.cpp:62 (Diff revision 5) > +IsNewLine(char16_t c){ > + return c == '\n' || c == '\r'; > +} > + > +nsresult > +ReadChainIntoCertList(const nsACString& aCertChain, CERTCertList* aCertList) { Do we actually want to support p7 cert chains here as well? (I don't think we should, just a thought) ::: security/manager/ssl/ContentSignatureVerifier.cpp:83 (Diff revision 5) > + if (token.Equals(footer)) { > + inBlock = false; > + certFound = true; > + // base64 decode data, make certs, append to chain > + nsAutoCString decoded; > + nsresult rv = Base64Decode(blockData, decoded); we use two different b64 decoder in this file. not sure if I like that. ::: security/manager/ssl/ContentSignatureVerifier.cpp:125 (Diff revision 5) > + * It sets signature, certificate chain, and context that shold be used to > + * verify the data. The optional data parameter is added to the data to verify. > + */ > +NS_IMETHODIMP > +ContentSignatureVerifier::CreateContext(const nsACString& aData, > + const nsACString& aCSHeader, indentation

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/5-6/

https://reviewboard.mozilla.org/r/43315/#review41087 > Do we actually want to support p7 cert chains here as well? (I don't think we should, just a thought) I don't think so. > we use two different b64 decoder in this file. not sure if I like that. Well spotted; I don't like it either. I'm going for NSSBase64_DecodeBuffer in each instance.

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43313/diff/4-5/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/6-7/

https://reviewboard.mozilla.org/r/43313/#review41069 > spaces? see next comment I don't think it matters. Removed anyway. > what's a wrong signing key? Is the provided signature done with a key that is not the one from the provided chain (which is correct)? Yeah. I made the comment clearer (I hope).

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer https://reviewboard.mozilla.org/r/43313/#review41221 r+ with that additional test (I don't think anything could go wrong here but more test coverage is always better :) ::: security/manager/ssl/tests/unit/test_content_signing.js:154 (Diff revision 5) > + ok(verifier.end(), > + "A good signature should verify using the stream interface"); > + > + // Check that the streaming interface works with multiple update calls > + verifier = getSignatureVerifier(); > + verifier.createContext("", GOOD_SIGNATURE, chain1, verifier.ONECRL); another test with verifier.createContext(DATA1, ...) and then verifier.update(DATA2) would be nice as well. Or did I miss that one?

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka https://reviewboard.mozilla.org/r/43315/#review41349 This is looking good. I'd like to look at the interdiff after addressing these comments, though. ::: security/manager/ssl/CSTrustDomain.h:7 (Diff revision 7) > +/* vim: set ts=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_psm_CSTrustDomain_h nit: I think since this file is included as '#include "CSTrustDomain.h"', the guard is just CSTrustDomain_h. ::: security/manager/ssl/CSTrustDomain.h:25 (Diff revision 7) > +class CSTrustDomain final : public mozilla::pkix::TrustDomain > +{ > +public: > + typedef mozilla::pkix::Result Result; > + > + CSTrustDomain(ScopedCERTCertList& certChain, void* pinArg); Why even have a pinArg argument if it's always null when CSTrustDomain is instantiated? ::: security/manager/ssl/CSTrustDomain.h:71 (Diff revision 7) > + size_t digestBufLen) override; > + > +private: > + /*out*/ ScopedCERTCertList& mCertChain; > + void* mPinArg; // non-owning! > + unsigned int mMinRSABits; I'm not sure this needs to be a member variable - we can just hard-code it in CheckRSAPublicKeyModulusSizeInBits. ::: security/manager/ssl/CSTrustDomain.cpp:146 (Diff revision 7) > + > +Result > +CSTrustDomain::IsChainValid(const DERArray& certChain, Time time) > +{ > + // Check that our chain is not empty > + if (certChain.GetLength() > 0) { nit: I would probably phrase this the other way (i.e. if the chain is empty, return the error), but no big deal ::: security/manager/ssl/CSTrustDomain.cpp:158 (Diff revision 7) > +Result > +CSTrustDomain::CheckSignatureDigestAlgorithm(DigestAlgorithm digestAlg, > + EndEntityOrCA endEntityOrCA, > + Time notBefore) > +{ > + return Success; Should we restrict this to SHA-2? ::: security/manager/ssl/ContentSignatureVerifier.h:8 (Diff revision 7) > +/* 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_psm_ContentSignatureVerifier_h nit: same idea with the guard here ::: security/manager/ssl/ContentSignatureVerifier.h:32 (Diff revision 7) > +{ > +public: > + NS_DECL_ISUPPORTS > + NS_DECL_NSICONTENTSIGNATUREVERIFIER > + > + ContentSignatureVerifier() : mCx(nullptr), mMutex("CSVerifier::mMutex") nit: initialize these one per line ::: security/manager/ssl/ContentSignatureVerifier.h:43 (Diff revision 7) > + { > + destructorSafeDestroyNSSReference(); > + } > + > +private: > + ~ContentSignatureVerifier() This should probably be in the implementation file. ::: security/manager/ssl/ContentSignatureVerifier.h:57 (Diff revision 7) > + > + nsresult UpdateInternal(const nsACString& aData, MutexAutoLock& /*proofOfLock*/); > + > + void destructorSafeDestroyNSSReference() > + { > + mCx = nullptr; Need to release mKey here too, right? ::: security/manager/ssl/ContentSignatureVerifier.cpp:37 (Diff revision 7) > +// Content-Signature prefix > +const nsLiteralCString kPREFIX = NS_LITERAL_CSTRING("Content-Signature:\x00"); > + > +nsresult > +ContentSignatureVerifier::VerifyContentSignature(const nsACString& aData, > + const nsACString& aCSHeader, nit: indentation You might have to format this like so: ContentSignatureVerifier::VerifyContentSignature( const nsACString& aData, const nsACString& aCSHeader, const nsACString& aCertChain, const uint32_t aSource, bool* _retval) { ::: security/manager/ssl/ContentSignatureVerifier.cpp:61 (Diff revision 7) > +IsNewLine(char16_t c){ > + return c == '\n' || c == '\r'; > +} > + > +nsresult > +ReadChainIntoCertList(const nsACString& aCertChain, CERTCertList* aCertList) { It would be best if this function took a proof of nss shutdown prevention lock. ::: security/manager/ssl/ContentSignatureVerifier.cpp:71 (Diff revision 7) > + const nsCString footer = NS_LITERAL_CSTRING("-----END CERTIFICATE-----"); > + > + nsCWhitespaceTokenizerTemplate<IsNewLine> tokenizer(aCertChain); > + > + nsAutoCString blockData; > + while (tokenizer.hasMoreTokens()){ nit: space before { ::: security/manager/ssl/ContentSignatureVerifier.cpp:91 (Diff revision 7) > + return NS_ERROR_FAILURE; > + } > + CERTCertificate* tmpCert = > + CERT_NewTempCertificate(CERT_GetDefaultCertDB(), der.get(), nullptr, > + false, true); > + SECStatus res = CERT_AddCertToListTail(aCertList, tmpCert); First check that CERT_NewTempCertificate succeeded (i.e. that tmpCert is non-null). ::: security/manager/ssl/ContentSignatureVerifier.cpp:92 (Diff revision 7) > + } > + CERTCertificate* tmpCert = > + CERT_NewTempCertificate(CERT_GetDefaultCertDB(), der.get(), nullptr, > + false, true); > + SECStatus res = CERT_AddCertToListTail(aCertList, tmpCert); > + if (res != SECSuccess) { If CERT_AddCertToListTail fails, this will leak a reference to tmpCert. ::: security/manager/ssl/ContentSignatureVerifier.cpp:95 (Diff revision 7) > + false, true); > + SECStatus res = CERT_AddCertToListTail(aCertList, tmpCert); > + if (res != SECSuccess) { > + return MapSECStatus(res); > + } > + continue; Maybe instead of using continue we could just do } else { blockData.Append(token); }. Not a big deal, though. ::: security/manager/ssl/ContentSignatureVerifier.cpp:99 (Diff revision 7) > + } > + continue; > + } > + blockData.Append(token); > + } else { > + if (token.Equals(header)) { nit: this can be an else if ::: security/manager/ssl/ContentSignatureVerifier.cpp:102 (Diff revision 7) > + blockData.Append(token); > + } else { > + if (token.Equals(header)) { > + inBlock = true; > + blockData = ""; > + continue; Not sure this continue is necessary here. ::: security/manager/ssl/ContentSignatureVerifier.cpp:114 (Diff revision 7) > + return NS_ERROR_FAILURE; > + } > + return NS_OK; > +} > + > +/** nit: let's stick to //-style comments ::: security/manager/ssl/ContentSignatureVerifier.cpp:155 (Diff revision 7) > + } > + > + SECItem* certSecItem = &node->cert->derCert; > + > + Input certDER; > + certDER.Init(reinterpret_cast<const uint8_t*>(certSecItem->data), Check the return value from Init ::: security/manager/ssl/ContentSignatureVerifier.cpp:156 (Diff revision 7) > + > + SECItem* certSecItem = &node->cert->derCert; > + > + Input certDER; > + certDER.Init(reinterpret_cast<const uint8_t*>(certSecItem->data), > + certSecItem->len); nit: indentation ::: security/manager/ssl/ContentSignatureVerifier.cpp:225 (Diff revision 7) > + CSVerifier_LOG(("CSVerifier: decoding the signature failed\n")); > + return NS_ERROR_FAILURE; > + } > + > + // get signature object > + UniqueSECItem signatureItem(::SECITEM_AllocItem(nullptr, nullptr, 0)); null-check signatureItem ::: security/manager/ssl/ContentSignatureVerifier.cpp:243 (Diff revision 7) > + > + // this is the only OID we support for now > + SECOidTag oid = SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE; > + > + mCx = UniqueVFYContext( > + VFY_CreateContext(mKey.get(), signatureItem.get(), oid, NULL)); nit: s/NULL/nullptr ::: security/manager/ssl/ContentSignatureVerifier.cpp:253 (Diff revision 7) > + > + if (VFY_Begin(mCx.get()) != SECSuccess) { > + return NS_ERROR_INVALID_SIGNATURE; > + } > + > + // add data if we got any nit: seems like this comment should be a few lines later ::: security/manager/ssl/ContentSignatureVerifier.cpp:264 (Diff revision 7) > +} > + > +nsresult > +ContentSignatureVerifier::UpdateInternal(const nsACString& aData, > + MutexAutoLock& /*proofOfLock*/) { > + nsNSSShutDownPreventionLock locker; If you also pass in a proof of nss shut down prevention lock, checking for shutdown again wouldn't be necessary. ::: security/manager/ssl/ContentSignatureVerifier.cpp:335 (Diff revision 7) > + return NS_ERROR_INVALID_SIGNATURE; > + } > + > + CSVerifier_LOG(("CSVerifier: found a ContentSignature directive\n")); > + contentSignature = NS_ConvertUTF8toUTF16(directive->mValue); > + mSignature = directive->mValue; Seems like mSignature could serve the same purpose as contentSignature here. ::: security/manager/ssl/nsIContentSignatureVerifier.idl:9 (Diff revision 7) > + > + > +#include "nsISupports.idl" > + > +/** > + * An interface for verifying content-signatures Maybe link to rfc? Also, let's document that this should be used with do_CreateInstance, not do_GetService, etc. ::: security/manager/ssl/nsIContentSignatureVerifier.idl:16 (Diff revision 7) > +[scriptable, uuid(45a5fe2f-c350-4b86-962d-02d5aaaa955a)] > +interface nsIContentSignatureVerifier : nsISupports > +{ > + > + /** > + * Verification sources Let's document what these mean - e.g. that if something comes from ABOUT_NEWTAB, it must be valid for the hostname "remote-newtab-signer.mozilla.org". ::: security/manager/ssl/nsNSSComponent.cpp:1792 (Diff revision 7) > MutexAutoLock lock(mutex); > mTestBuiltInRootHash = Preferences::GetString("security.test.built_in_root_hash"); > #endif // DEBUG > } else if (prefName.Equals(kFamilySafetyModePref)) { > MaybeEnableFamilySafetyCompatibility(); > + } else if (prefName.EqualsLiteral("security.content.signature.root")) { nit: maybe have "hash" in the pref name? ::: security/manager/ssl/nsNSSModule.cpp:211 (Diff revision 7) > NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsureChromeOrContent, nsCryptoHash) > NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsureChromeOrContent, nsCryptoHMAC) > NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsureChromeOrContent, nsKeyObject) > NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsureChromeOrContent, nsKeyObjectFactory) > NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsDataSignatureVerifier) > +NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, ContentSignatureVerifier) To confirm, this is only expected to be used in the parent process, right?

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer https://reviewboard.mozilla.org/r/43313/#review41439 This is good. I have some suggestions for more tests. (That's the main, thing, really - more test-cases.) ::: security/manager/ssl/tests/unit/test_content_signing.js:11 (Diff revision 5) > +// These tests ensure content signatures are working correctly. > + > +// First, we need to set up some data > +const PREF_SIGNATURE_ROOT = "security.content.signature.root"; > + > +const TEST_DATA_DIR= "test_content_signing/"; nit: space before = ::: security/manager/ssl/tests/unit/test_content_signing.js:39 (Diff revision 5) > + const DATA = readFile(do_get_file(TEST_DATA_DIR + 'test.txt')); > + const GOOD_SIGNATURE = "p384ecdsa=" + > + readFile(do_get_file(TEST_DATA_DIR + 'test.txt.signature')) > + .trim(); > + > + const BAD_SIGNATURE = "p384ecdsa=WqRXFQ7tnlVufpg7A-ZavXvWd2Zln0o4woHBy26C2rUWM4GJke4pE8ecHiXoi-7KnZXty6Pe3s4o3yAIyKDP9jUC52Ek1Gq25j_X703nP5rk5gM1qz5Fe-qCWakPPl6L"; Can we wrap this line? ::: security/manager/ssl/tests/unit/test_content_signing.js:89 (Diff revision 5) > + "A signature should not verify if the chain is incomplete (missing root)"); > + > + // Check a good signature from cert with good SAN but with no path to root > + let missingInt = [oneCRLChain[0], oneCRLChain[2]].join("\n"); > + verifier = getSignatureVerifier(); > + ok(!verifier.verifyContentSignature(DATA, BAD_SIGNATURE, missingInt, This should be GOOD_SIGNATURE instead of BAD_SIGNATURE, right? ::: security/manager/ssl/tests/unit/test_content_signing.js:108 (Diff revision 5) > + verifier.ONECRL), > + "A newtab signature should not verify if the signer has the OneCRL SAN"); > + > + // Check malformed signature data > + chain1 = oneCRLChain.join("\n"); > + let bad_signatures= [ Maybe we should also test that an RSA signature is rejected (in fact, we should definitely test the case where the end-entity has an RSA key). ::: security/manager/ssl/tests/unit/test_content_signing.js:133 (Diff revision 5) > + "", > + // completely wrong data > + "blah blah \n blah", > + // data that looks like PEM but isn't > + "-----BEGIN CERTIFICATE-----\nBSsPRlYp5+gaFMRIczwUzaioRfteCjr94xyz0g==\n", > + "-----BEGIN CERTIFICATE-----\nBSsPRlYp5+gaFMRIczwUzaioRfteCjr94xyz0g==\n-----END CERTIFICATE-----", Maybe we should have more invalid chain testcases: "-----BEGIN CERTIFICATE----- ... some stuff... -----BEGIN CERTIFICATE-----" "-----END CERTIFICATE----- .... whatever... -----BEGIN CERTIFICATE-----" "-----BEGIN CERTIFICATE-----" non-base64-stuff -----END CERTIFICATE-----" and so on

(In reply to David Keeler [:keeler] (use needinfo?) from comment #29) > > +NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, ContentSignatureVerifier) > > To confirm, this is only expected to be used in the parent process, right? I think so. Franziskus can confirm.

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43313/diff/5-6/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/7-8/

https://reviewboard.mozilla.org/r/43315/#review41543 Forgive the drive by review... ::: security/manager/ssl/ContentSignatureVerifier.h:13 (Diff revision 8) > +#ifndef ContentSignatureVerifier_h > +#define ContentSignatureVerifier_h > + > +#include "cert.h" > +#include "CSTrustDomain.h" > +#include "mozilla/Attributes.h" Nit: Looks like nothing from this header is used? ::: security/manager/ssl/ContentSignatureVerifier.cpp:64 (Diff revision 8) > + > + return End(_retval); > +} > + > +bool > +IsNewLine(char16_t c){ Nit: { on next line ::: security/manager/ssl/ContentSignatureVerifier.cpp:70 (Diff revision 8) > + return c == '\n' || c == '\r'; > +} > + > +nsresult > +ReadChainIntoCertList(const nsACString& aCertChain, CERTCertList* aCertList, > + const nsNSSShutDownPreventionLock& /*proofOfLock*/) { Nit: { on next line ::: security/manager/ssl/ContentSignatureVerifier.cpp:309 (Diff revision 8) > + > +/** > + * Finish signature verification and return the result in _retval. > + */ > +NS_IMETHODIMP > +ContentSignatureVerifier::End(bool* _retval) Should probably add a |NS_ENSURE_ARG_POINTER(_retval)| here since this method is basically public. ::: security/manager/ssl/nsNSSComponent.h:191 (Diff revision 8) > #endif > > #ifdef DEBUG > nsAutoString mTestBuiltInRootHash; > #endif > + nsAutoString mContentSigningRootHash; Nit: Probably shouldn't use nsAutoString as a member - see Bug 1225682.

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka https://reviewboard.mozilla.org/r/43315/#review41559 Awesome. Just some nits. ::: security/manager/ssl/ContentSignatureVerifier.h:33 (Diff revisions 7 - 8) > public: > NS_DECL_ISUPPORTS > NS_DECL_NSICONTENTSIGNATUREVERIFIER > > - ContentSignatureVerifier() : mCx(nullptr), mMutex("CSVerifier::mMutex") > + ContentSignatureVerifier() : mCx(nullptr), > + mMutex("CSVerifier::mMutex") nit: I think the recommended style for this is: ContentSignatureVerifier() : mCx(nullptr) , mMutex("CSVerifier::mMutex") { } ::: security/manager/ssl/ContentSignatureVerifier.cpp:70 (Diff revisions 7 - 8) > return c == '\n' || c == '\r'; > } > > nsresult > -ReadChainIntoCertList(const nsACString& aCertChain, CERTCertList* aCertList) { > +ReadChainIntoCertList(const nsACString& aCertChain, CERTCertList* aCertList, > + const nsNSSShutDownPreventionLock& /*proofOfLock*/) { nit: { on the next line ::: security/manager/ssl/ContentSignatureVerifier.cpp:100 (Diff revisions 7 - 8) > return NS_ERROR_FAILURE; > } > CERTCertificate* tmpCert = > CERT_NewTempCertificate(CERT_GetDefaultCertDB(), der.get(), nullptr, > false, true); > + if (!tmpCert ){ nit: if (!tmpCert) { ::: security/manager/ssl/ContentSignatureVerifier.cpp:107 (Diff revisions 7 - 8) > + } > SECStatus res = CERT_AddCertToListTail(aCertList, tmpCert); > if (res != SECSuccess) { > + CERT_DestroyCertificate(tmpCert); > return MapSECStatus(res); > } Might be nice to add a comment that aCertList now owns tmpCert.

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43313/diff/6-7/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/8-9/

(In reply to Mark Goodwin [:mgoodwin] from comment #31) > (In reply to David Keeler [:keeler] (use needinfo?) from comment #29) > > > +NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, ContentSignatureVerifier) > > > > To confirm, this is only expected to be used in the parent process, right? > > I think so. Franziskus can confirm. We might have to be able to call this from a child process (at least in the newtab case). But as discussed with Mark we can do this in a follow up when using this new signature verifier.

Comment on attachment 8736474 [details] MozReview Request: Bug 1252882 - Content-Signature Service - some tests r?keeler,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43313/diff/7-8/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/9-10/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43315/diff/10-11/

Comment on attachment 8736475 [details] MozReview Request: Bug 1252882 - Add a Content Signature Service r?keeler,r?franziskus,r?Cykesiopka https://reviewboard.mozilla.org/r/43315/#review42559 r+ with regards to the header include changes.

https://reviewboard.mozilla.org/r/43315/#review42573 lgtm

https://hg.mozilla.org/integration/mozilla-inbound/rev/8206ecdf17e8c08cf6127d1b2bcc572c83f40bbe Bug 1252882 - Content-Signature Service - some tests r=keeler,r=fkiefer https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6b876450fb64170ba9d4b287351401c0b06c4a Bug 1252882 - Add a Content Signature Service r=keeler,r=franziskus,r=Cykesiopka

https://hg.mozilla.org/mozilla-central/rev/8206ecdf17e8 https://hg.mozilla.org/mozilla-central/rev/cb6b876450fb