Abort import of pubkeys where secret keys exist but new pubkey has more subkeys, fix #696

3 files changed, 34 insertions, 9 deletions

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 caacb948e..424420e72 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
@@ -485,6 +485,10 @@ public class UncachedKeyRing {
 
             // Replace modified key in the keyring
             ring = replacePublicKey(ring, modified);
+            if (ring == null) {
+                log.add(LogLevel.ERROR, LogType.MSG_MG_NO_SECRET_KEYRING, indent);
+                return null;
+            }
             indent -= 1;
 
         }
@@ -652,6 +656,10 @@ public class UncachedKeyRing {
             }
             // replace pubkey in keyring
             ring = replacePublicKey(ring, modified);
+            if (ring == null) {
+                log.add(LogLevel.ERROR, LogType.MSG_MG_NO_SECRET_KEYRING, indent);
+                return null;
+            }
             indent -= 1;
         }
 
@@ -741,6 +749,10 @@ public class UncachedKeyRing {
                     } else {
                         // otherwise, just insert the public key
                         result = replacePublicKey(result, key);
+                        if (result == null) {
+                            log.add(LogLevel.ERROR, LogType.MSG_MG_NO_SECRET_KEYRING, indent);
+                            return null;
+                        }
                     }
                     continue;
                 }
@@ -769,6 +781,10 @@ public class UncachedKeyRing {
                 if (!key.isMasterKey()) {
                     if (modified != resultKey) {
                         result = replacePublicKey(result, modified);
+                        if (result == null) {
+                            log.add(LogLevel.ERROR, LogType.MSG_MG_NO_SECRET_KEYRING, indent);
+                            return null;
+                        }
                     }
                     continue;
                 }
@@ -793,6 +809,10 @@ public class UncachedKeyRing {
                 // If anything changed, save the updated (sub)key
                 if (modified != resultKey) {
                     result = replacePublicKey(result, modified);
+                    if (result == null) {
+                        log.add(LogLevel.ERROR, LogType.MSG_MG_NO_SECRET_KEYRING, indent);
+                        return null;
+                    }
                 }
 
             }
@@ -838,16 +858,19 @@ public class UncachedKeyRing {
      */
     private static PGPKeyRing replacePublicKey(PGPKeyRing ring, PGPPublicKey key) {
         if (ring instanceof PGPPublicKeyRing) {
-            return PGPPublicKeyRing.insertPublicKey((PGPPublicKeyRing) ring, key);
-        }
-        PGPSecretKeyRing secRing = (PGPSecretKeyRing) ring;
-        PGPSecretKey sKey = secRing.getSecretKey(key.getKeyID());
-        // TODO generate secret key with S2K dummy, if none exists! for now, just die.
-        if (sKey == null) {
-            throw new RuntimeException("dummy secret key generation not yet implemented");
+            PGPPublicKeyRing pubRing = (PGPPublicKeyRing) ring;
+            return PGPPublicKeyRing.insertPublicKey(pubRing, key);
+        } else {
+            PGPSecretKeyRing secRing = (PGPSecretKeyRing) ring;
+            PGPSecretKey sKey = secRing.getSecretKey(key.getKeyID());
+            // TODO generate secret key with S2K dummy, if none exists!
+            if (sKey == null) {
+                Log.e(Constants.TAG, "dummy secret key generation not yet implemented");
+                return null;
+            }
+            sKey = PGPSecretKey.replacePublicKey(sKey, key);
+            return PGPSecretKeyRing.insertSecretKey(secRing, sKey);
         }
-        sKey = PGPSecretKey.replacePublicKey(sKey, key);
-        return PGPSecretKeyRing.insertSecretKey(secRing, sKey);
     }
 
     /** This method removes a subkey in a keyring.
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 886f77068..ddc456e34 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
@@ -339,6 +339,7 @@ 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),
         MSG_MG_UNCHANGED (R.string.msg_mg_unchanged),
+        MSG_MG_NO_SECRET_KEYRING (R.string.msg_mg_no_secret_keyring),
 
         // secret key create
         MSG_CR (R.string.msg_cr),
diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml
index 2eb9f2b97..d8c73bd9e 100644
--- a/OpenKeychain/src/main/res/values/strings.xml
+++ b/OpenKeychain/src/main/res/values/strings.xml
@@ -626,6 +626,7 @@
     <string name="msg_mg_new_subkey">Adding new subkey %s</string>
     <string name="msg_mg_found_new">Found %s new certificates in keyring</string>
     <string name="msg_mg_unchanged">No new certificates</string>
+    <string name="msg_mg_no_secret_keyring">No secret keyring to add subkey to</string>
 
     <!-- createSecretKeyRing -->
     <string name="msg_cr">Generating new master key</string>