[Patch draft] It would be far too easy to hijack a plugin with malicious code

[ris]

(apologies if there is already a bug on this subject, I couldn't find it)

Right now, all a malicious actor would have to do is replace a link to a relatively popular plugin on https://josm.openstreetmap.de/wiki/PluginsSource and potentially thousands of users would have their josm "automatically upgrade" to it. Possibly the change could be disguised by just using a very-similarly-named github user.

I see this as quite a real problem and could be something that harms JOSM adoption, even if it doesn't ever happen to get exploited. I suspect it might not pass an organization's thorough IT audit and could preclude its use in large organizations or government bodies. Just this month I was proposing to an NGO implementing some tool they wanted as a JOSM plugin, and as a professional I really have to consider whether I would want to expose their systems to this risk.

Clearly, following java's trodden-path of jar signing and requiring all plugin authors to have jar-signing-capable PKI certificates would be a significant burden (or would it if you were able to be your own pseudo-CA and bundle your own root certificate with josm?), but it might also be possible to use java's jar-signing mechanism to implement a form of TOFU which would go a long way towards settling the hijacking worry (josm would show a warning if trying to upgrade a plugin that was signed with a different key?).

What are peoples' thoughts on this?

[Don-vip]

This already almost happened once with plugin "​no more mapping". But this was only a prank.

Putting in place a PKI/signing infrastructure would be overkill. We rely on the good faith of the community.

Yet I agree this is a risk in a professional environment. What we could do to mitigate it is rely on the notion of "external plugin". We could offer a security mechanism on client side which blocks any plugin which is, or suddenly becomes, external (i.e coming form a different location than svn.openstreetmap.org and github.com/JOSM. The list is currently customizable I think, so you could also add private company repositories).

It should not be very difficult, maybe you would be interested in bringing in this feature?

[ris]

I'd be far more interested in adding TOFU support for external plugins, as e.g. personally I have no immediate plans to de-externalize my plugin(s). Indeed, "internal" plugins could probably skip this check.

The example "popular" plugin I had in mind as a target was Mapillary, still being an external plugin.

[Don-vip]

Replying to ris:

I'd be far more interested in adding TOFU support for external plugins

Had to look ​what TOFU means, but yes, sounds nice :)

[stoecker]

In this scenario TOFU should be implemented in the server side checks dropping a plugin entry from the list when irregularities are detected.

But how do you want to introduce a secret on the plugin build side? Most development scenarios which we have ATM prevent this.

[ris]

In this scenario TOFU should be implemented in the server side checks dropping a plugin entry from the list when irregularities are detected.

That's interesting yeah I had been thinking around this area, but was leaning on the check being done on the user side to give them the opportunity to ignore a mismatch. One of the drawbacks of TOFU is when people lose their keys or want to switch them, it's going to trigger an alert - if that causes a hard failure, it would be ... bad. I guess you've also got to consider the administrative burden of doing things server side: if a key doesn't check out, is it now a chore one of us has to perform to go and contact that author and verify that it really is a genuine switch? What if people are always losing their keys?

Certainly a server-side check would be useful, whether or not there was also an in-JOSM check.

how do you want to introduce a secret on the plugin build side? Most development scenarios which we have ATM prevent this.

Not sure what you mean by this. The jdk comes with tools for key generation and signing...?

[stoecker]

how do you want to introduce a secret on the plugin build side? Most development scenarios which we have ATM prevent this.

Not sure what you mean by this. The jdk comes with tools for key generation and signing...?

A secret must be secret. That conflicts with open source development models. Plugin developers are groups of people and also change relatively frequently. I fear that code signing actually is not really a really good way to solve the security issue. While in theory it can solve the malicious code problem in praxis I fear it is still extremely easy to introduce bad code if nobody watches.

JOSM server code could store a list of valid certs (also self-signed, officials are too expensive) per plugin which overcomes the "one valid cert" problem, so each developer can have an own cert. If looking at the troubles to get plugins update deprecated code I'm not convinced that code signing will actually improve the situation except for a minority of plugins (yours?). But it probably also does no harm.

Two ways could be:

• We make a JOSM wiki page containing signing information (write protected for anybody except admins)
• a)
  • The "/plugins" entry transports this additional signing information
  • JOSM code checks if signing is ok when downloading updates
• b)
  • The server side code removes the entry for invalid signing.
• c)
  • a and b

Question is: Is it worth the effort?

[ris]

Right, so, a lot of plugins are group-maintained and don't have a particular "owner" as such. That would indeed make TOFU painful.

You two solutions both look viable, but I've got to note that they are slightly like building a homebrew CA :) It would probably be important too to capture the list of plugins a certificate holder is authorized to make new releases for.

Worth the effort? If nobody else is interested, it's something I'd probably be interested in taking on.

[stoecker]

Replying to ris:

You two solutions both look viable, but I've got to note that they are slightly like building a homebrew CA :) It would probably be important too to capture the list of plugins a certificate holder is authorized to make new releases for.

Actually that was part of the plan. Like

• plugin1.jar
  • pub-key1
  • pub-key2
  • ...
• ...

[ris]

Here's an example of how things could work using our own certificate chain. First detailing the generation of the various keys using keytool. A root keypair & certificate is used to sign a number of certificates for the "Core developers" who sign the actual code-signing certificates.

# A root keypair & certificate is generated. The "extension" parameters are
# specified here because at the top level they need to get applied to the
# root key's self-signed certificate
keytool -genkeypair -storepass banana -keypass banana -keystore keys/keystore.jks -alias root -dname "cn=JOSM" -ext bc:c=ca:true,pathlen:1 -ext ku:c=digitalSignature,nonRepudiation,keyCertSign,cRLSign -keyalg EC

# Core Developer A generates their keypair and certificate signing request,
# no extended parameters as those are decided by the certificate issuer,
# which in this case will be root.
keytool -genkeypair -storepass banana -keypass banana -keystore keys/keystore.jks -alias core-a -dname "cn=Core Developer A" -keyalg EC
keytool -certreq -storepass banana -keypass banana -keystore keys/keystore.jks -alias core-a -file keys/core-a-certreq.csr

# root signs Core Developer A's key, granting it CA abilities but not the
# ability to create further CAs (pathlen:0)
keytool -gencert -storepass banana -keypass banana -keystore keys/keystore.jks -alias root -infile keys/core-a-certreq.csr -outfile keys/core-a-cert.der -ext bc:c=ca:true,pathlen:0 -ext ku:c=digitalSignature,nonRepudiation,keyCertSign -ext eku=codeSigning

# Core Developer A imports that back into their keychain
keytool -importcert -storepass banana -keypass banana -keystore keys/keystore.jks -alias core-a -file keys/core-a-cert.der

# Plugin Author B generates their keypair and certificate signing request
keytool -genkeypair -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-b -dname "cn=Author B" -keyalg EC
keytool -certreq -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-b -file keys/author-b-certreq.csr

# Core Developer A signs this generating a certificate. The extension params
# include the plugin name(s) Author B is permitted to sign against encoded as
# URI subjectAlternativeNames. These are effectively arbitrary URIs where we
# can choose our own convention
keytool -gencert -storepass banana -keypass banana -keystore keys/keystore.jks -alias core-a -infile keys/author-b-certreq.csr -outfile keys/author-b-cert.der -ext san=uri:http://josm.openstreetmap.de/plugin/foo,uri:http://josm.openstreetmap.de/plugin/bar,uri:http://josm.openstreetmap.de/plugin/baz -ext bc=ca:false -ext ku:c=digitalSignature,nonRepudiation -ext eku:c=codeSigning

# Author B imports this certificate back into their keychain
keytool -importcert -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-b -file keys/author-b-cert.der

# The root public certificate is exported and shipped with the verification code...
keytool -exportcert -storepass banana -keypass banana -keystore keys/keystore.jks -alias root -file keys/rootCertificate.pem

Author B can now go ahead and sign their jar before release:

jarsigner -keystore keys/keystore.jks dist/baz.jar author-b

Verification code would look something like this:

public final class PluginVerifier {
    /**
     * Simple class to contain the results from a verifyJarFile call
     */
    public static class RelevantCodeSigners {
        final public Set<CodeSigner> authorizedSigners = new HashSet<CodeSigner>();

        /**
         * CodeSigner(s) whose certificates couldn't be traced back to our TrustAnchor found on JarEntrys
         * which had no otherwise authorized signers for them
         */
        final public Set<CodeSigner> unverifiedSigners = new HashSet<CodeSigner>();

        /** CodeSigner(s) whose certificates could be traced back to our TrustAnchor but didn't have the
         * correct plugin-name permissions, found on JarEntrys which had no otherwise authorized signers
         * for them
         */
        final public Set<CodeSigner> unauthorizedSigners = new HashSet<CodeSigner>();

        public boolean passedValidation() {
            return this.unverifiedSigners.isEmpty() && this.unauthorizedSigners.isEmpty();
        }

        @Override
        public String toString() {
            return String.format(
                "authorizedSigners: %s\nunverifiedSigners: %s\nunauthorizedSigners: %s",
                this.authorizedSigners,
                this.unverifiedSigners,
                this.unauthorizedSigners
            );
        }
    }

    public static RelevantCodeSigners verifyJarFile(
        final File file,
        final String pluginName,
        final X509Certificate rootCertificate
    ) throws NoSuchAlgorithmException, InvalidAlgorithmParameterException, IOException, CertificateException {
        final JarFile jarFile = new JarFile(file, true);
        final byte[] readBuffer = new byte[1024];

        final RelevantCodeSigners jarSigners = new RelevantCodeSigners();

        final CertPathValidator validator = CertPathValidator.getInstance("PKIX");
        final TrustAnchor trustAnchor = new TrustAnchor(rootCertificate, null);
        final PKIXParameters pkixParameters = new PKIXParameters(Collections.singleton(trustAnchor));
        pkixParameters.setRevocationEnabled(false);

        for (JarEntry jarEntry : Collections.list(jarFile.entries())) {
            final String entryName = jarEntry.getName();
            if (entryName.endsWith("/") || entryName.startsWith("META-INF/"))
                // don't expect these to be inclued in signed portion
                continue;

            final InputStream entryInputStream = jarFile.getInputStream(jarEntry);
            // read to end of stream
            while (entryInputStream.read(readBuffer) != -1) {}

            final CodeSigner[] codeSigners = jarEntry.getCodeSigners();
            if (codeSigners == null || codeSigners.length == 0)
                // if we'd need to accept unsigned entries to allow this jar there's no point continuing
                // because we'd already be at the bottom rung of permissiveness
                return null;

            final Set<CodeSigner> entryUnverifiedSigners = new HashSet<CodeSigner>();
            final Set<CodeSigner> entryUnauthorizedSigners = new HashSet<CodeSigner>();

            boolean foundAuthorized = false;
            for (CodeSigner codeSigner : codeSigners) {
                try {
                    validator.validate(codeSigner.getSignerCertPath(), pkixParameters);
                } catch (CertPathValidatorException e) {
                    Logging.warn(e);
                    entryUnverifiedSigners.add(codeSigner);
                    continue;
                }

                final Collection<List<?>> sans = ((X509Certificate) codeSigner.getSignerCertPath().getCertificates().get(0)).getSubjectAlternativeNames();
                if (sans != null) {
                    final String targetURI = "http://josm.openstreetmap.de/plugin/" + pluginName;

                    for (List<?> san : sans) {
                        if (((Integer) san.get(0) == 6) && ((String) san.get(1)).equals(targetURI)) {
                            jarSigners.authorizedSigners.add(codeSigner);
                            foundAuthorized = true;
                            break;
                        }
                    }
                }

                if (foundAuthorized)
                    break;

                entryUnauthorizedSigners.add(codeSigner);
            }
            if (foundAuthorized)
                // don't bother turning over entryUnauthorizedSigners, entryUnverifiedSigners to the 
                // jar-wide sets as it's not relevant
                break;

            // copy this entry's unauthorized and unverified signers to jar-wide Sets as at least one of
            // them would have to be trusted as an exception were we to allow this jar
            jarSigners.unauthorizedSigners.addAll(entryUnauthorizedSigners);
            jarSigners.unverifiedSigners.addAll(entryUnverifiedSigners);
        }
        return jarSigners;
    }
}

We can continue and check some failure conditions... Signing a plugin with a non-covered name:

jarsigner -keystore keys/keystore.jks dist/buz.jar author-b

should result in Author B's CodeSigner showing up in unauthorizedSigners because they haven't been *authorized* to provide a plugin buz.

Author B should also not be able to delegate their powers by signing other certificates:

# Author X generates their key
keytool -genkeypair -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-x -dname "cn=Author X" -keyalg EC
keytool -certreq -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-x -file keys/author-x-certreq.csr

# Author B attempts to sign key for Author X
keytool -gencert -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-b -infile keys/author-x-certreq.csr -outfile keys/author-x-cert.der -ext san=uri:http://josm.openstreetmap.de/plugin/foo,uri:http://josm.openstreetmap.de/plugin/bar,uri:http://josm.openstreetmap.de/plugin/baz -ext bc=ca:false -ext ku:c=digitalSignature,nonRepudiation -ext eku:c=codeSigning
keytool -importcert -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-x -file keys/author-x-cert.der

# Author X signs code
jarsigner -keystore keys/keystore.jks dist/baz.jar author-b

Attempting to verify that jar should fail, putting Author X in unverifiedSigners.

Another case:

# Author C generates their keypair
keytool -genkeypair -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-c -dname "cn=Author C" -keyalg EC
keytool -certreq -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-c -file keys/author-c-certreq.csr

# Core Developer A mistakenly issues a CA certificate to Author C
keytool -gencert -storepass banana -keypass banana -keystore keys/keystore.jks -alias core-a -infile keys/author-c-certreq.csr -outfile keys/author-c-cert.der -ext san=uri:http://josm.openstreetmap.de/plugin/foo,uri:http://josm.openstreetmap.de/plugin/bar,uri:http://josm.openstreetmap.de/plugin/baz -ext bc:c=ca:true,pathlen:0 -ext ku:c=digitalSignature,nonRepudiation,keyCertSign -ext eku=codeSigning

# Author C imports certificate back into their keychain
keytool -importcert -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-c -file keys/author-c-cert.der


# Author Y generates their keypair
keytool -genkeypair -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-y -dname "cn=Author Y" -keyalg EC
keytool -certreq -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-y -file keys/author-y-certreq.csr

# Author C attempts to sign key for Author Y
keytool -gencert -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-c -infile keys/author-y-certreq.csr -outfile keys/author-y-cert.der -ext san=uri:http://josm.openstreetmap.de/plugin/foo,uri:http://josm.openstreetmap.de/plugin/bar,uri:http://josm.openstreetmap.de/plugin/baz -ext bc=ca:false -ext ku:c=digitalSignature,nonRepudiation -ext eku:c=codeSigning

# Author Y imports certificate back into their keychain
keytool -importcert -storepass banana -keypass banana -keystore keys/keystore.jks -alias author-y -file keys/author-y-cert.der

A jar signed by Author Y should also fail to validate, putting Author Y in the unverifiedSigners, because the "cert path" violates the constraints placed on both the root and Core Developer A certificates.

I implemented this in-josm as it was what I had in front of me, but it could just as well go in server-side checking code. For the rest of the in-josm implementation you can see my branch ​https://github.com/risicle/josm/compare/f242c086a4fe7e39bfe765a6f3ca1e1c80b56780...ris-pluginverifier which really just prints diagnostic messages for each PluginInformation constructed from a jar. But it compiles and is runnable.

[ris]

After writing a few more tests for my function as listed here, I realize that the second

    if (foundAuthorized)
        // ...
        break;

Should of course be a continue rather than a break. Got to love tests. Will post fixed/updated version on the previously mentioned github branch soon.