fix yubikey curve OID bug by reworking ASN1 parsing

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;
+    public abstract byte[] oidField();
-    private final ASN1ObjectIdentifier mECCurveOID;
 
-    public ECKeyFormat(final ASN1ObjectIdentifier ecCurveOid,
+    @Nullable // TODO
-                       final ECAlgorithmFormat ecAlgorithmFormat) {
+    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() {
+    public static KeyFormat getInstance(byte[] oidField, ECAlgorithmFormat from) {
-        return mECAlgorithmFormat;
+        return new AutoValue_ECKeyFormat(oidField, from);
     }
 
-    public ASN1ObjectIdentifier getCurveOID() {
+    public static ECKeyFormat getInstance(ASN1ObjectIdentifier oidAsn1, ECAlgorithmFormat from) {
-        return mECCurveOID;
+        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((byte) PublicKeyAlgorithmTags.ECDH, true, false),
-        ECDH_WITH_PUBKEY((byte) 18, true, true),
+        ECDH_WITH_PUBKEY((byte) PublicKeyAlgorithmTags.ECDH, true, true),
-        ECDSA((byte) 19, false, false),
+        ECDSA((byte) PublicKeyAlgorithmTags.ECDSA, false, false),
-        ECDSA_WITH_PUBKEY((byte) 19, false, true);
+        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) {
+        ECAlgorithmFormat(final byte algorithmId, final boolean isECDH, final boolean withPubkey) {
-            mValue = value;
+            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() {
+        public final byte getAlgorithmId() {
-            return mValue;
+            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));

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) {
+                return ECKeyFormat.getInstanceFromBytes(bytes);
-                    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]));
             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);

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");
         }
 

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);

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!");