diff options
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 { |