Closed
Bug 1265085
Opened 8 years ago
Closed 8 years ago
Replace verification source with a SAN in the content signature verifier interface
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mgoodwin, Assigned: mgoodwin)
References
Details
Attachments
(1 file)
Change the ContentSignatureVerifier interface such that a required SAN could be passed in instead of the verification source. Rationale; it enables us to add new verification sources (e.g. for browser features we ship as addons) without having to wait for a new verification source to ride the trains.
Assignee | ||
Comment 1•8 years ago
|
||
This change replaces the hardcoded 'sourceis' in nsIContentSignatureVerifier and ContentSignatureVerifier.cpp with a string parameter which allows the caller to specify which hostname the signing certificate must be valid for. This allows us to create and use new signing certificates without having to wait for new sources to ride the trains. Review commit: https://reviewboard.mozilla.org/r/47111/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47111/
Attachment #8742295 -
Flags: review?(franziskuskiefer)
Attachment #8742295 -
Flags: review?(cykesiopka.bmo)
Comment 2•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer https://reviewboard.mozilla.org/r/47111/#review43611 lgtm. But I'd like to see some additional tests now that check for invalid (randome) SANs.
Attachment #8742295 -
Flags: review?(franziskuskiefer)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/1-2/
Attachment #8742295 -
Flags: review?(franziskuskiefer)
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/47111/#review43625 ::: security/manager/ssl/ContentSignatureVerifier.cpp:128 (Diff revision 2) > } > return NS_OK; > } > > // Create a context for a content signature verification. > -// It sets signature, certificate chain, and context that shold be used to > +// It sets signature, certificate chain and name that shold be used to verify s/shold/should ::: security/manager/ssl/ContentSignatureVerifier.cpp (Diff revision 2) > // the chain is bad > CSVerifier_LOG(("CSVerifier: The supplied chain is bad\n")); > return NS_ERROR_INVALID_SIGNATURE; > } > > - // Check the SAN The comment should stay
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/2-3/
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/3-4/
Comment 7•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer https://reviewboard.mozilla.org/r/47111/#review43807 lgtm
Attachment #8742295 -
Flags: review?(franziskuskiefer) → review+
https://reviewboard.mozilla.org/r/47111/#review43815 ::: security/manager/ssl/ContentSignatureVerifier.cpp:199 (Diff revision 4) > if (result != Success) { > return NS_ERROR_FAILURE; > } > > + // Check we have the EKU extension - since a missing EKU extension prevents > + // CheckCertHostname from matching the required EKU BuildCertChain is what processes EKUs - CheckCertHostname doesn't. In any case, this change seems unrelated to this bug.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #8) > > + // Check we have the EKU extension - since a missing EKU extension prevents > > + // CheckCertHostname from matching the required EKU > > BuildCertChain is what processes EKUs - CheckCertHostname doesn't. In any > case, this change seems unrelated to this bug. Good catch; that's a typo (I meant BuildCertChain). Yes. Franziskus identified some more missing tests which identified a problem in the case of missing EKU extensions. Would you prefer I file a new bug and move this (and the corresponding test change) to it?
Flags: needinfo?(dkeeler)
Yes, I think that would be best.
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/4-5/
Comment 12•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer https://reviewboard.mozilla.org/r/47111/#review44001 Mostly looks good! ::: security/manager/ssl/ContentSignatureVerifier.cpp:45 (Diff revision 5) > } > destructorSafeDestroyNSSReference(); > shutdown(calledFromObject); > } > > nsresult Nit: Looks like |nsresult| here should be |NS_IMETHODIMP| instead. ::: security/manager/ssl/ContentSignatureVerifier.cpp:49 (Diff revision 5) > > nsresult > ContentSignatureVerifier::VerifyContentSignature( > const nsACString& aData, const nsACString& aCSHeader, > - const nsACString& aCertChain, const uint32_t aSource, bool* _retval) > + const nsACString& aCertChain, const nsACString& aName, bool* _retval) > { There should be a check like |NS_ENSURE_ARG_POINTER(_reval)| just after this line. ::: security/manager/ssl/ContentSignatureVerifier.cpp:129 (Diff revision 5) > -// verify the data. The optional data parameter is added to the data to verify. > +// the data. The optional data parameter is the first part of the data to > +// verify. Optional: "optional data parameter" leads me to think that a caller can omit the parameter entirely, which isn't true. Maybe the comment could mention that |data| is optional in the sense that it can be the empty string? Maybe it's just me though, up to you. ::: security/manager/ssl/nsIContentSignatureVerifier.idl:49 (Diff revision 5) > * @param aData The first chunk of data to be tested. > * This parameter is optional. Same "optional data parameter" comment I made previously. ::: security/manager/ssl/tests/unit/test_content_signing.js:64 (Diff revision 5) > + let wrongEKUChain = loadChain(TEST_DATA_DIR + "content_signing", > + ["onecrl_wrong_EKU_ee", "int", "root"]); Looks like this should be moved to the patch for Bug 1265499. ::: security/manager/ssl/tests/unit/test_content_signing.js:119 (Diff revision 5) > - // Check good signatures from good certificates with incorrect SANs > + // Check good signatures from good certificates with the wrong SANs > chain1 = oneCRLChain.join("\n"); > verifier = getSignatureVerifier(); > ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, > - verifier.ABOUT_NEWTAB), > - "A OneCRL signature should not verify if the signer has the newtab SAN"); > + ABOUT_NEWTAB_NAME), > + "A signature should not verify if the SANs do not match a supplied name"); Nit: This should be probably be reverted since ideally subtests should have unique messages to allow subtests to be differentiated in the logs. ::: security/manager/ssl/tests/unit/test_content_signing.js:125 (Diff revision 5) > > chain2 = remoteNewTabChain.join("\n"); > verifier = getSignatureVerifier(); > ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain2, > - verifier.ONECRL), > - "A newtab signature should not verify if the signer has the OneCRL SAN"); > + ONECRL_NAME), > + "A signature should not verify if the SANs do not match a supplied name"); Nit: This should be probably be reverted since ideally subtests should have unique messages to allow subtests to be differentiated in the logs. ::: security/manager/ssl/tests/unit/test_content_signing.js:129 (Diff revision 5) > + ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, ""), > + "A signature should not verify if the SANs do not match an empty name"); Optional, future clean up: This test file would probably be easier to read if fixed constants for e.g. |oneCRLChain.join("\n")| were used instead of |chain1| and |chain2|. I'm forgetful, so I find myself having to constantly look up what |chain1| and |chain2| mean in the local context. ::: security/manager/ssl/tests/unit/test_content_signing.js:151 (Diff revision 5) > + // check good signature from a chain with the wrong EKU on the end entity > + chain1 = wrongEKUChain.join("\n"); > + verifier = getSignatureVerifier(); > + ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, > + ONECRL_NAME), > + "A signature should not verify if the signer has the wrong EKU"); Looks like this should be moved to the patch for Bug 1265499. ::: security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_no_SAN_ee.pem.certspec:3 (Diff revision 5) > +issuer:int-CA > +subject:ee-int-CA > +subjectKey:secp384r1 This should have |extension:extKeyUsage:codeSigning| as well right? ::: security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_wrong_EKU_ee.pem:1 (Diff revision 5) > +-----BEGIN CERTIFICATE----- Looks like this should be moved to the patch for Bug 1265499. ::: security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_wrong_EKU_ee.pem.certspec:1 (Diff revision 5) > +issuer:int-CA Looks like this should be moved to the patch for Bug 1265499. ::: security/manager/ssl/tests/unit/test_content_signing/moz.build:13 (Diff revision 5) > #test_certificates = ( > # 'content_signing_root.pem', > # 'content_signing_int.pem', > # 'content_signing_onecrl_ee.pem', > +# 'content_signing_onecrl_no_SAN_ee.pem', > +# 'content_signing_onecrl_wrong_EKU_ee.pem', Looks like this should be moved to the patch for Bug 1265499.
Attachment #8742295 -
Flags: review?(cykesiopka.bmo) → review+
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5067fcc151a
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8742295 [details] MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/5-6/
Assignee | ||
Comment 15•8 years ago
|
||
https://reviewboard.mozilla.org/r/47111/#review44001 > Nit: This should be probably be reverted since ideally subtests should have unique messages to allow subtests to be differentiated in the logs. I've changed this to another message that's unique and similar to the old. I changed these because the old message doesn't actually communicate what's being tested - it's a signature with a chain with OneCRL in the EE SAN but we expect verification to fail because we're requiring a newtab SAN.
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d72b110307dc616e5d16cba45d2702e738c2ab4 Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r=Cykesiopka,r=fkiefer
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5d72b110307d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•