Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer draft
authorMark Goodwin <mgoodwin@mozilla.com>
Mon, 18 Apr 2016 14:55:56 +0100
changeset 354216 b5067fcc151a4cd322e46ebe2cc799abaece9095
parent 353712 ae7413abfa4d3954a6a4ce7c1613a7100f367f9a
child 518936 c0ef3e6745f346a7fa1bb2e2b3847f973918a277
push id16003
push usermgoodwin@mozilla.com
push dateWed, 20 Apr 2016 11:10:59 +0000
reviewersCykesiopka, fkiefer
bugs1265085
milestone48.0a1
Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer 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. MozReview-Commit-ID: KGpOVOuJrk3
security/manager/ssl/ContentSignatureVerifier.cpp
security/manager/ssl/nsIContentSignatureVerifier.idl
security/manager/ssl/tests/unit/test_content_signing.js
security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_no_SAN_ee.pem
security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_no_SAN_ee.pem.certspec
security/manager/ssl/tests/unit/test_content_signing/moz.build
--- a/security/manager/ssl/ContentSignatureVerifier.cpp
+++ b/security/manager/ssl/ContentSignatureVerifier.cpp
@@ -37,22 +37,23 @@ ContentSignatureVerifier::~ContentSignat
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     return;
   }
   destructorSafeDestroyNSSReference();
   shutdown(calledFromObject);
 }
 
-nsresult
+NS_IMETHODIMP
 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)
 {
-  nsresult rv = CreateContext(aData, aCSHeader, aCertChain, aSource);
+  NS_ENSURE_ARG(_retval);
+  nsresult rv = CreateContext(aData, aCSHeader, aCertChain, aName);
   if (NS_FAILED(rv)) {
     *_retval = false;
     CSVerifier_LOG(("CSVerifier: Signature verification failed\n"));
     if (rv == NS_ERROR_INVALID_SIGNATURE) {
       return NS_OK;
     }
     return rv;
   }
@@ -120,23 +121,24 @@ ReadChainIntoCertList(const nsACString& 
     // the PEM data did not end; bad data.
     CSVerifier_LOG(("CSVerifier: supplied chain contains bad data\n"));
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
 // Create a context for a content signature verification.
-// 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.
+// It sets signature, certificate chain and name that should be used to verify
+// the data. The data parameter is the first part of the data to verify (this
+// can be the empty string).
 NS_IMETHODIMP
 ContentSignatureVerifier::CreateContext(const nsACString& aData,
                                         const nsACString& aCSHeader,
                                         const nsACString& aCertChain,
-                                        const uint32_t aSource)
+                                        const nsACString& aName)
 {
   MutexAutoLock lock(mMutex);
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     CSVerifier_LOG(("CSVerifier: nss is already shutdown\n"));
     return NS_ERROR_FAILURE;
   }
 
@@ -181,34 +183,20 @@ ContentSignatureVerifier::CreateContext(
                           nullptr/*stapledOCSPResponse*/);
   if (result != Success) {
     // the chain is bad
     CSVerifier_LOG(("CSVerifier: The supplied chain is bad\n"));
     return NS_ERROR_INVALID_SIGNATURE;
   }
 
   // Check the SAN
-  nsAutoCString hostname;
-
   Input hostnameInput;
 
-  switch (aSource) {
-    case ABOUT_NEWTAB:
-      hostname = "remote-newtab-signer.mozilla.org";
-      break;
-    case ONECRL:
-      hostname = "oneCRL-signer.mozilla.org";
-      break;
-    default:
-      CSVerifier_LOG(("CSVerifier: bad context\n"));
-      return NS_ERROR_INVALID_ARG;
-  }
-
-  result = hostnameInput.Init(uint8_t_ptr_cast(hostname.BeginReading()),
-                              hostname.Length());
+  result = hostnameInput.Init(uint8_t_ptr_cast(aName.BeginReading()),
+                              aName.Length());
   if (result != Success) {
     return NS_ERROR_FAILURE;
   }
 
   BRNameMatchingPolicy nameMatchingPolicy(BRNameMatchingPolicy::Mode::Enforce);
   result = CheckCertHostname(certDER, hostnameInput, nameMatchingPolicy);
   if (result != Success) {
     return NS_ERROR_INVALID_SIGNATURE;
--- a/security/manager/ssl/nsIContentSignatureVerifier.idl
+++ b/security/manager/ssl/nsIContentSignatureVerifier.idl
@@ -20,59 +20,47 @@
  * information (and initial data), call update with more data as it becomes
  * available then, finally, call end to verify the signature.
  */
 [scriptable, uuid(45a5fe2f-c350-4b86-962d-02d5aaaa955a)]
 interface nsIContentSignatureVerifier : nsISupports
 {
 
   /**
-   * Verification sources.
-   * If the verification is from ABOUT_NEWTAB, the content signature can only be
-   * verified with a certificate chain where the end entity is valid for the
-   * hostname "remote-newtab-signer.mozilla.org".
-   * If the verification is from ONECRL, the end entity must be valid for the
-   * hostname "oneCRL-signer.mozilla.org"
-   */
-  const unsigned long ABOUT_NEWTAB = 0;
-  const unsigned long ONECRL = 1;
-
-  /**
    * Verifies that the data matches the data that was used to generate the
    * signature.
    *
    * @param aData                   The data to be tested.
    * @param aContentSignatureHeader The content-signature header,
    *                                url-safe base64 encoded.
    * @param aCertificateChain       The certificate chain to use for verification.
    *                                PEM encoded string.
-   * @param aSource                 The source of this verification (one of the
-   *                                values defined above).
+   * @param aName                   The (host)name for which the end entity must
+                                    be valid.
    * @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,
                                  in ACString aCertificateChain,
-                                 in unsigned long aSource);
+                                 in ACString aName);
 
   /**
    * Creates a context to verify a content signature against data that is added
    * later with update calls.
    *
    * @param aData                   The first chunk of data to be tested.
-   *                                This parameter is optional.
    * @param aContentSignatureHeader The signature of the data, url-safe base64
    *                                encoded.
    * @param aCertificateChain       The certificate chain to use for
    *                                verification. PEM encoded string.
-   * @param aSource                 The source of this verification (one of the
-   *                                values defined above).
+   * @param aName                   The (host)name for which the end entity must
+                                    be valid.
    */
   void createContext(in ACString aData, in ACString aSignature,
-                     in ACString aCertificateChain, in unsigned long aSource);
+                     in ACString aCertificateChain, in ACString aName);
 
   /**
    * Adds data to the context that was used to generate the signature.
    *
    * @param aData        More data to be tested.
    */
   void update(in ACString aData);
 
--- a/security/manager/ssl/tests/unit/test_content_signing.js
+++ b/security/manager/ssl/tests/unit/test_content_signing.js
@@ -6,16 +6,19 @@
 
 // These tests ensure content signatures are working correctly.
 
 // First, we need to set up some data
 const PREF_SIGNATURE_ROOT = "security.content.signature.root_hash";
 
 const TEST_DATA_DIR = "test_content_signing/";
 
+const ONECRL_NAME = "oneCRL-signer.mozilla.org";
+const ABOUT_NEWTAB_NAME = "remote-newtab-signer.mozilla.org";
+
 function getSignatureVerifier() {
   return Cc["@mozilla.org/security/contentsignatureverifier;1"]
            .createInstance(Ci.nsIContentSignatureVerifier);
 }
 
 function setRoot(filename) {
   let cert = constructCertFromFile(filename);
   Services.prefs.setCharPref(PREF_SIGNATURE_ROOT, cert.sha256Fingerprint);
@@ -50,77 +53,102 @@ function run_test() {
                               ["onecrl_ee", "int", "root"]);
 
   let oneCRLBadKeyChain = loadChain(TEST_DATA_DIR + "content_signing",
                                     ["onecrl_wrong_key_ee", "int", "root"]);
 
   let oneCRLRSAKeyChain = loadChain(TEST_DATA_DIR + "content_signing",
                                     ["onecrl_RSA_ee", "int", "root"]);
 
+  let noSANChain = loadChain(TEST_DATA_DIR + "content_signing",
+                             ["onecrl_no_SAN_ee", "int", "root"]);
+
   // Check good signatures from good certificates with the correct SAN
   let chain1 = oneCRLChain.join("\n");
   let verifier = getSignatureVerifier();
-  ok(verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1,
-                                     verifier.ONECRL),
+  ok(verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, ONECRL_NAME),
      "A OneCRL signature should verify with the OneCRL chain");
   let chain2 = remoteNewTabChain.join("\n");
   verifier = getSignatureVerifier();
   ok(verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain2,
-                                     verifier.ABOUT_NEWTAB),
+                                     ABOUT_NEWTAB_NAME),
      "A newtab signature should verify with the newtab chain");
 
   // Check a bad signature when a good chain is provided
   chain1 = oneCRLChain.join("\n");
   verifier = getSignatureVerifier();
-  ok(!verifier.verifyContentSignature(DATA, BAD_SIGNATURE, chain1,
-                                      verifier.ONECRL),
+  ok(!verifier.verifyContentSignature(DATA, BAD_SIGNATURE, chain1, ONECRL_NAME),
      "A bad signature should not verify");
 
   // Check a good signature from cert with good SAN but a different key than the
   // one used to create the signature
   let badKeyChain = oneCRLBadKeyChain.join("\n");
   verifier = getSignatureVerifier();
   ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, badKeyChain,
-                                      verifier.ONECRL),
+                                      ONECRL_NAME),
      "A signature should not verify if the signing key is wrong");
 
   // Check a good signature from cert with good SAN but a different key than the
   // one used to create the signature (this time, an RSA key)
   let rsaKeyChain = oneCRLBadKeyChain.join("\n");
   verifier = getSignatureVerifier();
   ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, rsaKeyChain,
-                                      verifier.ONECRL),
+                                      ONECRL_NAME),
      "A signature should not verify if the signing key is wrong (RSA)");
 
   // Check a good signature from cert with good SAN but with chain missing root
   let missingRoot = [oneCRLChain[0], oneCRLChain[1]].join("\n");
   verifier = getSignatureVerifier();
   ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, missingRoot,
-                                      verifier.ONECRL),
+                                      ONECRL_NAME),
      "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, GOOD_SIGNATURE, missingInt,
-                                      verifier.ONECRL),
+                                      ONECRL_NAME),
      "A signature should not verify if the chain is incomplete (missing int)");
 
-  // 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 OneCRL signature should not verify if we require the newtab SAN");
 
   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 newtab signature should not verify if we require the OneCRL SAN");
+
+  // Check good signatures with good chains with some other invalid names
+  verifier = getSignatureVerifier();
+  ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, ""),
+     "A signature should not verify if the SANs do not match an empty name");
+
+  let relatedName = "subdomain." + ONECRL_NAME;
+  verifier = getSignatureVerifier();
+  ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1,
+                                      relatedName),
+     "A signature should not verify if the SANs do not match a related name");
+
+  let randomName = "\xb1\x9bU\x1c\xae\xaa3\x19H\xdb\xed\xa1\xa1\xe0\x81\xfb" +
+                   "\xb2\x8f\x1cP\xe5\x8b\x9c\xc2s\xd3\x1f\x8e\xbbN";
+  verifier = getSignatureVerifier();
+  ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, randomName),
+     "A signature should not verify if the SANs do not match a random name");
+
+  // check good signatures with chains that have strange or missing SANs
+  chain1 = noSANChain.join("\n");
+  verifier = getSignatureVerifier();
+  ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1,
+                                      ONECRL_NAME),
+     "A signature should not verify if the SANs do not match a supplied name");
 
   // Check malformed signature data
   chain1 = oneCRLChain.join("\n");
   let bad_signatures = [
     // wrong length
     "p384ecdsa=WqRXFQ7tnlVufpg7A-ZavXvWd2Zln0o4woHBy26C2rUWM4GJke4pE8ecHiXoi-" +
     "7KnZXty6Pe3s4o3yAIyKDP9jUC52Ek1Gq25j_X703nP5rk5gM1qz5Fe-qCWakPPl6L==",
     // incorrectly encoded
@@ -134,18 +162,17 @@ function run_test() {
     "6ob3l3gCTXrsMnOXMeht0kPP3wLfVgXbuuO135pQnsv0c-ltRMWLe56Cm4S4Z6E7WWKLPWaj" +
     "jhAcG5dZxjffP9g7tuPP4lTUJztyc4d1z_zQZakEG7R0vN7P5_CaX9MiMzP4R7nC3H4Ba6yi" +
     "yjlGvsZwJ_C5zDQzWWs95czUbMzbDScEZ_7AWnidw91jZn-fUK3xLb6m-Zb_b4GAqZ-vnXIf" +
     "LpLB1Nzal42BQZn7i4rhAldYdcVvy7rOMlsTUb5Zz6vpVW9LCT9lMJ7Sq1xbU-0g=="
     ];
   for (let badSig of bad_signatures) {
     throws(() => {
       verifier = getSignatureVerifier();
-      verifier.verifyContentSignature(DATA, badSig, chain1,
-                                      verifier.ONECRL);
+      verifier.verifyContentSignature(DATA, badSig, chain1, ONECRL_NAME);
     }, /NS_ERROR/, `Bad or malformed signature "${badSig}" should be rejected`);
   }
 
   // Check malformed and missing certificate chain data
   let chainSuffix = [oneCRLChain[1], oneCRLChain[2]].join("\n");
   let badChains = [
     // no data
     "",
@@ -170,66 +197,65 @@ function run_test() {
     // ... and as part of a chain with good certificates
     badChains.push(badSection + '\n' + chainSuffix);
   }
 
   for (let badChain of badChains) {
     throws(() => {
       verifier = getSignatureVerifier();
       verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, badChain,
-                                      verifier.ONECRL);
+                                      ONECRL_NAME);
     }, /NS_ERROR/, `Bad chain data starting "${badChain.substring(0, 80)}" ` +
                    "should be rejected");
   }
 
   // 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);
+  verifier.createContext("", GOOD_SIGNATURE, chain1, ONECRL_NAME);
   verifier.update(DATA);
   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);
+  verifier.createContext("", GOOD_SIGNATURE, chain1, ONECRL_NAME);
   for (let c of DATA) {
     verifier.update(c);
   }
   ok(verifier.end(),
      "A good signature should verify using multiple updates");
 
   // Check that the streaming interface works with multiple update calls and
   // some data provided in CreateContext
   verifier = getSignatureVerifier();
   let start = DATA.substring(0, 5);
   let rest = DATA.substring(start.length);
-  verifier.createContext(start, GOOD_SIGNATURE, chain1, verifier.ONECRL);
+  verifier.createContext(start, GOOD_SIGNATURE, chain1, ONECRL_NAME);
   for (let c of rest) {
     verifier.update(c);
   }
   ok(verifier.end(),
      "A good signature should verify using data in CreateContext and updates");
 
   // Check that a bad chain / data combination fails
   verifier = getSignatureVerifier();
-  verifier.createContext("", GOOD_SIGNATURE, chain1, verifier.ONECRL);
+  verifier.createContext("", GOOD_SIGNATURE, chain1, ONECRL_NAME);
   ok(!verifier.end(),
      "A bad signature should fail using the stream interface");
 
   // Check that re-creating a context throws ...
   verifier = getSignatureVerifier();
-  verifier.createContext("", GOOD_SIGNATURE, chain1, verifier.ONECRL);
+  verifier.createContext("", GOOD_SIGNATURE, chain1, ONECRL_NAME);
 
   // ... firstly, creating a context explicitly
   throws(() => {
-    verifier.createContext(DATA, GOOD_SIGNATURE, chain1, verifier.ONECRL);
+    verifier.createContext(DATA, GOOD_SIGNATURE, chain1, ONECRL_NAME);
   }, /NS_ERROR/, "Ensure a verifier cannot be re-used with createContext");
 
   // ... secondly, by calling verifyContentSignature
   throws(() => {
-    verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1,
-                                    verifier.ONECRL);
+    verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, ONECRL_NAME);
   }, /NS_ERROR/, "Ensure a verifier cannot be re-used with verifyContentSignature");
 
   run_next_test();
 }
new file mode 100644
--- /dev/null
+++ b/security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_no_SAN_ee.pem
@@ -0,0 +1,14 @@
+-----BEGIN CERTIFICATE-----
+MIICHDCCAQagAwIBAgIUTeyXrUzlo8XY7YXkbLMw/fzVANQwCwYJKoZIhvcNAQEL
+MBExDzANBgNVBAMMBmludC1DQTAiGA8yMDE0MTEyNzAwMDAwMFoYDzIwMTcwMjA0
+MDAwMDAwWjAUMRIwEAYDVQQDDAllZS1pbnQtQ0EwdjAQBgcqhkjOPQIBBgUrgQQA
+IgNiAAShaHJDNitcexiJ83kVRhWhxz+0je6GPgIpFdtgjiUt5LcTLajOmOgxU05q
+nAwLCcjWOa3oMgbluoE0c6EfozDgXajJbkOD/ieHPalxA74oiM/wAvBa9xof3cyD
+dKpuqc6jFzAVMBMGA1UdJQQMMAoGCCsGAQUFBwMDMAsGCSqGSIb3DQEBCwOCAQEA
+i3LD0MmaiOG7TvVV6z0ULN5iAMuj1BaTSHHv+Nu4ghSEuVzaTH7AfC8I8ZZr4T4A
+6zb7SQoUUy+6ltmNyNh2foL4Yr0MM9IOeyw61Qni17v0VSEzue/01ZuzgXnPYGcd
+JcXrlvBRsDEMvl5vf+mXx9jjcJfTqlR6oWAi4+WTlbZeEgZoSD6t/FN771wmXtR0
+6ELD0QNODXKbWhYVUkVNzLogjvcTV/x9Ud07G8Fq69DP/GmEtF/Vre4zhrlnNnMR
+N5FW8ynKH+dL7BYxxwPd23ABAIcemF6MR7reoIHazQJEwzMW7+E9Cpf4lLJnDSQ1
+h6xWbLCpvfvHPWIYI+Gy3w==
+-----END CERTIFICATE-----
new file mode 100644
--- /dev/null
+++ b/security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_no_SAN_ee.pem.certspec
@@ -0,0 +1,4 @@
+issuer:int-CA
+subject:ee-int-CA
+subjectKey:secp384r1
+extension:extKeyUsage:codeSigning
--- a/security/manager/ssl/tests/unit/test_content_signing/moz.build
+++ b/security/manager/ssl/tests/unit/test_content_signing/moz.build
@@ -4,14 +4,15 @@
 # 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/.
 
 # Temporarily disabled. See bug 1256495.
 #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_key_ee.pem',
 #    'content_signing_remote_newtab_ee.pem',
 #)
 #
 #for test_certificate in test_certificates:
 #    GeneratedTestCertificate(test_certificate)