From 2a0625e615e600e9645a5ac52520365ce469f08f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Tue, 23 Feb 2021 18:10:15 +0100 Subject: [PATCH] fix yubikey curve OID bug by reworking ASN1 parsing --- .../keychain/securitytoken/ECKeyFormat.java | 141 ++++++++++++++---- .../keychain/securitytoken/KeyFormat.java | 20 +-- .../securitytoken/SCP11bSecureMessaging.java | 6 +- .../securitytoken/SecurityTokenUtils.java | 6 +- .../operations/PsoDecryptTokenOp.java | 4 +- 5 files changed, 127 insertions(+), 50 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/ECKeyFormat.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/ECKeyFormat.java index fd3e6b63e..11afd0e5b 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/ECKeyFormat.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/ECKeyFormat.java @@ -17,63 +17,151 @@ package org.sufficientlysecure.keychain.securitytoken; +import androidx.annotation.Nullable; + +import com.google.auto.value.AutoValue; + import org.bouncycastle.asn1.ASN1ObjectIdentifier; import org.bouncycastle.asn1.nist.NISTNamedCurves; +import org.bouncycastle.asn1.x9.ECNamedCurveTable; import org.bouncycastle.asn1.x9.X9ECParameters; +import org.bouncycastle.bcpg.PublicKeyAlgorithmTags; import org.bouncycastle.bcpg.sig.KeyFlags; import org.bouncycastle.math.ec.ECCurve; +import org.bouncycastle.util.Arrays; import org.sufficientlysecure.keychain.service.SaveKeyringParcel; import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyAdd; +import java.io.IOException; + +import timber.log.Timber; + // 4.3.3.6 Algorithm Attributes -public class ECKeyFormat extends KeyFormat { +@AutoValue +public abstract class ECKeyFormat extends KeyFormat { - private final ECAlgorithmFormat mECAlgorithmFormat; - private final ASN1ObjectIdentifier mECCurveOID; + public abstract byte[] oidField(); - public ECKeyFormat(final ASN1ObjectIdentifier ecCurveOid, - final ECAlgorithmFormat ecAlgorithmFormat) { + @Nullable // TODO + public abstract ECAlgorithmFormat ecAlgorithmFormat(); + + private static final byte ATTRS_IMPORT_FORMAT_WITH_PUBKEY = (byte) 0xff; + + ECKeyFormat() { super(KeyFormatType.ECKeyFormatType); - mECAlgorithmFormat = ecAlgorithmFormat; - mECCurveOID = ecCurveOid; } - public ECKeyFormat.ECAlgorithmFormat getAlgorithmFormat() { - return mECAlgorithmFormat; + public static KeyFormat getInstance(byte[] oidField, ECAlgorithmFormat from) { + return new AutoValue_ECKeyFormat(oidField, from); } - public ASN1ObjectIdentifier getCurveOID() { - return mECCurveOID; + public static ECKeyFormat getInstance(ASN1ObjectIdentifier oidAsn1, ECAlgorithmFormat from) { + byte[] oidField = asn1ToOidField(oidAsn1); + return new AutoValue_ECKeyFormat(oidField, from); + } + + public static KeyFormat getInstanceFromBytes(byte[] bytes) { + if (bytes.length < 2) { + throw new IllegalArgumentException("Bad length for EC attributes"); + } + + int len = bytes.length - 1; + if (bytes[bytes.length - 1] == ATTRS_IMPORT_FORMAT_WITH_PUBKEY) { + len -= 1; + } + + final byte[] oidField = new byte[len]; + System.arraycopy(bytes, 1, oidField, 0, len); + return getInstance(oidField, ECKeyFormat.ECAlgorithmFormat.from(bytes[0], bytes[bytes.length - 1])); + } + + public byte[] toBytes(KeyType slot) { + byte[] attrs = new byte[1 + oidField().length + 1]; + + attrs[0] = ecAlgorithmFormat().getAlgorithmId(); + System.arraycopy(oidField(), 0, attrs, 1, oidField().length); + attrs[attrs.length - 1] = ATTRS_IMPORT_FORMAT_WITH_PUBKEY; + + return attrs; + } + + public ASN1ObjectIdentifier asn1ParseOid() { + ASN1ObjectIdentifier asn1CurveOid = oidFieldToOidAsn1(oidField()); + String curveName = ECNamedCurveTable.getName(asn1CurveOid); + if (curveName == null) { + Timber.w("Unknown curve OID: %s. Could be YubiKey firmware bug < 5.2.8. Trying again with last byte removed.", asn1CurveOid.getId()); + + // https://bugs.chromium.org/p/chromium/issues/detail?id=1120933#c10 + // The OpenPGP applet of a Yubikey with firmware version below 5.2.8 appends + // a potentially arbitrary byte to the intended byte representation of an ECC + // curve OID. This case is handled by retrying the decoding with the last + // byte stripped if the resulting OID does not label a known curve. + byte[] oidRemoveLastByte = Arrays.copyOf(oidField(), oidField().length - 1); + ASN1ObjectIdentifier asn1CurveOidYubikey = oidFieldToOidAsn1(oidRemoveLastByte); + curveName = ECNamedCurveTable.getName(asn1CurveOidYubikey); + + if (curveName != null) { + Timber.w("Detected curve OID: %s", asn1CurveOidYubikey.getId()); + return asn1CurveOidYubikey; + } else { + Timber.e("Still Unknown curve OID: %s", asn1CurveOidYubikey.getId()); + return asn1CurveOid; + } + } + + return asn1CurveOid; + } + + private static byte[] asn1ToOidField(ASN1ObjectIdentifier oidAsn1) { + byte[] encodedAsn1Oid; + try { + encodedAsn1Oid = oidAsn1.getEncoded(); + } catch (IOException e) { + throw new IllegalStateException("Failed to encode curve OID!"); + } + byte[] oidField = new byte[encodedAsn1Oid.length - 2]; + System.arraycopy(encodedAsn1Oid, 2, oidField, 0, encodedAsn1Oid.length - 2); + + return oidField; + } + + private static ASN1ObjectIdentifier oidFieldToOidAsn1(byte[] oidField) { + final byte[] boid = new byte[2 + oidField.length]; + boid[0] = (byte) 0x06; + boid[1] = (byte) oidField.length; + System.arraycopy(oidField, 0, boid, 2, oidField.length); + return ASN1ObjectIdentifier.getInstance(boid); } public enum ECAlgorithmFormat { - ECDH((byte) 18, true, false), - ECDH_WITH_PUBKEY((byte) 18, true, true), - ECDSA((byte) 19, false, false), - ECDSA_WITH_PUBKEY((byte) 19, false, true); + ECDH((byte) PublicKeyAlgorithmTags.ECDH, true, false), + ECDH_WITH_PUBKEY((byte) PublicKeyAlgorithmTags.ECDH, true, true), + ECDSA((byte) PublicKeyAlgorithmTags.ECDSA, false, false), + ECDSA_WITH_PUBKEY((byte) PublicKeyAlgorithmTags.ECDSA, false, true); - private final byte mValue; + private final byte mAlgorithmId; private final boolean mIsECDH; private final boolean mWithPubkey; - ECAlgorithmFormat(final byte value, final boolean isECDH, final boolean withPubkey) { - mValue = value; + ECAlgorithmFormat(final byte algorithmId, final boolean isECDH, final boolean withPubkey) { + mAlgorithmId = algorithmId; mIsECDH = isECDH; mWithPubkey = withPubkey; } public static ECKeyFormat.ECAlgorithmFormat from(final byte bFirst, final byte bLast) { for (ECKeyFormat.ECAlgorithmFormat format : values()) { - if (format.mValue == bFirst && ((bLast == (byte) 0xff) == format.isWithPubkey())) { + if (format.mAlgorithmId == bFirst && + ((bLast == ATTRS_IMPORT_FORMAT_WITH_PUBKEY) == format.isWithPubkey())) { return format; } } return null; } - public final byte getValue() { - return mValue; + public final byte getAlgorithmId() { + return mAlgorithmId; } public final boolean isECDH() { @@ -86,7 +174,8 @@ public class ECKeyFormat extends KeyFormat { } public void addToSaveKeyringParcel(SaveKeyringParcel.Builder builder, int keyFlags) { - final X9ECParameters params = NISTNamedCurves.getByOID(mECCurveOID); + ASN1ObjectIdentifier oidAsn1 = asn1ParseOid(); + final X9ECParameters params = NISTNamedCurves.getByOID(oidAsn1); final ECCurve curve = params.getCurve(); SaveKeyringParcel.Algorithm algo = SaveKeyringParcel.Algorithm.ECDSA; @@ -96,14 +185,14 @@ public class ECKeyFormat extends KeyFormat { } SaveKeyringParcel.Curve scurve; - if (mECCurveOID.equals(NISTNamedCurves.getOID("P-256"))) { + if (oidAsn1.equals(NISTNamedCurves.getOID("P-256"))) { scurve = SaveKeyringParcel.Curve.NIST_P256; - } else if (mECCurveOID.equals(NISTNamedCurves.getOID("P-384"))) { + } else if (oidAsn1.equals(NISTNamedCurves.getOID("P-384"))) { scurve = SaveKeyringParcel.Curve.NIST_P384; - } else if (mECCurveOID.equals(NISTNamedCurves.getOID("P-521"))) { + } else if (oidAsn1.equals(NISTNamedCurves.getOID("P-521"))) { scurve = SaveKeyringParcel.Curve.NIST_P521; } else { - throw new IllegalArgumentException("Unsupported curve " + mECCurveOID); + throw new IllegalArgumentException("Unsupported curve " + oidAsn1); } builder.addSubkeyAdd(SubkeyAdd.createSubkeyAdd(algo, curve.getFieldSize(), scurve, keyFlags, 0L)); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyFormat.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyFormat.java index fe043cd07..e99b11551 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyFormat.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/KeyFormat.java @@ -53,19 +53,7 @@ public abstract class KeyFormat { case PublicKeyAlgorithmTags.ECDH: case PublicKeyAlgorithmTags.ECDSA: - if (bytes.length < 2) { - throw new IllegalArgumentException("Bad length for EC attributes"); - } - int len = bytes.length - 1; - if (bytes[bytes.length - 1] == (byte)0xff) { - len -= 1; - } - final byte[] boid = new byte[2 + len]; - boid[0] = (byte)0x06; - boid[1] = (byte)len; - System.arraycopy(bytes, 1, boid, 2, len); - final ASN1ObjectIdentifier oid = ASN1ObjectIdentifier.getInstance(boid); - return new ECKeyFormat(oid, ECKeyFormat.ECAlgorithmFormat.from(bytes[0], bytes[bytes.length - 1])); + return ECKeyFormat.getInstanceFromBytes(bytes); case PublicKeyAlgorithmTags.EDDSA: return new EdDSAKeyFormat(); @@ -87,11 +75,11 @@ public abstract class KeyFormat { case RSA_4096: return new RSAKeyFormat(4096, elen, RSAKeyFormat.RSAAlgorithmFormat.CRT_WITH_MODULUS); case ECC_P256: - return new ECKeyFormat(NISTNamedCurves.getOID("P-256"), kf); + return ECKeyFormat.getInstance(NISTNamedCurves.getOID("P-256"), kf); case ECC_P384: - return new ECKeyFormat(NISTNamedCurves.getOID("P-384"), kf); + return ECKeyFormat.getInstance(NISTNamedCurves.getOID("P-384"), kf); case ECC_P521: - return new ECKeyFormat(NISTNamedCurves.getOID("P-521"), kf); + return ECKeyFormat.getInstance(NISTNamedCurves.getOID("P-521"), kf); } throw new IllegalArgumentException("Unsupported Algorithm id " + t); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SCP11bSecureMessaging.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SCP11bSecureMessaging.java index 249b2af8e..3fab42670 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SCP11bSecureMessaging.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SCP11bSecureMessaging.java @@ -154,7 +154,7 @@ class SCP11bSecureMessaging implements SecureMessaging { throws NoSuchProviderException, NoSuchAlgorithmException, InvalidParameterSpecException { final AlgorithmParameters algoParams = AlgorithmParameters.getInstance(SCP11B_KEY_AGREEMENT_KEY_ALGO, PROVIDER); - algoParams.init(new ECGenParameterSpec(ECNamedCurveTable.getName(kf.getCurveOID()))); + algoParams.init(new ECGenParameterSpec(ECNamedCurveTable.getName(kf.asn1ParseOid()))); return algoParams.getParameterSpec(ECParameterSpec.class); } @@ -167,7 +167,7 @@ class SCP11bSecureMessaging implements SecureMessaging { ecdhFactory = KeyFactory.getInstance(SCP11B_KEY_AGREEMENT_KEY_TYPE, PROVIDER); } - final X9ECParameters params = NISTNamedCurves.getByOID(kf.getCurveOID()); + final X9ECParameters params = NISTNamedCurves.getByOID(kf.asn1ParseOid()); if (params == null) { throw new InvalidParameterSpecException("unsupported curve"); } @@ -305,7 +305,7 @@ class SCP11bSecureMessaging implements SecureMessaging { final ECKeyFormat eckf = (ECKeyFormat)kf; - if (eckf.getCurveOID() == null) { + if (eckf.asn1ParseOid() == null) { throw new SecureMessagingException("unsupported curve"); } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java index 63a6423b8..5a1c12146 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtils.java @@ -43,9 +43,9 @@ public class SecurityTokenUtils { byte[] attrs = new byte[1 + (oid.length - 2) + 1]; if (slot.equals(KeyType.SIGN)) - attrs[0] = ECKeyFormat.ECAlgorithmFormat.ECDSA_WITH_PUBKEY.getValue(); + attrs[0] = ECKeyFormat.ECAlgorithmFormat.ECDSA_WITH_PUBKEY.getAlgorithmId(); else { - attrs[0] = ECKeyFormat.ECAlgorithmFormat.ECDH_WITH_PUBKEY.getValue(); + attrs[0] = ECKeyFormat.ECAlgorithmFormat.ECDH_WITH_PUBKEY.getAlgorithmId(); } System.arraycopy(oid, 2, attrs, 1, (oid.length - 2)); @@ -174,7 +174,7 @@ public class SecurityTokenUtils { template.write(Hex.decode("92")); template.write(encodeLength(data.size())); - if (format.getAlgorithmFormat().isWithPubkey()) { + if (format.ecAlgorithmFormat().isWithPubkey()) { data.write(Hex.decode("04")); writeBits(data, publicKey.getW().getAffineX(), csize); writeBits(data, publicKey.getW().getAffineY(), csize); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java index 83c8349de..b723518d3 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java @@ -198,10 +198,10 @@ public class PsoDecryptTokenOp { private byte[] getEcDecipherPayload(ECKeyFormat eckf, byte[] encryptedPoint) throws CardException { // TODO is this the right curve? - if (CryptlibObjectIdentifiers.curvey25519.equals(eckf.getCurveOID())) { + if (CryptlibObjectIdentifiers.curvey25519.equals(eckf.asn1ParseOid())) { return Arrays.copyOfRange(encryptedPoint, 1, 33); } else { - X9ECParameters x9Params = ECNamedCurveTable.getByOID(eckf.getCurveOID()); + X9ECParameters x9Params = ECNamedCurveTable.getByOID(eckf.asn1ParseOid()); ECPoint p = x9Params.getCurve().decodePoint(encryptedPoint); if (!p.isValid()) { throw new CardException("Invalid EC point!");