From c7b0b650c1467f7f0d7d8d62798c458597be0225 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 30 Dec 2014 00:44:12 +0100 Subject: introduce handling of notation direct key signatures, and experimental pin notation packet --- .../keychain/pgp/PgpKeyOperation.java | 49 ++++++++++++++++-- .../keychain/pgp/UncachedKeyRing.java | 59 ++++++++++++++++++---- .../keychain/pgp/WrappedSignature.java | 18 +++++++ 3 files changed, 112 insertions(+), 14 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') 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 c125165a8..08d8164ca 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -34,6 +34,7 @@ import org.spongycastle.openpgp.PGPSecretKeyRing; import org.spongycastle.openpgp.PGPSignature; import org.spongycastle.openpgp.PGPSignatureGenerator; import org.spongycastle.openpgp.PGPSignatureSubpacketGenerator; +import org.spongycastle.openpgp.PGPSignatureSubpacketVector; import org.spongycastle.openpgp.operator.PBESecretKeyDecryptor; import org.spongycastle.openpgp.operator.PBESecretKeyEncryptor; import org.spongycastle.openpgp.operator.PGPContentSignerBuilder; @@ -877,7 +878,8 @@ public class PgpKeyOperation { log.add(LogType.MSG_MF_PASSPHRASE, indent); indent += 1; - sKR = applyNewUnlock(sKR, masterPublicKey, passphrase, saveParcel.mNewUnlock, log, indent); + sKR = applyNewUnlock(sKR, masterPublicKey, masterPrivateKey, + passphrase, saveParcel.mNewUnlock, log, indent); if (sKR == null) { // The error has been logged above, just return a bad state return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); @@ -909,19 +911,60 @@ public class PgpKeyOperation { private static PGPSecretKeyRing applyNewUnlock( PGPSecretKeyRing sKR, PGPPublicKey masterPublicKey, + PGPPrivateKey masterPrivateKey, String passphrase, ChangeUnlockParcel newUnlock, OperationLog log, int indent) throws PGPException { if (newUnlock.mNewPassphrase != null) { - return applyNewPassphrase(sKR, masterPublicKey, passphrase, newUnlock.mNewPassphrase, log, indent); + sKR = applyNewPassphrase(sKR, masterPublicKey, passphrase, newUnlock.mNewPassphrase, log, indent); + + // add packet with EMPTY notation data (updates old one, but will be stripped later) + PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( + masterPrivateKey.getPublicKeyPacket().getAlgorithm(), HashAlgorithmTags.SHA512) + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); + PGPSignatureGenerator sGen = new PGPSignatureGenerator(signerBuilder); + { // set subpackets + PGPSignatureSubpacketGenerator hashedPacketsGen = new PGPSignatureSubpacketGenerator(); + hashedPacketsGen.setExportable(false, false); + sGen.setHashedSubpackets(hashedPacketsGen.generate()); + } + sGen.init(PGPSignature.DIRECT_KEY, masterPrivateKey); + PGPSignature emptySig = sGen.generateCertification(masterPublicKey); + + masterPublicKey = PGPPublicKey.addCertification(masterPublicKey, emptySig); + PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey); + + return sKR; + } + + if (newUnlock.mNewPin != null) { + sKR = applyNewPassphrase(sKR, masterPublicKey, passphrase, newUnlock.mNewPin, log, indent); + + // add packet with EMPTY notation data (updates old one, but will be stripped later) + PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( + masterPrivateKey.getPublicKeyPacket().getAlgorithm(), HashAlgorithmTags.SHA512) + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); + PGPSignatureGenerator sGen = new PGPSignatureGenerator(signerBuilder); + { // set subpackets + PGPSignatureSubpacketGenerator hashedPacketsGen = new PGPSignatureSubpacketGenerator(); + hashedPacketsGen.setExportable(false, false); + hashedPacketsGen.setNotationData(false, false, "pin@unlock.sufficientlysecure.org", "1"); + sGen.setHashedSubpackets(hashedPacketsGen.generate()); + } + sGen.init(PGPSignature.DIRECT_KEY, masterPrivateKey); + PGPSignature emptySig = sGen.generateCertification(masterPublicKey); + + masterPublicKey = PGPPublicKey.addCertification(masterPublicKey, emptySig); + PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey); + + return sKR; } throw new UnsupportedOperationException("PIN passphrases not yet implemented!"); } - private static PGPSecretKeyRing applyNewPassphrase( PGPSecretKeyRing sKR, PGPPublicKey masterPublicKey, diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index b343c779a..8d1eaa40e 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -41,7 +41,6 @@ import org.sufficientlysecure.keychain.util.IterableIterator; import org.sufficientlysecure.keychain.util.Log; import org.sufficientlysecure.keychain.util.Utf8Util; -import java.io.BufferedInputStream; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -302,6 +301,7 @@ public class UncachedKeyRing { PGPPublicKey modified = masterKey; PGPSignature revocation = null; + PGPSignature notation = null; for (PGPSignature zert : new IterableIterator(masterKey.getKeySignatures())) { int type = zert.getSignatureType(); @@ -318,9 +318,9 @@ public class UncachedKeyRing { } WrappedSignature cert = new WrappedSignature(zert); - if (type != PGPSignature.KEY_REVOCATION) { + if (type != PGPSignature.KEY_REVOCATION && type != PGPSignature.DIRECT_KEY) { // Unknown type, just remove - log.add(LogType.MSG_KC_REVOKE_BAD_TYPE, indent, "0x" + Integer.toString(type, 16)); + log.add(LogType.MSG_KC_BAD_TYPE, indent, "0x" + Integer.toString(type, 16)); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; @@ -334,14 +334,6 @@ public class UncachedKeyRing { continue; } - if (cert.isLocal()) { - // Remove revocation certs with "local" flag - log.add(LogType.MSG_KC_REVOKE_BAD_LOCAL, indent); - modified = PGPPublicKey.removeCertification(modified, zert); - badCerts += 1; - continue; - } - try { cert.init(masterKey); if (!cert.verifySignature(masterKey)) { @@ -357,6 +349,41 @@ public class UncachedKeyRing { continue; } + if (cert.isLocal()) { + // Remove revocation certs with "local" flag + log.add(LogType.MSG_KC_REVOKE_BAD_LOCAL, indent); + modified = PGPPublicKey.removeCertification(modified, zert); + badCerts += 1; + continue; + } + + // special case: direct key signatures! + if (cert.getSignatureType() == PGPSignature.DIRECT_KEY) { + // must be local, otherwise strip! + if (!cert.isLocal()) { + log.add(LogType.MSG_KC_BAD_TYPE, indent); + modified = PGPPublicKey.removeCertification(modified, zert); + badCerts += 1; + continue; + } + + // first notation? fine then. + if (notation == null) { + notation = zert; + // more notations? at least one is superfluous, then. + } else if (notation.getCreationTime().before(zert.getCreationTime())) { + log.add(LogType.MSG_KC_NOTATION_DUP, indent); + modified = PGPPublicKey.removeCertification(modified, notation); + redundantCerts += 1; + notation = zert; + } else { + log.add(LogType.MSG_KC_NOTATION_DUP, indent); + modified = PGPPublicKey.removeCertification(modified, zert); + redundantCerts += 1; + } + continue; + } + // first revocation? fine then. if (revocation == null) { revocation = zert; @@ -373,6 +400,16 @@ public class UncachedKeyRing { } } + // If we have a notation packet, check if there is even any data in it? + if (notation != null) { + // If there isn't, might as well strip it + if (new WrappedSignature(notation).getNotation().isEmpty()) { + log.add(LogType.MSG_KC_NOTATION_EMPTY, indent); + modified = PGPPublicKey.removeCertification(modified, notation); + redundantCerts += 1; + } + } + ArrayList processedUserIds = new ArrayList(); for (byte[] rawUserId : new IterableIterator(masterKey.getRawUserIDs())) { String userId = Utf8Util.fromUTF8ByteArrayReplaceBadEncoding(rawUserId); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java index 93afb987a..132a28604 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java @@ -21,6 +21,7 @@ package org.sufficientlysecure.keychain.pgp; import org.spongycastle.bcpg.SignatureSubpacket; import org.spongycastle.bcpg.SignatureSubpacketTags; import org.spongycastle.bcpg.sig.Exportable; +import org.spongycastle.bcpg.sig.NotationData; import org.spongycastle.bcpg.sig.Revocable; import org.spongycastle.bcpg.sig.RevocationReason; import org.spongycastle.openpgp.PGPException; @@ -37,6 +38,7 @@ import java.io.IOException; import java.security.SignatureException; import java.util.ArrayList; import java.util.Date; +import java.util.HashMap; /** OpenKeychain wrapper around PGPSignature objects. * @@ -239,4 +241,20 @@ public class WrappedSignature { SignatureSubpacket p = mSig.getHashedSubPackets().getSubpacket(SignatureSubpacketTags.EXPORTABLE); return ! ((Exportable) p).isExportable(); } + + public HashMap getNotation() { + HashMap result = new HashMap(); + + // If there is any notation data + if (mSig.getHashedSubPackets() != null + && mSig.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.NOTATION_DATA)) { + // Iterate over notation data + for (NotationData data : mSig.getHashedSubPackets().getNotationDataOccurrences()) { + result.put(data.getNotationName(), data.getNotationValueBytes()); + } + } + + return result; + } + } -- cgit v1.2.3 From 438405d3d2b12e6254ea166cd402ae6ed3a4209c Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 30 Dec 2014 12:34:24 +0100 Subject: add PIN as a SecretKeyType --- .../keychain/pgp/CanonicalizedSecretKey.java | 10 +++++++++- .../keychain/pgp/CanonicalizedSecretKeyRing.java | 14 ++++++++++++++ .../sufficientlysecure/keychain/pgp/PgpKeyOperation.java | 2 +- .../sufficientlysecure/keychain/pgp/WrappedSignature.java | 6 +++--- 4 files changed, 27 insertions(+), 5 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') 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 f9fa41528..5c99a9854 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java @@ -49,6 +49,7 @@ import java.security.NoSuchAlgorithmException; import java.security.NoSuchProviderException; import java.security.SignatureException; import java.util.Date; +import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -83,7 +84,7 @@ public class CanonicalizedSecretKey extends CanonicalizedPublicKey { } public enum SecretKeyType { - UNAVAILABLE(0), GNU_DUMMY(1), PASSPHRASE(2), PASSPHRASE_EMPTY(3), DIVERT_TO_CARD(4); + UNAVAILABLE(0), GNU_DUMMY(1), PASSPHRASE(2), PASSPHRASE_EMPTY(3), DIVERT_TO_CARD(4), PIN(5); final int mNum; @@ -101,6 +102,8 @@ public class CanonicalizedSecretKey extends CanonicalizedPublicKey { return PASSPHRASE_EMPTY; case 4: return DIVERT_TO_CARD; + case 5: + return PIN; // if this case happens, it's probably a check from a database value default: return UNAVAILABLE; @@ -135,6 +138,11 @@ public class CanonicalizedSecretKey extends CanonicalizedPublicKey { // It means the passphrase is empty return SecretKeyType.PASSPHRASE_EMPTY; } catch (PGPException e) { + HashMap notation = getRing().getLocalNotationData(); + if (notation.containsKey("unlock.pin@sufficientlysecure.org") + && "1".equals(notation.get("unlock.pin@sufficientlysecure.org"))) { + return SecretKeyType.PIN; + } // Otherwise, it's just a regular ol' passphrase return SecretKeyType.PASSPHRASE; } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java index e20155cc6..eb589c3f9 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java @@ -26,6 +26,7 @@ import org.spongycastle.openpgp.PGPPrivateKey; import org.spongycastle.openpgp.PGPPublicKey; import org.spongycastle.openpgp.PGPSecretKey; import org.spongycastle.openpgp.PGPSecretKeyRing; +import org.spongycastle.openpgp.PGPSignature; import org.spongycastle.openpgp.operator.PBESecretKeyDecryptor; import org.spongycastle.openpgp.operator.jcajce.JcePBESecretKeyDecryptorBuilder; import org.sufficientlysecure.keychain.Constants; @@ -36,6 +37,7 @@ import org.sufficientlysecure.keychain.util.IterableIterator; import org.sufficientlysecure.keychain.util.Log; import java.io.IOException; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -130,4 +132,16 @@ public class CanonicalizedSecretKeyRing extends CanonicalizedKeyRing { }); } + public HashMap getLocalNotationData() { + HashMap result = new HashMap(); + Iterator it = getRing().getPublicKey().getKeySignatures(); + while (it.hasNext()) { + WrappedSignature sig = new WrappedSignature(it.next()); + if (sig.isLocal()) { + result.putAll(sig.getNotation()); + } + } + return result; + } + } 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 08d8164ca..6fedbc683 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -949,7 +949,7 @@ public class PgpKeyOperation { { // set subpackets PGPSignatureSubpacketGenerator hashedPacketsGen = new PGPSignatureSubpacketGenerator(); hashedPacketsGen.setExportable(false, false); - hashedPacketsGen.setNotationData(false, false, "pin@unlock.sufficientlysecure.org", "1"); + hashedPacketsGen.setNotationData(false, true, "unlock.pin@sufficientlysecure.org", "1"); sGen.setHashedSubpackets(hashedPacketsGen.generate()); } sGen.init(PGPSignature.DIRECT_KEY, masterPrivateKey); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java index 132a28604..c395ca52d 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java @@ -242,15 +242,15 @@ public class WrappedSignature { return ! ((Exportable) p).isExportable(); } - public HashMap getNotation() { - HashMap result = new HashMap(); + public HashMap getNotation() { + HashMap result = new HashMap(); // If there is any notation data if (mSig.getHashedSubPackets() != null && mSig.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.NOTATION_DATA)) { // Iterate over notation data for (NotationData data : mSig.getHashedSubPackets().getNotationDataOccurrences()) { - result.put(data.getNotationName(), data.getNotationValueBytes()); + result.put(data.getNotationName(), data.getNotationValue()); } } -- cgit v1.2.3 From 2223cdd4056341576a540b065bcb34a1e579b141 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 30 Dec 2014 13:07:56 +0100 Subject: some small notation data fixes --- .../sufficientlysecure/keychain/pgp/PgpKeyOperation.java | 6 ++++-- .../sufficientlysecure/keychain/pgp/UncachedKeyRing.java | 14 ++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') 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 6fedbc683..c8ce349f8 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -933,7 +933,8 @@ public class PgpKeyOperation { PGPSignature emptySig = sGen.generateCertification(masterPublicKey); masterPublicKey = PGPPublicKey.addCertification(masterPublicKey, emptySig); - PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey); + sKR = PGPSecretKeyRing.insertSecretKey(sKR, + PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey)); return sKR; } @@ -956,7 +957,8 @@ public class PgpKeyOperation { PGPSignature emptySig = sGen.generateCertification(masterPublicKey); masterPublicKey = PGPPublicKey.addCertification(masterPublicKey, emptySig); - PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey); + sKR = PGPSecretKeyRing.insertSecretKey(sKR, + PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey)); return sKR; } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index 8d1eaa40e..5e5a28e83 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -349,14 +349,6 @@ public class UncachedKeyRing { continue; } - if (cert.isLocal()) { - // Remove revocation certs with "local" flag - log.add(LogType.MSG_KC_REVOKE_BAD_LOCAL, indent); - modified = PGPPublicKey.removeCertification(modified, zert); - badCerts += 1; - continue; - } - // special case: direct key signatures! if (cert.getSignatureType() == PGPSignature.DIRECT_KEY) { // must be local, otherwise strip! @@ -382,6 +374,12 @@ public class UncachedKeyRing { redundantCerts += 1; } continue; + } else if (cert.isLocal()) { + // Remove revocation certs with "local" flag + log.add(LogType.MSG_KC_REVOKE_BAD_LOCAL, indent); + modified = PGPPublicKey.removeCertification(modified, zert); + badCerts += 1; + continue; } // first revocation? fine then. -- cgit v1.2.3 From 5e9d8ca8c6af6a3c274208799cd4b2a22de2d843 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 1 Jan 2015 20:21:32 +0100 Subject: tests: add multi key encryption/decryption test, new way for fake passphrase cache --- .../main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java | 2 +- .../main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncrypt.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java index 4f086c2a6..f5f0ce9a4 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java @@ -84,7 +84,7 @@ public class PgpDecryptVerify extends BaseOperation { private boolean mDecryptMetadataOnly; private byte[] mDecryptedSessionKey; - private PgpDecryptVerify(Builder builder) { + protected PgpDecryptVerify(Builder builder) { super(builder.mContext, builder.mProviderHelper, builder.mProgressable); // private Constructor can only be called from Builder diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncrypt.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncrypt.java index f89027a19..3c3bcc890 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncrypt.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncrypt.java @@ -95,7 +95,7 @@ public class PgpSignEncrypt extends BaseOperation { } } - private PgpSignEncrypt(Builder builder) { + protected PgpSignEncrypt(Builder builder) { super(builder.mContext, builder.mProviderHelper, builder.mProgressable); // private Constructor can only be called from Builder -- cgit v1.2.3 From 2d38079574381ddafc47bd1f31dc6e35a2d67b6a Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 1 Jan 2015 20:46:39 +0100 Subject: add empty notation data packet only if necessary --- .../keychain/pgp/PgpKeyOperation.java | 51 +++++++++++++++------- 1 file changed, 35 insertions(+), 16 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') 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 c8ce349f8..e39bde6b0 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -919,22 +919,25 @@ public class PgpKeyOperation { if (newUnlock.mNewPassphrase != null) { sKR = applyNewPassphrase(sKR, masterPublicKey, passphrase, newUnlock.mNewPassphrase, log, indent); - // add packet with EMPTY notation data (updates old one, but will be stripped later) - PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( - masterPrivateKey.getPublicKeyPacket().getAlgorithm(), HashAlgorithmTags.SHA512) - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); - PGPSignatureGenerator sGen = new PGPSignatureGenerator(signerBuilder); - { // set subpackets - PGPSignatureSubpacketGenerator hashedPacketsGen = new PGPSignatureSubpacketGenerator(); - hashedPacketsGen.setExportable(false, false); - sGen.setHashedSubpackets(hashedPacketsGen.generate()); - } - sGen.init(PGPSignature.DIRECT_KEY, masterPrivateKey); - PGPSignature emptySig = sGen.generateCertification(masterPublicKey); + // if there is any old packet with notation data + if (hasNotationData(sKR)) { + // add packet with EMPTY notation data (updates old one, but will be stripped later) + PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( + masterPrivateKey.getPublicKeyPacket().getAlgorithm(), HashAlgorithmTags.SHA512) + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); + PGPSignatureGenerator sGen = new PGPSignatureGenerator(signerBuilder); + { // set subpackets + PGPSignatureSubpacketGenerator hashedPacketsGen = new PGPSignatureSubpacketGenerator(); + hashedPacketsGen.setExportable(false, false); + sGen.setHashedSubpackets(hashedPacketsGen.generate()); + } + sGen.init(PGPSignature.DIRECT_KEY, masterPrivateKey); + PGPSignature emptySig = sGen.generateCertification(masterPublicKey); - masterPublicKey = PGPPublicKey.addCertification(masterPublicKey, emptySig); - sKR = PGPSecretKeyRing.insertSecretKey(sKR, - PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey)); + masterPublicKey = PGPPublicKey.addCertification(masterPublicKey, emptySig); + sKR = PGPSecretKeyRing.insertSecretKey(sKR, + PGPSecretKey.replacePublicKey(sKR.getSecretKey(), masterPublicKey)); + } return sKR; } @@ -942,7 +945,7 @@ public class PgpKeyOperation { if (newUnlock.mNewPin != null) { sKR = applyNewPassphrase(sKR, masterPublicKey, passphrase, newUnlock.mNewPin, log, indent); - // add packet with EMPTY notation data (updates old one, but will be stripped later) + // add packet with "pin" notation data PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( masterPrivateKey.getPublicKeyPacket().getAlgorithm(), HashAlgorithmTags.SHA512) .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); @@ -967,6 +970,22 @@ public class PgpKeyOperation { } + /** This method returns true iff the provided keyring has a local direct key signature + * with notation data. + */ + private static boolean hasNotationData(PGPSecretKeyRing sKR) { + // noinspection unchecked + Iterator sigs = sKR.getPublicKey().getKeySignatures(); + while (sigs.hasNext()) { + WrappedSignature sig = new WrappedSignature(sigs.next()); + if (sig.getSignatureType() == PGPSignature.DIRECT_KEY + && sig.isLocal() && !sig.getNotation().isEmpty()) { + return true; + } + } + return false; + } + private static PGPSecretKeyRing applyNewPassphrase( PGPSecretKeyRing sKR, PGPPublicKey masterPublicKey, -- cgit v1.2.3 From 1c1ae769ef78a9bd3c95f97b95215616ee9b15db Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 1 Jan 2015 21:13:07 +0100 Subject: small improvements regarding pin tests and logging --- .../java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') 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 e39bde6b0..5ac5f7a9a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -921,6 +921,9 @@ public class PgpKeyOperation { // if there is any old packet with notation data if (hasNotationData(sKR)) { + + log.add(LogType.MSG_MF_NOTATION_EMPTY, indent); + // add packet with EMPTY notation data (updates old one, but will be stripped later) PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( masterPrivateKey.getPublicKeyPacket().getAlgorithm(), HashAlgorithmTags.SHA512) @@ -945,6 +948,8 @@ public class PgpKeyOperation { if (newUnlock.mNewPin != null) { sKR = applyNewPassphrase(sKR, masterPublicKey, passphrase, newUnlock.mNewPin, log, indent); + log.add(LogType.MSG_MF_NOTATION_PIN, indent); + // add packet with "pin" notation data PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( masterPrivateKey.getPublicKeyPacket().getAlgorithm(), HashAlgorithmTags.SHA512) -- cgit v1.2.3 From 7223abcf0cfa99a866a10483b017555fb768738f Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 2 Jan 2015 00:07:05 +0100 Subject: extend canonicalize to strip local certificates on export --- .../keychain/pgp/UncachedKeyRing.java | 53 ++++++++++++++++++---- 1 file changed, 45 insertions(+), 8 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index 5e5a28e83..c4cd0b3e5 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -266,6 +266,35 @@ public class UncachedKeyRing { */ @SuppressWarnings("ConstantConditions") public CanonicalizedKeyRing canonicalize(OperationLog log, int indent) { + return canonicalize(log, indent, false); + } + + + /** "Canonicalizes" a public key, removing inconsistencies in the process. + * + * More specifically: + * - Remove all non-verifying self-certificates + * - Remove all "future" self-certificates + * - Remove all certificates flagged as "local" + * - Remove all certificates which are superseded by a newer one on the same target, + * including revocations with later re-certifications. + * - Remove all certificates in other positions if not of known type: + * - key revocation signatures on the master key + * - subkey binding signatures for subkeys + * - certifications and certification revocations for user ids + * - If a subkey retains no valid subkey binding certificate, remove it + * - If a user id retains no valid self certificate, remove it + * - If the key is a secret key, remove all certificates by foreign keys + * - If no valid user id remains, log an error and return null + * + * This operation writes an OperationLog which can be used as part of an OperationResultParcel. + * + * @param forExport if this is true, non-exportable signatures will be removed + * @return A canonicalized key, or null on fatal error (log will include a message in this case) + * + */ + @SuppressWarnings("ConstantConditions") + public CanonicalizedKeyRing canonicalize(OperationLog log, int indent, boolean forExport) { log.add(isSecret() ? LogType.MSG_KC_SECRET : LogType.MSG_KC_PUBLIC, indent, KeyFormattingUtils.convertKeyIdToHex(getMasterKeyId())); @@ -311,7 +340,7 @@ public class UncachedKeyRing { || type == PGPSignature.CASUAL_CERTIFICATION || type == PGPSignature.POSITIVE_CERTIFICATION || type == PGPSignature.CERTIFICATION_REVOCATION) { - log.add(LogType.MSG_KC_REVOKE_BAD_TYPE_UID, indent); + log.add(LogType.MSG_KC_MASTER_BAD_TYPE_UID, indent); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; @@ -320,7 +349,7 @@ public class UncachedKeyRing { if (type != PGPSignature.KEY_REVOCATION && type != PGPSignature.DIRECT_KEY) { // Unknown type, just remove - log.add(LogType.MSG_KC_BAD_TYPE, indent, "0x" + Integer.toString(type, 16)); + log.add(LogType.MSG_KC_MASTER_BAD_TYPE, indent, "0x" + Integer.toString(type, 16)); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; @@ -328,7 +357,7 @@ public class UncachedKeyRing { if (cert.getCreationTime().after(nowPlusOneDay)) { // Creation date in the future? No way! - log.add(LogType.MSG_KC_REVOKE_BAD_TIME, indent); + log.add(LogType.MSG_KC_MASTER_BAD_TIME, indent); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; @@ -337,23 +366,31 @@ public class UncachedKeyRing { try { cert.init(masterKey); if (!cert.verifySignature(masterKey)) { - log.add(LogType.MSG_KC_REVOKE_BAD, indent); + log.add(LogType.MSG_KC_MASTER_BAD, indent); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; } } catch (PgpGeneralException e) { - log.add(LogType.MSG_KC_REVOKE_BAD_ERR, indent); + log.add(LogType.MSG_KC_MASTER_BAD_ERR, indent); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; } - // special case: direct key signatures! + // if this is for export, we always remove any non-exportable certs + if (forExport && cert.isLocal()) { + // Remove revocation certs with "local" flag + log.add(LogType.MSG_KC_MASTER_LOCAL, indent); + modified = PGPPublicKey.removeCertification(modified, zert); + continue; + } + + // special case: non-exportable, direct key signatures for notations! if (cert.getSignatureType() == PGPSignature.DIRECT_KEY) { // must be local, otherwise strip! if (!cert.isLocal()) { - log.add(LogType.MSG_KC_BAD_TYPE, indent); + log.add(LogType.MSG_KC_MASTER_BAD_TYPE, indent); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; @@ -376,7 +413,7 @@ public class UncachedKeyRing { continue; } else if (cert.isLocal()) { // Remove revocation certs with "local" flag - log.add(LogType.MSG_KC_REVOKE_BAD_LOCAL, indent); + log.add(LogType.MSG_KC_MASTER_BAD_LOCAL, indent); modified = PGPPublicKey.removeCertification(modified, zert); badCerts += 1; continue; -- cgit v1.2.3 From b52fb903803c2dfca578c140dfdc776927c6fc4b Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 2 Jan 2015 01:57:49 +0100 Subject: fix and test for bad certificate version numbers (#1012) closes #1012 --- .../keychain/pgp/UncachedKeyRing.java | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index c4cd0b3e5..006af6073 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -143,17 +143,22 @@ public class UncachedKeyRing { throw new PgpGeneralException("Object not recognized as PGPKeyRing!"); } - UncachedKeyRing ring = parsed.next(); + try { + UncachedKeyRing ring = parsed.next(); - if (parsed.hasNext()) { - throw new PgpGeneralException("Expected single keyring in stream, found at least two"); - } + if (parsed.hasNext()) { + throw new PgpGeneralException("Expected single keyring in stream, found at least two"); + } - return ring; + return ring; + } catch (RuntimeException e) { + // yes this is bad style. we should rework this in a better way + throw new PgpGeneralException(e.getCause()); + } } - public static Iterator fromStream(final InputStream stream) throws IOException { + public static Iterator fromStream(final InputStream stream) { return new Iterator() { @@ -190,7 +195,8 @@ public class UncachedKeyRing { mObjectFactory = null; } } catch (IOException e) { - Log.e(Constants.TAG, "IOException while processing stream. ArmoredInputStream CRC check failed?", e); + throw new RuntimeException(e); + // Log.e(Constants.TAG, "IOException while processing stream. ArmoredInputStream CRC check failed?", e); } catch (ArrayIndexOutOfBoundsException e) { Log.e(Constants.TAG, "ArmoredInputStream decode failed, symbol is not in decodingTable!", e); } -- cgit v1.2.3 From 56f2a3137bbe56fee328bfbece625cf4a2f9e51d Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 2 Jan 2015 02:05:12 +0100 Subject: get rid of RuntimeException control flow in UncachedKeyRing.fromStream --- .../keychain/pgp/UncachedKeyRing.java | 42 +++++++++------------- 1 file changed, 17 insertions(+), 25 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index 006af6073..a445e161f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -137,35 +137,30 @@ public class UncachedKeyRing { public static UncachedKeyRing decodeFromData(byte[] data) throws PgpGeneralException, IOException { - Iterator parsed = fromStream(new ByteArrayInputStream(data)); + IteratorWithIOThrow parsed = fromStream(new ByteArrayInputStream(data)); if ( ! parsed.hasNext()) { throw new PgpGeneralException("Object not recognized as PGPKeyRing!"); } - try { - UncachedKeyRing ring = parsed.next(); + UncachedKeyRing ring = parsed.next(); - if (parsed.hasNext()) { - throw new PgpGeneralException("Expected single keyring in stream, found at least two"); - } + if (parsed.hasNext()) { + throw new PgpGeneralException("Expected single keyring in stream, found at least two"); + } - return ring; + return ring; - } catch (RuntimeException e) { - // yes this is bad style. we should rework this in a better way - throw new PgpGeneralException(e.getCause()); - } } - public static Iterator fromStream(final InputStream stream) { + public static IteratorWithIOThrow fromStream(final InputStream stream) { - return new Iterator() { + return new IteratorWithIOThrow() { UncachedKeyRing mNext = null; PGPObjectFactory mObjectFactory = null; - private void cacheNext() { + private void cacheNext() throws IOException { if (mNext != null) { return; } @@ -194,22 +189,19 @@ public class UncachedKeyRing { // if we are past the while loop, that means the objectFactory had no next mObjectFactory = null; } - } catch (IOException e) { - throw new RuntimeException(e); - // Log.e(Constants.TAG, "IOException while processing stream. ArmoredInputStream CRC check failed?", e); } catch (ArrayIndexOutOfBoundsException e) { - Log.e(Constants.TAG, "ArmoredInputStream decode failed, symbol is not in decodingTable!", e); + throw new IOException(e); } } @Override - public boolean hasNext() { + public boolean hasNext() throws IOException { cacheNext(); return mNext != null; } @Override - public UncachedKeyRing next() { + public UncachedKeyRing next() throws IOException { try { cacheNext(); return mNext; @@ -217,15 +209,15 @@ public class UncachedKeyRing { mNext = null; } } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } }; } + public interface IteratorWithIOThrow { + public boolean hasNext() throws IOException; + public E next() throws IOException; + } + public void encodeArmored(OutputStream out, String version) throws IOException { ArmoredOutputStream aos = new ArmoredOutputStream(out); if (version != null) { -- cgit v1.2.3 From 5057ea1744eac05fc999db1756cf0d739ae41a72 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 2 Jan 2015 14:28:03 +0100 Subject: deal with non-existent user ids during import (#994) --- .../java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java index c4cacaca7..fe3ab96a5 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java @@ -186,12 +186,14 @@ public class UncachedPublicKey { } /** - * Returns primary user id if existing. If not, return first encountered user id. + * Returns primary user id if existing. If not, return first encountered user id. If there + * is no user id, return null (this can only happen for not yet canonicalized keys during import) */ public String getPrimaryUserIdWithFallback() { String userId = getPrimaryUserId(); if (userId == null) { - userId = (String) mPublicKey.getUserIDs().next(); + Iterator it = mPublicKey.getUserIDs(); + userId = it.hasNext() ? it.next() : null; } return userId; } -- cgit v1.2.3 From 320f7d35efb059b99b31506426554e9a8f138d8f Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sat, 3 Jan 2015 13:55:06 +0100 Subject: encapsulate high level edit key into new operation class --- .../keychain/pgp/PgpKeyOperation.java | 79 +++++++++++----------- 1 file changed, 39 insertions(+), 40 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') 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 5ac5f7a9a..128928bb3 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -34,7 +34,6 @@ import org.spongycastle.openpgp.PGPSecretKeyRing; import org.spongycastle.openpgp.PGPSignature; import org.spongycastle.openpgp.PGPSignatureGenerator; import org.spongycastle.openpgp.PGPSignatureSubpacketGenerator; -import org.spongycastle.openpgp.PGPSignatureSubpacketVector; import org.spongycastle.openpgp.operator.PBESecretKeyDecryptor; import org.spongycastle.openpgp.operator.PBESecretKeyEncryptor; import org.spongycastle.openpgp.operator.PGPContentSignerBuilder; @@ -50,7 +49,7 @@ import org.sufficientlysecure.keychain.R; import org.sufficientlysecure.keychain.operations.results.OperationResult; import org.sufficientlysecure.keychain.operations.results.OperationResult.LogType; import org.sufficientlysecure.keychain.operations.results.OperationResult.OperationLog; -import org.sufficientlysecure.keychain.operations.results.EditKeyResult; +import org.sufficientlysecure.keychain.operations.results.PgpEditKeyResult; import org.sufficientlysecure.keychain.service.SaveKeyringParcel; import org.sufficientlysecure.keychain.service.SaveKeyringParcel.Algorithm; import org.sufficientlysecure.keychain.service.SaveKeyringParcel.ChangeUnlockParcel; @@ -302,7 +301,7 @@ public class PgpKeyOperation { } } - public EditKeyResult createSecretKeyRing(SaveKeyringParcel saveParcel) { + public PgpEditKeyResult createSecretKeyRing(SaveKeyringParcel saveParcel) { OperationLog log = new OperationLog(); int indent = 0; @@ -315,23 +314,23 @@ public class PgpKeyOperation { if (saveParcel.mAddSubKeys.isEmpty()) { log.add(LogType.MSG_CR_ERROR_NO_MASTER, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } if (saveParcel.mAddUserIds.isEmpty()) { log.add(LogType.MSG_CR_ERROR_NO_USER_ID, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } SubkeyAdd add = saveParcel.mAddSubKeys.remove(0); if ((add.mFlags & KeyFlags.CERTIFY_OTHER) != KeyFlags.CERTIFY_OTHER) { log.add(LogType.MSG_CR_ERROR_NO_CERTIFY, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } if (add.mExpiry == null) { log.add(LogType.MSG_CR_ERROR_NULL_EXPIRY, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } subProgressPush(10, 30); @@ -340,7 +339,7 @@ public class PgpKeyOperation { // return null if this failed (an error will already have been logged by createKey) if (keyPair == null) { - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } progress(R.string.progress_building_master_key, 40); @@ -367,10 +366,10 @@ public class PgpKeyOperation { } catch (PGPException e) { log.add(LogType.MSG_CR_ERROR_INTERNAL_PGP, indent); Log.e(Constants.TAG, "pgp error encoding key", e); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } catch (IOException e) { Log.e(Constants.TAG, "io error encoding key", e); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } } @@ -390,7 +389,7 @@ public class PgpKeyOperation { * handling of errors should be done in UI code! * */ - public EditKeyResult modifySecretKeyRing(CanonicalizedSecretKeyRing wsKR, SaveKeyringParcel saveParcel, + public PgpEditKeyResult modifySecretKeyRing(CanonicalizedSecretKeyRing wsKR, SaveKeyringParcel saveParcel, String passphrase) { OperationLog log = new OperationLog(); @@ -415,7 +414,7 @@ public class PgpKeyOperation { // Make sure this is called with a proper SaveKeyringParcel if (saveParcel.mMasterKeyId == null || saveParcel.mMasterKeyId != wsKR.getMasterKeyId()) { log.add(LogType.MSG_MF_ERROR_KEYID, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // We work on bouncycastle object level here @@ -426,7 +425,7 @@ public class PgpKeyOperation { if (saveParcel.mFingerprint == null || !Arrays.equals(saveParcel.mFingerprint, masterSecretKey.getPublicKey().getFingerprint())) { log.add(LogType.MSG_MF_ERROR_FINGERPRINT, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // read masterKeyFlags, and use the same as before. @@ -440,7 +439,7 @@ public class PgpKeyOperation { } - private EditKeyResult internal(PGPSecretKeyRing sKR, PGPSecretKey masterSecretKey, + private PgpEditKeyResult internal(PGPSecretKeyRing sKR, PGPSecretKey masterSecretKey, int masterKeyFlags, long masterKeyExpiry, SaveKeyringParcel saveParcel, String passphrase, OperationLog log) { @@ -462,7 +461,7 @@ public class PgpKeyOperation { masterPrivateKey = masterSecretKey.extractPrivateKey(keyDecryptor); } catch (PGPException e) { log.add(LogType.MSG_MF_UNLOCK_ERROR, indent + 1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } } @@ -471,7 +470,7 @@ public class PgpKeyOperation { // Check if we were cancelled if (checkCancelled()) { log.add(LogType.MSG_OPERATION_CANCELLED, indent); - return new EditKeyResult(EditKeyResult.RESULT_CANCELLED, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_CANCELLED, log, null); } { // work on master secret key @@ -488,7 +487,7 @@ public class PgpKeyOperation { if (userId.equals("")) { log.add(LogType.MSG_MF_UID_ERROR_EMPTY, indent + 1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // this operation supersedes all previous binding and revocation certificates, @@ -500,7 +499,7 @@ public class PgpKeyOperation { if (cert.getKeyID() != masterPublicKey.getKeyID()) { // foreign certificate?! error error error log.add(LogType.MSG_MF_ERROR_INTEGRITY, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } if (cert.getSignatureType() == PGPSignature.CERTIFICATION_REVOCATION || cert.getSignatureType() == PGPSignature.NO_CERTIFICATION @@ -542,7 +541,7 @@ public class PgpKeyOperation { } if (!exists) { log.add(LogType.MSG_MF_ERROR_NOEXIST_REVOKE, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // a duplicate revocation will be removed during canonicalization, so no need to @@ -573,7 +572,7 @@ public class PgpKeyOperation { if (cert.getKeyID() != masterPublicKey.getKeyID()) { // foreign certificate?! error error error log.add(LogType.MSG_MF_ERROR_INTEGRITY, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // we know from canonicalization that if there is any revocation here, it // is valid and not superseded by a newer certification. @@ -594,7 +593,7 @@ public class PgpKeyOperation { if (currentCert == null) { // no certificate found?! error error error log.add(LogType.MSG_MF_ERROR_INTEGRITY, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // we definitely should not update certifications of revoked keys, so just leave it. @@ -602,7 +601,7 @@ public class PgpKeyOperation { // revoked user ids cannot be primary! if (userId.equals(saveParcel.mChangePrimaryUserId)) { log.add(LogType.MSG_MF_ERROR_REVOKED_PRIMARY, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } continue; } @@ -651,7 +650,7 @@ public class PgpKeyOperation { if (!ok) { log.add(LogType.MSG_MF_ERROR_NOEXIST_PRIMARY, indent); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } } @@ -667,7 +666,7 @@ public class PgpKeyOperation { // Check if we were cancelled - again if (checkCancelled()) { log.add(LogType.MSG_OPERATION_CANCELLED, indent); - return new EditKeyResult(EditKeyResult.RESULT_CANCELLED, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_CANCELLED, log, null); } // 4a. For each subkey change, generate new subkey binding certificate @@ -683,7 +682,7 @@ public class PgpKeyOperation { if (sKey == null) { log.add(LogType.MSG_MF_ERROR_SUBKEY_MISSING, indent + 1, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId)); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // expiry must not be in the past @@ -691,7 +690,7 @@ public class PgpKeyOperation { new Date(change.mExpiry*1000).before(new Date())) { log.add(LogType.MSG_MF_ERROR_PAST_EXPIRY, indent + 1, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId)); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // if this is the master key, update uid certificates instead @@ -701,7 +700,7 @@ public class PgpKeyOperation { if ((flags & KeyFlags.CERTIFY_OTHER) != KeyFlags.CERTIFY_OTHER) { log.add(LogType.MSG_MF_ERROR_NO_CERTIFY, indent + 1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } PGPPublicKey pKey = @@ -709,7 +708,7 @@ public class PgpKeyOperation { flags, expiry, indent, log); if (pKey == null) { // error log entry has already been added by updateMasterCertificates itself - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } masterSecretKey = PGPSecretKey.replacePublicKey(sKey, pKey); masterPublicKey = pKey; @@ -764,7 +763,7 @@ public class PgpKeyOperation { if (sKey == null) { log.add(LogType.MSG_MF_ERROR_SUBKEY_MISSING, indent+1, KeyFormattingUtils.convertKeyIdToHex(revocation)); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } PGPPublicKey pKey = sKey.getPublicKey(); @@ -789,7 +788,7 @@ public class PgpKeyOperation { if (sKey == null) { log.add(LogType.MSG_MF_ERROR_SUBKEY_MISSING, indent+1, KeyFormattingUtils.convertKeyIdToHex(strip)); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // IT'S DANGEROUS~ @@ -807,7 +806,7 @@ public class PgpKeyOperation { // Check if we were cancelled - again. This operation is expensive so we do it each loop. if (checkCancelled()) { log.add(LogType.MSG_OPERATION_CANCELLED, indent); - return new EditKeyResult(EditKeyResult.RESULT_CANCELLED, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_CANCELLED, log, null); } progress(R.string.progress_modify_subkeyadd, (i-1) * (100 / saveParcel.mAddSubKeys.size())); @@ -817,12 +816,12 @@ public class PgpKeyOperation { if (add.mExpiry == null) { log.add(LogType.MSG_MF_ERROR_NULL_EXPIRY, indent +1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } if (add.mExpiry > 0L && new Date(add.mExpiry*1000).before(new Date())) { log.add(LogType.MSG_MF_ERROR_PAST_EXPIRY, indent +1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // generate a new secret key (privkey only for now) @@ -834,7 +833,7 @@ public class PgpKeyOperation { subProgressPop(); if (keyPair == null) { log.add(LogType.MSG_MF_ERROR_PGP, indent +1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } // add subkey binding signature (making this a sub rather than master key) @@ -869,7 +868,7 @@ public class PgpKeyOperation { // Check if we were cancelled - again. This operation is expensive so we do it each loop. if (checkCancelled()) { log.add(LogType.MSG_OPERATION_CANCELLED, indent); - return new EditKeyResult(EditKeyResult.RESULT_CANCELLED, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_CANCELLED, log, null); } // 6. If requested, change passphrase @@ -882,7 +881,7 @@ public class PgpKeyOperation { passphrase, saveParcel.mNewUnlock, log, indent); if (sKR == null) { // The error has been logged above, just return a bad state - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } indent -= 1; @@ -891,20 +890,20 @@ public class PgpKeyOperation { } catch (IOException e) { Log.e(Constants.TAG, "encountered IOException while modifying key", e); log.add(LogType.MSG_MF_ERROR_ENCODE, indent+1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } catch (PGPException e) { Log.e(Constants.TAG, "encountered pgp error while modifying key", e); log.add(LogType.MSG_MF_ERROR_PGP, indent+1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } catch (SignatureException e) { Log.e(Constants.TAG, "encountered SignatureException while modifying key", e); log.add(LogType.MSG_MF_ERROR_SIG, indent+1); - return new EditKeyResult(EditKeyResult.RESULT_ERROR, log, null); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } progress(R.string.progress_done, 100); log.add(LogType.MSG_MF_SUCCESS, indent); - return new EditKeyResult(OperationResult.RESULT_OK, log, new UncachedKeyRing(sKR)); + return new PgpEditKeyResult(OperationResult.RESULT_OK, log, new UncachedKeyRing(sKR)); } -- cgit v1.2.3 From 2192a6baddd09ae81f86a8bfd5e844086154f5ba Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sat, 3 Jan 2015 21:07:33 +0100 Subject: add more descriptive string about non-mdc encrypted data --- .../main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java index f5f0ce9a4..b58df085f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java @@ -633,7 +633,7 @@ public class PgpDecryptVerify extends BaseOperation { // Handle missing integrity protection like failed integrity protection! // The MDC packet can be stripped by an attacker! if (!signatureResultBuilder.isValidSignature()) { - log.add(LogType.MSG_DC_ERROR_INTEGRITY_CHECK, indent); + log.add(LogType.MSG_DC_ERROR_INTEGRITY_MISSING, indent); return new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log); } } -- cgit v1.2.3 From 47ace7cea31ee794ed88bdf4163dd38fc33e8fc5 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sat, 3 Jan 2015 22:14:12 +0100 Subject: make certify routines more robust (#1016) --- .../org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp') 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 42e59b3bc..6965ca7cb 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java @@ -300,6 +300,12 @@ public class CanonicalizedSecretKey extends CanonicalizedPublicKey { if (mPrivateKeyState == PRIVATE_KEY_STATE_LOCKED) { throw new PrivateKeyNotUnlockedException(); } + if (!isMasterKey()) { + throw new AssertionError("tried to certify with non-master key, this is a programming error!"); + } + if (publicKeyRing.getMasterKeyId() == getKeyId()) { + throw new AssertionError("key tried to self-certify, this is a programming error!"); + } // create a signatureGenerator from the supplied masterKeyId and passphrase PGPSignatureGenerator signatureGenerator; -- cgit v1.2.3