add test case for duplicate keys in keyring (#870)

3 files changed, 75 insertions, 4 deletions

diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java
index 4fae49694..e8232a934 100644
--- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java
+++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java
@@ -26,8 +26,10 @@ import org.junit.Before;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.shadows.ShadowLog;
 import org.spongycastle.bcpg.BCPGInputStream;
+import org.spongycastle.bcpg.HashAlgorithmTags;
 import org.spongycastle.bcpg.Packet;
 import org.spongycastle.bcpg.PacketTags;
+import org.spongycastle.bcpg.SymmetricKeyAlgorithmTags;
 import org.spongycastle.bcpg.UserIDPacket;
 import org.spongycastle.bcpg.sig.KeyFlags;
 import org.spongycastle.jce.provider.BouncyCastleProvider;
@@ -40,16 +42,22 @@ import org.spongycastle.openpgp.PGPSignatureGenerator;
 import org.spongycastle.openpgp.PGPSignatureSubpacketGenerator;
 import org.spongycastle.openpgp.PGPUtil;
 import org.spongycastle.openpgp.operator.PBESecretKeyDecryptor;
+import org.spongycastle.openpgp.operator.PBESecretKeyEncryptor;
 import org.spongycastle.openpgp.operator.PGPContentSignerBuilder;
+import org.spongycastle.openpgp.operator.PGPDigestCalculator;
 import org.spongycastle.openpgp.operator.jcajce.JcaKeyFingerprintCalculator;
 import org.spongycastle.openpgp.operator.jcajce.JcaPGPContentSignerBuilder;
+import org.spongycastle.openpgp.operator.jcajce.JcaPGPDigestCalculatorProviderBuilder;
 import org.spongycastle.openpgp.operator.jcajce.JcePBESecretKeyDecryptorBuilder;
+import org.spongycastle.openpgp.operator.jcajce.JcePBESecretKeyEncryptorBuilder;
 import org.spongycastle.util.Strings;
 import org.sufficientlysecure.keychain.Constants;
 import org.sufficientlysecure.keychain.service.results.OperationResult;
 import org.sufficientlysecure.keychain.service.results.EditKeyResult;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel.Algorithm;
+import org.sufficientlysecure.keychain.service.results.OperationResult.LogType;
+import org.sufficientlysecure.keychain.service.results.OperationResult.OperationLog;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper.RawPacket;
 
@@ -465,6 +473,69 @@ public class UncachedKeyringCanonicalizeTest {
 
     }
 
+    @Test
+    public void testDuplicateSubkey() throws Exception {
+
+        { // duplicate subkey
+
+            // get subkey packets
+            Iterator<RawPacket> it = KeyringTestingHelper.parseKeyring(ring.getEncoded());
+            RawPacket subKey = KeyringTestingHelper.getNth(it, 5);
+            RawPacket subSig = it.next();
+
+            // inject at a second position
+            UncachedKeyRing modified = ring;
+            modified = KeyringTestingHelper.injectPacket(modified, subKey.buf, 7);
+            modified = KeyringTestingHelper.injectPacket(modified, subSig.buf, 8);
+
+            // canonicalize, and check if we lose the bad signature
+            OperationLog log = new OperationLog();
+            CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0);
+            Assert.assertNull("canonicalization with duplicate subkey should fail", canonicalized);
+            Assert.assertTrue("log should contain dup_key event", log.containsType(LogType.MSG_KC_ERROR_DUP_KEY));
+        }
+
+        { // duplicate subkey, which is the same as the master key
+
+            // We actually encountered one of these in the wild:
+            // https://www.sparkasse-holstein.de/firmenkunden/electronic_banking/secure-e-mail/pdf/Spk_Holstein_PGP_Domain-Zertifikat.asc
+
+            CanonicalizedSecretKeyRing canonicalized = (CanonicalizedSecretKeyRing) ring.canonicalize(log, 0);
+
+            CanonicalizedSecretKey masterSecretKey = canonicalized.getSecretKey();
+            masterSecretKey.unlock("");
+            PGPPublicKey masterPublicKey = masterSecretKey.getPublicKey();
+            PGPSignature cert = PgpKeyOperation.generateSubkeyBindingSignature(
+                    masterPublicKey, masterSecretKey.getPrivateKey(), masterSecretKey.getPrivateKey(),
+                    masterPublicKey, masterSecretKey.getKeyUsage(), 0);
+            PGPPublicKey subPubKey = PGPPublicKey.addSubkeyBindingCertification(masterPublicKey, cert);
+
+            PGPSecretKey sKey;
+            {
+                // Build key encrypter and decrypter based on passphrase
+                PGPDigestCalculator encryptorHashCalc = new JcaPGPDigestCalculatorProviderBuilder()
+                        .build().get(HashAlgorithmTags.SHA256);
+                PBESecretKeyEncryptor keyEncryptor = new JcePBESecretKeyEncryptorBuilder(
+                        SymmetricKeyAlgorithmTags.AES_256, encryptorHashCalc, 10)
+                        .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build("".toCharArray());
+
+                // NOTE: only SHA1 is supported for key checksum calculations.
+                PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder()
+                        .build().get(HashAlgorithmTags.SHA1);
+                sKey = new PGPSecretKey(masterSecretKey.getPrivateKey(), subPubKey, sha1Calc, false, keyEncryptor);
+            }
+
+            UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sKey.getEncoded(), 5);
+
+            // canonicalize, and check if we lose the bad signature
+            OperationLog log = new OperationLog();
+            CanonicalizedKeyRing result = modified.canonicalize(log, 0);
+            Assert.assertNull("canonicalization with duplicate subkey (from master) should fail", result);
+            Assert.assertTrue("log should contain dup_key event", log.containsType(LogType.MSG_KC_ERROR_DUP_KEY));
+        }
+
+    }
+
     private static final int[] sigtypes_direct = new int[] {
         PGPSignature.KEY_REVOCATION,
         PGPSignature.DIRECT_KEY,
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
index bf28465f5..4106ab73d 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
@@ -329,9 +329,9 @@ public class CanonicalizedSecretKey extends CanonicalizedPublicKey {
         return new UncachedSecretKey(mSecretKey);
     }
 
-    // HACK
-    public PGPSecretKey getSecretKey() {
-        return mSecretKey;
+    // HACK, for TESTING ONLY!!
+    PGPPrivateKey getPrivateKey () {
+        return mPrivateKey;
     }
 
 }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
index 0a8774029..c4070e39a 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java
@@ -1118,7 +1118,7 @@ public class PgpKeyOperation {
                 pKey, flags, expiry);
     }
 
-    private static PGPSignature generateSubkeyBindingSignature(
+    static PGPSignature generateSubkeyBindingSignature(
             PGPPublicKey masterPublicKey, PGPPrivateKey masterPrivateKey,
             PGPPrivateKey subPrivateKey, PGPPublicKey pKey, int flags, long expiry)
             throws IOException, PGPException, SignatureException {