bug 1421413 - add a preference to control which add-on signature algorithms are valid r?jcj draft
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 28 Nov 2017 14:24:11 -0800
changeset 705235 9e42963a89843be1e981f8c7a8d2c76a06f0f905
parent 705036 3f6b9aaed8cd57954e0c960cde06d25228196456
child 742311 3e183e8e07330891f714c60c117cefd4e3e243a7
push id91416
push userbmo:dkeeler@mozilla.com
push dateWed, 29 Nov 2017 18:14:56 +0000
reviewersjcj
bugs1421413
milestone59.0a1
bug 1421413 - add a preference to control which add-on signature algorithms are valid r?jcj MozReview-Commit-ID: EwkpY9ADAtw
security/apps/AppSignatureVerification.cpp
security/manager/ssl/security-prefs.js
security/manager/ssl/tests/unit/test_signed_apps.js
--- a/security/apps/AppSignatureVerification.cpp
+++ b/security/apps/AppSignatureVerification.cpp
@@ -11,16 +11,17 @@
 #include "NSSCertDBTrustDomain.h"
 #include "ScopedNSSTypes.h"
 #include "SharedCertVerifier.h"
 #include "certdb.h"
 #include "cms.h"
 #include "mozilla/Base64.h"
 #include "mozilla/Casting.h"
 #include "mozilla/Logging.h"
+#include "mozilla/Preferences.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/Unused.h"
 #include "nsCOMPtr.h"
 #include "nsComponentManagerUtils.h"
 #include "nsDependentString.h"
 #include "nsHashKeys.h"
 #include "nsIDirectoryEnumerator.h"
@@ -834,18 +835,25 @@ VerifySignature(AppTrustedRoot trustedRo
     return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
   }
 
   return MapSECStatus(
     NSS_CMSSignerInfo_Verify(signerInfo, const_cast<SECItem*>(detachedDigest),
                              &pkcs7DataOid));
 }
 
+// This corresponds to the preference "security.signed_app_signatures.policy".
+enum class SignaturePolicy {
+  PKCS7WithSHA1OrSHA256 = 0,
+  PKCS7WithSHA256 = 1,
+};
+
 nsresult
 OpenSignedAppFile(AppTrustedRoot aTrustedRoot, nsIFile* aJarFile,
+                  SignaturePolicy aPolicy,
                   /*out, optional */ nsIZipReader** aZipReader,
                   /*out, optional */ nsIX509Cert** aSignerCert)
 {
   NS_ENSURE_ARG_POINTER(aJarFile);
 
   if (aZipReader) {
     *aZipReader = nullptr;
   }
@@ -900,16 +908,26 @@ OpenSignedAppFile(AppTrustedRoot aTruste
   UniqueCERTCertList builtChain;
   SECOidTag digestToUse;
   rv = VerifySignature(aTrustedRoot, sigBuffer, sfCalculatedSHA1Digest.get(),
                        sfCalculatedSHA256Digest.get(), digestToUse, builtChain);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
+  switch (aPolicy) {
+    case SignaturePolicy::PKCS7WithSHA256:
+      if (digestToUse != SEC_OID_SHA256) {
+        return NS_ERROR_SIGNED_JAR_WRONG_SIGNATURE;
+      }
+      break;
+    case SignaturePolicy::PKCS7WithSHA1OrSHA256:
+      break;
+  }
+
   nsAutoCString mfDigest;
   rv = ParseSF(BitwiseCast<char*, unsigned char*>(sfBuffer.data), digestToUse,
                mfDigest);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   // Manifest (MF) file
@@ -1032,59 +1050,82 @@ OpenSignedAppFile(AppTrustedRoot aTruste
 
   return NS_OK;
 }
 
 class OpenSignedAppFileTask final : public CryptoTask
 {
 public:
   OpenSignedAppFileTask(AppTrustedRoot aTrustedRoot, nsIFile* aJarFile,
+                        SignaturePolicy aPolicy,
                         nsIOpenSignedAppFileCallback* aCallback)
     : mTrustedRoot(aTrustedRoot)
     , mJarFile(aJarFile)
+    , mPolicy(aPolicy)
     , mCallback(new nsMainThreadPtrHolder<nsIOpenSignedAppFileCallback>(
         "OpenSignedAppFileTask::mCallback", aCallback))
   {
   }
 
 private:
   virtual nsresult CalculateResult() override
   {
-    return OpenSignedAppFile(mTrustedRoot, mJarFile,
+    return OpenSignedAppFile(mTrustedRoot, mJarFile, mPolicy,
                              getter_AddRefs(mZipReader),
                              getter_AddRefs(mSignerCert));
   }
 
   // nsNSSCertificate implements nsNSSShutdownObject, so there's nothing that
   // needs to be released
   virtual void ReleaseNSSResources() override { }
 
   virtual void CallCallback(nsresult rv) override
   {
     (void) mCallback->OpenSignedAppFileFinished(rv, mZipReader, mSignerCert);
   }
 
   const AppTrustedRoot mTrustedRoot;
   const nsCOMPtr<nsIFile> mJarFile;
+  const SignaturePolicy mPolicy;
   nsMainThreadPtrHandle<nsIOpenSignedAppFileCallback> mCallback;
   nsCOMPtr<nsIZipReader> mZipReader; // out
   nsCOMPtr<nsIX509Cert> mSignerCert; // out
 };
 
+static const SignaturePolicy sDefaultSignaturePolicy =
+  SignaturePolicy::PKCS7WithSHA1OrSHA256;
+
 } // unnamed namespace
 
 NS_IMETHODIMP
 nsNSSCertificateDB::OpenSignedAppFileAsync(
   AppTrustedRoot aTrustedRoot, nsIFile* aJarFile,
   nsIOpenSignedAppFileCallback* aCallback)
 {
   NS_ENSURE_ARG_POINTER(aJarFile);
   NS_ENSURE_ARG_POINTER(aCallback);
+  if (!NS_IsMainThread()) {
+    return NS_ERROR_NOT_SAME_THREAD;
+  }
+  SignaturePolicy policy =
+    static_cast<SignaturePolicy>(
+      Preferences::GetInt("security.signed_app_signatures.policy",
+                          static_cast<int32_t>(sDefaultSignaturePolicy)));
+  switch (policy) {
+    case SignaturePolicy::PKCS7WithSHA1OrSHA256:
+      break;
+    case SignaturePolicy::PKCS7WithSHA256:
+      break;
+    default:
+      policy = sDefaultSignaturePolicy;
+      break;
+  }
   RefPtr<OpenSignedAppFileTask> task(new OpenSignedAppFileTask(aTrustedRoot,
                                                                aJarFile,
+                                                               policy,
                                                                aCallback));
   return task->Dispatch("SignedJAR");
 }
 
 NS_IMETHODIMP
 nsNSSCertificateDB::VerifySignedDirectoryAsync(AppTrustedRoot, nsIFile*,
   nsIVerifySignedDirectoryCallback* aCallback)
 {
--- a/security/manager/ssl/security-prefs.js
+++ b/security/manager/ssl/security-prefs.js
@@ -62,16 +62,22 @@ pref("security.OCSP.timeoutMilliseconds.
 pref("security.OCSP.timeoutMilliseconds.hard", 10000);
 
 pref("security.pki.cert_short_lifetime_in_days", 10);
 // NB: Changes to this pref affect CERT_CHAIN_SHA1_POLICY_STATUS telemetry.
 // See the comment in CertVerifier.cpp.
 // 3 = only allow SHA-1 for certificates issued by an imported root.
 pref("security.pki.sha1_enforcement_level", 3);
 
+// This preference controls what signature algorithms are accepted for signed
+// apps (i.e. add-ons).
+// 0: SHA-1 and/or SHA-256 PKCS#7 allowed
+// 1: SHA-256 PKCS#7 allowed
+pref("security.signed_app_signatures.policy", 0);
+
 // security.pki.name_matching_mode controls how the platform matches hostnames
 // to name information in TLS certificates. The possible values are:
 // 0: always fall back to the subject common name if necessary (as in, if the
 //    subject alternative name extension is either not present or does not
 //    contain any DNS names or IP addresses)
 // 1: fall back to the subject common name for certificates valid before 23
 //    August 2016 if necessary
 // 2: fall back to the subject common name for certificates valid before 23
--- a/security/manager/ssl/tests/unit/test_signed_apps.js
+++ b/security/manager/ssl/tests/unit/test_signed_apps.js
@@ -184,148 +184,188 @@ var hashTestcases = [
   { name: "app_mf-256_sf-1-256_p7-1",
     expectedResult: Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID },
   { name: "app_mf-256_sf-1_p7-1",
     expectedResult: Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID },
   { name: "app_mf-256_sf-256_p7-1",
     expectedResult: Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID },
 ];
 
+// Policy values for the preference "security.signed_app_signatures.policy"
+const PKCS7WithSHA1OrSHA256 = 0;
+const PKCS7WithSHA256 = 1;
+
+function add_signature_test(policy, test) {
+  // First queue up a test to set the desired policy:
+  add_test(function () {
+    Services.prefs.setIntPref("security.signed_app_signatures.policy", policy);
+    run_next_test();
+  });
+  // Then queue up the test itself:
+  add_test(test);
+}
+
 for (let testcase of hashTestcases) {
-  add_test(function () {
+  add_signature_test(PKCS7WithSHA1OrSHA256, function () {
     certdb.openSignedAppFileAsync(
       Ci.nsIX509CertDB.AppXPCShellRoot,
       original_app_path(testcase.name),
       check_open_result(testcase.name, testcase.expectedResult));
   });
 }
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot,
     original_app_path("empty_signerInfos"),
     check_open_result("the signerInfos in the PKCS#7 signature is empty",
                       Cr.NS_ERROR_CMS_VERIFY_NOT_SIGNED));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, original_app_path("unsigned_app"),
     check_open_result("unsigned", Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, original_app_path("unknown_issuer_app"),
     check_open_result("unknown_issuer",
                       getXPCOMStatusFromNSS(SEC_ERROR_UNKNOWN_ISSUER)));
 });
 
 // Sanity check to ensure a no-op tampering gives a valid result
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("identity_tampering");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered, { }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, original_app_path("app_mf-1_sf-1_p7-1"),
     check_open_result("identity_tampering", Cr.NS_OK));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("missing_rsa");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "META-INF/A.RSA": removeEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("missing_rsa", Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("missing_sf");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "META-INF/A.SF": removeEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("missing_sf", Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("missing_manifest_mf");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "META-INF/MANIFEST.MF": removeEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("missing_manifest_mf",
                       Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("missing_entry");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "manifest.json": removeEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("missing_entry", Cr.NS_ERROR_SIGNED_JAR_ENTRY_MISSING));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("truncated_entry");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "manifest.json": truncateEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("truncated_entry",
                       Cr.NS_ERROR_SIGNED_JAR_MODIFIED_ENTRY));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("truncated_manifestFile");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "META-INF/MANIFEST.MF": truncateEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("truncated_manifestFile",
                       Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("truncated_signatureFile");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "META-INF/A.SF": truncateEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("truncated_signatureFile",
                       getXPCOMStatusFromNSS(SEC_ERROR_PKCS7_BAD_SIGNATURE)));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("truncated_pkcs7File");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered,
          { "META-INF/A.RSA": truncateEntry }, []);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("truncated_pkcs7File",
                       Cr.NS_ERROR_CMS_VERIFY_NOT_SIGNED));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("unsigned_entry");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered, {},
     [ { "name": "unsigned.txt", "content": "unsigned content!" } ]);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("unsigned_entry", Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY));
 });
 
-add_test(function () {
+add_signature_test(PKCS7WithSHA1OrSHA256, function () {
   let tampered = tampered_app_path("unsigned_metainf_entry");
   tamper(original_app_path("app_mf-1_sf-1_p7-1"), tampered, {},
     [ { name: "META-INF/unsigned.txt", content: "unsigned content!" } ]);
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, tampered,
     check_open_result("unsigned_metainf_entry",
                       Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY));
 });
 
+add_signature_test(PKCS7WithSHA256, function testSHA1Disabled() {
+  certdb.openSignedAppFileAsync(
+    Ci.nsIX509CertDB.AppXPCShellRoot,
+    original_app_path("app_mf-1_sf-1_p7-1"),
+    check_open_result("SHA-1 should not be accepted if disabled by policy",
+                      Cr.NS_ERROR_SIGNED_JAR_WRONG_SIGNATURE));
+});
+
+add_signature_test(PKCS7WithSHA256, function testSHA256WorksWithSHA1Disabled() {
+  certdb.openSignedAppFileAsync(
+    Ci.nsIX509CertDB.AppXPCShellRoot,
+    original_app_path("app_mf-256_sf-256_p7-256"),
+    check_open_result("SHA-256 should work if SHA-1 is disabled by policy",
+                      Cr.NS_OK));
+});
+
+add_signature_test(PKCS7WithSHA256,
+  function testMultipleSignaturesWorkWithSHA1Disabled() {
+    certdb.openSignedAppFileAsync(
+      Ci.nsIX509CertDB.AppXPCShellRoot,
+      original_app_path("app_mf-1-256_sf-1-256_p7-1-256"),
+      check_open_result("Multiple signatures should work if SHA-1 is " +
+                        "disabled by policy (if SHA-256 signature verifies)",
+                        Cr.NS_OK));
+});
+
 // TODO: tampered MF, tampered SF
 // TODO: too-large MF, too-large RSA, too-large SF
 // TODO: MF and SF that end immediately after the last main header
 //       (no CR nor LF)
 // TODO: broken headers to exercise the parser