author David Keeler <dkeeler@mozilla.com>

Tue, 24 Oct 2017 15:27:53 -0700

changeset 689671 0ce20c460403e496d0672ccc7702365519b9df46

parent 689670 f6120ea5356f123644d0d3b2c480d0ecc4190f55

child 689672 978d419fe840806ccdba7bb833cf8f6421faae24

push id 87085

push user bmo:dkeeler@mozilla.com

push date Tue, 31 Oct 2017 21:34:47 +0000

reviewers jcj, dveditz

bugs 1357815

milestone 58.0a1

js/xpconnect/src/xpc.msg file | annotate | diff | comparison | revisions

security/apps/AppSignatureVerification.cpp file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/pycms.py file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/sign_app.py file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps.js file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/empty_signerInfos.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/moz.build file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/sha1_and_sha256_manifest_sha1_signature_file.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/sha1_and_sha256_manifest_sha256_signature_file.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/sha1_manifest_sha1_and_sha256_signature_file.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/sha256_manifest_sha1_and_sha256_signature_file.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/signed_app.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha1_and_sha256.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha256.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha256_manifest.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha256_signature_file.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/unknown_issuer_app.zip file | annotate | diff | comparison | revisions

security/manager/ssl/tests/unit/test_signed_apps/unsigned_app.zip file | annotate | diff | comparison | revisions
--- a/js/xpconnect/src/xpc.msg
+++ b/js/xpconnect/src/xpc.msg
@@ -198,16 +198,17 @@ XPC_MSG_DEF(NS_ERROR_ILLEGAL_INPUT      
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_NOT_SIGNED          , "The JAR is not signed.")
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_MODIFIED_ENTRY      , "An entry in the JAR has been modified after the JAR was signed.")
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY      , "An entry in the JAR has not been signed.")
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_ENTRY_MISSING       , "An entry is missing from the JAR file.")
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_WRONG_SIGNATURE     , "The JAR's signature is wrong.")
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_ENTRY_TOO_LARGE     , "An entry in the JAR is too large.")
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_ENTRY_INVALID       , "An entry in the JAR is invalid.")
 XPC_MSG_DEF(NS_ERROR_SIGNED_JAR_MANIFEST_INVALID    , "The JAR's manifest or signature file is invalid.")
+XPC_MSG_DEF(NS_ERROR_CMS_VERIFY_NOT_SIGNED          , "The PKCS#7 information is not signed.")
 
 /* Codes related to signed manifests */
 XPC_MSG_DEF(NS_ERROR_SIGNED_APP_MANIFEST_INVALID   , "The signed app manifest or signature file is invalid.")
 
 /* Codes for printing-related errors. */
 XPC_MSG_DEF(NS_ERROR_GFX_PRINTER_NO_PRINTER_AVAILABLE , "No printers available.")
 XPC_MSG_DEF(NS_ERROR_GFX_PRINTER_NAME_NOT_FOUND       , "The selected printer could not be found.")
 XPC_MSG_DEF(NS_ERROR_GFX_PRINTER_COULD_NOT_OPEN_FILE  , "Failed to open output file for print to file.")
--- a/security/apps/AppSignatureVerification.cpp
+++ b/security/apps/AppSignatureVerification.cpp
@@ -136,18 +136,18 @@ ReadStream(const nsCOMPtr<nsIInputStream
   }
 
   buf.data[buf.len - 1] = 0; // null-terminate
 
   return NS_OK;
 }
 
 // Finds exactly one (signature metadata) JAR entry that matches the given
-// search pattern, and then load it. Fails if there are no matches or if
-// there is more than one match. If bugDigest is not null then on success
+// search pattern, and then loads it. Fails if there are no matches or if
+// there is more than one match. If bufDigest is not null then on success
 // bufDigest will contain the digeset of the entry using the given digest
 // algorithm.
 nsresult
 FindAndLoadOneEntry(nsIZipReader* zip,
                     const nsACString& searchPattern,
                     /*out*/ nsACString& filename,
                     /*out*/ SECItem& buf,
                     /*optional, in*/ SECOidTag digestAlgorithm = SEC_OID_SHA1,
@@ -462,101 +462,98 @@ CheckManifestVersion(const char* & nextL
   if (!curLine.Equals(expectedHeader)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
   return NS_OK;
 }
 
 // Parses a signature file (SF) based on the JDK 8 JAR Specification.
 //
-// The SF file must contain a SHA1-Digest-Manifest (or, preferrably,
-// SHA256-Digest-Manifest) attribute in the main section. All other sections are
-// ignored. This means that this will NOT parse old-style signature files that
-// have separate digests per entry.
+// The SF file must contain a SHA*-Digest-Manifest attribute in the main
+// section (where the * is either 1 or 256, depending on the given digest
+// algorithm). All other sections are ignored. This means that this will NOT
+// parse old-style signature files that have separate digests per entry.
 // The JDK8 x-Digest-Manifest variant is better because:
 //
 //   (1) It allows us to follow the principle that we should minimize the
 //       processing of data that we do before we verify its signature. In
 //       particular, with the x-Digest-Manifest style, we can verify the digest
 //       of MANIFEST.MF before we parse it, which prevents malicious JARs
 //       exploiting our MANIFEST.MF parser.
 //   (2) It is more time-efficient and space-efficient to have one
 //       x-Digest-Manifest instead of multiple x-Digest values.
 //
 // filebuf must be null-terminated. On output, mfDigest will contain the
-// decoded value of SHA1-Digest-Manifest or SHA256-Digest-Manifest, if found,
-// as well as an identifier indicating which algorithm was found.
+// decoded value of the appropriate SHA*-DigestManifest, if found.
 nsresult
-ParseSF(const char* filebuf, /*out*/ DigestWithAlgorithm& mfDigest)
+ParseSF(const char* filebuf, SECOidTag digestAlgorithm,
+        /*out*/ nsAutoCString& mfDigest)
 {
+  const char* digestNameToFind = nullptr;
+  switch (digestAlgorithm) {
+    case SEC_OID_SHA256:
+      digestNameToFind = "sha256-digest-manifest";
+      break;
+    case SEC_OID_SHA1:
+      digestNameToFind = "sha1-digest-manifest";
+      break;
+    default:
+      MOZ_ASSERT_UNREACHABLE("bad argument to ParseSF");
+      return NS_ERROR_FAILURE;
+  }
+
   const char* nextLineStart = filebuf;
   nsresult rv = CheckManifestVersion(nextLineStart,
                                      NS_LITERAL_CSTRING(JAR_SF_HEADER));
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  nsAutoCString savedSHA1Digest;
-  // Search for SHA256-Digest-Manifest and SHA1-Digest-Manifest. Prefer the
-  // former.
   for (;;) {
     nsAutoCString curLine;
     rv = ReadLine(nextLineStart, curLine);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     if (curLine.Length() == 0) {
-      // End of main section (blank line or end-of-file). We didn't find
-      // SHA256-Digest-Manifest, but maybe we found SHA1-Digest-Manifest.
-      if (!savedSHA1Digest.IsEmpty()) {
-        mfDigest.mDigest.Assign(savedSHA1Digest);
-        mfDigest.mAlgorithm = SEC_OID_SHA1;
-        return NS_OK;
-      }
+      // End of main section (blank line or end-of-file). We didn't find the
+      // SHA*-Digest-Manifest we were looking for.
       return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
     }
 
     nsAutoCString attrName;
     nsAutoCString attrValue;
     rv = ParseAttribute(curLine, attrName, attrValue);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
-    if (attrName.LowerCaseEqualsLiteral("sha256-digest-manifest")) {
-      rv = Base64Decode(attrValue, mfDigest.mDigest);
+    if (attrName.EqualsIgnoreCase(digestNameToFind)) {
+      rv = Base64Decode(attrValue, mfDigest);
       if (NS_FAILED(rv)) {
         return rv;
       }
-      mfDigest.mAlgorithm = SEC_OID_SHA256;
 
       // There could be multiple SHA*-Digest-Manifest attributes, which
       // would be an error, but it's better to just skip any erroneous
       // duplicate entries rather than trying to detect them, because:
       //
       //   (1) It's simpler, and simpler generally means more secure
       //   (2) An attacker can't make us accept a JAR we would otherwise
       //       reject just by adding additional SHA*-Digest-Manifest
       //       attributes.
       return NS_OK;
     }
 
-    if (attrName.LowerCaseEqualsLiteral("sha1-digest-manifest") &&
-        savedSHA1Digest.IsEmpty()) {
-      rv = Base64Decode(attrValue, savedSHA1Digest);
-      if (NS_FAILED(rv)) {
-        return rv;
-      }
-    }
-
     // ignore unrecognized attributes
   }
 
-  return NS_OK;
+  MOZ_ASSERT_UNREACHABLE("somehow exited loop in ParseSF without returning");
+  return NS_ERROR_FAILURE;
 }
 
 // Parses MANIFEST.MF. The filenames of all entries will be returned in
 // mfItems. buf must be a pre-allocated scratch buffer that is used for doing
 // I/O. Each file's contents are verified against the entry in the manifest with
 // the digest algorithm that matches the given one. This algorithm comes from
 // the signature file. If the signature file has a SHA-256 digest, then SHA-256
 // entries must be present in the manifest file. If the signature file only has
@@ -753,26 +750,66 @@ VerifyCertificate(CERTCertificate* signe
   }
   if (result != Success) {
     return mozilla::psm::GetXPCOMFromNSSError(MapResultToPRErrorCode(result));
   }
 
   return NS_OK;
 }
 
+// Given a SECOidTag representing a digest algorithm (either SEC_OID_SHA1 or
+// SEC_OID_SHA256), returns the first signerInfo in the given signedData that
+// purports to have been created using that digest algorithm, or nullptr if
+// there is none.
+// The returned signerInfo is owned by signedData, so the caller must ensure
+// that the lifetime of the signerInfo is contained by the lifetime of the
+// signedData.
+NSSCMSSignerInfo*
+GetSignerInfoForDigestAlgorithm(NSSCMSSignedData* signedData,
+                                SECOidTag digestAlgorithm)
+{
+  MOZ_ASSERT(digestAlgorithm == SEC_OID_SHA1 ||
+             digestAlgorithm == SEC_OID_SHA256);
+  if (digestAlgorithm != SEC_OID_SHA1 && digestAlgorithm != SEC_OID_SHA256) {
+    return nullptr;
+  }
+
+  int numSigners = NSS_CMSSignedData_SignerInfoCount(signedData);
+  if (numSigners < 1) {
+    return nullptr;
+  }
+  for (int i = 0; i < numSigners; i++) {
+    NSSCMSSignerInfo* signerInfo =
+      NSS_CMSSignedData_GetSignerInfo(signedData, i);
+    // NSS_CMSSignerInfo_GetDigestAlgTag isn't exported from NSS.
+    SECOidData* digestAlgOID = SECOID_FindOID(&signerInfo->digestAlg.algorithm);
+    if (!digestAlgOID) {
+      continue;
+    }
+    if (digestAlgorithm == digestAlgOID->offset) {
+      return signerInfo;
+    }
+  }
+  return nullptr;
+}
+
 nsresult
 VerifySignature(AppTrustedRoot trustedRoot, const SECItem& buffer,
-                const SECItem& detachedDigest,
+                const SECItem& detachedSHA1Digest,
+                const SECItem& detachedSHA256Digest,
+                /*out*/ SECOidTag& digestAlgorithm,
                 /*out*/ UniqueCERTCertList& builtChain)
 {
   // Currently, this function is only called within the CalculateResult() method
   // of CryptoTasks. As such, NSS should not be shut down at this point and the
   // CryptoTask implementation should already hold a nsNSSShutDownPreventionLock.
-  if (NS_WARN_IF(!buffer.data && buffer.len > 0) ||
-      NS_WARN_IF(!detachedDigest.data && detachedDigest.len > 0)) {
+
+  if (NS_WARN_IF(!buffer.data || buffer.len == 0 || !detachedSHA1Digest.data ||
+                 detachedSHA1Digest.len == 0 || !detachedSHA256Digest.data ||
+                 detachedSHA256Digest.len == 0)) {
     return NS_ERROR_INVALID_ARG;
   }
 
   UniqueNSSCMSMessage
     cmsMsg(NSS_CMSMessage_CreateFromDER(const_cast<SECItem*>(&buffer), nullptr,
                                         nullptr, nullptr, nullptr, nullptr,
                                         nullptr));
   if (!cmsMsg) {
@@ -796,22 +833,16 @@ VerifySignature(AppTrustedRoot trustedRo
 
   // signedData is non-owning
   NSSCMSSignedData* signedData =
     static_cast<NSSCMSSignedData*>(NSS_CMSContentInfo_GetContent(cinfo));
   if (!signedData) {
     return NS_ERROR_CMS_VERIFY_NO_CONTENT_INFO;
   }
 
-  // Set digest value.
-  if (NSS_CMSSignedData_SetDigestValue(signedData, SEC_OID_SHA1,
-                                       const_cast<SECItem*>(&detachedDigest))) {
-    return NS_ERROR_CMS_VERIFY_BAD_DIGEST;
-  }
-
   // Parse the certificates into CERTCertificate objects held in memory so
   // verifyCertificate will be able to find them during path building.
   UniqueCERTCertList certs(CERT_NewCertList());
   if (!certs) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   if (signedData->rawCerts) {
     for (size_t i = 0; signedData->rawCerts[i]; ++i) {
@@ -827,47 +858,53 @@ VerifySignature(AppTrustedRoot trustedRo
       if (CERT_AddCertToListTail(certs.get(), cert.get()) != SECSuccess) {
         return NS_ERROR_OUT_OF_MEMORY;
       }
 
       Unused << cert.release(); // Ownership transferred to the cert list.
     }
   }
 
-  // Get the end-entity certificate.
-  int numSigners = NSS_CMSSignedData_SignerInfoCount(signedData);
-  if (NS_WARN_IF(numSigners != 1)) {
-    return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
+  NSSCMSSignerInfo* signerInfo =
+    GetSignerInfoForDigestAlgorithm(signedData, SEC_OID_SHA256);
+  const SECItem* detachedDigest = &detachedSHA256Digest;
+  digestAlgorithm = SEC_OID_SHA256;
+  if (!signerInfo) {
+    signerInfo = GetSignerInfoForDigestAlgorithm(signedData, SEC_OID_SHA1);
+    if (!signerInfo) {
+      return NS_ERROR_CMS_VERIFY_NOT_SIGNED;
+    }
+    detachedDigest = &detachedSHA1Digest;
+    digestAlgorithm = SEC_OID_SHA1;
   }
-  // signer is non-owning.
-  NSSCMSSignerInfo* signer = NSS_CMSSignedData_GetSignerInfo(signedData, 0);
-  if (NS_WARN_IF(!signer)) {
-    return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
-  }
+
+  // Get the end-entity certificate.
   CERTCertificate* signerCert =
-    NSS_CMSSignerInfo_GetSigningCertificate(signer, CERT_GetDefaultCertDB());
+    NSS_CMSSignerInfo_GetSigningCertificate(signerInfo,
+                                            CERT_GetDefaultCertDB());
   if (!signerCert) {
     return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
   }
 
   nsresult rv = VerifyCertificate(signerCert, trustedRoot, builtChain);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
+  if (NS_FAILED(rv)) {
     return rv;
   }
 
-  // See NSS_CMSContentInfo_GetContentTypeOID, which isn't exported from NSS.
-  SECOidData* contentTypeOidData =
-    SECOID_FindOID(&signedData->contentInfo.contentType);
-  if (!contentTypeOidData) {
+  // Ensure that the PKCS#7 data OID is present as the PKCS#9 contentType.
+  const char* pkcs7DataOidString = "1.2.840.113549.1.7.1";
+  ScopedAutoSECItem pkcs7DataOid;
+  if (SEC_StringToOID(nullptr, &pkcs7DataOid, pkcs7DataOidString, 0)
+        != SECSuccess) {
     return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
   }
 
-  return MapSECStatus(NSS_CMSSignerInfo_Verify(signer,
-                         const_cast<SECItem*>(&detachedDigest),
-                         &contentTypeOidData->oid));
+  return MapSECStatus(
+    NSS_CMSSignerInfo_Verify(signerInfo, const_cast<SECItem*>(detachedDigest),
+                             &pkcs7DataOid));
 }
 
 nsresult
 OpenSignedAppFile(AppTrustedRoot aTrustedRoot, nsIFile* aJarFile,
                   /*out, optional */ nsIZipReader** aZipReader,
                   /*out, optional */ nsIX509Cert** aSignerCert)
 {
   NS_ENSURE_ARG_POINTER(aJarFile);
@@ -896,64 +933,79 @@ OpenSignedAppFile(AppTrustedRoot aTruste
                            sigFilename, sigBuffer);
   if (NS_FAILED(rv)) {
     return NS_ERROR_SIGNED_JAR_NOT_SIGNED;
   }
 
   // Signature (SF) file
   nsAutoCString sfFilename;
   ScopedAutoSECItem sfBuffer;
-  Digest sfCalculatedDigest;
   rv = FindAndLoadOneEntry(zip, NS_LITERAL_CSTRING(JAR_SF_SEARCH_STRING),
-                           sfFilename, sfBuffer, SEC_OID_SHA1,
-                           &sfCalculatedDigest);
+                           sfFilename, sfBuffer);
   if (NS_FAILED(rv)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
 
-  sigBuffer.type = siBuffer;
-  UniqueCERTCertList builtChain;
-  rv = VerifySignature(aTrustedRoot, sigBuffer, sfCalculatedDigest.get(),
-                       builtChain);
+  // Calculate both the SHA-1 and SHA-256 hashes of the signature file - we
+  // don't know what algorithm the PKCS#7 signature used.
+  Digest sfCalculatedSHA1Digest;
+  rv = sfCalculatedSHA1Digest.DigestBuf(SEC_OID_SHA1, sfBuffer.data,
+                                        sfBuffer.len - 1);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  Digest sfCalculatedSHA256Digest;
+  rv = sfCalculatedSHA256Digest.DigestBuf(SEC_OID_SHA256, sfBuffer.data,
+                                          sfBuffer.len - 1);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  DigestWithAlgorithm mfDigest;
-  rv = ParseSF(BitwiseCast<char*, unsigned char*>(sfBuffer.data), mfDigest);
+  sigBuffer.type = siBuffer;
+  UniqueCERTCertList builtChain;
+  SECOidTag digestToUse;
+  rv = VerifySignature(aTrustedRoot, sigBuffer, sfCalculatedSHA1Digest.get(),
+                       sfCalculatedSHA256Digest.get(), digestToUse, builtChain);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  nsAutoCString mfDigest;
+  rv = ParseSF(BitwiseCast<char*, unsigned char*>(sfBuffer.data), digestToUse,
+               mfDigest);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   // Manifest (MF) file
   nsAutoCString mfFilename;
   ScopedAutoSECItem manifestBuffer;
   Digest mfCalculatedDigest;
   rv = FindAndLoadOneEntry(zip, NS_LITERAL_CSTRING(JAR_MF_SEARCH_STRING),
-                           mfFilename, manifestBuffer, mfDigest.mAlgorithm,
+                           mfFilename, manifestBuffer, digestToUse,
                            &mfCalculatedDigest);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   nsDependentCSubstring calculatedDigest(
     DigestToDependentString(mfCalculatedDigest));
-  if (!mfDigest.mDigest.Equals(calculatedDigest)) {
+  if (!mfDigest.Equals(calculatedDigest)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
 
   // Allocate the I/O buffer only once per JAR, instead of once per entry, in
   // order to minimize malloc/free calls and in order to avoid fragmenting
   // memory.
   ScopedAutoSECItem buf(128 * 1024);
 
   nsTHashtable<nsCStringHashKey> items;
 
   rv = ParseMF(BitwiseCast<char*, unsigned char*>(manifestBuffer.data), zip,
-               mfDigest.mAlgorithm, items, buf);
+               digestToUse, items, buf);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   // Verify every entry in the file.
   nsCOMPtr<nsIUTF8StringEnumerator> entries;
   rv = zip->FindEntries(EmptyCString(), getter_AddRefs(entries));
   if (NS_SUCCEEDED(rv) && !entries) {
@@ -1471,66 +1523,81 @@ VerifySignedDirectory(AppTrustedRoot aTr
 
   // Load the signature (SF) file and verify the signature.
   // The .sf and .rsa files must have the same name apart from the extension.
 
   nsAutoString sfFilename(Substring(sigFilename, 0, sigFilename.Length() - 3)
                           + NS_LITERAL_STRING("sf"));
 
   ScopedAutoSECItem sfBuffer;
-  Digest sfCalculatedDigest;
-  rv = LoadOneMetafile(metaDir, sfFilename, sfBuffer, SEC_OID_SHA1,
-                       &sfCalculatedDigest);
+  rv = LoadOneMetafile(metaDir, sfFilename, sfBuffer);
   if (NS_FAILED(rv)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
 
+  // Calculate both the SHA-1 and SHA-256 hashes of the signature file - we
+  // don't know what algorithm the PKCS#7 signature used.
+  Digest sfCalculatedSHA1Digest;
+  rv = sfCalculatedSHA1Digest.DigestBuf(SEC_OID_SHA1, sfBuffer.data,
+                                        sfBuffer.len - 1);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  Digest sfCalculatedSHA256Digest;
+  rv = sfCalculatedSHA256Digest.DigestBuf(SEC_OID_SHA256, sfBuffer.data,
+                                          sfBuffer.len - 1);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
   sigBuffer.type = siBuffer;
   UniqueCERTCertList builtChain;
-  rv = VerifySignature(aTrustedRoot, sigBuffer, sfCalculatedDigest.get(),
-                       builtChain);
+  SECOidTag digestToUse;
+  rv = VerifySignature(aTrustedRoot, sigBuffer, sfCalculatedSHA1Digest.get(),
+                       sfCalculatedSHA256Digest.get(), digestToUse, builtChain);
   if (NS_FAILED(rv)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
 
   // Get the expected manifest hash from the signed .sf file
 
-  DigestWithAlgorithm mfDigest;
-  rv = ParseSF(BitwiseCast<char*, unsigned char*>(sfBuffer.data), mfDigest);
+  nsAutoCString mfDigest;
+  rv = ParseSF(BitwiseCast<char*, unsigned char*>(sfBuffer.data), digestToUse,
+               mfDigest);
   if (NS_FAILED(rv)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
 
   // Load manifest (MF) file and verify signature
 
   nsAutoString mfFilename(NS_LITERAL_STRING("manifest.mf"));
   ScopedAutoSECItem manifestBuffer;
   Digest mfCalculatedDigest;
-  rv = LoadOneMetafile(metaDir, mfFilename, manifestBuffer, mfDigest.mAlgorithm,
+  rv = LoadOneMetafile(metaDir, mfFilename, manifestBuffer, digestToUse,
                        &mfCalculatedDigest);
   if (NS_FAILED(rv)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
 
   nsDependentCSubstring calculatedDigest(
     DigestToDependentString(mfCalculatedDigest));
-  if (!mfDigest.mDigest.Equals(calculatedDigest)) {
+  if (!mfDigest.Equals(calculatedDigest)) {
     return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
   }
 
   // Parse manifest and verify signed hash of all listed files
 
   // Allocate the I/O buffer only once per JAR, instead of once per entry, in
   // order to minimize malloc/free calls and in order to avoid fragmenting
   // memory.
   ScopedAutoSECItem buf(128 * 1024);
 
   nsTHashtable<nsStringHashKey> items;
   rv = ParseMFUnpacked(BitwiseCast<char*, unsigned char*>(manifestBuffer.data),
-                       aDirectory, mfDigest.mAlgorithm, items, buf);
+                       aDirectory, digestToUse, items, buf);
   if (NS_FAILED(rv)){
     return rv;
   }
 
   // We've checked that everything listed in the manifest exists and is signed
   // correctly. Now check on disk for extra (unsigned) files.
   // Deletes found entries from items as it goes.
   rv = CheckDirForUnsignedFiles(aDirectory, EmptyString(), items,
--- a/security/manager/ssl/tests/unit/pycms.py
+++ b/security/manager/ssl/tests/unit/pycms.py
@@ -5,24 +5,30 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 """
 Reads a specification from stdin and outputs a PKCS7 (CMS) message with
 the desired properties.
 
 The specification format is as follows:
 
-hash:<hex string>
+sha1:<hex string>
+sha256:<hex string>
 signer:
 <pycert specification>
 
-hash is the value that will be put in the messageDigest attribute in
-each SignerInfo of the signerInfos field of the SignedData.
+Eith or both of sha1 and sha256 may be specified. The value of
+each hash directive is what will be put in the messageDigest
+attribute of the SignerInfo that corresponds to the signature
+algorithm defined by the hash algorithm and key type of the
+default key. Together, these comprise the signerInfos field of
+the SignedData. If neither hash is specified, the signerInfos
+will be an empty SET (i.e. there will be no actual signature
+information).
 The certificate specification must come last.
-Currently only SHA-1 is supported.
 """
 
 from pyasn1.codec.der import decoder
 from pyasn1.codec.der import encoder
 from pyasn1.type import tag, univ
 from pyasn1_modules import rfc2315, rfc2459
 import StringIO
 import base64
@@ -47,26 +53,29 @@ class UnknownDirectiveError(Error):
         return 'Unknown directive %s' % repr(self.directive)
 
 
 class CMS(object):
     """Utility class for reading a CMS specification and
     generating a CMS message"""
 
     def __init__(self, paramStream):
-        self.hash = ''
+        self.sha1 = ''
+        self.sha256 = ''
         signerSpecification = StringIO.StringIO()
         readingSignerSpecification = False
         for line in paramStream.readlines():
             if readingSignerSpecification:
                 print >>signerSpecification, line.strip()
             elif line.strip() == 'signer:':
                 readingSignerSpecification = True
-            elif line.startswith('hash:'):
-                self.hash = line.strip()[len('hash:'):]
+            elif line.startswith('sha1:'):
+                self.sha1 = line.strip()[len('sha1:'):]
+            elif line.startswith('sha256:'):
+                self.sha256 = line.strip()[len('sha256:'):]
             else:
                 raise UnknownDirectiveError(line.strip())
         signerSpecification.seek(0)
         self.signer = pycert.Certificate(signerSpecification)
         self.signingKey = pykey.keyFromSpecification('default')
 
     def buildAuthenticatedAttributes(self, value, implicitTag=None):
         """Utility function to build a pyasn1 AuthenticatedAttributes
@@ -92,21 +101,25 @@ class CMS(object):
         hashAttribute['values'][0] = univ.OctetString(hexValue=value)
         authenticatedAttributes[1] = hashAttribute
         return authenticatedAttributes
 
     def pykeyHashToDigestAlgorithm(self, pykeyHash):
         """Given a pykey hash algorithm identifier, builds an
         AlgorithmIdentifier for use with pyasn1."""
         if pykeyHash == pykey.HASH_SHA1:
-            algorithmIdentifier = rfc2459.AlgorithmIdentifier()
-            algorithmIdentifier['algorithm'] = univ.ObjectIdentifier('1.3.14.3.2.26')
-            algorithmIdentifier['parameters'] = univ.Null()
-            return algorithmIdentifier
-        raise pykey.UnknownHashAlgorithmError(pykeyHash)
+            oidString = '1.3.14.3.2.26'
+        elif pykeyHash == pykey.HASH_SHA256:
+            oidString = '2.16.840.1.101.3.4.2.1'
+        else:
+            raise pykey.UnknownHashAlgorithmError(pykeyHash)
+        algorithmIdentifier = rfc2459.AlgorithmIdentifier()
+        algorithmIdentifier['algorithm'] = univ.ObjectIdentifier(oidString)
+        algorithmIdentifier['parameters'] = univ.Null()
+        return algorithmIdentifier
 
     def buildSignerInfo(self, certificate, pykeyHash, digestValue):
         """Given a pyasn1 certificate, a pykey hash identifier
         and a hash value, creates a SignerInfo with the
         appropriate values."""
         signerInfo = rfc2315.SignerInfo()
         signerInfo['version'] = 1
         issuerAndSerialNumber = rfc2315.IssuerAndSerialNumber()
@@ -152,17 +165,22 @@ class CMS(object):
         certificate = decoder.decode(self.signer.toDER(),
             asn1Spec=rfc2459.Certificate())[0]
         extendedCertificateOrCertificate['certificate'] = certificate
         certificates[0] = extendedCertificateOrCertificate
         signedData['certificates'] = certificates
 
         signerInfos = rfc2315.SignerInfos()
 
-        signerInfos[0] = self.buildSignerInfo(certificate, pykey.HASH_SHA1, self.hash)
+        if len(self.sha1) > 0:
+            signerInfos[len(signerInfos)] = self.buildSignerInfo(certificate,
+                pykey.HASH_SHA1, self.sha1)
+        if len(self.sha256) > 0:
+            signerInfos[len(signerInfos)] = self.buildSignerInfo(certificate,
+                pykey.HASH_SHA256, self.sha256)
         signedData['signerInfos'] = signerInfos
 
         encoded = encoder.encode(signedData)
         anyTag = univ.Any(encoded).subtype(
             explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatConstructed, 0))
 
         contentInfo['content'] = anyTag
         return encoder.encode(contentInfo)
--- a/security/manager/ssl/tests/unit/sign_app.py
+++ b/security/manager/ssl/tests/unit/sign_app.py
@@ -28,24 +28,25 @@ def walkDirectory(directory):
     for path, dirs, files in os.walk(directory):
         for f in files:
             fullPath = os.path.join(path, f)
             internalPath = re.sub(r'^/', '', fullPath.replace(directory, ''))
             paths.append((fullPath, internalPath))
     return paths
 
 def signZip(appDirectory, outputFile, issuerName, manifestHashes,
-            signatureHashes, doSign):
+            signatureHashes, pkcs7Hashes, doSign):
     """Given a directory containing the files to package up,
     an output filename to write to, the name of the issuer of
     the signing certificate, a list of hash algorithms to use in
     the manifest file, a similar list for the signature file,
-    and whether or not to actually sign the resulting package,
-    packages up the files in the directory and creates the
-    output as appropriate."""
+    a similar list for the pkcs#7 signature, and whether or not
+    to actually sign the resulting package, packages up the
+    files in the directory and creates the output as
+    appropriate."""
     mfEntries = []
 
     with zipfile.ZipFile(outputFile, 'w') as outZip:
         for (fullPath, internalPath) in walkDirectory(appDirectory):
             with open(fullPath) as inputFile:
                 contents = inputFile.read()
             outZip.writestr(internalPath, contents)
 
@@ -61,17 +62,22 @@ def signZip(appDirectory, outputFile, is
             return
 
         mfContents = 'Manifest-Version: 1.0\n\n' + '\n'.join(mfEntries)
         sfContents = 'Signature-Version: 1.0\n'
         for (hashFunc, name) in signatureHashes:
             base64hash = b64encode(hashFunc(mfContents).digest())
             sfContents += '%s-Digest-Manifest: %s\n' % (name, base64hash)
 
-        cmsSpecification = 'hash:%s\nsigner:\n' % sha1(sfContents).hexdigest() + \
+        cmsSpecification = ''
+        for name in pkcs7Hashes:
+            hashFunc, _ = hashNameToFunctionAndIdentifier(name)
+            cmsSpecification += '%s:%s\n' % (name,
+                                             hashFunc(sfContents).hexdigest())
+        cmsSpecification += 'signer:\n' + \
             'issuer:%s\n' % issuerName + \
             'subject:xpcshell signed app test signer\n' + \
             'extension:keyUsage:digitalSignature'
         cmsSpecificationStream = StringIO.StringIO()
         print >>cmsSpecificationStream, cmsSpecification
         cmsSpecificationStream.seek(0)
         cms = pycms.CMS(cmsSpecificationStream)
         p7 = cms.toDER()
@@ -113,17 +119,25 @@ def main(outputFile, appPath, *args):
     parser.add_argument('-i', '--issuer', action='store', help='Issuer name',
                         default='xpcshell signed apps test root')
     parser.add_argument('-m', '--manifest-hash', action='append',
                         help='Hash algorithms to use in manifest',
                         default=[])
     parser.add_argument('-s', '--signature-hash', action='append',
                         help='Hash algorithms to use in signature file',
                         default=[])
+    group = parser.add_mutually_exclusive_group()
+    group.add_argument('-p', '--pkcs7-hash', action='append',
+                       help='Hash algorithms to use in PKCS#7 signature',
+                       default=[])
+    group.add_argument('-e', '--empty-signerInfos', action='store_true',
+                       help='Emit pkcs#7 SignedData with empty signerInfos')
     parsed = parser.parse_args(args)
     if len(parsed.manifest_hash) == 0:
         parsed.manifest_hash.append('sha256')
     if len(parsed.signature_hash) == 0:
         parsed.signature_hash.append('sha256')
+    if len(parsed.pkcs7_hash) == 0 and not parsed.empty_signerInfos:
+        parsed.pkcs7_hash.append('sha256')
     signZip(appPath, outputFile, parsed.issuer,
             map(hashNameToFunctionAndIdentifier, parsed.manifest_hash),
             map(hashNameToFunctionAndIdentifier, parsed.signature_hash),
-            not parsed.no_sign)
+            parsed.pkcs7_hash, not parsed.no_sign)
--- a/security/manager/ssl/tests/unit/test_signed_apps.js
+++ b/security/manager/ssl/tests/unit/test_signed_apps.js
@@ -138,24 +138,25 @@ add_test(function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, original_app_path("signed_app"),
     check_open_result("valid", Cr.NS_OK));
 });
 
 add_test(function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, original_app_path("signed_app_sha256"),
-    check_open_result("valid with sha256 hashes in manifest", Cr.NS_OK));
+    check_open_result("valid with sha256 everywhere", Cr.NS_OK));
 });
 
 add_test(function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot,
     original_app_path("signed_app_sha1_and_sha256"),
-    check_open_result("valid with sha1 and sha256 hashes in manifest", Cr.NS_OK));
+    check_open_result("valid with sha1 and sha256 hashes everywhere",
+                      Cr.NS_OK));
 });
 
 add_test(function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot,
     original_app_path("signed_app_sha256_manifest"),
     check_open_result("sha256 hashes in manifest, but only a sha1 hash in the signature file",
                       Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID));
@@ -177,33 +178,41 @@ add_test(function () {
                       Cr.NS_OK));
 });
 
 add_test(function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot,
     original_app_path("sha1_and_sha256_manifest_sha256_signature_file"),
     check_open_result("sha1 and sha256 hashes in the manifest, but only sha256 hash in the signature file",
-                      Cr.NS_OK));
+                      Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID));
 });
 
 add_test(function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot,
     original_app_path("sha1_manifest_sha1_and_sha256_signature_file"),
     check_open_result("only sha1 in the manifest, sha1 and sha256 hashes in the signature file",
-                      Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID));
+                      Cr.NS_OK));
 });
 
 add_test(function () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot,
     original_app_path("sha256_manifest_sha1_and_sha256_signature_file"),
     check_open_result("only sha256 in the manifest, sha1 and sha256 hashes in the signature file",
-                      Cr.NS_OK));
+                      Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID));
+});
+
+add_test(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 () {
   certdb.openSignedAppFileAsync(
     Ci.nsIX509CertDB.AppXPCShellRoot, original_app_path("unsigned_app"),
     check_open_result("unsigned", Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED));
 });
 
--- a/security/manager/ssl/tests/unit/test_signed_apps/moz.build
+++ b/security/manager/ssl/tests/unit/test_signed_apps/moz.build
@@ -20,23 +20,24 @@ def SignedAppFile(name, flags):
     # TEST_HARNESS_FILES.xpcshell.security.manager.ssl.tests.unit.test_signed_apps.
     files = TEST_HARNESS_FILES.xpcshell
     for part in RELATIVEDIR.split('/'):
         files = files[part]
     files += ['!%s' % name]
 
 # Temporarily disabled. See bug 1256495.
 #signed_app_files = (
-#    ['signed_app.zip', '-m', 'sha1', '-s', 'sha1'],
-#    ['unknown_issuer_app.zip', '-i', 'unknown issuer', '-m', 'sha1', '-s', 'sha1'],
+#    ['signed_app.zip', '-m', 'sha1', '-s', 'sha1', '-p', 'sha1'],
+#    ['unknown_issuer_app.zip', '-i', 'unknown issuer', '-m', 'sha1', '-s', 'sha1', '-p', 'sha1'],
 #    ['unsigned_app.zip', '-n'],
 #    ['signed_app_sha256.zip'],
-#    ['signed_app_sha1_and_sha256.zip', '-m', 'sha256', '-m', 'sha1', '-s', 'sha256', '-s', 'sha1'],
-#    ['signed_app_sha256_manifest.zip', '-s', 'sha1'],
-#    ['signed_app_sha256_signature_file.zip', '-m', 'sha1'],
-#    ['sha1_and_sha256_manifest_sha1_signature_file.zip', '-m', 'sha256', '-m', 'sha1', '-s', 'sha1'],
-#    ['sha1_and_sha256_manifest_sha256_signature_file.zip', '-m', 'sha256', '-m', 'sha1'],
-#    ['sha1_manifest_sha1_and_sha256_signature_file.zip', '-m', 'sha1', '-s', 'sha1', '-s', 'sha256'],
-#    ['sha256_manifest_sha1_and_sha256_signature_file.zip', '-s', 'sha1', '-s', 'sha256'],
+#    ['signed_app_sha1_and_sha256.zip', '-m', 'sha256', '-m', 'sha1', '-s', 'sha256', '-s', 'sha1', '-p', 'sha1', '-p', 'sha256'],
+#    ['signed_app_sha256_manifest.zip', '-s', 'sha1', '-p', 'sha1'],
+#    ['signed_app_sha256_signature_file.zip', '-m', 'sha1', '-p', 'sha1'],
+#    ['sha1_and_sha256_manifest_sha1_signature_file.zip', '-m', 'sha256', '-m', 'sha1', '-s', 'sha1', '-p', 'sha1'],
+#    ['sha1_and_sha256_manifest_sha256_signature_file.zip', '-m', 'sha256', '-m', 'sha1', '-p', 'sha1'],
+#    ['sha1_manifest_sha1_and_sha256_signature_file.zip', '-m', 'sha1', '-s', 'sha1', '-s', 'sha256', '-p', 'sha1'],
+#    ['sha256_manifest_sha1_and_sha256_signature_file.zip', '-s', 'sha1', '-s', 'sha256', '-p', 'sha1'],
+#    ['empty_signerInfos.zip', '-e'],
 #)
 #
 #for signed_app_file_params in signed_app_files:
 #    SignedAppFile(signed_app_file_params[0], signed_app_file_params[1:])
author	David Keeler <dkeeler@mozilla.com>
	Tue, 24 Oct 2017 15:27:53 -0700
changeset 689671	0ce20c460403e496d0672ccc7702365519b9df46
parent 689670	f6120ea5356f123644d0d3b2c480d0ecc4190f55
child 689672	978d419fe840806ccdba7bb833cf8f6421faae24
push id	87085
push user	bmo:dkeeler@mozilla.com
push date	Tue, 31 Oct 2017 21:34:47 +0000
reviewers	jcj, dveditz
bugs	1357815
milestone	58.0a1
js/xpconnect/src/xpc.msg		file \| annotate \| diff \| comparison \| revisions
security/apps/AppSignatureVerification.cpp		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/pycms.py		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/sign_app.py		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps.js		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/empty_signerInfos.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/moz.build		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/sha1_and_sha256_manifest_sha1_signature_file.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/sha1_and_sha256_manifest_sha256_signature_file.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/sha1_manifest_sha1_and_sha256_signature_file.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/sha256_manifest_sha1_and_sha256_signature_file.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/signed_app.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha1_and_sha256.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha256.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha256_manifest.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/signed_app_sha256_signature_file.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/unknown_issuer_app.zip		file \| annotate \| diff \| comparison \| revisions
security/manager/ssl/tests/unit/test_signed_apps/unsigned_app.zip		file \| annotate \| diff \| comparison \| revisions