bug 1357815 - 2/4: refactor away unnecessary parts of certificate verification in add-on signature verification r?jcj draft
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 24 Oct 2017 13:32:02 -0700
changeset 689670 f6120ea5356f123644d0d3b2c480d0ecc4190f55
parent 689669 b71cd89080bb493a59d076568dd666a15bd8a572
child 689671 0ce20c460403e496d0672ccc7702365519b9df46
push id87085
push userbmo:dkeeler@mozilla.com
push dateTue, 31 Oct 2017 21:34:47 +0000
reviewersjcj
bugs1357815
milestone58.0a1
bug 1357815 - 2/4: refactor away unnecessary parts of certificate verification in add-on signature verification r?jcj MozReview-Commit-ID: 4JKWIZ0wnuO
security/apps/AppSignatureVerification.cpp
--- a/security/apps/AppSignatureVerification.cpp
+++ b/security/apps/AppSignatureVerification.cpp
@@ -699,33 +699,26 @@ ParseMF(const char* filebuf, nsIZipReade
     }
 
     // unrecognized attributes must be ignored
   }
 
   return NS_OK;
 }
 
-struct VerifyCertificateContext {
-  AppTrustedRoot trustedRoot;
-  UniqueCERTCertList& builtChain;
-};
-
 nsresult
-VerifyCertificate(CERTCertificate* signerCert, void* voidContext, void* pinArg)
+VerifyCertificate(CERTCertificate* signerCert, AppTrustedRoot trustedRoot,
+                  /*out*/ UniqueCERTCertList& builtChain)
 {
-  // TODO: null pinArg is tolerated.
-  if (NS_WARN_IF(!signerCert) || NS_WARN_IF(!voidContext)) {
+  if (NS_WARN_IF(!signerCert)) {
     return NS_ERROR_INVALID_ARG;
   }
-  const VerifyCertificateContext& context =
-    *static_cast<const VerifyCertificateContext*>(voidContext);
-
-  AppTrustDomain trustDomain(context.builtChain, pinArg);
-  nsresult rv = trustDomain.SetTrustedRoot(context.trustedRoot);
+  // TODO: pinArg is null.
+  AppTrustDomain trustDomain(builtChain, nullptr);
+  nsresult rv = trustDomain.SetTrustedRoot(trustedRoot);
   if (NS_FAILED(rv)) {
     return rv;
   }
   Input certDER;
   mozilla::pkix::Result result = certDER.Init(signerCert->derCert.data,
                                               signerCert->derCert.len);
   if (result != Success) {
     return mozilla::psm::GetXPCOMFromNSSError(MapResultToPRErrorCode(result));
@@ -761,28 +754,25 @@ VerifyCertificate(CERTCertificate* signe
   if (result != Success) {
     return mozilla::psm::GetXPCOMFromNSSError(MapResultToPRErrorCode(result));
   }
 
   return NS_OK;
 }
 
 nsresult
-VerifyCMSDetachedSignatureIncludingCertificate(
-  const SECItem& buffer, const SECItem& detachedDigest,
-  nsresult (*verifyCertificate)(CERTCertificate* cert, void* context,
-                                void* pinArg),
-  void* verifyCertificateContext, void* pinArg,
-  const nsNSSShutDownPreventionLock& /*proofOfLock*/)
+VerifySignature(AppTrustedRoot trustedRoot, const SECItem& buffer,
+                const SECItem& detachedDigest,
+                /*out*/ UniqueCERTCertList& builtChain)
 {
-  // XXX: missing pinArg is tolerated.
+  // 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) ||
-      (!verifyCertificate) ||
-      NS_WARN_IF(!verifyCertificateContext)) {
+      NS_WARN_IF(!detachedDigest.data && detachedDigest.len > 0)) {
     return NS_ERROR_INVALID_ARG;
   }
 
   UniqueNSSCMSMessage
     cmsMsg(NSS_CMSMessage_CreateFromDER(const_cast<SECItem*>(&buffer), nullptr,
                                         nullptr, nullptr, nullptr, nullptr,
                                         nullptr));
   if (!cmsMsg) {
@@ -853,17 +843,17 @@ VerifyCMSDetachedSignatureIncludingCerti
     return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
   }
   CERTCertificate* signerCert =
     NSS_CMSSignerInfo_GetSigningCertificate(signer, CERT_GetDefaultCertDB());
   if (!signerCert) {
     return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
   }
 
-  nsresult rv = verifyCertificate(signerCert, verifyCertificateContext, pinArg);
+  nsresult rv = VerifyCertificate(signerCert, trustedRoot, builtChain);
   if (NS_WARN_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) {
@@ -871,35 +861,16 @@ VerifyCMSDetachedSignatureIncludingCerti
   }
 
   return MapSECStatus(NSS_CMSSignerInfo_Verify(signer,
                          const_cast<SECItem*>(&detachedDigest),
                          &contentTypeOidData->oid));
 }
 
 nsresult
-VerifySignature(AppTrustedRoot trustedRoot, const SECItem& buffer,
-                const SECItem& detachedDigest,
-                /*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.
-  // We acquire a nsNSSShutDownPreventionLock here solely to prove we did to
-  // VerifyCMSDetachedSignatureIncludingCertificate().
-  nsNSSShutDownPreventionLock locker;
-  VerifyCertificateContext context = { trustedRoot, builtChain };
-  // XXX: missing pinArg
-  return VerifyCMSDetachedSignatureIncludingCertificate(buffer, detachedDigest,
-                                                        VerifyCertificate,
-                                                        &context, nullptr,
-                                                        locker);
-}
-
-nsresult
 OpenSignedAppFile(AppTrustedRoot aTrustedRoot, nsIFile* aJarFile,
                   /*out, optional */ nsIZipReader** aZipReader,
                   /*out, optional */ nsIX509Cert** aSignerCert)
 {
   NS_ENSURE_ARG_POINTER(aJarFile);
 
   if (aZipReader) {
     *aZipReader = nullptr;