386153 - XPI files dropped in the extensions folder always appear unsigned

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alexey Ousov]

User-Agent:       Opera/9.20 (Windows NT 5.1; U; en)
Build Identifier: 2.0.0.4

There is a bug in extension manager, so even if extension is signed with vallid certificate it is reported as unsigned while installing it. The code in nsExtensionManager.js is shown below:

try {
  var jar = zipReader.QueryInterface(Components.interfaces.nsIJAR);
  var principal = { };
  var certPrincipal = zipReader.getCertificatePrincipal(null, principal);
  // XXXbz This string could be empty.  This needs better
  // UI to present principal.value.certificate's subject.
  prettyName = principal.value.prettyName;
}
catch (e) { }

There are 2 bugs:
1 - zipReader is an instance of "@mozilla.org/libjar/zip-reader;1" so it can't be queried for nsIJAR
2 - getCertificatePrincipal is called on a zipReader not on a jar

So this code always throw an exception, and extension appears to be unsigned in all cases.

Reproducible: Always

Steps to Reproduce:
1. Create signed extension
2. Copy it to "C:\<Program Files>\Mozilla Thunderbird\extensions folder
3. Start Thunderbird
Actual Results:  
Extension appears unsigned

Expected Results:  
Should be show as signed

Comment 1

[Alexey Ousov]

Attachment: sample signed extension to reproduce problem

Comment 2

[Adam Guthrie]

This is fixed on trunk, no? http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.223#3257

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> This is fixed on trunk, no?
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.223#3257
> 

It definitely looks fixed on trunk (by bug 286034), I'm not sure about if this would be fixed for a 2.0.0.x release.

I'm moving this to the Firefox/Extension Manager component as it is core code and it'll more likely get the correct attention there.

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> (In reply to comment #2)
> > This is fixed on trunk, no?
> > http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.223#3257
> > 
> 
> It definitely looks fixed on trunk (by bug 286034), I'm not sure about if this
> would be fixed for a 2.0.0.x release.

No, bug 286034 introduced the code in the first place, this is still broken however not for any of the reasons listed in comment 0.

> There are 2 bugs:
> 1 - zipReader is an instance of "@mozilla.org/libjar/zip-reader;1" so it can't
> be queried for nsIJAR

The zipreader component certainly does implement nsIJAR so querying it for that shouldn't notmally be a problem.

> 2 - getCertificatePrincipal is called on a zipReader not on a jar

That doesn't matter, xpconnect magic means that nsIJAR would be exposed on zipReader if the QI was successful.

The problem is just that nsIJAR cannot be used from script. That would need to be fixed for us to be able to test whether an xpi is signed directly from the extension manager.

Comment 7

[Dave Townsend [:mossop]]

Attachment: make nsIJAR scriptable rev 1 [checked in]

Boris, not sure if you're the right person to review this. It makes nsIJAR scriptable and adds some mochitests to verify that we can check the signed status of a zip file. I've also changed the getCertificatePrincipal to be more helpful for JS callers, this doesn't affect binary callers and so doesn't need an iid change I think, but this change isn't necessary and could be dropped if you think it is a problem.

In the tests, signed.zip is a correctly signed zip file, unsigned.zip is a normal zip file. signed-badca.zip is a zip signed by an unknown CA. signed-tampered.zip is a zip signed but that has had some of its contents since signing. signed-added.zip has had an additional file added to it since it was signed.

Comment 8

[Dave Townsend [:mossop]]

Attachment: EM patch rev 1

This fixes up the signature checks for dropped in files, in particular it verifies that every file in the xpi is signed now rather than just trusting the existence of a certificate.

The test certificate db was generated using the following commands (jartests-object.ca comes from http://hg.mozilla.org/mozilla-central/file/0ba03471b3b0/build/pgo/certs/jartests-object.ca):

mkdir certdb
cd certdb
certutil -N -d .
certutil -A -i ../jartests-object.ca -d . -n "jartests" -t "CT,,CT"

The signed XPIs were generated using the instructions in bug 424488 comment 24

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 270152 [details]
sample signed extension to reproduce problem

This is not a valid testcase as it was signed by an untrusted certificate

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 376179 [details] [diff] [review]
make nsIJAR scriptable rev 1 [checked in]

I'm probably as good as anyone for this, yeah.  Sorry for the really laggy review.  This looks good as far as the interface goes; I didn't look at the tests in detail.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 376179 [details] [diff] [review]
make nsIJAR scriptable rev 1 [checked in]

Landed this patch: http://hg.mozilla.org/mozilla-central/rev/c8ae09375c34

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 376205 [details] [diff] [review]
EM patch rev 1

Just a couple of nits

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>@@ -817,16 +817,44 @@ function getZipReaderForFile(zipFile) {
>   catch (e) {
>     zipReader.close();
>     throw e;
>   }
>   return zipReader;
> }
> 
> /**
>+ * Verifies that a zip files contents are all signed by the same principal.
nit: s/files/file's/

For accuracy you should call out that only files within the zip are checked and that the META-INF directory's contents are not checked.

>+ * @param   zip
>+ *          A nsIZipReader to test
s/test/check/

>+              if (principal && principal.hasCertificate) {
>+                if (principal.hasCertificate && verifyZipSigning(zipReader, principal)) {
>+                  // XXXbz This string could be empty.  This needs better
>+                  // UI to present principal.value.certificate's subject.
Can you file a bug on this if there isn't one already?

Comment 13

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/37d2e8a7899f

Comment 14

[Dave Townsend [:mossop]]

Needed a bustage fix, we must close the zip file before windows will let us delete the file: http://hg.mozilla.org/mozilla-central/rev/8737a137215c