835177 - Replace usage of nsIZipReader::getCertificatePrincipal and verifyZipSigning with nsIX509CertDB::openSignedJARFile for XPI signature checking

Status: RESOLVED WORKSFORME

(Toolkit Graveyard :: Security, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #805305 +++

There are several problems with nsISignatureVerifier, which is used by nsIZipReader::getCertificatePrincipal:

* It uses the untrustworthy signing time to validate the certificate chain, instead of the current system time.
* It does disk I/O on the main thread, and it does a public key operation on the main thread; public key operations are very expensive, CPU-wise.
* It doesn't have any tests.
* Soon, it will not exist, because of the above problems.

openSignedJARFile solves all of these problems. It is used by the app signature verification code in Webapps.jsm already.

The trickiest part about this will be converting the code in Toolkit to support the asynchronous control flow instead of the synchronous flow it has now.

There will be one semantic change that has some compatibility impact: Certificates will be validated as of the current time, instead of the time at which the archive was purported to be signed. This means that addon developers would need to re-sign their extensions whenever their certificate expires. However, I think that is the right thing to do anyway.

Old code:

    let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
                    createInstance(Ci.nsIZipReader);
    zipreader.open(this.file);
    
    let principal = zipreader.getCertificatePrincipal(null);
    if (principal && principal.hasCertificate) {
      LOG("Verifying XPI signature");
      if (verifyZipSigning(zipreader, principal)) {
        let x509 = principal.certificate;
      }
    }

    this.addon = loadManifestFromZipReader(zipreader);
    ...

new code:

    let file = this.file;
    let certdb = certdb = Cc["@mozilla.org/security/x509certdb;1"]
                            .getService(Ci.nsIX509CertDB);
    certdb.openSignedJARFileAsync(file, function(aRv, aZipReader, x509) {
       let isSigned;
       if (Components.isSuccessCode(aRv)) {
          isSigned = true;
          zipReader = aZipReader;
       } else if (aRv != Cr.NS_ERROR_SIGNED_JAR_NOT_SIGNED) {
          // Handle the error
       } else {
         isSigned = false;
         zipReader = Cc["@mozilla.org/libjar/zip-reader;1"]
                       .createInstance(Ci.nsIZipReader);
         zipReader.open(zipFile);
       }

       this.addon = loadManifestFromZipReader(zipreader);
       ...
    });

Comment 1

[Kaspar]

(In reply to Brian Smith (:bsmith) from comment #0)
> There will be one semantic change that has some compatibility impact:
> Certificates will be validated as of the current time, instead of the time
> at which the archive was purported to be signed.

As noted by Ryan in bug 835105 comment 1, that's not how other common code signing schemes work: both Microsoft's Authenticode and Java's JAR signing (as of Java SE 5.0 or later) make use of timestamping facilities to ensure that signatures will also validate after expiration of the signing cert. Authenticode uses a PKCS#9 countersignature attribute, while Java relies on RFC-3161 style timestamp tokens (both are added as unsigned attributes to a CMS SignerInfo structure).

> This means that addon developers would need to re-sign their extensions
> whenever their certificate expires. However, I think that is the right thing
> to do anyway.

That would make XPI signing even more cumbersome than it already is (see e.g. bug 321156). I would certainly welcome a switch to openSignedJARFile, in particular if it would also lift the limitation of the .rsa/.dsa file having to appear first in the XPI (currently imposed by XPIProvider.jsm:loadManifest, I assume - see also bug 248751).

If openSignedJARFile is enhanced to also process RFC-3161 timestamp tokens, then it might not even be needed to pimp signtool with timestamp fetching capabilities in a first step, as developers would be able to simply use Java's jarsigner for the purpose of signing XPIs.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

If we fix bug 858832 then we won't need to do this.

Comment 3

[Daniel Veditz [:dveditz]]

We've essentially done this for Firefox. nsJAR still has the old code, though, for use by other products. Might be worth ripping out (but I think Bug #805305 covers that)