From ed8c394eba244bcfdc78c40384c3edfb4d35e835 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 9 Mar 2024 10:18:10 +0100 Subject: [gpg] Fix reading ed25519 GPG keys The S-expression parser from Bouncy Castle parsed such keys wrongly; there is a "flags" sub-list before the "q" value. Additionally, the parser validates the key read against the given public key, this failed because Bouncy Castle does not know the OID of curve name "Ed25519". Fix this and add a test for reading an ed25519 GPG key. Bug: jgit-27 Change-Id: Ia50445b88759927d2e80b9871d498fbe5ad201bc Signed-off-by: Thomas Wolf --- .../62D43D7F117F7A5E4998ECB6617EE9942D069C14.asc | 9 ++ .../62D43D7F117F7A5E4998ECB6617EE9942D069C14.key | 7 ++ .../jgit/gpg/bc/internal/keys/SecretKeysTest.java | 3 +- .../eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java | 28 ++---- .../jgit/gpg/bc/internal/keys/ObjectIds.java | 109 +++++++++++++++++++++ .../jgit/gpg/bc/internal/keys/SExprParser.java | 37 ++++++- 6 files changed, 167 insertions(+), 26 deletions(-) create mode 100644 org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.asc create mode 100644 org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.key create mode 100644 org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/ObjectIds.java diff --git a/org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.asc b/org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.asc new file mode 100644 index 0000000000..ee8207ec41 --- /dev/null +++ b/org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.asc @@ -0,0 +1,9 @@ +-----BEGIN PGP PUBLIC KEY BLOCK----- + +mDMEZet5vRYJKwYBBAHaRw8BAQdA1lUwXTD4ia1i4+ckhPr0O0a9aQAarg6U8prB +6H85XJG0GFRlc3RlciA8dGVzdGVyQHRlc3QuY29tPoiQBBMWCgA4FiEEwLQ/UWQ8 +ydO7u8ea7hn0dWq7fbwFAmXreb0CGwEFCwkIBwMFFQoJCAsFFgIDAQACHgECF4AA +CgkQ7hn0dWq7fbxhPwEA3a0COi4sV7Uxd91H9P5DXJA2XzYtyvYsxZJEICFZPo8B +AO6fF9Ii5ATO5USSMf6bNCevcBlDFBNXIO+pwjemrBYJ +=LYEV +-----END PGP PUBLIC KEY BLOCK----- diff --git a/org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.key b/org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.key new file mode 100644 index 0000000000..03ed01cb0b --- /dev/null +++ b/org.eclipse.jgit.gpg.bc.test/tst-rsrc/org/eclipse/jgit/gpg/bc/internal/keys/62D43D7F117F7A5E4998ECB6617EE9942D069C14.key @@ -0,0 +1,7 @@ +Key: (protected-private-key (ecc (curve Ed25519)(flags eddsa)(q + #40D655305D30F889AD62E3E72484FAF43B46BD69001AAE0E94F29AC1E87F395C91#) + (protected openpgp-s2k3-ocb-aes ((sha1 #EBA45EE5104E7ED6# + "24672256")#36CB86BDBEA4947789F555B6#)#2D3CBB52F66DED8E0E7B0A1FEE9732 + 4FC1624B32069CD1ED507877E26B3099E62C2AC615DA7E8DAAD335EC613AD2CD9B89C4 + D1BCDEADEA3C67785428#)(protected-at "20240308T204911"))) +Created: 20240308T204901 diff --git a/org.eclipse.jgit.gpg.bc.test/tst/org/eclipse/jgit/gpg/bc/internal/keys/SecretKeysTest.java b/org.eclipse.jgit.gpg.bc.test/tst/org/eclipse/jgit/gpg/bc/internal/keys/SecretKeysTest.java index 5e5e303319..fed06103b6 100644 --- a/org.eclipse.jgit.gpg.bc.test/tst/org/eclipse/jgit/gpg/bc/internal/keys/SecretKeysTest.java +++ b/org.eclipse.jgit.gpg.bc.test/tst/org/eclipse/jgit/gpg/bc/internal/keys/SecretKeysTest.java @@ -97,6 +97,7 @@ public class SecretKeysTest { new TestData("2FB05DBB70FC07CB84C13431F640CA6CEA1DBF8A", false, true), new TestData("66CCECEC2AB46A9735B10FEC54EDF9FD0F77BAF9", true, true), new TestData("F727FAB884DA3BD402B6E0F5472E108D21033124", true, true), + new TestData("62D43D7F117F7A5E4998ECB6617EE9942D069C14", true, true), new TestData("faked", false, true) }; } @@ -152,7 +153,7 @@ public class SecretKeysTest { assertNotNull(secretKey); } catch (PGPException e) { // Currently we may not be able to load OCB-encrypted keys. - assertTrue(e.getMessage().contains("OCB")); + assertTrue(e.toString(), e.getMessage().contains("OCB")); assertTrue(data.encrypted); assertFalse(ocbAvailable()); } diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java index 3eee18aef5..9b5d592fa8 100644 --- a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java +++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/KeyGrip.java @@ -43,17 +43,6 @@ import org.eclipse.jgit.util.sha1.SHA1; */ public final class KeyGrip { - // Some OIDs apparently unknown to BouncyCastle. - - private static String OID_OPENPGP_ED25519 = "1.3.6.1.4.1.11591.15.1"; //$NON-NLS-1$ - - private static String OID_RFC8410_CURVE25519 = "1.3.101.110"; //$NON-NLS-1$ - - private static String OID_RFC8410_ED25519 = "1.3.101.112"; //$NON-NLS-1$ - - private static ASN1ObjectIdentifier CURVE25519 = ECNamedCurveTable - .getOID("curve25519"); //$NON-NLS-1$ - private KeyGrip() { // No instantiation } @@ -99,20 +88,15 @@ public final class KeyGrip { break; case PublicKeyAlgorithmTags.ECDH: case PublicKeyAlgorithmTags.ECDSA: - case PublicKeyAlgorithmTags.EDDSA: + case PublicKeyAlgorithmTags.EDDSA_LEGACY: + case PublicKeyAlgorithmTags.Ed25519: ECPublicBCPGKey ec = (ECPublicBCPGKey) publicKey .getPublicKeyPacket().getKey(); ASN1ObjectIdentifier curveOID = ec.getCurveOID(); // BC doesn't know these OIDs. - if (OID_OPENPGP_ED25519.equals(curveOID.getId()) - || OID_RFC8410_ED25519.equals(curveOID.getId())) { + if (ObjectIds.isEd25519(curveOID)) { return hashEd25519(grip, ec.getEncodedPoint()); - } else if (CURVE25519.equals(curveOID) - || OID_RFC8410_CURVE25519.equals(curveOID.getId())) { - // curvey25519 actually is the OpenPGP OID for Curve25519 and is - // known to BC, but the parameters are for the short Weierstrass - // form. See https://github.com/bcgit/bc-java/issues/399 . - // libgcrypt uses Montgomery form. + } else if (ObjectIds.isCurve25519(curveOID)) { return hashCurve25519(grip, ec.getEncodedPoint()); } X9ECParameters params = getX9Parameters(curveOID); @@ -141,7 +125,9 @@ public final class KeyGrip { hash(grip, b.toByteArray(), 'b', false); hash(grip, g, 'g', false); hash(grip, n.toByteArray(), 'n', false); - if (publicKey.getAlgorithm() == PublicKeyAlgorithmTags.EDDSA) { + int algorithm = publicKey.getAlgorithm(); + if (algorithm == PublicKeyAlgorithmTags.EDDSA_LEGACY + || algorithm == PublicKeyAlgorithmTags.Ed25519) { hashQ25519(grip, q); } else { hash(grip, q.toByteArray(), 'q', false); diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/ObjectIds.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/ObjectIds.java new file mode 100644 index 0000000000..3d3098158a --- /dev/null +++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/ObjectIds.java @@ -0,0 +1,109 @@ +/* + * Copyright (C) 2024, Thomas Wolf and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ +package org.eclipse.jgit.gpg.bc.internal.keys; + +import org.bouncycastle.asn1.ASN1ObjectIdentifier; +import org.bouncycastle.asn1.x9.ECNamedCurveTable; + +/** + * Some OIDs apparently unknown to Bouncy Castle. + */ +public final class ObjectIds { + + /** + * Legacy OID for ed25519 used in OpenPGP. + */ + public static final ASN1ObjectIdentifier OID_OPENPGP_ED25519 = + new ASN1ObjectIdentifier("1.3.6.1.4.1.11591.15.1"); //$NON-NLS-1$ + + /** + * OID for ed25519 according to RFC 8410. + */ + public static final ASN1ObjectIdentifier OID_RFC8410_ED25519 = + new ASN1ObjectIdentifier("1.3.101.112"); //$NON-NLS-1$ + + /** + * Legacy OID for curve25519 used in OpenPGP. + */ + public static final ASN1ObjectIdentifier OID_OPENPGP_CURVE25519 = + ECNamedCurveTable.getOID("curve25519"); //$NON-NLS-1$ + // This is 1.3.6.1.4.1.3029.1.5.1 + + /** + * OID for curve25519 according to RFC 8410. + */ + public static final ASN1ObjectIdentifier OID_RFC8410_CURVE25519 = new ASN1ObjectIdentifier( + "1.3.101.110"); //$NON-NLS-1$ + + /** + * Determines whether the given {@code oid} denoted ed25519. + * + * @param oid + * to test + * @return {@code true} if it is ed25519, {@code false} otherwise + */ + public static boolean isEd25519(ASN1ObjectIdentifier oid) { + return OID_OPENPGP_ED25519.equals(oid) + || OID_RFC8410_ED25519.equals(oid); + } + + /** + * Determines whether the given {@code oid} denoted curve25519. + * + * @param oid + * to test + * @return {@code true} if it is curve25519, {@code false} otherwise + */ + public static boolean isCurve25519(ASN1ObjectIdentifier oid) { + return OID_RFC8410_CURVE25519.equals(oid) + || OID_OPENPGP_CURVE25519.equals(oid); + } + + /** + * Retrieves an OID by name. + * + * @param name + * to get the OID for + * @return the OID, or {@code null} if unknown. + */ + public static ASN1ObjectIdentifier getByName(String name) { + if (name != null) { + switch (name) { + case "Ed25519": //$NON-NLS-1$ + return OID_OPENPGP_ED25519; + case "Curve25519": //$NON-NLS-1$ + return OID_OPENPGP_CURVE25519; + default: + break; + } + } + return null; + } + + /** + * Checks whether two OIDs denote the same algorithm. + * + * @param oid1 + * First OID to test + * @param oid2 + * Second OID to test + * @return {@code true} if the two OIDs match, {@code false} otherwise + */ + public static boolean match(ASN1ObjectIdentifier oid1, + ASN1ObjectIdentifier oid2) { + if (isEd25519(oid1)) { + return isEd25519(oid2); + } + if (isCurve25519(oid1)) { + return isCurve25519(oid2); + } + return oid1 != null && oid1.equals(oid2); + } +} diff --git a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java index c93c2164c9..fd030ee032 100644 --- a/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java +++ b/org.eclipse.jgit.gpg.bc/src/org/eclipse/jgit/gpg/bc/internal/keys/SExprParser.java @@ -30,6 +30,7 @@ import java.io.OutputStream; import java.math.BigInteger; import java.util.Date; +import org.bouncycastle.asn1.ASN1ObjectIdentifier; import org.bouncycastle.asn1.x9.ECNamedCurveTable; import org.bouncycastle.bcpg.DSAPublicBCPGKey; import org.bouncycastle.bcpg.DSASecretBCPGKey; @@ -62,9 +63,12 @@ import org.bouncycastle.util.Strings; * modified by the JGit team to: *
    *
  • handle unencrypted DSA, EC, and ElGamal keys (upstream only handles - * unencrypted RSA), and
    • + * unencrypted RSA) *
  • handle secret keys using AES/OCB as encryption (those don't have a - * hash).
    • + * hash) + *
  • fix EC parsing to account for "flags" sub-list present for ed25519 and + * curve25519
    • + *
  • add support for ed25519 OIDs unknown to BouncyCastle
    • *
*/ @SuppressWarnings("nls") @@ -128,6 +132,15 @@ public class SExprParser { SXprUtils.skipOpenParenthesis(inputStream); type = SXprUtils.readString(inputStream, inputStream.read()); + // JGit: c.f. https://github.com/bcgit/bc-java/issues/1590. + // There may be a flags sub-list here for ed25519 or curve25519. + if (type.equals("flags")) { + SXprUtils.readString(inputStream, inputStream.read()); + SXprUtils.skipCloseParenthesis(inputStream); + SXprUtils.skipOpenParenthesis(inputStream); + type = SXprUtils.readString(inputStream, + inputStream.read()); + } if (type.equals("q")) { qVal = SXprUtils.readBytes(inputStream, inputStream.read()); } else { @@ -143,12 +156,19 @@ public class SExprParser { curveName = curveName.substring("NIST ".length()); } + // JGit: BC doesn't know Ed25519 curve name. + ASN1ObjectIdentifier curveOid = ECNamedCurveTable + .getOID(curveName); + if (curveOid == null) { + curveOid = ObjectIds.getByName(curveName); + } ECPublicBCPGKey basePubKey = new ECDSAPublicBCPGKey( - ECNamedCurveTable.getOID(curveName), + curveOid, new BigInteger(1, qVal)); ECPublicBCPGKey assocPubKey = (ECPublicBCPGKey) pubKey .getPublicKeyPacket().getKey(); - if (!basePubKey.getCurveOID().equals(assocPubKey.getCurveOID()) + if (!ObjectIds.match(basePubKey.getCurveOID(), + assocPubKey.getCurveOID()) || !basePubKey.getEncodedPoint() .equals(assocPubKey.getEncodedPoint())) { throw new PGPException( @@ -292,6 +312,15 @@ public class SExprParser { SXprUtils.skipOpenParenthesis(inputStream); type = SXprUtils.readString(inputStream, inputStream.read()); + // JGit: c.f. https://github.com/bcgit/bc-java/issues/1590. + // There may be a flags sub-list here for ed25519 or curve25519. + if (type.equals("flags")) { + SXprUtils.readString(inputStream, inputStream.read()); + SXprUtils.skipCloseParenthesis(inputStream); + SXprUtils.skipOpenParenthesis(inputStream); + type = SXprUtils.readString(inputStream, + inputStream.read()); + } if (type.equals("q")) { qVal = SXprUtils.readBytes(inputStream, inputStream.read()); } else { -- cgit v1.2.3