From 642f83c1f4a69e513334321f7f5810bdfc7f1ad2 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 8 Jul 2015 01:41:55 +0200 Subject: better handling of divert-to-card keys for subkey modifications --- .../operations/results/OperationResult.java | 1 + .../keychain/pgp/PgpKeyOperation.java | 75 ++++++++++++++-------- OpenKeychain/src/main/res/values/strings.xml | 1 + .../keychain/pgp/PgpKeyOperationTest.java | 34 ++++++++-- .../pgp/UncachedKeyringCanonicalizeTest.java | 5 +- 5 files changed, 81 insertions(+), 35 deletions(-) (limited to 'OpenKeychain') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java index 5ae068b35..a54ddff68 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java @@ -499,6 +499,7 @@ public abstract class OperationResult implements Parcelable { MSG_MF_ERROR_RESTRICTED(LogLevel.ERROR, R.string.msg_mf_error_restricted), MSG_MF_ERROR_REVOKED_PRIMARY (LogLevel.ERROR, R.string.msg_mf_error_revoked_primary), MSG_MF_ERROR_SIG (LogLevel.ERROR, R.string.msg_mf_error_sig), + MSG_MF_ERROR_SUB_STRIPPED(LogLevel.ERROR, R.string.msg_mf_error_sub_stripped), MSG_MF_ERROR_SUBKEY_MISSING(LogLevel.ERROR, R.string.msg_mf_error_subkey_missing), MSG_MF_ERROR_CONFLICTING_NFC_COMMANDS(LogLevel.ERROR, R.string.msg_mf_error_conflicting_nfc_commands), MSG_MF_ERROR_DUPLICATE_KEYTOCARD_FOR_SLOT(LogLevel.ERROR, R.string.msg_mf_error_duplicate_keytocard_for_slot), 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 8445272fc..414ed1f62 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -897,18 +897,35 @@ public class PgpKeyOperation { pKey = PGPPublicKey.removeCertification(pKey, sig); } - PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder() - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build( - cryptoInput.getPassphrase().getCharArray()); - PGPPrivateKey subPrivateKey = sKey.extractPrivateKey(keyDecryptor); - PGPSignature sig = generateSubkeyBindingSignature( - getSignatureGenerator(masterSecretKey, cryptoInput), - cryptoInput.getSignatureTime(), - masterPublicKey, masterPrivateKey, subPrivateKey, pKey, flags, expiry); + PGPPrivateKey subPrivateKey; + if (!isDivertToCard(sKey)) { + PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder() + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build( + cryptoInput.getPassphrase().getCharArray()); + subPrivateKey = sKey.extractPrivateKey(keyDecryptor); + // super special case: subkey is allowed to sign, but isn't available + if (subPrivateKey == null) { + log.add(LogType.MSG_MF_ERROR_SUB_STRIPPED, + indent + 1, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId)); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); + } + } else { + subPrivateKey = null; + } + try { + PGPSignature sig = generateSubkeyBindingSignature( + getSignatureGenerator(masterSecretKey, cryptoInput), + cryptoInput.getSignatureTime(), masterPublicKey, masterPrivateKey, + getSignatureGenerator(sKey, cryptoInput), subPrivateKey, + pKey, flags, expiry); + + // generate and add new signature + pKey = PGPPublicKey.addCertification(pKey, sig); + sKR = PGPSecretKeyRing.insertSecretKey(sKR, PGPSecretKey.replacePublicKey(sKey, pKey)); + } catch (NfcInteractionNeeded e) { + nfcSignOps.addHash(e.hashToSign, e.hashAlgo); + } - // generate and add new signature - pKey = PGPPublicKey.addCertification(pKey, sig); - sKR = PGPSecretKeyRing.insertSecretKey(sKR, PGPSecretKey.replacePublicKey(sKey, pKey)); } subProgressPop(); @@ -987,7 +1004,8 @@ public class PgpKeyOperation { PGPSignature cert = generateSubkeyBindingSignature( getSignatureGenerator(masterSecretKey, cryptoInput), cryptoInput.getSignatureTime(), - masterPublicKey, masterPrivateKey, keyPair.getPrivateKey(), pKey, + masterPublicKey, masterPrivateKey, + getSignatureGenerator(pKey, cryptoInput, false), keyPair.getPrivateKey(), pKey, add.mFlags, add.mExpiry); pKey = PGPPublicKey.addSubkeyBindingCertification(pKey, cert); } catch (NfcInteractionNeeded e) { @@ -1002,7 +1020,8 @@ public class PgpKeyOperation { PgpConstants.SECRET_KEY_ENCRYPTOR_SYMMETRIC_ALGO, encryptorHashCalc, PgpConstants.SECRET_KEY_ENCRYPTOR_S2K_COUNT) .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME).build( - cryptoInput.getPassphrase().getCharArray()); + cryptoInput.hasPassphrase() + ? cryptoInput.getPassphrase().getCharArray() : new char[]{} ); PGPDigestCalculator sha1Calc = new JcaPGPDigestCalculatorProviderBuilder() .build().get(PgpConstants.SECRET_KEY_SIGNATURE_CHECKSUM_HASH_ALGO); @@ -1402,22 +1421,27 @@ public class PgpKeyOperation { static PGPSignatureGenerator getSignatureGenerator( PGPSecretKey secretKey, CryptoInputParcel cryptoInput) { - PGPContentSignerBuilder builder; - S2K s2k = secretKey.getS2K(); - if (s2k != null && s2k.getType() == S2K.GNU_DUMMY_S2K - && s2k.getProtectionMode() == S2K.GNU_PROTECTION_MODE_DIVERT_TO_CARD) { + boolean isDivertToCard = s2k != null && s2k.getType() == S2K.GNU_DUMMY_S2K + && s2k.getProtectionMode() == S2K.GNU_PROTECTION_MODE_DIVERT_TO_CARD; + + return getSignatureGenerator(secretKey.getPublicKey(), cryptoInput, isDivertToCard); + } + + static PGPSignatureGenerator getSignatureGenerator( + PGPPublicKey pKey, CryptoInputParcel cryptoInput, boolean divertToCard) { + + PGPContentSignerBuilder builder; + if (divertToCard) { // use synchronous "NFC based" SignerBuilder builder = new NfcSyncPGPContentSignerBuilder( - secretKey.getPublicKey().getAlgorithm(), - PgpConstants.SECRET_KEY_SIGNATURE_HASH_ALGO, - secretKey.getKeyID(), cryptoInput.getCryptoData()) + pKey.getAlgorithm(), PgpConstants.SECRET_KEY_SIGNATURE_HASH_ALGO, + pKey.getKeyID(), cryptoInput.getCryptoData()) .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); } else { // content signer based on signing key algorithm and chosen hash algorithm builder = new JcaPGPContentSignerBuilder( - secretKey.getPublicKey().getAlgorithm(), - PgpConstants.SECRET_KEY_SIGNATURE_HASH_ALGO) + pKey.getAlgorithm(), PgpConstants.SECRET_KEY_SIGNATURE_HASH_ALGO) .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); } @@ -1524,7 +1548,8 @@ public class PgpKeyOperation { static PGPSignature generateSubkeyBindingSignature( PGPSignatureGenerator sGen, Date creationTime, PGPPublicKey masterPublicKey, PGPPrivateKey masterPrivateKey, - PGPPrivateKey subPrivateKey, PGPPublicKey pKey, int flags, long expiry) + PGPSignatureGenerator subSigGen, PGPPrivateKey subPrivateKey, PGPPublicKey pKey, + int flags, long expiry) throws IOException, PGPException, SignatureException { PGPSignatureSubpacketGenerator unhashedPacketsGen = new PGPSignatureSubpacketGenerator(); @@ -1534,10 +1559,6 @@ public class PgpKeyOperation { // cross-certify signing keys PGPSignatureSubpacketGenerator subHashedPacketsGen = new PGPSignatureSubpacketGenerator(); subHashedPacketsGen.setSignatureCreationTime(false, creationTime); - PGPContentSignerBuilder signerBuilder = new JcaPGPContentSignerBuilder( - pKey.getAlgorithm(), PgpConstants.SECRET_KEY_SIGNATURE_HASH_ALGO) - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); - PGPSignatureGenerator subSigGen = new PGPSignatureGenerator(signerBuilder); subSigGen.init(PGPSignature.PRIMARYKEY_BINDING, subPrivateKey); subSigGen.setHashedSubpackets(subHashedPacketsGen.generate()); PGPSignature certification = subSigGen.generateCertification(masterPublicKey, pKey); diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index cae2faef2..1be726447 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -991,6 +991,7 @@ "Fatal error decrypting master key! This is likely a programming error, please file a bug report!" "Internal OpenPGP error!" "Signature exception!" + "Cannot modify stripped subkey %s!" "Tried to operate on missing subkey %s!" "Cannot move key to smart card in same operation that creates an on-card signature." "Smart card supports only one slot per key type." diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java index f32730b47..c0e28cd4b 100644 --- a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java @@ -94,11 +94,11 @@ public class PgpKeyOperationTest { SaveKeyringParcel parcel = new SaveKeyringParcel(); parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd( - Algorithm.RSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L)); + Algorithm.DSA, 1024, null, KeyFlags.CERTIFY_OTHER, 0L)); parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd( - Algorithm.DSA, 1024, null, KeyFlags.SIGN_DATA, 0L)); + Algorithm.RSA, 2048, null, KeyFlags.SIGN_DATA, 0L)); parcel.mAddSubKeys.add(new SaveKeyringParcel.SubkeyAdd( - Algorithm.RSA, 2048, null, KeyFlags.ENCRYPT_COMMS, 0L)); + Algorithm.RSA, 1024, null, KeyFlags.ENCRYPT_COMMS, 0L)); parcel.mAddUserIds.add("twi"); parcel.mAddUserIds.add("pink"); @@ -821,6 +821,15 @@ public class PgpKeyOperationTest { Assert.assertEquals("new packet should have GNU_DUMMY protection mode stripped", S2K.GNU_PROTECTION_MODE_NO_PRIVATE_KEY, ((SecretKeyPacket) p).getS2K().getProtectionMode()); } + + { // trying to edit a subkey with signing capability should fail + parcel.reset(); + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true)); + + assertModifyFailure("subkey modification for signing-enabled but stripped subkey should fail", + modified, parcel, LogType.MSG_MF_ERROR_SUB_STRIPPED); + } + } @Test @@ -829,7 +838,7 @@ public class PgpKeyOperationTest { UncachedKeyRing modified; { // keytocard should fail with BAD_NFC_SIZE when presented with the RSA-1024 key - long keyId = KeyringTestingHelper.getSubkeyId(ring, 0); + long keyId = KeyringTestingHelper.getSubkeyId(ring, 2); parcel.reset(); parcel.mChangeSubKeys.add(new SubkeyChange(keyId, false, true)); @@ -838,7 +847,7 @@ public class PgpKeyOperationTest { } { // keytocard should fail with BAD_NFC_ALGO when presented with the DSA-1024 key - long keyId = KeyringTestingHelper.getSubkeyId(ring, 1); + long keyId = KeyringTestingHelper.getSubkeyId(ring, 0); parcel.reset(); parcel.mChangeSubKeys.add(new SubkeyChange(keyId, false, true)); @@ -846,9 +855,10 @@ public class PgpKeyOperationTest { parcel, cryptoInput, LogType.MSG_MF_ERROR_BAD_NFC_ALGO); } + long keyId = KeyringTestingHelper.getSubkeyId(ring, 1); + { // keytocard should return a pending NFC_MOVE_KEY_TO_CARD result when presented with the RSA-2048 // key, and then make key divert-to-card when it gets a serial in the cryptoInputParcel. - long keyId = KeyringTestingHelper.getSubkeyId(ring, 2); parcel.reset(); parcel.mChangeSubKeys.add(new SubkeyChange(keyId, false, true)); @@ -880,7 +890,19 @@ public class PgpKeyOperationTest { S2K.GNU_PROTECTION_MODE_DIVERT_TO_CARD, ((SecretKeyPacket) p).getS2K().getProtectionMode()); Assert.assertArrayEquals("new packet should have correct serial number as iv", serial, ((SecretKeyPacket) p).getIV()); + } + + { // editing a signing subkey requires a primary key binding sig -> pendinginput + parcel.reset(); + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true)); + CanonicalizedSecretKeyRing secretRing = + new CanonicalizedSecretKeyRing(modified.getEncoded(), false, 0); + PgpKeyOperation op = new PgpKeyOperation(null); + PgpEditKeyResult result = op.modifySecretKeyRing(secretRing, cryptoInput, parcel); + Assert.assertTrue("keytocard operation should be pending", result.isPending()); + Assert.assertEquals("required input should be RequiredInputType.NFC_SIGN", + RequiredInputType.NFC_SIGN, result.getRequiredInputParcel().mType); } } diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java index 0143ae289..ad4c1fb75 100644 --- a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java @@ -558,8 +558,9 @@ public class UncachedKeyringCanonicalizeTest { PGPSignature cert = PgpKeyOperation.generateSubkeyBindingSignature( PgpKeyOperation.getSignatureGenerator(masterSecretKey.getSecretKey(), cryptoInput), cryptoInput.getSignatureTime(), - masterPublicKey, masterSecretKey.getPrivateKey(), masterSecretKey.getPrivateKey(), - masterPublicKey, masterSecretKey.getKeyUsage(), 0); + masterPublicKey, masterSecretKey.getPrivateKey(), + PgpKeyOperation.getSignatureGenerator(masterSecretKey.getSecretKey(), null), + masterSecretKey.getPrivateKey(), masterPublicKey, masterSecretKey.getKeyUsage(), 0); PGPPublicKey subPubKey = PGPPublicKey.addSubkeyBindingCertification(masterPublicKey, cert); PGPSecretKey sKey; -- cgit v1.2.3