Closed Bug 1038072 Opened 10 years ago Closed 9 years ago

signature verification for unpacked add-ons

Categories

(Core Graveyard :: Installer: XPInstall Engine, enhancement, P1)

enhancement

Tracking

(firefox40 verified, firefox41 verified)

VERIFIED FIXED
mozilla40
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: dveditz, Assigned: dveditz)

References

Details

(Whiteboard: [fxsearch][searchhijacking])

Attachments

(2 files, 4 obsolete files)

Some add-ons are installed "unpacked" for various reasons (binary components, searchplugins, ??). We will have to write code to
 * preserve the signature information when they are unpacked
 * modify the JAR signature verification code to work on a
   directory sub-tree instead of an archive.
Blocks: 810149
Blocks: 1047247
Assignee: nobody → dveditz
Flags: qe-verify?
Flags: firefox-backlog+
Points: --- → 5
Blocks: 1149657
No longer blocks: signed-addons
Blocks: 1123918
No longer blocks: 1149657
[Tracking Requested - why for this release]:

First two stages of add-ons signing work are targeted at Firefox 39.
Whiteboard: [fxsearch][searchhijacking]
Priority: -- → P1
How are things going here Dan? Is this going to be complete by the end of next week?
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Attachment #8600738 - Flags: review?(dkeeler)
David: this patch adds support for verifying the signatures of "unpacked addons" to the security/apps. It will be used in XPIProvider.jsm in the same way as the more normal openSignedAppFileAsync() used for .xpi files that are kept in that form.

(your account name says to use needinfo? so I am, but I don't know if that's redundant given a r?)
Flags: needinfo?(dkeeler)
Comment on attachment 8600739 [details] [diff] [review]
Part 2: call directory signature verification from XPIProvider.jsm

Review of attachment 8600739 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but please also move the references to the test_signed_* tests from toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini to toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini. That should make the existing tests run for unpacked add-ons too.
Attachment #8600739 - Flags: review?(dtownsend) → review+
Comment on attachment 8600738 [details] [diff] [review]
certdb support for unpacked signed archives

Review of attachment 8600738 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I noted some style nits and suggested some additional tests. In particular, I think we need to specifically handle things like '..' being in entry paths in manifest files. Also, it would be good to test packages that have an actual directory hierarchy.

::: security/apps/AppSignatureVerification.cpp
@@ +162,5 @@
>  //            already been allocated. The size of this buffer is the unit
>  //            size of our I/O.
>  nsresult
> +VerifyStreamContentDigest(nsIInputStream* stream,
> +                          const SECItem & digestFromManifest, SECItem & buf)

nit: while we're touching this line, let's move the &s to the left

@@ +223,5 @@
>  }
>  
> +nsresult
> +VerifyEntryContentDigest(nsIZipReader * zip, const nsACString & aFilename,
> +                         const SECItem & digestFromManifest, SECItem & buf)

nit: all &/* to the left

@@ +241,5 @@
> +//                           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 SECItem & digestFromManifest, SECItem & buf)

nit: same

@@ +246,5 @@
> +{
> +  // Find the file corresponding to the manifest path
> +  nsCOMPtr<nsIFile> file;
> +  nsresult rv = aDir->Clone(getter_AddRefs(file));
> +  NS_ENSURE_SUCCESS(rv, rv);

nit: let's do

if (NS_FAILED(rv)) {
  return rv;
}

instead of NS_ENSURE_SUCCESS for new code

@@ +255,5 @@
> +  // 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] == '/') {

What if an entry were a directory but wasn't in the manifest with a trailing slash? Shouldn't we specifically check that the resulting nsIFile isn't a directory? (Or does NS_NewLocalFileInputStream fail if given a path to a directory?)

@@ +259,5 @@
> +  if (namelen == 0 || aFilename[namelen - 1] == '/') {
> +    return NS_ERROR_SIGNED_JAR_ENTRY_INVALID;
> +  }
> +
> +  // Append path segments one by one

Do we not have a utility function for this?

@@ +263,5 @@
> +  // 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)));

What about things like ".."?

@@ +265,5 @@
> +    slash = aFilename.FindChar('/', pos);
> +    int32_t segend = (slash == kNotFound) ? namelen : slash;
> +    rv = file->Append(Substring(aFilename, pos, (segend - pos)));
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_SIGNED_JAR_ENTRY_INVALID);
> +    pos = slash+1;

nit: space around operators: slash + 1

@@ +278,5 @@
> +  }
> +  return VerifyStreamContentDigest(stream, digestFromManifest, buf);
> +}
> +
> +

nit: unnecessary extra blank line

@@ +1013,5 @@
> +// 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)

nit: */& to the left

@@ +1025,5 @@
> +
> +  bool found = false;
> +  nsCOMPtr<nsIFile> file;
> +  rv = files->GetNextFile(getter_AddRefs(file));
> +  

nit: trailing spaces

@@ +1029,5 @@
> +  
> +  while (NS_SUCCEEDED(rv) && file) {
> +    nsAutoString leafname;
> +    rv = file->GetLeafName(leafname);
> +    if (NS_SUCCEEDED(rv)) {

If we check/return early on errors, we can do without the extra scope here. Not a huge deal in this case, though.

@@ +1035,5 @@
> +        if (!found) {
> +          found = true;
> +          aFilename = leafname;
> +        }
> +        else {

nit: } else { all on one line

@@ +1055,5 @@
> +}
> +
> +// 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 SHA-1 digeset of the entry.

typo: "digest" (after SHA-1)

@@ +1060,5 @@
> +nsresult
> +LoadOneMetafile(nsIFile * aMetaDir,
> +                const nsAString & aFilename,
> +                /*out*/ SECItem & aBuf,
> +                /*optional, out*/ Digest * aBufDigest)

nit: */& to the left

@@ +1073,5 @@
> +  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 the others means a broken signature.

Maybe "missing other metadata files means..."

@@ +1095,5 @@
> +}
> +
> +// Parses MANIFEST.MF and verifies the contents of the unpacked files
> +// listed in the manifest.
> +// The filenames of all entries will be returned in mfItems. buf must

nit: aMfItems and aBuf, looks like

@@ +1100,5 @@
> +// be a pre-allocated scratch buffer that is used for doing I/O.
> +nsresult
> +ParseMFUnpacked(const char* aFilebuf, nsIFile * aDir,
> +                /*out*/ nsTHashtable<nsStringHashKey> & aMfItems,
> +                ScopedAutoSECItem & aBuf)

nit: */& to the left

@@ +1158,5 @@
> +
> +      // Verify that the file's content digest matches the digest from this
> +      // MF section.
> +      rv = VerifyFileContentDigest(aDir, curItemName, digest, aBuf);
> +      if (NS_FAILED(rv))

nit: braces around conditionals

@@ +1163,5 @@
> +        return rv;
> +
> +      aMfItems.PutEntry(curItemName);
> +
> +      if (*nextLineStart == '\0') // end-of-file

nit: braces around conditionals

@@ +1185,5 @@
> +    // Lines to look for:
> +
> +    // (1) Digest:
> +    if (attrName.LowerCaseEqualsLiteral("sha1-digest"))
> +    {

nit: { on the previous line

@@ +1186,5 @@
> +
> +    // (1) Digest:
> +    if (attrName.LowerCaseEqualsLiteral("sha1-digest"))
> +    {
> +      if (digest.len > 0) // multiple SHA1 digests in section

nit: braces around conditionals

@@ +1254,5 @@
> +
> +  while (NS_SUCCEEDED(rv)) {
> +    nsCOMPtr<nsIFile> file;
> +    rv = files->GetNextFile(getter_AddRefs(file));
> +    if (NS_FAILED(rv) || !file)

nit: braces around conditionals

@@ +1273,5 @@
> +      curName.Append(NS_LITERAL_STRING("/"));
> +      rv = CheckDirForUnsignedFiles(file, curName, aItems,
> +                                    sigFilename, sfFilename, mfFilename);
> +    }
> +    else {

nit: } else { all on one line

@@ +1304,5 @@
> +/*
> + * Verify the signature of a directory structure as if it were a
> + * signed JAR file (used for unpacked JARs)
> + */
> +NS_IMETHODIMP

nit: nsresult

@@ +1322,5 @@
> +  nsresult rv = aDirectory->Clone(getter_AddRefs(metaDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = metaDir->Append(NS_LITERAL_STRING(JAR_META_DIR));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  

nit: trailing spaces

@@ +1324,5 @@
> +  rv = metaDir->Append(NS_LITERAL_STRING(JAR_META_DIR));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  bool exists;
> +  bool isDirectory;

nit: let's put isDirectory just above where it's used

@@ +1339,5 @@
> +
> +  nsAutoString sigFilename;
> +  rv = FindSignatureFilename(metaDir, sigFilename);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  

nit: trailing spaces

@@ +1349,5 @@
> +
> +  // 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)

nit: no space after '(', but spaces after ',' and between operators ('...Length() - 3')

@@ +1382,5 @@
> +    return NS_ERROR_SIGNED_JAR_MANIFEST_INVALID;
> +  }
> +
> +  // Parse manifest and verify signed hash of all listed files
> +  

nit: trailing spaces

@@ +1440,5 @@
> +                                 mDirectory,
> +                                 getter_AddRefs(mSignerCert));
> +  }
> +
> +  // nsNSSCertificate implements nsNSSShutdownObject, so there's nothing that

Might be more accurate to say this class doesn't directly hold NSS resources, so there's nothing to release.

::: security/manager/ssl/tests/unit/test_signed_dir.js
@@ +12,5 @@
> +
> +// tamper data structure, each element is optional
> +// { copy:    [[path,newname], [path2,newname2], ...],
> +//   delete:  [path, path2, ...],
> +//   corrupt: [path, path2, ...] 

nit: trailing space

@@ +17,5 @@
> +// }
> +
> +function prepare(tamper) {
> +  ZipUtils.extractFiles(gSignedXPI, gTarget);
> +  

nit: trailing spaces here (and a few more 10 lines later, and then 20 lines after that)

@@ +43,5 @@
> +      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);

nit: space after ','

@@ +115,5 @@
> +});
> +
> +add_test(function() {
> +  verifyDirAsync("'extra .sf file'", Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY,
> +                 {copy: [["META-INF/zigbert.rsa","extra.sf"]]});

What about a case where there's an extra .sf file that has a correct entry in the manifest?

@@ +149,5 @@
> +    gTarget.remove(true);
> +});
> +
> +function run_test() {
> +  run_next_test();

We should also have tests where the manifest has paths with ".." in them and the package has a directory structure (and maybe some empty directories, or instances of directories being in the manifest).
Attachment #8600738 - Flags: review?(dkeeler) → review+
I assume this is what you meant, but I'd like an explicit r+ on it in case it's not.
Attachment #8600739 - Attachment is obsolete: true
Attachment #8601228 - Flags: review?(dtownsend)
Comment on attachment 8601228 [details] [diff] [review]
Part 2: call directory signature verification from XPIProvider.jsm

Review of attachment 8601228 [details] [diff] [review]:
-----------------------------------------------------------------

Yep
Attachment #8601228 - Flags: review?(dtownsend) → review+
Addressed the nits, swapped in a different test file that has directories.

> What if an entry were a directory but wasn't in the manifest with a
> trailing slash? Shouldn't we specifically check that the resulting
> nsIFile isn't a directory? (Or does NS_NewLocalFileInputStream fail
> if given a path to a directory?)

I added this check (I don't think NS_NewLocalFileInputStream will fail).

> > +  // Append path segments one by one
>
> Do we not have a utility function for this?

Not that I found. There's AppendRelative() but that requires the separators to be the one used by the underlying OS which seems less than useful. Easier to segment the path than try to have platform-specific versions.

> > +    rv = file->Append(Substring(aFilename, pos, (segend - pos)));
>
> What about things like ".."? 

Append("..") will fail on windows, arguably it ought to on Linux as well (but I don't think it does). If it ends up referencing something inside the archive/directory structure no harm done. If it's reaching outside it's unlikely that our signing machine and the target machine will have identical copies of the same file in the same location, but what if it does? Addon unpacking will choke on a path with .. in it, and if it's sideloaded why would an attacker want to reference an external file in the manifest? 

We could check to be extra paranoid, but I think it just clutters up the code.
Attachment #8600738 - Attachment is obsolete: true
Attachment #8601274 - Flags: review?(dkeeler)
Attachment #8601274 - Attachment is patch: true
Blocks: 1149657
Comment on attachment 8601274 [details] [diff] [review]
certdb support for unpacked signed archives (v2)

Review of attachment 8601274 [details] [diff] [review]:
-----------------------------------------------------------------

Minor drive-by nits...

::: security/apps/AppSignatureVerification.cpp
@@ +19,5 @@
>  #include "nsIFile.h"
>  #include "nsIFileStreams.h"
>  #include "nsIInputStream.h"
>  #include "nsIStringEnumerator.h"
> +#include "nsIDirectoryEnumerator.h"

Would be nice to sort this - the other includes are pretty well sorted.

@@ +234,5 @@
> +
> +  return VerifyStreamContentDigest(stream, digestFromManifest, buf);
> +}
> +
> +// @oaram aDir       directory containing the unpacked signed archive

s/oaram/param/
Comment on attachment 8601274 [details] [diff] [review]
certdb support for unpacked signed archives (v2)

Review of attachment 8601274 [details] [diff] [review]:
-----------------------------------------------------------------

Ok - looks good. I think there are a couple more test cases we should cover, though (see comments).

::: security/apps/AppSignatureVerification.cpp
@@ +1114,5 @@
> +// 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,

Huh - I just realized much of this code is is copied/modified from the current implementation. It would be nice to factor out the common parts, but at this point I guess that can be a follow-up bug.

@@ +1316,5 @@
> +        continue;
> +      }
> +
> +      // make sure the current file was found in the manifest
> +      nsStringHashKey *item = aItems.GetEntry(curName);

nit: * to the left

::: security/manager/ssl/tests/unit/test_signed_dir.js
@@ +75,5 @@
> +//
> +// the tests
> +//
> +
> +add_test(function () {

nit: no space after function to be consistent in this file

@@ +78,5 @@
> +
> +add_test(function () {
> +  verifyDirAsync("'valid'", Cr.NS_OK,
> +                 {}  /* no tampering */
> +                );

nit: I imagine this could all be on one line, but it's not a big deal

@@ +120,5 @@
> +});
> +
> +add_test(function() {
> +  verifyDirAsync("'extra .sf file (valid)'", Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY,
> +                 {copy: [["META-INF/mozilla.sf","extra.sf"]]});

What about a package that had a manifest entry for some file "META-INF/extra.sf"? Would this be valid? (given that it's properly signed)

@@ +160,5 @@
> +});
> +
> +add_test(function() {
> +  verifyDirAsync("'extra file in dir'", Cr.NS_ERROR_SIGNED_JAR_UNSIGNED_ENTRY,
> +                 {copy: [["content/options.xul","extra"]]});

What if the manifest had an entry for just "content"?

@@ +164,5 @@
> +                 {copy: [["content/options.xul","extra"]]});
> +});
> +
> +do_register_cleanup(function() {
> +  if (gTarget.exists())

nit: braces around conditional
Attachment #8601274 - Flags: review?(dkeeler) → review+
Fixing nits and carrying over keeler's r+

> What about a package that had a manifest entry for some file
> "META-INF/extra.sf"? Would this be valid? (given that it's properly signed)

Our signing tool wouldn't do that, but if it were included in the manifest it would be a valid-but-useless file in the archive. Because the base part of the name does not match the .rsa file it will be ignored as a signature part.

NB: this is different from the zipped version, which uses a regexp to file the .sf and declares having two of them to be a disaster. But our zip signing is a subset of Java archive signing and having the .rsa and .sf files name-match is critical for distinguishing which pairs go together in the multiple signature case. Our current tools and the old tools all create matched pairs, we should just enforce that (and that's what I've done).

> > +             {copy: [["content/options.xul","extra"]]});
>
> What if the manifest had an entry for just "content"?

The test file does have a _zip_ entry for "content/", but it (correctly) is not in the manifest. If we found one my code would declare it an invalid entry -- either because Append() would return an error if the '/' were included or because VerifyFileContentDigest would return an error when it finds that "content" is a directory.  

Our signing tools will not sign a directory, and if other people write tools in the future they should follow suit.
Attachment #8601872 - Flags: review+
Attachment #8601274 - Attachment is obsolete: true
I will need someone to check in Part 1 and Part 2 for me.
Keywords: checkin-needed
(In reply to Daniel Veditz [:dveditz] from comment #14)
> I will need someone to check in Part 1 and Part 2 for me.

done :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a02ea85607a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/266853fe8965
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #18)
> (See
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=266853fe8965 for the test failures.)

also and we get a try run before requesting landing again ? :)
Comment on attachment 8601228 [details] [diff] [review]
Part 2: call directory signature verification from XPIProvider.jsm

Review of attachment 8601228 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +1364,5 @@
> +  if (!REQUIRE_SIGNING && Preferences.get(PREF_XPI_SIGNATURES_DEV_ROOT, false))
> +    root = Ci.nsIX509CertDB.AddonsStageRoot;
> +
> +  return new Promise(resolve => {
> +    certDB.verifySignedDirectoryAsync(root, aFile, (aRv, aCert) => {

Oops. aDir not aFile!
fixed incorrect variable name, mochitests and reftests that were failing before now run fine (tested locally).
Attachment #8601228 - Attachment is obsolete: true
Attachment #8602833 - Flags: review+
Iteration: --- → 40.3 - 11 May
Flags: qe-verify? → qe-verify+
QA Contact: vasilica.mihasca
Does this bug need manual testing?
Flags: needinfo?(dtownsend)
A brief check would be good. If you take a fully signed add-on and extract its files to the directory <profile>/extensions/<id> then start Firefox it should show up as a sideloaded add-on and in the add-ons manager shouldn't show any signing warnings.
Flags: needinfo?(dtownsend)
Verified fixed on Firefox 41.0a1 (2015-06-16) and Firefox 40.0a2 (2015-06-16) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
Depends on: 1267012
You need to log in before you can comment on or make changes to this bug.