1038072 - signature verification for unpacked add-ons

Status: VERIFIED FIXED

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

Description

[Daniel Veditz [:dveditz]]

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.

Comment 1

[Dave Townsend [:mossop]]

[Tracking Requested - why for this release]:

First two stages of add-ons signing work are targeted at Firefox 39.

Comment 2

[Dave Townsend [:mossop]]

How are things going here Dan? Is this going to be complete by the end of next week?

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: certdb support for unpacked signed archives

Comment 4

[Daniel Veditz [:dveditz]]

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?)

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: Part 2: call directory signature verification from XPIProvider.jsm

Comment 6

[Dave Townsend [:mossop]]

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.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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).

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: Part 2: call directory signature verification from XPIProvider.jsm

I assume this is what you meant, but I'd like an explicit r+ on it in case it's not.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8601228 [details] [diff] [review]
Part 2: call directory signature verification from XPIProvider.jsm

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

Yep

Comment 10

[Daniel Veditz [:dveditz]]

Attachment: certdb support for unpacked signed archives (v2)

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.

Comment 11

[:Cykesiopka]

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 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 13

[Daniel Veditz [:dveditz]]

Attachment: Part 1: certdb support for unpacked signed archives (v3)

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.

Comment 14

[Daniel Veditz [:dveditz]]

I will need someone to check in Part 1 and Part 2 for me.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a02ea85607a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/266853fe8965

Comment 16

[Carsten Book [:Tomcat]]

(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

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out for breaking most (yes, really, most) tests.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae4aaad2fe9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ed296c05ad

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(See https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=266853fe8965 for the test failures.)

Comment 19

[Carsten Book [:Tomcat]]

(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 20

[Dave Townsend [:mossop]]

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!

Comment 21

[Daniel Veditz [:dveditz]]

Attachment: Part 2: call directory signature verification from XPIProvider.jsm (v3)

fixed incorrect variable name, mochitests and reftests that were failing before now run fine (tested locally).

Comment 22

[Dave Townsend [:mossop]]

Re-landed after what looks like a good try run:

https://hg.mozilla.org/integration/fx-team/rev/e8c071496de1
https://hg.mozilla.org/integration/fx-team/rev/128faa863164

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8c071496de1
https://hg.mozilla.org/mozilla-central/rev/128faa863164

Comment 24

[Vasilica Tamas, QA [:vtamas]]

Does this bug need manual testing?

Comment 25

[Dave Townsend [:mossop]]

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.

Comment 26

[Vasilica Tamas, QA [:vtamas]]

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