Fix PgpDecryptVerify signature verification: search for right signature subkey instead of using first subkey for verification

2 files changed, 58 insertions, 51 deletions

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 40d487a48..b3572d4a2 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java
@@ -276,6 +276,7 @@ public class PgpDecryptVerify {
                     // continue with the next packet in the while loop
                     continue;
                 }
+                // get subkey which has been used for this encryption packet
                 secretEncryptionKey = secretKeyRing.getSecretKey(encData.getKeyID());
                 if (secretEncryptionKey == null) {
                     // continue with the next packet in the while loop
@@ -390,7 +391,6 @@ public class PgpDecryptVerify {
         OpenPgpSignatureResultBuilder signatureResultBuilder = new OpenPgpSignatureResultBuilder();
         PGPPublicKey signatureKey = null;
         int signatureIndex = -1;
-        boolean isSignatureKeyCertified = false;
 
         if (dataChunk instanceof PGPCompressedData) {
             updateProgress(R.string.progress_decompressing_data, currentProgress, 100);
@@ -426,18 +426,23 @@ public class PgpDecryptVerify {
 
             if (masterKeyId != null) {
                 // key found in our database!
+                signature = sigList.get(signatureIndex);
+
+                PGPPublicKeyRing publicKeyRing = null;
                 try {
-                    signatureKey = mProviderHelper
-                            .getPGPPublicKeyRing(masterKeyId).getPublicKey();
+                    publicKeyRing = mProviderHelper
+                            .getPGPPublicKeyRing(masterKeyId);
                 } catch (ProviderHelper.NotFoundException e) {
                     // can't happen
                 }
 
-                signature = sigList.get(signatureIndex);
+                // get the subkey which has been used to generate this signature
+                signatureKey = publicKeyRing.getPublicKey(signature.getKeyID());
 
                 signatureResultBuilder.knownKey(true);
-                signatureResultBuilder.userId(PgpKeyHelper.getMainUserId(signatureKey));
-                signatureResultBuilder.keyId(signature.getKeyID());
+                // TODO: uses the first pubkey for information
+                signatureResultBuilder.userId(PgpKeyHelper.getMainUserId(publicKeyRing.getPublicKey()));
+                signatureResultBuilder.keyId(publicKeyRing.getPublicKey().getKeyID());
 
                 JcaPGPContentVerifierBuilderProvider contentVerifierBuilderProvider =
                         new JcaPGPContentVerifierBuilderProvider()
@@ -449,8 +454,8 @@ public class PgpDecryptVerify {
                         KeychainContract.KeyRings.buildUnifiedKeyRingUri(Long.toString(masterKeyId)),
                         KeyRings.VERIFIED,
                         ProviderHelper.FIELD_TYPE_INTEGER);
-
-                isSignatureKeyCertified = ((Long) data > 0);
+                boolean isSignatureKeyCertified = ((Long) data > 0);
+                signatureResultBuilder.signatureKeyCertified(isSignatureKeyCertified);
             } else {
                 // no key in our database -> return "unknown pub key" status including the first key id
                 signatureResultBuilder.knownKey(false);
@@ -529,7 +534,6 @@ public class PgpDecryptVerify {
 
                 signatureResultBuilder.validSignature(validSignature);
                 signatureResultBuilder.validKeyBinding(validKeyBinding);
-                signatureResultBuilder.signatureKeyCertified(isSignatureKeyCertified);
             }
         }
 
@@ -622,22 +626,25 @@ public class PgpDecryptVerify {
 
         PGPSignature signature = null;
         PGPPublicKey signatureKey = null;
-        boolean isSignatureKeyCertified = false;
         if (masterKeyId != null) {
             // key found in our database!
+            signature = sigList.get(signatureIndex);
 
+            PGPPublicKeyRing publicKeyRing = null;
             try {
-                signatureKey = mProviderHelper
-                        .getPGPPublicKeyRing(masterKeyId).getPublicKey();
+                publicKeyRing = mProviderHelper
+                        .getPGPPublicKeyRing(masterKeyId);
             } catch (ProviderHelper.NotFoundException e) {
                 // can't happen
             }
 
-            signature = sigList.get(signatureIndex);
+            // get the subkey which has been used to generate this signature
+            signatureKey = publicKeyRing.getPublicKey(signature.getKeyID());
 
             signatureResultBuilder.knownKey(true);
-            signatureResultBuilder.userId(PgpKeyHelper.getMainUserId(signatureKey));
-            signatureResultBuilder.keyId(signature.getKeyID());
+            // TODO: uses the first pubkey for information
+            signatureResultBuilder.userId(PgpKeyHelper.getMainUserId(publicKeyRing.getPublicKey()));
+            signatureResultBuilder.keyId(publicKeyRing.getPublicKey().getKeyID());
 
             JcaPGPContentVerifierBuilderProvider contentVerifierBuilderProvider =
                     new JcaPGPContentVerifierBuilderProvider()
@@ -649,8 +656,8 @@ public class PgpDecryptVerify {
                     KeychainContract.KeyRings.buildUnifiedKeyRingUri(Long.toString(masterKeyId)),
                     KeyRings.VERIFIED,
                     ProviderHelper.FIELD_TYPE_INTEGER);
-
-            isSignatureKeyCertified = ((Long) data > 0);
+            boolean isSignatureKeyCertified = ((Long) data > 0);
+            signatureResultBuilder.signatureKeyCertified(isSignatureKeyCertified);
         } else {
             // no key in our database -> return "unknown pub key" status including the first key id
             signatureResultBuilder.knownKey(false);
@@ -692,7 +699,6 @@ public class PgpDecryptVerify {
 
             signatureResultBuilder.validSignature(validSignature);
             signatureResultBuilder.validKeyBinding(validKeyBinding);
-            signatureResultBuilder.signatureKeyCertified(isSignatureKeyCertified);
         }
 
         result.setSignatureResult(signatureResultBuilder.build());
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyHelper.java
index d36dd8efe..6450ad63d 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyHelper.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyHelper.java
@@ -60,6 +60,34 @@ public class PgpKeyHelper {
         return key.getPublicKey().getCreationTime();
     }
 
+    public static Date getExpiryDate(PGPPublicKey key) {
+        Date creationDate = getCreationDate(key);
+        if (key.getValidDays() == 0) {
+            // no expiry
+            return null;
+        }
+        Calendar calendar = GregorianCalendar.getInstance();
+        calendar.setTime(creationDate);
+        calendar.add(Calendar.DATE, key.getValidDays());
+
+        return calendar.getTime();
+    }
+
+    public static Date getExpiryDate(PGPSecretKey key) {
+        return getExpiryDate(key.getPublicKey());
+    }
+
+    public static boolean isExpired(PGPPublicKey key) {
+        Date creationDate = getCreationDate(key);
+        Date expiryDate = getExpiryDate(key);
+        Date now = new Date();
+        if (now.compareTo(creationDate) >= 0
+                && (expiryDate == null || now.compareTo(expiryDate) <= 0)) {
+            return false;
+        }
+        return true;
+    }
+
     @SuppressWarnings("unchecked")
     public static PGPSecretKey getKeyNum(PGPSecretKeyRing keyRing, long num) {
         long cnt = 0;
@@ -77,7 +105,7 @@ public class PgpKeyHelper {
     }
 
     @SuppressWarnings("unchecked")
-    public static Vector<PGPPublicKey> getEncryptKeys(PGPPublicKeyRing keyRing) {
+    private static Vector<PGPPublicKey> getEncryptKeys(PGPPublicKeyRing keyRing) {
         Vector<PGPPublicKey> encryptKeys = new Vector<PGPPublicKey>();
 
         for (PGPPublicKey key : new IterableIterator<PGPPublicKey>(keyRing.getPublicKeys())) {
@@ -90,7 +118,7 @@ public class PgpKeyHelper {
     }
 
     @SuppressWarnings("unchecked")
-    public static Vector<PGPSecretKey> getSigningKeys(PGPSecretKeyRing keyRing) {
+    private static Vector<PGPSecretKey> getSigningKeys(PGPSecretKeyRing keyRing) {
         Vector<PGPSecretKey> signingKeys = new Vector<PGPSecretKey>();
 
         for (PGPSecretKey key : new IterableIterator<PGPSecretKey>(keyRing.getSecretKeys())) {
@@ -103,7 +131,7 @@ public class PgpKeyHelper {
     }
 
     @SuppressWarnings("unchecked")
-    public static Vector<PGPSecretKey> getCertificationKeys(PGPSecretKeyRing keyRing) {
+    private static Vector<PGPSecretKey> getCertificationKeys(PGPSecretKeyRing keyRing) {
         Vector<PGPSecretKey> signingKeys = new Vector<PGPSecretKey>();
 
         for (PGPSecretKey key : new IterableIterator<PGPSecretKey>(keyRing.getSecretKeys())) {
@@ -115,7 +143,7 @@ public class PgpKeyHelper {
         return signingKeys;
     }
 
-    public static Vector<PGPPublicKey> getUsableEncryptKeys(PGPPublicKeyRing keyRing) {
+    private static Vector<PGPPublicKey> getUsableEncryptKeys(PGPPublicKeyRing keyRing) {
         Vector<PGPPublicKey> usableKeys = new Vector<PGPPublicKey>();
         Vector<PGPPublicKey> encryptKeys = getEncryptKeys(keyRing);
         PGPPublicKey masterKey = null;
@@ -135,18 +163,7 @@ public class PgpKeyHelper {
         return usableKeys;
     }
 
-    public static boolean isExpired(PGPPublicKey key) {
-        Date creationDate = getCreationDate(key);
-        Date expiryDate = getExpiryDate(key);
-        Date now = new Date();
-        if (now.compareTo(creationDate) >= 0
-                && (expiryDate == null || now.compareTo(expiryDate) <= 0)) {
-            return false;
-        }
-        return true;
-    }
-
-    public static Vector<PGPSecretKey> getUsableCertificationKeys(PGPSecretKeyRing keyRing) {
+    private static Vector<PGPSecretKey> getUsableCertificationKeys(PGPSecretKeyRing keyRing) {
         Vector<PGPSecretKey> usableKeys = new Vector<PGPSecretKey>();
         Vector<PGPSecretKey> signingKeys = getCertificationKeys(keyRing);
         PGPSecretKey masterKey = null;
@@ -164,7 +181,7 @@ public class PgpKeyHelper {
         return usableKeys;
     }
 
-    public static Vector<PGPSecretKey> getUsableSigningKeys(PGPSecretKeyRing keyRing) {
+    private static Vector<PGPSecretKey> getUsableSigningKeys(PGPSecretKeyRing keyRing) {
         Vector<PGPSecretKey> usableKeys = new Vector<PGPSecretKey>();
         Vector<PGPSecretKey> signingKeys = getSigningKeys(keyRing);
         PGPSecretKey masterKey = null;
@@ -182,22 +199,6 @@ public class PgpKeyHelper {
         return usableKeys;
     }
 
-    public static Date getExpiryDate(PGPPublicKey key) {
-        Date creationDate = getCreationDate(key);
-        if (key.getValidDays() == 0) {
-            // no expiry
-            return null;
-        }
-        Calendar calendar = GregorianCalendar.getInstance();
-        calendar.setTime(creationDate);
-        calendar.add(Calendar.DATE, key.getValidDays());
-
-        return calendar.getTime();
-    }
-
-    public static Date getExpiryDate(PGPSecretKey key) {
-        return getExpiryDate(key.getPublicKey());
-    }
 
     public static PGPPublicKey getFirstEncryptSubkey(PGPPublicKeyRing keyRing) {
         Vector<PGPPublicKey> encryptKeys = getUsableEncryptKeys(keyRing);