Backed out changeset 0a5795108e0a
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 20 Feb 2018 12:21:55 -0800
changeset 404678 6bbf8dc0b86e3e229f90ad38c50f0f56da3a1a11
parent 404677 9fc0c79890cb95ea48d83cd358b19e8f90e030cc
child 404679 4560309df57b52d4946e309d12c150ddef61ef66
push id100064
push userdkeeler@mozilla.com
push dateWed, 21 Feb 2018 18:36:00 +0000
treeherdermozilla-inbound@4560309df57b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
milestone60.0a1
backs out0a5795108e0a3c751b29eaef7f772db22a9e76b5
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset 0a5795108e0a MozReview-Commit-ID: DT67yB63lSn
security/apps/AppSignatureVerification.cpp
security/manager/ssl/nsIX509CertDB.idl
security/manager/ssl/tests/unit/test_signed_dir.js
toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
--- a/security/apps/AppSignatureVerification.cpp
+++ b/security/apps/AppSignatureVerification.cpp
@@ -281,16 +281,79 @@ VerifyEntryContentDigest(nsIZipReader* z
   nsresult rv = zip->GetInputStream(aFilename, getter_AddRefs(stream));
   if (NS_FAILED(rv)) {
     return NS_ERROR_SIGNED_JAR_ENTRY_MISSING;
   }
 
   return VerifyStreamContentDigest(stream, digestFromManifest, buf);
 }
 
+// @oaram aDir       directory containing the unpacked signed archive
+// @param aFilename  path of the target file relative to aDir
+// @param digestFromManifest The digest that we're supposed to check the file's
+//                           contents against, from the manifest
+// @param buf A scratch buffer that we use for doing the I/O
+nsresult
+VerifyFileContentDigest(nsIFile* aDir, const nsAString& aFilename,
+                        const DigestWithAlgorithm& digestFromManifest,
+                        SECItem& buf)
+{
+  // Find the file corresponding to the manifest path
+  nsCOMPtr<nsIFile> file;
+  nsresult rv = aDir->Clone(getter_AddRefs(file));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  // We don't know how to handle JARs with signed directory entries.
+  // It's technically possible in the manifest but makes no sense on disk.
+  // Inside an archive we just ignore them, but here we have to treat it
+  // as an error because the signed bytes never got unpacked.
+  int32_t pos = 0;
+  int32_t slash;
+  int32_t namelen = aFilename.Length();
+  if (namelen == 0 || aFilename[namelen - 1] == '/') {
+    return NS_ERROR_SIGNED_JAR_ENTRY_INVALID;
+  }
+
+  // Append path segments one by one
+  do {
+    slash = aFilename.FindChar('/', pos);
+    int32_t segend = (slash == kNotFound) ? namelen : slash;
+    rv = file->Append(Substring(aFilename, pos, (segend - pos)));
+    if (NS_FAILED(rv)) {
+      return NS_ERROR_SIGNED_JAR_ENTRY_INVALID;
+    }
+    pos = slash + 1;
+  }  while (pos < namelen && slash != kNotFound);
+
+  bool exists;
+  rv = file->Exists(&exists);
+  if (NS_FAILED(rv) || !exists) {
+    return NS_ERROR_SIGNED_JAR_ENTRY_MISSING;
+  }
+
+  bool isDir;
+  rv = file->IsDirectory(&isDir);
+  if (NS_FAILED(rv) || isDir) {
+    // We only support signed files, not directory entries
+    return NS_ERROR_SIGNED_JAR_ENTRY_INVALID;
+  }
+
+  // Open an input stream for that file and verify it.
+  nsCOMPtr<nsIInputStream> stream;
+  rv = NS_NewLocalFileInputStream(getter_AddRefs(stream), file, -1, -1,
+                                  nsIFileInputStream::CLOSE_ON_EOF);
+  if (NS_FAILED(rv) || !stream) {
+    return NS_ERROR_SIGNED_JAR_ENTRY_MISSING;
+  }
+
+  return VerifyStreamContentDigest(stream, digestFromManifest, buf);
+}
+
 // On input, nextLineStart is the start of the current line. On output,
 // nextLineStart is the start of the next line.
 nsresult
 ReadLine(/*in/out*/ const char* & nextLineStart, /*out*/ nsCString & line,
          bool allowContinuations = true)
 {
   line.Truncate();
   size_t previousLength = 0;
@@ -1449,16 +1512,539 @@ nsNSSCertificateDB::OpenSignedAppFileAsy
   SignaturePolicy policy(policyInt);
   RefPtr<OpenSignedAppFileTask> task(new OpenSignedAppFileTask(aTrustedRoot,
                                                                aJarFile,
                                                                policy,
                                                                aCallback));
   return task->Dispatch("SignedJAR");
 }
 
+//
+// Signature verification for archives unpacked into a file structure
+//
+
+// Finds the "*.rsa" signature file in the META-INF directory and returns
+// the name. It is an error if there are none or more than one .rsa file
+nsresult
+FindSignatureFilename(nsIFile* aMetaDir,
+                      /*out*/ nsAString& aFilename)
+{
+  nsCOMPtr<nsISimpleEnumerator> entries;
+  nsresult rv = aMetaDir->GetDirectoryEntries(getter_AddRefs(entries));
+  nsCOMPtr<nsIDirectoryEnumerator> files = do_QueryInterface(entries);
+  if (NS_FAILED(rv) || !files) {
+    return NS_ERROR_SIGNED_JAR_NOT_SIGNED;
+  }
+
+  bool found = false;
+  nsCOMPtr<nsIFile> file;
+  rv = files->GetNextFile(getter_AddRefs(file));
+
+  while (NS_SUCCEEDED(rv) && file) {
+    nsAutoString leafname;
+    rv = file->GetLeafName(leafname);
+    if (NS_SUCCEEDED(rv)) {
+      if (StringEndsWith(leafname, NS_LITERAL_STRING(".rsa"))) {
+        if (!found) {
+          found = true;
+          aFilename = leafname;
+        } else {
+          // second signature file is an error
+          rv = NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+          break;
+        }
+      }
+      rv = files->GetNextFile(getter_AddRefs(file));
+    }
+  }
+
+  if (!found) {
+    rv = NS_ERROR_SIGNED_JAR_NOT_SIGNED;
+  }
+
+  files->Close();
+  return rv;
+}
+
+// Loads the signature metadata file that matches the given filename in
+// the passed-in Meta-inf directory. If bufDigest is not null then on
+// success bufDigest will contain the SHA1 or SHA256 digest of the entry
+// (depending on what aDigestAlgorithm is).
+nsresult
+LoadOneMetafile(nsIFile* aMetaDir,
+                const nsAString& aFilename,
+                /*out*/ SECItem& aBuf,
+                /*optional, in*/ SECOidTag aDigestAlgorithm = SEC_OID_SHA1,
+                /*optional, out*/ Digest* aBufDigest = nullptr)
+{
+  nsCOMPtr<nsIFile> metafile;
+  nsresult rv = aMetaDir->Clone(getter_AddRefs(metafile));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  rv = metafile->Append(aFilename);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  bool exists;
+  rv = metafile->Exists(&exists);
+  if (NS_FAILED(rv) || !exists) {
+    // we can call a missing .rsa file "unsigned" but FindSignatureFilename()
+    // already found one: missing other metadata files means a broken signature.
+    return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+  }
+
+  nsCOMPtr<nsIInputStream> stream;
+  rv = NS_NewLocalFileInputStream(getter_AddRefs(stream), metafile);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  rv = ReadStream(stream, aBuf);
+  stream->Close();
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  if (aBufDigest) {
+    rv = aBufDigest->DigestBuf(aDigestAlgorithm, aBuf.data, aBuf.len - 1);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  return NS_OK;
+}
+
+// Parses MANIFEST.MF and verifies the contents of the unpacked files
+// listed in the manifest.
+// The filenames of all entries will be returned in aMfItems. aBuf must
+// be a pre-allocated scratch buffer that is used for doing I/O.
+nsresult
+ParseMFUnpacked(const char* aFilebuf, nsIFile* aDir, SECOidTag aDigestAlgorithm,
+                /*out*/ nsTHashtable<nsStringHashKey>& aMfItems,
+                ScopedAutoSECItem& aBuf)
+{
+  const char* digestNameToFind = nullptr;
+  switch (aDigestAlgorithm) {
+    case SEC_OID_SHA256:
+      digestNameToFind = "sha256-digest";
+      break;
+    case SEC_OID_SHA1:
+      digestNameToFind = "sha1-digest";
+      break;
+    default:
+      MOZ_ASSERT_UNREACHABLE("bad argument to ParseMF");
+      return NS_ERROR_FAILURE;
+  }
+
+  const char* nextLineStart = aFilebuf;
+  nsresult rv = CheckManifestVersion(nextLineStart,
+                                     NS_LITERAL_CSTRING(JAR_MF_HEADER));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  // Skip the rest of the header section, which ends with a blank line.
+  {
+    nsAutoCString line;
+    do {
+      rv = ReadLine(nextLineStart, line);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+    } while (line.Length() > 0);
+
+    // Manifest containing no file entries is OK, though useless.
+    if (*nextLineStart == '\0') {
+      return NS_OK;
+    }
+  }
+
+  nsAutoString curItemName;
+  nsAutoCString digest;
+
+  for (;;) {
+    nsAutoCString curLine;
+    rv = ReadLine(nextLineStart, curLine);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+
+    if (curLine.Length() == 0) {
+      // end of section (blank line or end-of-file)
+
+      if (curItemName.Length() == 0) {
+        // '...Each section must start with an attribute with the name as
+        // "Name",...', so every section must have a Name attribute.
+        return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+      }
+
+      if (digest.IsEmpty()) {
+        // We require every entry to have a digest, since we require every
+        // entry to be signed and we don't allow duplicate entries.
+        return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+      }
+
+      if (aMfItems.Contains(curItemName)) {
+        // Duplicate entry
+        return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+      }
+
+      // Verify that the file's content digest matches the digest from this
+      // MF section.
+      DigestWithAlgorithm digestWithAlgorithm = { digest, aDigestAlgorithm };
+      rv = VerifyFileContentDigest(aDir, curItemName, digestWithAlgorithm,
+                                   aBuf);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+
+      aMfItems.PutEntry(curItemName);
+
+      if (*nextLineStart == '\0') {
+        // end-of-file
+        break;
+      }
+
+      // reset so we know we haven't encountered either of these for the next
+      // item yet.
+      curItemName.Truncate();
+      digest.Truncate();
+
+      continue; // skip the rest of the loop below
+    }
+
+    nsAutoCString attrName;
+    nsAutoCString attrValue;
+    rv = ParseAttribute(curLine, attrName, attrValue);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+
+    // Lines to look for:
+
+    // (1) Digest:
+    if (attrName.EqualsIgnoreCase(digestNameToFind)) {
+      if (!digest.IsEmpty()) {
+        // multiple SHA* digests in section
+        return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+      }
+
+      rv = Base64Decode(attrValue, digest);
+      if (NS_FAILED(rv)) {
+        return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+      }
+
+      continue;
+    }
+
+    // (2) Name: associates this manifest section with a file in the jar.
+    if (attrName.LowerCaseEqualsLiteral("name")) {
+      if (MOZ_UNLIKELY(curItemName.Length() > 0)) {
+        // multiple names in section
+        return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+      }
+
+      if (MOZ_UNLIKELY(attrValue.Length() == 0)) {
+        return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+      }
+
+      curItemName = NS_ConvertUTF8toUTF16(attrValue);
+
+      continue;
+    }
+
+    // (3) Magic: the only other must-understand attribute
+    if (attrName.LowerCaseEqualsLiteral("magic")) {
+      // We don't understand any magic, so we can't verify an entry that
+      // requires magic. Since we require every entry to have a valid
+      // signature, we have no choice but to reject the entry.
+      return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+    }
+
+    // unrecognized attributes must be ignored
+  }
+
+  return NS_OK;
+}
+
+// recursively check a directory tree for files not in the list of
+// verified files we found in the manifest. For each file we find
+// Check it against the files found in the manifest. If the file wasn't
+// in the manifest then it's unsigned and we can stop looking. Otherwise
+// remove it from the collection so we can check leftovers later.
+//
+// @param aDir   Directory to check
+// @param aPath  Relative path to that directory (to check against aItems)
+// @param aItems All the files found
+// @param *Filename  signature files that won't be in the manifest
+nsresult
+CheckDirForUnsignedFiles(nsIFile* aDir,
+                         const nsString& aPath,
+                         /* in/out */ nsTHashtable<nsStringHashKey>& aItems,
+                         const nsAString& sigFilename,
+                         const nsAString& sfFilename,
+                         const nsAString& mfFilename)
+{
+  nsCOMPtr<nsISimpleEnumerator> entries;
+  nsresult rv = aDir->GetDirectoryEntries(getter_AddRefs(entries));
+  nsCOMPtr<nsIDirectoryEnumerator> files = do_QueryInterface(entries);
+  if (NS_FAILED(rv) || !files) {
+    return NS_ERROR_SIGNED_JAR_ENTRY_MISSING;
+  }
+
+  bool inMeta = StringBeginsWith(aPath, NS_LITERAL_STRING(JAR_META_DIR));
+
+  while (NS_SUCCEEDED(rv)) {
+    nsCOMPtr<nsIFile> file;
+    rv = files->GetNextFile(getter_AddRefs(file));
+    if (NS_FAILED(rv) || !file) {
+      break;
+    }
+
+    nsAutoString leafname;
+    rv = file->GetLeafName(leafname);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+
+    nsAutoString curName(aPath + leafname);
+
+    bool isDir;
+    rv = file->IsDirectory(&isDir);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+
+    // if it's a directory we need to recurse
+    if (isDir) {
+      curName.AppendLiteral(u"/");
+      rv = CheckDirForUnsignedFiles(file, curName, aItems,
+                                    sigFilename, sfFilename, mfFilename);
+    } else {
+      // The files that comprise the signature mechanism are not covered by the
+      // signature.
+      //
+      // XXX: This is OK for a single signature, but doesn't work for
+      // multiple signatures because the metadata for the other signatures
+      // is not signed either.
+      if (inMeta && ( leafname == sigFilename ||
+                      leafname == sfFilename ||
+                      leafname == mfFilename )) {
+        continue;
+      }
+
+      // make sure the current file was found in the manifest
+      nsStringHashKey* item = aItems.GetEntry(curName);
+      if (!item) {
+        return NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY;
+      }
+
+      // Remove the item so we can check for leftover items later
+      aItems.RemoveEntry(item);
+    }
+  }
+  files->Close();
+  return rv;
+}
+
+/*
+ * Verify the signature of a directory structure as if it were a
+ * signed JAR file (used for unpacked JARs)
+ */
+nsresult
+VerifySignedDirectory(AppTrustedRoot aTrustedRoot,
+                      nsIFile* aDirectory,
+                      /*out, optional */ nsIX509Cert** aSignerCert)
+{
+  NS_ENSURE_ARG_POINTER(aDirectory);
+
+  if (aSignerCert) {
+    *aSignerCert = nullptr;
+  }
+
+  // Make sure there's a META-INF directory
+
+  nsCOMPtr<nsIFile> metaDir;
+  nsresult rv = aDirectory->Clone(getter_AddRefs(metaDir));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  rv = metaDir->Append(NS_LITERAL_STRING(JAR_META_DIR));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  bool exists;
+  rv = metaDir->Exists(&exists);
+  if (NS_FAILED(rv) || !exists) {
+    return NS_ERROR_SIGNED_JAR_NOT_SIGNED;
+  }
+  bool isDirectory;
+  rv = metaDir->IsDirectory(&isDirectory);
+  if (NS_FAILED(rv) || !isDirectory) {
+    return NS_ERROR_SIGNED_JAR_NOT_SIGNED;
+  }
+
+  // Find and load the Signature (RSA) file
+
+  nsAutoString sigFilename;
+  rv = FindSignatureFilename(metaDir, sigFilename);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  ScopedAutoSECItem sigBuffer;
+  rv = LoadOneMetafile(metaDir, sigFilename, sigBuffer);
+  if (NS_FAILED(rv)) {
+    return NS_ERROR_SIGNED_JAR_NOT_SIGNED;
+  }
+
+  // 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;
+  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;
+  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
+
+  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, digestToUse,
+                       &mfCalculatedDigest);
+  if (NS_FAILED(rv)) {
+    return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
+  }
+
+  nsDependentCSubstring calculatedDigest(
+    DigestToDependentString(mfCalculatedDigest));
+  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, 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,
+                                sigFilename, sfFilename, mfFilename);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  // We verified that every entry that we require to be signed is signed. But,
+  // were there any missing entries--that is, entries that are mentioned in the
+  // manifest but missing from the directory tree? (There shouldn't be given
+  // ParseMFUnpacked() checking them all, but it's a cheap sanity check.)
+  if (items.Count() != 0) {
+    return NS_ERROR_SIGNED_JAR_ENTRY_MISSING;
+  }
+
+  // Return the signer's certificate to the reader if they want it.
+  // XXX: We should return an nsIX509CertList with the whole validated chain.
+  if (aSignerCert) {
+    CERTCertListNode* signerCertNode = CERT_LIST_HEAD(builtChain);
+    if (!signerCertNode || CERT_LIST_END(signerCertNode, builtChain) ||
+        !signerCertNode->cert) {
+      return NS_ERROR_FAILURE;
+    }
+    nsCOMPtr<nsIX509Cert> signerCert =
+      nsNSSCertificate::Create(signerCertNode->cert);
+    NS_ENSURE_TRUE(signerCert, NS_ERROR_OUT_OF_MEMORY);
+    signerCert.forget(aSignerCert);
+  }
+
+  return NS_OK;
+}
+
+class VerifySignedDirectoryTask final : public CryptoTask
+{
+public:
+  VerifySignedDirectoryTask(AppTrustedRoot aTrustedRoot, nsIFile* aUnpackedJar,
+                            nsIVerifySignedDirectoryCallback* aCallback)
+    : mTrustedRoot(aTrustedRoot)
+    , mDirectory(aUnpackedJar)
+    , mCallback(new nsMainThreadPtrHolder<nsIVerifySignedDirectoryCallback>(
+        "VerifySignedDirectoryTask::mCallback", aCallback))
+  {
+  }
+
+private:
+  virtual nsresult CalculateResult() override
+  {
+    return VerifySignedDirectory(mTrustedRoot,
+                                 mDirectory,
+                                 getter_AddRefs(mSignerCert));
+  }
+
+  // This class doesn't directly hold NSS resources so there's nothing that
+  // needs to be released
+  virtual void ReleaseNSSResources() override { }
+
+  virtual void CallCallback(nsresult rv) override
+  {
+    (void) mCallback->VerifySignedDirectoryFinished(rv, mSignerCert);
+  }
+
+  const AppTrustedRoot mTrustedRoot;
+  const nsCOMPtr<nsIFile> mDirectory;
+  nsMainThreadPtrHandle<nsIVerifySignedDirectoryCallback> mCallback;
+  nsCOMPtr<nsIX509Cert> mSignerCert; // out
+};
+
 NS_IMETHODIMP
-nsNSSCertificateDB::VerifySignedDirectoryAsync(AppTrustedRoot, nsIFile*,
+nsNSSCertificateDB::VerifySignedDirectoryAsync(
+  AppTrustedRoot aTrustedRoot, nsIFile* aUnpackedJar,
   nsIVerifySignedDirectoryCallback* aCallback)
 {
+  NS_ENSURE_ARG_POINTER(aUnpackedJar);
   NS_ENSURE_ARG_POINTER(aCallback);
-  return aCallback->VerifySignedDirectoryFinished(
-    NS_ERROR_SIGNED_JAR_NOT_SIGNED, nullptr);
+  RefPtr<VerifySignedDirectoryTask> task(new VerifySignedDirectoryTask(aTrustedRoot,
+                                                                       aUnpackedJar,
+                                                                       aCallback));
+  return task->Dispatch("UnpackedJar");
 }
--- a/security/manager/ssl/nsIX509CertDB.idl
+++ b/security/manager/ssl/nsIX509CertDB.idl
@@ -23,18 +23,16 @@ typedef uint32_t AppTrustedRoot;
 [scriptable, function, uuid(fc2b60e5-9a07-47c2-a2cd-b83b68a660ac)]
 interface nsIOpenSignedAppFileCallback : nsISupports
 {
   void openSignedAppFileFinished(in nsresult rv,
                                  in nsIZipReader aZipReader,
                                  in nsIX509Cert aSignerCert);
 };
 
-// Only relevant while we transition away from legacy add-ons. rv will always be
-// NS_ERROR_SIGNED_JAR_NOT_SIGNED. aSignerCert will always be null.
 [scriptable, function, uuid(d5f97827-622a-488f-be08-d850432ac8ec)]
 interface nsIVerifySignedDirectoryCallback : nsISupports
 {
   void verifySignedDirectoryFinished(in nsresult rv,
                                      in nsIX509Cert aSignerCert);
 };
 
 /**
@@ -265,19 +263,26 @@ interface nsIX509CertDB : nsISupports {
    */
   const AppTrustedRoot DeveloperImportedRoot = 10;
   [must_use]
   void openSignedAppFileAsync(in AppTrustedRoot trustedRoot,
                               in nsIFile aJarFile,
                               in nsIOpenSignedAppFileCallback callback);
 
   /**
-   * Vestigial implementation of verifying signed unpacked add-ons. trustedRoot
-   * and aUnpackedDir are ignored. The callback is always called with
-   * NS_ERROR_SIGNED_JAR_NOT_SIGNED and a null signer cert.
+   *  Verifies the signature on a directory representing an unpacked signed
+   *  JAR file. To be considered valid, there must be exactly one signature
+   *  on the directory structure and that signature must have signed every
+   *  entry. Further, the signature must come from a certificate that
+   *  is trusted for code signing.
+   *
+   *  On success NS_OK and the trusted certificate that signed the
+   *  unpacked JAR are returned.
+   *
+   *  On failure, an error code is returned.
    */
   [must_use]
   void verifySignedDirectoryAsync(in AppTrustedRoot trustedRoot,
                                   in nsIFile aUnpackedDir,
                                   in nsIVerifySignedDirectoryCallback callback);
 
   /*
    * Add a cert to a cert DB from a binary string.
--- a/security/manager/ssl/tests/unit/test_signed_dir.js
+++ b/security/manager/ssl/tests/unit/test_signed_dir.js
@@ -1,14 +1,15 @@
 // Any copyright is dedicated to the Public Domain.
 // http://creativecommons.org/publicdomain/zero/1.0/
 "use strict";
 
-// Tests that signed extensions extracted/unpacked into a directory do not pass
-// signature verification, because that's no longer supported.
+// Tests that signed extensions extracted/unpacked into a directory work pass
+// verification when non-tampered, and fail verification when tampered via
+// various means.
 
 const { ZipUtils } = ChromeUtils.import("resource://gre/modules/ZipUtils.jsm", {});
 
 do_get_profile(); // must be called before getting nsIX509CertDB
 const certdb = Cc["@mozilla.org/security/x509certdb;1"]
                  .getService(Ci.nsIX509CertDB);
 
 /**
@@ -22,47 +23,179 @@ var gSignedXPI =
 /**
  * The directory that the test extension will be extracted to.
  * @type nsIFile
  */
 var gTarget = FileUtils.getDir("TmpD", ["test_signed_dir"]);
 gTarget.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
 
 /**
+ * Each property below is optional. Defining none of them means "don't tamper".
+ *
+ * @typedef {TamperInstructions}
+ * @type Object
+ * @property {String[][]} copy
+ *           Format: [[path,newname], [path2,newname2], ...]
+ *           Copy the file located at |path| and name it |newname|.
+ * @property {String[]} delete
+ *           List of paths to files to delete.
+ * @property {String[]} corrupt
+ *           List of paths to files to corrupt.
+ */
+
+/**
  * Extracts the signed XPI into a directory, and tampers the files in that
  * directory if instructed.
  *
+ * @param {TamperInstructions} tamper
+ *        Instructions on whether to tamper any files, and if so, how.
  * @returns {nsIFile}
  *          The directory where the XPI was extracted to.
  */
-function prepare() {
+function prepare(tamper) {
   ZipUtils.extractFiles(gSignedXPI, gTarget);
+
+  // copy files
+  if (tamper.copy) {
+    tamper.copy.forEach(i => {
+      let f = gTarget.clone();
+      i[0].split("/").forEach(seg => { f.append(seg); });
+      f.copyTo(null, i[1]);
+    });
+  }
+
+  // delete files
+  if (tamper.delete) {
+    tamper.delete.forEach(i => {
+      let f = gTarget.clone();
+      i.split("/").forEach(seg => { f.append(seg); });
+      f.remove(true);
+    });
+  }
+
+  // corrupt files
+  if (tamper.corrupt) {
+    tamper.corrupt.forEach(i => {
+      let f = gTarget.clone();
+      i.split("/").forEach(seg => { f.append(seg); });
+      let s = FileUtils.openFileOutputStream(f, FileUtils.MODE_WRONLY);
+      const str = "Kilroy was here";
+      s.write(str, str.length);
+      s.close();
+    });
+  }
+
   return gTarget;
 }
 
 function checkResult(expectedRv, dir, resolve) {
   return function verifySignedDirCallback(rv, aSignerCert) {
     equal(rv, expectedRv, "Actual and expected return value should match");
     equal(aSignerCert != null, Components.isSuccessCode(expectedRv),
           "expecting certificate:");
     dir.remove(true);
     resolve();
   };
 }
 
-function verifyDirAsync(expectedRv) {
-  let targetDir = prepare();
+function verifyDirAsync(expectedRv, tamper) {
+  let targetDir = prepare(tamper);
   return new Promise((resolve, reject) => {
     certdb.verifySignedDirectoryAsync(
       Ci.nsIX509CertDB.AddonsPublicRoot, targetDir,
       checkResult(expectedRv, targetDir, resolve));
   });
 }
 
-add_task(async function testAPIFails() {
-  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED);
+//
+// the tests
+//
+add_task(async function testValid() {
+  await verifyDirAsync(Cr.NS_OK, {} /* no tampering */);
+});
+
+add_task(async function testNoMetaDir() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED,
+                       {delete: ["META-INF"]});
+});
+
+add_task(async function testEmptyMetaDir() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED,
+                       {delete: ["META-INF/mozilla.rsa",
+                                 "META-INF/mozilla.sf",
+                                 "META-INF/manifest.mf"]});
+});
+
+add_task(async function testTwoRSAFiles() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID,
+                       {copy: [["META-INF/mozilla.rsa", "extra.rsa"]]});
+});
+
+add_task(async function testCorruptRSAFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID,
+                       {corrupt: ["META-INF/mozilla.rsa"]});
+});
+
+add_task(async function testMissingSFFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID,
+                       {delete: ["META-INF/mozilla.sf"]});
+});
+
+add_task(async function testCorruptSFFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID,
+                       {corrupt: ["META-INF/mozilla.sf"]});
+});
+
+add_task(async function testExtraInvalidSFFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY,
+                       {copy: [["META-INF/mozilla.rsa", "extra.sf"]]});
+});
+
+add_task(async function testExtraValidSFFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY,
+                       {copy: [["META-INF/mozilla.sf", "extra.sf"]]});
+});
+
+add_task(async function testMissingManifest() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID,
+                       {delete: ["META-INF/manifest.mf"]});
+});
+
+add_task(async function testCorruptManifest() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MANIFEST_INVALID,
+                       {corrupt: ["META-INF/manifest.mf"]});
+});
+
+add_task(async function testMissingFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_ENTRY_MISSING,
+                       {delete: ["bootstrap.js"]});
+});
+
+add_task(async function testCorruptFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MODIFIED_ENTRY,
+                       {corrupt: ["bootstrap.js"]});
+});
+
+add_task(async function testExtraFile() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY,
+                       {copy: [["bootstrap.js", "extra"]]});
+});
+
+add_task(async function testMissingFileInDir() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_ENTRY_MISSING,
+                       {delete: ["lib/ui.js"]});
+});
+
+add_task(async function testCorruptFileInDir() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_MODIFIED_ENTRY,
+                       {corrupt: ["lib/ui.js"]});
+});
+
+add_task(async function testExtraFileInDir() {
+  await verifyDirAsync(Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY,
+                       {copy: [["lib/ui.js", "extra"]]});
 });
 
 registerCleanupFunction(function() {
   if (gTarget.exists()) {
     gTarget.remove(true);
   }
 });
--- a/toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
+++ b/toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
@@ -248,16 +248,18 @@ skip-if = os == "android"
 fail-if = os == "android"
 tags = blocklist
 [test_pref_properties.js]
 [test_registry.js]
 [test_safemode.js]
 [test_signed_updatepref.js]
 run-if = addon_signing
 skip-if = require_signing || !allow_legacy_extensions
+[test_signed_verify.js]
+run-if = addon_signing
 [test_signed_inject.js]
 run-if = addon_signing
 # Bug 1394122
 skip-if = true
 [test_signed_install.js]
 run-if = addon_signing
 run-sequentially = Uses hardcoded ports in xpi files.
 [test_signed_long.js]
--- a/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
+++ b/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
@@ -54,12 +54,10 @@ tags = webextensions
 [test_delay_update_webextension.js]
 skip-if = appname == "thunderbird"
 tags = webextensions
 [test_dependencies.js]
 [test_system_delay_update.js]
 [test_schema_change.js]
 [test_registerchrome.js]
 [test_system_allowed.js]
-[test_signed_verify.js]
-run-if = addon_signing
 
 [include:xpcshell-shared.ini]