From de698b89552a02a445f808cf97d3ce94d35a2777 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sun, 29 Jun 2014 22:34:53 +0200 Subject: [PATCH 1/5] add create key capabilities to SaveKeyringParcel --- .../keychain/pgp/PgpKeyOperation.java | 76 ++++++++++++++++--- .../keychain/pgp/WrappedSecretKeyRing.java | 26 ------- .../service/KeychainIntentService.java | 44 +++++++---- .../service/OperationResultParcel.java | 5 ++ .../keychain/service/SaveKeyringParcel.java | 25 +++--- OpenKeychain/src/main/res/values/strings.xml | 2 + .../test/java/tests/PgpDecryptVerifyTest.java | 1 - 7 files changed, 118 insertions(+), 61 deletions(-) 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 1b59e7cc0..bde79eee0 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -38,6 +38,7 @@ 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.JcaPGPKeyPair; @@ -50,6 +51,8 @@ import org.sufficientlysecure.keychain.service.OperationResultParcel.LogLevel; import org.sufficientlysecure.keychain.service.OperationResultParcel.LogType; import org.sufficientlysecure.keychain.service.OperationResultParcel.OperationLog; import org.sufficientlysecure.keychain.service.SaveKeyringParcel; +import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyAdd; +import org.sufficientlysecure.keychain.util.Log; import org.sufficientlysecure.keychain.util.Primes; import java.io.IOException; @@ -175,6 +178,40 @@ public class PgpKeyOperation { } } + public UncachedKeyRing createSecretKeyRing(SaveKeyringParcel saveParcel, OperationLog log, + int indent) { + + try { + + log.add(LogLevel.ERROR, LogType.MSG_MF_ERROR_KEYID, indent); + indent += 1; + updateProgress(R.string.progress_building_key, 0, 100); + + if (saveParcel.addSubKeys == null || saveParcel.addSubKeys.isEmpty()) { + log.add(LogLevel.ERROR, LogType.MSG_CR_ERROR_NO_MASTER, indent); + return null; + } + + SubkeyAdd add = saveParcel.addSubKeys.remove(0); + PGPSecretKey masterSecretKey = createKey(add.mAlgorithm, add.mKeysize, + saveParcel.newPassphrase, true); + PGPSecretKeyRing sKR = new PGPSecretKeyRing( + masterSecretKey.getEncoded(), new JcaKeyFingerprintCalculator()); + + return internal(sKR, masterSecretKey, saveParcel, saveParcel.newPassphrase, log, indent); + + } catch (PGPException e) { + Log.e(Constants.TAG, "pgp error encoding key", e); + return null; + } catch (IOException e) { + Log.e(Constants.TAG, "io error encoding key", e); + return null; + } catch (PgpGeneralMsgIdException e) { + return null; + } + + } + /** This method introduces a list of modifications specified by a SaveKeyringParcel to a * WrappedSecretKeyRing. * @@ -204,28 +241,49 @@ public class PgpKeyOperation { indent += 1; updateProgress(R.string.progress_building_key, 0, 100); + // Make sure this is called with a proper SaveKeyringParcel + if (saveParcel.mMasterKeyId == null || saveParcel.mMasterKeyId != wsKR.getMasterKeyId()) { + log.add(LogLevel.ERROR, LogType.MSG_MF_ERROR_KEYID, indent); + return null; + } + // We work on bouncycastle object level here PGPSecretKeyRing sKR = wsKR.getRing(); - PGPPublicKey masterPublicKey = sKR.getPublicKey(); PGPSecretKey masterSecretKey = sKR.getSecretKey(); + // Make sure the fingerprint matches + if (saveParcel.mFingerprint == null + || !Arrays.equals(saveParcel.mFingerprint, + masterSecretKey.getPublicKey().getFingerprint())) { + log.add(LogLevel.ERROR, LogType.MSG_MF_ERROR_FINGERPRINT, indent); + return null; + } + + return internal(sKR, masterSecretKey, saveParcel, passphrase, log, indent); + + } + + private UncachedKeyRing internal(PGPSecretKeyRing sKR, PGPSecretKey masterSecretKey, + SaveKeyringParcel saveParcel, String passphrase, + OperationLog log, int indent) { + + updateProgress(R.string.progress_certifying_master_key, 20, 100); + + PGPPublicKey masterPublicKey = masterSecretKey.getPublicKey(); + // 1. Unlock private key log.add(LogLevel.DEBUG, LogType.MSG_MF_UNLOCK, indent); - PGPPrivateKey masterPrivateKey; { + PGPPrivateKey masterPrivateKey; + { try { PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder().setProvider( Constants.BOUNCY_CASTLE_PROVIDER_NAME).build(passphrase.toCharArray()); masterPrivateKey = masterSecretKey.extractPrivateKey(keyDecryptor); } catch (PGPException e) { - log.add(LogLevel.ERROR, LogType.MSG_MF_UNLOCK_ERROR, indent+1); + log.add(LogLevel.ERROR, LogType.MSG_MF_UNLOCK_ERROR, indent + 1); return null; } } - if (!Arrays.equals(saveParcel.mFingerprint, sKR.getPublicKey().getFingerprint())) { - return null; - } - - updateProgress(R.string.progress_certifying_master_key, 20, 100); // work on master secret key try { @@ -262,7 +320,7 @@ public class PgpKeyOperation { } - // 4a. For each subkey change, generate new subkey binding certificate + // 4a. For each subkey change, generate new subkey binding certificate for (SaveKeyringParcel.SubkeyChange change : saveParcel.changeSubKeys) { log.add(LogLevel.INFO, LogType.MSG_MF_SUBKEY_CHANGE, indent, PgpKeyHelper.convertKeyIdToHex(change.mKeyId)); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSecretKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSecretKeyRing.java index d7148f710..c737b7c46 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSecretKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSecretKeyRing.java @@ -8,16 +8,13 @@ import org.spongycastle.openpgp.PGPPublicKey; import org.spongycastle.openpgp.PGPSecretKey; import org.spongycastle.openpgp.PGPSecretKeyRing; import org.spongycastle.openpgp.operator.PBESecretKeyDecryptor; -import org.spongycastle.openpgp.operator.jcajce.JcaPGPDigestCalculatorProviderBuilder; import org.spongycastle.openpgp.operator.jcajce.JcePBESecretKeyDecryptorBuilder; -import org.spongycastle.openpgp.operator.jcajce.JcePBESecretKeyEncryptorBuilder; import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; import org.sufficientlysecure.keychain.util.IterableIterator; import org.sufficientlysecure.keychain.util.Log; import java.io.IOException; -import java.security.NoSuchProviderException; import java.util.Iterator; public class WrappedSecretKeyRing extends WrappedKeyRing { @@ -91,29 +88,6 @@ public class WrappedSecretKeyRing extends WrappedKeyRing { } } - public UncachedKeyRing changeSecretKeyPassphrase(String oldPassphrase, - String newPassphrase) - throws IOException, PGPException, NoSuchProviderException { - - if (oldPassphrase == null) { - oldPassphrase = ""; - } - if (newPassphrase == null) { - newPassphrase = ""; - } - - PGPSecretKeyRing newKeyRing = PGPSecretKeyRing.copyWithNewPassword( - mRing, - new JcePBESecretKeyDecryptorBuilder(new JcaPGPDigestCalculatorProviderBuilder() - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build()).setProvider( - Constants.BOUNCY_CASTLE_PROVIDER_NAME).build(oldPassphrase.toCharArray()), - new JcePBESecretKeyEncryptorBuilder(mRing.getSecretKey() - .getKeyEncryptionAlgorithm()).build(newPassphrase.toCharArray())); - - return new UncachedKeyRing(newKeyRing); - - } - public IterableIterator secretKeyIterator() { final Iterator it = mRing.getSecretKeys(); return new IterableIterator(new Iterator() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java index e1514b16f..4fbdfab76 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java @@ -209,6 +209,10 @@ public class KeychainIntentService extends IntentService mMessenger = (Messenger) extras.get(EXTRA_MESSENGER); Bundle data = extras.getBundle(EXTRA_DATA); + if (data == null) { + Log.e(Constants.TAG, "data extra is null!"); + return; + } OtherHelper.logDebugBundle(data, "EXTRA_DATA"); @@ -320,33 +324,40 @@ public class KeychainIntentService extends IntentService try { /* Input */ SaveKeyringParcel saveParcel = data.getParcelable(SAVE_KEYRING_PARCEL); - long masterKeyId = saveParcel.mMasterKeyId; + if (saveParcel == null) { + Log.e(Constants.TAG, "bug: missing save_keyring_parcel in data!"); + return; + } /* Operation */ ProviderHelper providerHelper = new ProviderHelper(this); PgpKeyOperation keyOperations = new PgpKeyOperation(new ProgressScaler(this, 10, 50, 100)); try { - String passphrase = data.getString(SAVE_KEYRING_PASSPHRASE); - WrappedSecretKeyRing secRing = providerHelper.getWrappedSecretKeyRing(masterKeyId); - OperationLog log = new OperationLog(); - UncachedKeyRing ring = keyOperations.modifySecretKeyRing(secRing, saveParcel, - passphrase, log, 0); - providerHelper.saveSecretKeyRing(ring, new ProgressScaler(this, 60, 95, 100)); + UncachedKeyRing ring; + if (saveParcel.mMasterKeyId != null) { + String passphrase = data.getString(SAVE_KEYRING_PASSPHRASE); + WrappedSecretKeyRing secRing = + providerHelper.getWrappedSecretKeyRing(saveParcel.mMasterKeyId); + + ring = keyOperations.modifySecretKeyRing(secRing, saveParcel, + passphrase, log, 0); + } else { + ring = keyOperations.createSecretKeyRing(saveParcel, log, 0); + } + + providerHelper.saveSecretKeyRing(ring, new ProgressScaler(this, 10, 95, 100)); + + if (saveParcel.newPassphrase != null) { + PassphraseCacheService.addCachedPassphrase(this, ring.getMasterKeyId(), + saveParcel.newPassphrase); + } } catch (ProviderHelper.NotFoundException e) { - // UncachedKeyRing ring = keyOperations.(saveParcel); //new Keyring - // save the pair - setProgress(R.string.progress_saving_key_ring, 95, 100); - // providerHelper.saveSecretKeyRing(ring); sendErrorToHandler(e); } setProgress(R.string.progress_done, 100, 100); - if (saveParcel.newPassphrase != null) { - PassphraseCacheService.addCachedPassphrase(this, masterKeyId, saveParcel.newPassphrase); - } - /* Output */ sendMessageToHandler(KeychainIntentServiceHandler.MESSAGE_OKAY); } catch (Exception e) { @@ -437,7 +448,7 @@ public class KeychainIntentService extends IntentService new FileOutputStream(outputFile)); if (mIsCanceled) { - boolean isDeleted = new File(outputFile).delete(); + new File(outputFile).delete(); } sendMessageToHandler(KeychainIntentServiceHandler.MESSAGE_OKAY, resultData); @@ -593,6 +604,7 @@ public class KeychainIntentService extends IntentService return; } Message msg = Message.obtain(); + assert msg != null; msg.arg1 = arg1; if (arg2 != null) { msg.arg2 = arg2; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java index 7f91ab490..535fa08cf 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java @@ -233,9 +233,14 @@ public class OperationResultParcel implements Parcelable { MSG_MG_NEW_SUBKEY (R.string.msg_mg_new_subkey), MSG_MG_FOUND_NEW (R.string.msg_mg_found_new), + // secret key create + MSG_CR_ERROR_NO_MASTER (R.string.msg_mr), + // secret key modify MSG_MF (R.string.msg_mr), MSG_MF_ERROR_ENCODE (R.string.msg_mf_error_encode), + MSG_MF_ERROR_FINGERPRINT (R.string.msg_mf_error_fingerprint), + MSG_MF_ERROR_KEYID (R.string.msg_mf_error_keyid), MSG_MF_ERROR_PGP (R.string.msg_mf_error_pgp), MSG_MF_ERROR_SIG (R.string.msg_mf_error_sig), MSG_MF_PASSPHRASE (R.string.msg_mf_passphrase), diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java index 020b808b9..1ad19cdd0 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java @@ -22,10 +22,10 @@ import java.util.ArrayList; */ public class SaveKeyringParcel implements Parcelable { - // the master key id to be edited - public final long mMasterKeyId; - // the key fingerprint, for safety - public final byte[] mFingerprint; + // the master key id to be edited. if this is null, a new one will be created + public Long mMasterKeyId; + // the key fingerprint, for safety. MUST be null for a new key. + public byte[] mFingerprint; public String newPassphrase; @@ -38,9 +38,7 @@ public class SaveKeyringParcel implements Parcelable { public ArrayList revokeUserIds; public ArrayList revokeSubKeys; - public SaveKeyringParcel(long masterKeyId, byte[] fingerprint) { - mMasterKeyId = masterKeyId; - mFingerprint = fingerprint; + public SaveKeyringParcel() { addUserIds = new ArrayList(); addSubKeys = new ArrayList(); changeSubKeys = new ArrayList(); @@ -48,6 +46,12 @@ public class SaveKeyringParcel implements Parcelable { revokeSubKeys = new ArrayList(); } + public SaveKeyringParcel(long masterKeyId, byte[] fingerprint) { + this(); + mMasterKeyId = masterKeyId; + mFingerprint = fingerprint; + } + // performance gain for using Parcelable here would probably be negligible, // use Serializable instead. public static class SubkeyAdd implements Serializable { @@ -75,7 +79,7 @@ public class SaveKeyringParcel implements Parcelable { } public SaveKeyringParcel(Parcel source) { - mMasterKeyId = source.readLong(); + mMasterKeyId = source.readInt() != 0 ? source.readLong() : null; mFingerprint = source.createByteArray(); addUserIds = source.createStringArrayList(); @@ -90,7 +94,10 @@ public class SaveKeyringParcel implements Parcelable { @Override public void writeToParcel(Parcel destination, int flags) { - destination.writeLong(mMasterKeyId); + destination.writeInt(mMasterKeyId == null ? 0 : 1); + if(mMasterKeyId != null) { + destination.writeLong(mMasterKeyId); + } destination.writeByteArray(mFingerprint); destination.writeStringList(addUserIds); diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index 39b105664..d886fe241 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -626,6 +626,8 @@ Modifying keyring %s Encoding exception! + Actual key fingerprint does not match expected! + No keyid. This is a programming error, please file a bug report! PGP internal exception! Signature exception! Changing passphrase diff --git a/OpenKeychain/src/test/java/tests/PgpDecryptVerifyTest.java b/OpenKeychain/src/test/java/tests/PgpDecryptVerifyTest.java index 346a1f9df..d759bce05 100644 --- a/OpenKeychain/src/test/java/tests/PgpDecryptVerifyTest.java +++ b/OpenKeychain/src/test/java/tests/PgpDecryptVerifyTest.java @@ -22,7 +22,6 @@ public class PgpDecryptVerifyTest { Assert.assertEquals(expectedSignatureResult, status); } - @Test public void testVerifyFailure() throws Exception { From 858be93f3d4538d2e4e69353a5043ed3273e9e76 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 2 Jul 2014 15:02:11 +0200 Subject: [PATCH 2/5] update spongycastle to include addSubkeyBindingCertification --- extern/spongycastle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extern/spongycastle b/extern/spongycastle index 09d85b7d7..968405ee5 160000 --- a/extern/spongycastle +++ b/extern/spongycastle @@ -1 +1 @@ -Subproject commit 09d85b7d7a64b3003210d065c4210ff7fb7a8c6d +Subproject commit 968405ee5d4272330cffdf75f7eee4cd9f5c8646 From 9fb92c8642faf9338a13459a717892edbea0855e Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 2 Jul 2014 15:03:28 +0200 Subject: [PATCH 3/5] fix subkey addition --- .../keychain/pgp/PgpKeyOperation.java | 96 ++++++++++++------- .../service/KeychainIntentServiceHandler.java | 3 + 2 files changed, 62 insertions(+), 37 deletions(-) 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 bde79eee0..528d36da2 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -102,18 +102,13 @@ public class PgpKeyOperation { } /** Creates new secret key. */ - private PGPSecretKey createKey(int algorithmChoice, int keySize, String passphrase, - boolean isMasterKey) throws PgpGeneralMsgIdException { + private PGPKeyPair createKey(int algorithmChoice, int keySize) throws PgpGeneralMsgIdException { try { if (keySize < 512) { throw new PgpGeneralMsgIdException(R.string.error_key_size_minimum512bit); } - if (passphrase == null) { - passphrase = ""; - } - int algorithm; KeyPairGenerator keyGen; @@ -126,9 +121,6 @@ public class PgpKeyOperation { } case Constants.choice.algorithm.elgamal: { - if (isMasterKey) { - throw new PgpGeneralMsgIdException(R.string.error_master_key_must_not_be_el_gamal); - } keyGen = KeyPairGenerator.getInstance("ElGamal", Constants.BOUNCY_CASTLE_PROVIDER_NAME); BigInteger p = Primes.getBestPrime(keySize); BigInteger g = new BigInteger("2"); @@ -154,19 +146,8 @@ public class PgpKeyOperation { } // build new key pair - PGPKeyPair keyPair = new JcaPGPKeyPair(algorithm, keyGen.generateKeyPair(), new Date()); + return new JcaPGPKeyPair(algorithm, keyGen.generateKeyPair(), new Date()); - // define hashing and signing algos - PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder().build().get( - HashAlgorithmTags.SHA1); - - // Build key encrypter and decrypter based on passphrase - PBESecretKeyEncryptor keyEncryptor = new JcePBESecretKeyEncryptorBuilder( - PGPEncryptedData.CAST5, sha1Calc) - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build(passphrase.toCharArray()); - - return new PGPSecretKey(keyPair.getPrivateKey(), keyPair.getPublicKey(), - sha1Calc, isMasterKey, keyEncryptor); } catch(NoSuchProviderException e) { throw new RuntimeException(e); } catch(NoSuchAlgorithmException e) { @@ -193,12 +174,26 @@ public class PgpKeyOperation { } SubkeyAdd add = saveParcel.addSubKeys.remove(0); - PGPSecretKey masterSecretKey = createKey(add.mAlgorithm, add.mKeysize, - saveParcel.newPassphrase, true); + PGPKeyPair keyPair = createKey(add.mAlgorithm, add.mKeysize); + + if (add.mAlgorithm == Constants.choice.algorithm.elgamal) { + throw new PgpGeneralMsgIdException(R.string.error_master_key_must_not_be_el_gamal); + } + + // define hashing and signing algos + PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder() + .build().get(HashAlgorithmTags.SHA1); + // Build key encrypter and decrypter based on passphrase + PBESecretKeyEncryptor keyEncryptor = new JcePBESecretKeyEncryptorBuilder( + PGPEncryptedData.CAST5, sha1Calc) + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build("".toCharArray()); + PGPSecretKey masterSecretKey = new PGPSecretKey(keyPair.getPrivateKey(), keyPair.getPublicKey(), + sha1Calc, true, keyEncryptor); + PGPSecretKeyRing sKR = new PGPSecretKeyRing( masterSecretKey.getEncoded(), new JcaKeyFingerprintCalculator()); - return internal(sKR, masterSecretKey, saveParcel, saveParcel.newPassphrase, log, indent); + return internal(sKR, masterSecretKey, saveParcel, "", log, indent); } catch (PGPException e) { Log.e(Constants.TAG, "pgp error encoding key", e); @@ -207,6 +202,7 @@ public class PgpKeyOperation { Log.e(Constants.TAG, "io error encoding key", e); return null; } catch (PgpGeneralMsgIdException e) { + Log.e(Constants.TAG, "pgp msg id error", e); return null; } @@ -374,16 +370,36 @@ public class PgpKeyOperation { } log.add(LogLevel.INFO, LogType.MSG_MF_SUBKEY_NEW, indent); - PGPSecretKey sKey = createKey(add.mAlgorithm, add.mKeysize, passphrase, false); + + // generate a new secret key (privkey only for now) + PGPKeyPair keyPair = createKey(add.mAlgorithm, add.mKeysize); + + // add subkey binding signature (making this a sub rather than master key) + PGPPublicKey pKey = keyPair.getPublicKey(); + PGPSignature cert = generateSubkeyBindingSignature( + masterPublicKey, masterPrivateKey, keyPair.getPrivateKey(), pKey, + add.mFlags, add.mExpiry); + pKey = PGPPublicKey.addSubkeyBindingCertification(pKey, cert); + + PGPSecretKey sKey; { + // define hashing and signing algos + PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder() + .build().get(HashAlgorithmTags.SHA1); + + // Build key encrypter and decrypter based on passphrase + PBESecretKeyEncryptor keyEncryptor = new JcePBESecretKeyEncryptorBuilder( + PGPEncryptedData.CAST5, sha1Calc) + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build("".toCharArray()); + + sKey = new PGPSecretKey(keyPair.getPrivateKey(), pKey, + sha1Calc, false, keyEncryptor); + } + log.add(LogLevel.DEBUG, LogType.MSG_MF_SUBKEY_NEW_ID, indent+1, PgpKeyHelper.convertKeyIdToHex(sKey.getKeyID())); - PGPPublicKey pKey = sKey.getPublicKey(); - PGPSignature cert = generateSubkeyBindingSignature(masterPublicKey, masterPrivateKey, - sKey, pKey, add.mFlags, add.mExpiry, passphrase); - pKey = PGPPublicKey.addCertification(pKey, cert); - sKey = PGPSecretKey.replacePublicKey(sKey, pKey); - sKR = PGPSecretKeyRing.insertSecretKey(sKR, PGPSecretKey.replacePublicKey(sKey, pKey)); + sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey); + } catch (PgpGeneralMsgIdException e) { return null; } @@ -478,6 +494,18 @@ public class PgpKeyOperation { PGPPublicKey masterPublicKey, PGPPrivateKey masterPrivateKey, PGPSecretKey sKey, PGPPublicKey pKey, int flags, Long expiry, String passphrase) throws IOException, PGPException, SignatureException { + PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder() + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build( + passphrase.toCharArray()); + PGPPrivateKey subPrivateKey = sKey.extractPrivateKey(keyDecryptor); + return generateSubkeyBindingSignature(masterPublicKey, masterPrivateKey, subPrivateKey, + pKey, flags, expiry); + } + + private static PGPSignature generateSubkeyBindingSignature( + PGPPublicKey masterPublicKey, PGPPrivateKey masterPrivateKey, + PGPPrivateKey subPrivateKey, PGPPublicKey pKey, int flags, Long expiry) + throws IOException, PGPException, SignatureException { // date for signing Date todayDate = new Date(); @@ -485,12 +513,6 @@ public class PgpKeyOperation { // If this key can sign, we need a primary key binding signature if ((flags & KeyFlags.SIGN_DATA) != 0) { - - PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder() - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build( - passphrase.toCharArray()); - PGPPrivateKey subPrivateKey = sKey.extractPrivateKey(keyDecryptor); - // cross-certify signing keys PGPSignatureSubpacketGenerator subHashedPacketsGen = new PGPSignatureSubpacketGenerator(); subHashedPacketsGen.setSignatureCreationTime(false, todayDate); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentServiceHandler.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentServiceHandler.java index d5d02081a..755827482 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentServiceHandler.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentServiceHandler.java @@ -27,8 +27,10 @@ import android.support.v4.app.FragmentManager; import com.devspark.appmsg.AppMsg; +import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.R; import org.sufficientlysecure.keychain.ui.dialog.ProgressDialogFragment; +import org.sufficientlysecure.keychain.util.Log; public class KeychainIntentServiceHandler extends Handler { @@ -126,6 +128,7 @@ public class KeychainIntentServiceHandler extends Handler { break; default: + Log.e(Constants.TAG, "unknown handler message!"); break; } } From 6f558add35c7c68a6b28a638dadfff75838ef9ba Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 2 Jul 2014 15:05:02 +0200 Subject: [PATCH 4/5] use expert create key for key creation testing (revert this later on!) --- .../keychain/ui/KeyListActivity.java | 55 +++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/KeyListActivity.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/KeyListActivity.java index 7ce7a06aa..849576284 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/KeyListActivity.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/KeyListActivity.java @@ -17,21 +17,33 @@ package org.sufficientlysecure.keychain.ui; +import android.app.ProgressDialog; import android.content.Intent; import android.os.Bundle; +import android.os.Message; +import android.os.Messenger; import android.view.Menu; import android.view.MenuItem; import com.devspark.appmsg.AppMsg; +import org.spongycastle.bcpg.sig.KeyFlags; import org.sufficientlysecure.keychain.Constants; +import org.sufficientlysecure.keychain.Constants.choice.algorithm; import org.sufficientlysecure.keychain.R; import org.sufficientlysecure.keychain.helper.ExportHelper; +import org.sufficientlysecure.keychain.helper.OtherHelper; +import org.sufficientlysecure.keychain.keyimport.ImportKeysListEntry; import org.sufficientlysecure.keychain.provider.KeychainContract; import org.sufficientlysecure.keychain.provider.KeychainDatabase; +import org.sufficientlysecure.keychain.service.KeychainIntentService; +import org.sufficientlysecure.keychain.service.KeychainIntentServiceHandler; +import org.sufficientlysecure.keychain.service.SaveKeyringParcel; +import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyAdd; import org.sufficientlysecure.keychain.util.Log; import java.io.IOException; +import java.util.ArrayList; public class KeyListActivity extends DrawerActivity { @@ -121,9 +133,42 @@ public class KeyListActivity extends DrawerActivity { } private void createKeyExpert() { - Intent intent = new Intent(this, EditKeyActivity.class); - intent.setAction(EditKeyActivity.ACTION_CREATE_KEY); - startActivityForResult(intent, 0); - } + Intent intent = new Intent(this, KeychainIntentService.class); + intent.setAction(KeychainIntentService.ACTION_SAVE_KEYRING); -} + // Message is received after importing is done in KeychainIntentService + KeychainIntentServiceHandler saveHandler = new KeychainIntentServiceHandler( + this, + getString(R.string.progress_importing), + ProgressDialog.STYLE_HORIZONTAL) { + public void handleMessage(Message message) { + // handle messages by standard KeychainIntentServiceHandler first + super.handleMessage(message); + Bundle data = message.getData(); + // OtherHelper.logDebugBundle(data, "message reply"); + } + }; + + // fill values for this action + Bundle data = new Bundle(); + + SaveKeyringParcel parcel = new SaveKeyringParcel(); + parcel.addSubKeys.add(new SubkeyAdd(algorithm.rsa, 1024, KeyFlags.CERTIFY_OTHER, null)); + parcel.addSubKeys.add(new SubkeyAdd(algorithm.rsa, 1024, KeyFlags.SIGN_DATA, null)); + parcel.addUserIds.add("swagerinho"); + parcel.newPassphrase = "swag"; + + // get selected key entries + data.putParcelable(KeychainIntentService.SAVE_KEYRING_PARCEL, parcel); + + intent.putExtra(KeychainIntentService.EXTRA_DATA, data); + + // Create a new Messenger for the communication back + Messenger messenger = new Messenger(saveHandler); + intent.putExtra(KeychainIntentService.EXTRA_MESSENGER, messenger); + + saveHandler.showProgressDialog(this); + + startService(intent); + } +} \ No newline at end of file From ebcd243e975ce8f04298f6cd94cb940bfcea3d59 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 2 Jul 2014 16:02:56 +0200 Subject: [PATCH 5/5] support changing primary user id in SaveKeyringParcel Closes #695 --- .../keychain/pgp/PgpKeyOperation.java | 115 +++++++++++++++++- .../service/OperationResultParcel.java | 2 + OpenKeychain/src/main/res/values/strings.xml | 2 + 3 files changed, 115 insertions(+), 4 deletions(-) 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 528d36da2..9a08290e4 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -52,6 +52,7 @@ import org.sufficientlysecure.keychain.service.OperationResultParcel.LogType; import org.sufficientlysecure.keychain.service.OperationResultParcel.OperationLog; import org.sufficientlysecure.keychain.service.SaveKeyringParcel; import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyAdd; +import org.sufficientlysecure.keychain.util.IterableIterator; import org.sufficientlysecure.keychain.util.Log; import org.sufficientlysecure.keychain.util.Primes; @@ -66,6 +67,7 @@ import java.security.SignatureException; import java.util.Arrays; import java.util.Calendar; import java.util.Date; +import java.util.Iterator; import java.util.TimeZone; /** @@ -289,14 +291,42 @@ public class PgpKeyOperation { // 2a. Add certificates for new user ids for (String userId : saveParcel.addUserIds) { log.add(LogLevel.INFO, LogType.MSG_MF_UID_ADD, indent); + + // this operation supersedes all previous binding and revocation certificates, + // so remove those to retain assertions from canonicalization for later operations + @SuppressWarnings("unchecked") + Iterator it = modifiedPublicKey.getSignaturesForID(userId); + if (it != null) { + for (PGPSignature cert : new IterableIterator(it)) { + // if it's not a self cert, never mind + if (cert.getKeyID() != masterPublicKey.getKeyID()) { + continue; + } + if (cert.getSignatureType() == PGPSignature.CERTIFICATION_REVOCATION + || cert.getSignatureType() == PGPSignature.NO_CERTIFICATION + || cert.getSignatureType() == PGPSignature.CASUAL_CERTIFICATION + || cert.getSignatureType() == PGPSignature.POSITIVE_CERTIFICATION + || cert.getSignatureType() == PGPSignature.DEFAULT_CERTIFICATION) { + modifiedPublicKey = PGPPublicKey.removeCertification( + modifiedPublicKey, userId, cert); + } + } + } + + // if it's supposed to be primary, we can do that here as well + boolean isPrimary = saveParcel.changePrimaryUserId != null + && userId.equals(saveParcel.changePrimaryUserId); + // generate and add new certificate PGPSignature cert = generateUserIdSignature(masterPrivateKey, - masterPublicKey, userId, false); + masterPublicKey, userId, isPrimary); modifiedPublicKey = PGPPublicKey.addCertification(masterPublicKey, userId, cert); } // 2b. Add revocations for revoked user ids for (String userId : saveParcel.revokeUserIds) { log.add(LogLevel.INFO, LogType.MSG_MF_UID_REVOKE, indent); + // a duplicate revocatin will be removed during canonicalization, so no need to + // take care of that here. PGPSignature cert = generateRevocationSignature(masterPrivateKey, masterPublicKey, userId); modifiedPublicKey = PGPPublicKey.addCertification(masterPublicKey, userId, cert); @@ -305,7 +335,84 @@ public class PgpKeyOperation { // 3. If primary user id changed, generate new certificates for both old and new if (saveParcel.changePrimaryUserId != null) { log.add(LogLevel.INFO, LogType.MSG_MF_UID_PRIMARY, indent); - // todo + + // we work on the modifiedPublicKey here, to respect new or newly revoked uids + // noinspection unchecked + for (String userId : new IterableIterator(modifiedPublicKey.getUserIDs())) { + boolean isRevoked = false; + PGPSignature currentCert = null; + // noinspection unchecked + for (PGPSignature cert : new IterableIterator( + masterPublicKey.getSignaturesForID(userId))) { + // if it's not a self cert, never mind + if (cert.getKeyID() != masterPublicKey.getKeyID()) { + continue; + } + // we know from canonicalization that if there is any revocation here, it + // is valid and not superseded by a newer certification. + if (cert.getSignatureType() == PGPSignature.CERTIFICATION_REVOCATION) { + isRevoked = true; + continue; + } + // we know from canonicalization that there is only one binding + // certification here, so we can just work with the first one. + if (cert.getSignatureType() == PGPSignature.NO_CERTIFICATION || + cert.getSignatureType() == PGPSignature.CASUAL_CERTIFICATION || + cert.getSignatureType() == PGPSignature.POSITIVE_CERTIFICATION || + cert.getSignatureType() == PGPSignature.DEFAULT_CERTIFICATION) { + currentCert = cert; + } + } + + if (currentCert == null) { + // no certificate found?! error error error + log.add(LogLevel.ERROR, LogType.MSG_MF_ERROR_INTEGRITY, indent); + return null; + } + + // we definitely should not update certifications of revoked keys, so just leave it. + if (isRevoked) { + // revoked user ids cannot be primary! + if (userId.equals(saveParcel.changePrimaryUserId)) { + log.add(LogLevel.ERROR, LogType.MSG_MF_ERROR_REVOKED_PRIMARY, indent); + return null; + } + continue; + } + + // if this is~ the/a primary user id + if (currentCert.hasSubpackets() && currentCert.getHashedSubPackets().isPrimaryUserID()) { + // if it's the one we want, just leave it as is + if (userId.equals(saveParcel.changePrimaryUserId)) { + continue; + } + // otherwise, generate new non-primary certification + modifiedPublicKey = PGPPublicKey.removeCertification( + modifiedPublicKey, userId, currentCert); + PGPSignature newCert = generateUserIdSignature( + masterPrivateKey, masterPublicKey, userId, false); + modifiedPublicKey = PGPPublicKey.addCertification( + modifiedPublicKey, userId, newCert); + continue; + } + + // if we are here, this is not currently a primary user id + + // if it should be + if (userId.equals(saveParcel.changePrimaryUserId)) { + // add shiny new primary user id certificate + modifiedPublicKey = PGPPublicKey.removeCertification( + modifiedPublicKey, userId, currentCert); + PGPSignature newCert = generateUserIdSignature( + masterPrivateKey, masterPublicKey, userId, true); + modifiedPublicKey = PGPPublicKey.addCertification( + modifiedPublicKey, userId, newCert); + } + + // user id is not primary and is not supposed to be - nothing to do here. + + } + } // Update the secret key ring @@ -315,7 +422,6 @@ public class PgpKeyOperation { sKR = PGPSecretKeyRing.insertSecretKey(sKR, masterSecretKey); } - // 4a. For each subkey change, generate new subkey binding certificate for (SaveKeyringParcel.SubkeyChange change : saveParcel.changeSubKeys) { log.add(LogLevel.INFO, LogType.MSG_MF_SUBKEY_CHANGE, @@ -334,7 +440,8 @@ public class PgpKeyOperation { return null; } - // generate and add new signature + // generate and add new signature. we can be sloppy here and just leave the old one, + // it will be removed during canonicalization PGPSignature sig = generateSubkeyBindingSignature(masterPublicKey, masterPrivateKey, sKey, pKey, change.mFlags, change.mExpiry, passphrase); pKey = PGPPublicKey.addCertification(pKey, sig); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java index 535fa08cf..f5d5b8f97 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java @@ -241,6 +241,8 @@ public class OperationResultParcel implements Parcelable { MSG_MF_ERROR_ENCODE (R.string.msg_mf_error_encode), MSG_MF_ERROR_FINGERPRINT (R.string.msg_mf_error_fingerprint), MSG_MF_ERROR_KEYID (R.string.msg_mf_error_keyid), + MSG_MF_ERROR_INTEGRITY (R.string.msg_mf_error_integrity), + MSG_MF_ERROR_REVOKED_PRIMARY (R.string.msg_mf_error_revoked_primary), MSG_MF_ERROR_PGP (R.string.msg_mf_error_pgp), MSG_MF_ERROR_SIG (R.string.msg_mf_error_sig), MSG_MF_PASSPHRASE (R.string.msg_mf_passphrase), diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index d886fe241..d91707567 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -628,6 +628,8 @@ Encoding exception! Actual key fingerprint does not match expected! No keyid. This is a programming error, please file a bug report! + Internal error, integrity check failed! + Revoked user ids cannot be primary! PGP internal exception! Signature exception! Changing passphrase