JcaPGPKeyConverter cannot properly convert EC keys

[vanitasvitae]

JcaPGPKeyConverter does not properly decode elliptic curve keys in private BCPGKey getPublicBCPGKey(int algorithm, PGPAlgorithmParameters algorithmParameters, PublicKey pubKey).

The following lines decode a different OID than the original key has, which then causes the curve table lookup to fail.

https://github.com/bcgit/bc-java/blob/main/pg/src/main/java/org/bouncycastle/openpgp/operator/jcajce/JcaPGPKeyConverter.java#L482-L485

// BCECPublicKey ecPub = (BCECPublicKey) pubKey;
// ecPub.ecSpec = "brainpoolP256r1 (1.3.36.3.3.2.8.1.1.7)"
SubjectPublicKeyInfo keyInfo = SubjectPublicKeyInfo.getInstance(pubKey.getEncoded());
ASN1ObjectIdentifier curveOid = ASN1ObjectIdentifier.getInstance(keyInfo.getAlgorithm().getAlgorithm());
// curveOID = "1.2.840.10045.2.1"

My guess would be that either SubjectPublicKeyInfo.getInstance() is buggy or there is an issue in BCECPublicKey.getEncoded().

Here is a reproducer.

        // BrainpoolP256r1 based PGP key
        String curve = "-----BEGIN PGP PRIVATE KEY BLOCK-----\n" +
            "Version: BCPG v@RELEASE_NAME@\n" +
            "\n" +
            "lHgEZkH7VhMJKyQDAwIIAQEHAgMEj7YxVg4/2p4uuhcpRqGl2i+vDhjx8YhUUNJX\n" +
            "RNFozBuIWJ6zkW3wRKdD/7Y7tzKNwyHmZ4FBFCcUoLliLeD4SAABAIkEm4iT1g0B\n" +
            "Bo9vkUrUcP2b+vtOuwtmrvGrT0VzVXYlD5M=\n" +
            "=vZRh\n" +
            "-----END PGP PRIVATE KEY BLOCK-----";
        JcaPGPKeyConverter c = new JcaPGPKeyConverter();

        ByteArrayInputStream bIn = new ByteArrayInputStream(curve.getBytes(StandardCharsets.UTF_8));
        ArmoredInputStream aIn = new ArmoredInputStream(bIn);
        BCPGInputStream pIn = new BCPGInputStream(aIn);
        JcaPGPSecretKeyRing ring = new JcaPGPSecretKeyRing(pIn);
        PGPSecretKey sk = ring.getSecretKey();
        PGPKeyPair parsed = new PGPKeyPair(sk.getPublicKey(), sk.extractPrivateKey(null));

        byte[] pubEnc = parsed.getPublicKey().getEncoded();
        byte[] privEnc = parsed.getPrivateKey().getPrivateKeyDataPacket().getEncoded();

        // Fails
        JcaPGPKeyPair converted = new JcaPGPKeyPair(
                parsed.getPublicKey().getAlgorithm(),
                new KeyPair(
                        c.getPublicKey(parsed.getPublicKey()),
                        c.getPrivateKey(parsed.getPrivateKey())),
                parsed.getPublicKey().getCreationTime());

The same conversion method also fails for EC keys over the prime256v1, secp384r1, secp521r1, brainpool384r1, brainpool521r1 curves.

[vanitasvitae]

Some more insights:
The issue only occurs when converting keys that were parsed from existing encoding.

If instead, the keys are freshly generated, the conversion method succeeds.

One more symptom is, that freshly generated keys do have a different object type in the ecSpec field.
Freshly generated keys contain a ECNamedCurveSpec object, while parsed keys contain an NamedCurve object instead.
Further, the length of pubKey.getEncoded() differs. For secp384r1 keys, freshly generated keys return a length of 120 bytes, while parsed keys return a length of 441 bytes.

[vanitasvitae]

It turns out, that JcaPGPKeyConverter.implGetPublicKeyEC(String, ECPublicBCPGKey) decodes the proper curveOID, but the curve OID from X9ECParameters which is retrieved by calling JcaJcePGPUtil.getX9Parameters(curveOID) does no longer match.

[vanitasvitae]

BCECPublicKey.getEncoded() sets the OID to X9ObjectIdentifiers.id_ecPublicKey, discarding the proper OID.
See https://github.com/bcgit/bc-java/blob/main/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/ec/BCECPublicKey.java#L247

This results in the difference in encoding length I suppose.

[vanitasvitae]

I fixed this in #1671

[dghgit]

So the behavior of BCECPublicKey is correct - the X9.62 definition of the key encoding, which is what the straight ASN.1 is based on means the id_ecPublicKey is the algorithm associated with the key, the algorithm parameters associated with the key are where the curve details are. This is the definition for key encoding to be used for X.509.

There's three possibilities for this, explicit, implicit, named. Usually you will see named, which means the parameters will contain an object identifier giving the Curve OID, which is what PGP wants in it's encoding. Explicit and implicit contain actual curve parameters, in many cases these will still match a named curve, but sometimes they won't. I doubt you'd run into these, but it's worth being aware of the possibility.

Given that's how the universe is, does this mean this PR #1671 is correct? (It looks like it is, but I thought it worth clearing up the encoding issue just to make sure first). The main thing it needs to recognize is the converter is really translating between 2 very different ways of encoding.

[vanitasvitae]

There's three possibilities for this, explicit, implicit, named. Usually you will see named, which means the parameters will contain an object identifier giving the Curve OID, which is what PGP wants in it's encoding. Explicit and implicit contain actual curve parameters, in many cases these will still match a named curve, but sometimes they won't. I doubt you'd run into these, but it's worth being aware of the possibility.

I see. I'm currently reading up on X9.62 (I found https://safecurves.cr.yp.to/grouper.ieee.org/groups/1363/private/x9-62-09-20-98.pdf which appears to be the proper document?) to get a better picture.

From what I can say so far, we should document that 7ae4dbd#diff-4c679c2a6587b4244471fa022bebd469e7c66e5cb34ffda6436f0d139d78f35dR688 does lookup the named curve OID for an explicitly encoded key and remove the misleading comments I added.

[dghgit]

The document is close enough. Yes, I think that'll do.