better handling of divert-to-card keys for subkey modifications

5 files changed, 81 insertions, 35 deletions

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 @@
     <string name="msg_mf_error_passphrase_master">"Fatal error decrypting master key! This is likely a programming error, please file a bug report!"</string>
     <string name="msg_mf_error_pgp">"Internal OpenPGP error!"</string>
     <string name="msg_mf_error_sig">"Signature exception!"</string>
+    <string name="msg_mf_error_sub_stripped">"Cannot modify stripped subkey %s!"</string>
     <string name="msg_mf_error_subkey_missing">"Tried to operate on missing subkey %s!"</string>
     <string name="msg_mf_error_conflicting_nfc_commands">"Cannot move key to smart card in same operation that creates an on-card signature."</string>
     <string name="msg_mf_error_duplicate_keytocard_for_slot">"Smart card supports only one slot per key type."</string>
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;