Merge branch 'master' of github.com:open-keychain/open-keychain

OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java

@@ -370,8 +370,6 @@ public class UncachedKeyringCanonicalizeTest {
     @Test public void testSubkeyBindingNoPKB() throws Exception {
 
         UncachedPublicKey pKey = KeyringTestingHelper.getNth(ring.getPublicKeys(), 1);
-        Assert.assertTrue("second subkey must be able to sign", pKey.canSign());
-
         PGPSignature sig;
 
         subHashedPacketsGen.setKeyFlags(false, KeyFlags.SIGN_DATA);

OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/provider/ProviderHelperSaveTest.java

@@ -32,6 +32,7 @@ import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey;
 import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey.SecretKeyType;
 import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKeyRing;
 import org.sufficientlysecure.keychain.pgp.UncachedKeyRing;
+import org.sufficientlysecure.keychain.service.results.OperationResult.OperationLog;
 import org.sufficientlysecure.keychain.service.results.SaveKeyringResult;
 import org.sufficientlysecure.keychain.util.IterableIterator;
 import org.sufficientlysecure.keychain.util.ProgressScaler;
@@ -80,7 +81,8 @@ public class ProviderHelperSaveTest {
 
         UncachedKeyRing pub = readRingFromResource("/test-keys/mailvelope_07_no_key_flags.asc");
         long keyId = pub.getMasterKeyId();
-        Assert.assertNull("key flags should be null", pub.getPublicKey().getKeyUsage());
+        Assert.assertNull("key flags should be null",
+                pub.canonicalize(new OperationLog(), 0).getPublicKey().getKeyUsage());
 
         mProviderHelper.savePublicKeyRing(pub);
 

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java

@@ -18,6 +18,7 @@
 
 package org.sufficientlysecure.keychain.pgp;
 
+import org.spongycastle.bcpg.sig.KeyFlags;
 import org.spongycastle.openpgp.PGPPublicKey;
 import org.spongycastle.openpgp.operator.jcajce.JcePublicKeyKeyEncryptionMethodGenerator;
 import org.sufficientlysecure.keychain.util.IterableIterator;
@@ -46,12 +47,61 @@ public class CanonicalizedPublicKey extends UncachedPublicKey {
         return new IterableIterator<String>(mPublicKey.getUserIDs());
     }
 
-    public KeyRing getKeyRing() {
-        return mRing;
-    }
-
     JcePublicKeyKeyEncryptionMethodGenerator getPubKeyEncryptionGenerator() {
         return new JcePublicKeyKeyEncryptionMethodGenerator(mPublicKey);
     }
 
+    public boolean canSign() {
+        // if key flags subpacket is available, honor it!
+        if (getKeyUsage() != null) {
+            return (getKeyUsage() & KeyFlags.SIGN_DATA) != 0;
+        }
+
+        if (UncachedKeyRing.isSigningAlgo(mPublicKey.getAlgorithm())) {
+            return true;
+        }
+
+        return false;
+    }
+
+    public boolean canCertify() {
+        // if key flags subpacket is available, honor it!
+        if (getKeyUsage() != null) {
+            return (getKeyUsage() & KeyFlags.CERTIFY_OTHER) != 0;
+        }
+
+        if (UncachedKeyRing.isSigningAlgo(mPublicKey.getAlgorithm())) {
+            return true;
+        }
+
+        return false;
+    }
+
+    public boolean canEncrypt() {
+        // if key flags subpacket is available, honor it!
+        if (getKeyUsage() != null) {
+            return (getKeyUsage() & (KeyFlags.ENCRYPT_COMMS | KeyFlags.ENCRYPT_STORAGE)) != 0;
+        }
+
+        // RSA_GENERAL, RSA_ENCRYPT, ELGAMAL_ENCRYPT, ELGAMAL_GENERAL, ECDH
+        if (UncachedKeyRing.isEncryptionAlgo(mPublicKey.getAlgorithm())) {
+            return true;
+        }
+
+        return false;
+    }
+
+    public boolean canAuthenticate() {
+        // if key flags subpacket is available, honor it!
+        if (getKeyUsage() != null) {
+            return (getKeyUsage() & KeyFlags.AUTHENTICATION) != 0;
+        }
+
+        return false;
+    }
+
+    /** Same method as superclass, but we make it public. */
+    public Integer getKeyUsage() {
+        return super.getKeyUsage();
+    }
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java

@@ -22,7 +22,6 @@ import org.spongycastle.bcpg.ArmoredOutputStream;
 import org.spongycastle.bcpg.PublicKeyAlgorithmTags;
 import org.spongycastle.bcpg.SignatureSubpacketTags;
 import org.spongycastle.bcpg.sig.KeyFlags;
-import org.spongycastle.openpgp.PGPKeyFlags;
 import org.spongycastle.openpgp.PGPKeyRing;
 import org.spongycastle.openpgp.PGPObjectFactory;
 import org.spongycastle.openpgp.PGPPublicKey;
@@ -616,16 +615,31 @@ public class UncachedKeyRing {
                         continue;
                     }
 
-                    // if this certificate says it allows signing for the key
+                    boolean needsPrimaryBinding = false;
+
+                    // If the algorithm is even suitable for signing
+                    if (isSigningAlgo(key.getAlgorithm())) {
+
+                        // If this certificate says it allows signing for the key
                         if (zert.getHashedSubPackets() != null &&
                                 zert.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.KEY_FLAGS)) {
-
                             int flags = ((KeyFlags) zert.getHashedSubPackets()
                                     .getSubpacket(SignatureSubpacketTags.KEY_FLAGS)).getFlags();
-                        if ((flags & PGPKeyFlags.CAN_SIGN) == PGPKeyFlags.CAN_SIGN) {
+                            if ((flags & KeyFlags.SIGN_DATA) == KeyFlags.SIGN_DATA) {
+                                needsPrimaryBinding = true;
+                            }
+                        } else {
+                            // If there are no key flags, we STILL require this because the key can sign!
+                            needsPrimaryBinding = true;
+                        }
+
+                    }
+
+                    // If this key can sign, it MUST have a primary key binding certificate
+                    if (needsPrimaryBinding) {
                         boolean ok = false;
-                            // it MUST have an embedded primary key binding signature
+                        if (zert.getUnhashedSubPackets() != null) try {
-                            try {
+                            // Check all embedded signatures, if any of them fits
                             PGPSignatureList list = zert.getUnhashedSubPackets().getEmbeddedSignatures();
                             for (int i = 0; i < list.size(); i++) {
                                 WrappedSignature subsig = new WrappedSignature(list.get(i));
@@ -653,8 +667,6 @@ public class UncachedKeyRing {
                         }
                     }
 
-                    }
-
                     // if we already have a cert, and this one is older: skip it
                     if (selfCert != null && cert.getCreationTime().before(selfCert.getCreationTime())) {
                         log.add(LogType.MSG_KC_SUB_DUP, indent);
@@ -708,6 +720,24 @@ public class UncachedKeyRing {
                 continue;
             }
 
+            // If we have flags, check if the algorithm supports all of them
+            if (selfCert.getHashedSubPackets() == null
+                    && selfCert.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.KEY_FLAGS)) {
+                int flags = ((KeyFlags) selfCert.getHashedSubPackets().getSubpacket(SignatureSubpacketTags.KEY_FLAGS)).getFlags();
+                int algo = key.getAlgorithm();
+                // If this is a signing key, but not a signing algorithm, warn the user
+                if (!isSigningAlgo(algo) && (flags & KeyFlags.SIGN_DATA) == KeyFlags.SIGN_DATA) {
+                    log.add(LogType.MSG_KC_SUB_ALGO_BAD_SIGN, indent);
+                }
+                // If this is an encryption key, but not an encryption algorithm, warn the user
+                if (!isEncryptionAlgo(algo) && (
+                           (flags & KeyFlags.ENCRYPT_STORAGE) == KeyFlags.ENCRYPT_STORAGE
+                        || (flags & KeyFlags.ENCRYPT_COMMS) == KeyFlags.ENCRYPT_COMMS
+                    )) {
+                    log.add(LogType.MSG_KC_SUB_ALGO_BAD_ENCRYPT, indent);
+                }
+            }
+
             // re-add certification
             modified = PGPPublicKey.addCertification(modified, selfCert);
             // add revocation, if any
@@ -939,4 +969,23 @@ public class UncachedKeyRing {
         }
     }
 
+
+    /** Returns true if the algorithm is of a type which is suitable for signing. */
+    static boolean isSigningAlgo(int algorithm) {
+        return algorithm == PGPPublicKey.RSA_GENERAL
+                || algorithm == PGPPublicKey.RSA_SIGN
+                || algorithm == PGPPublicKey.DSA
+                || algorithm == PGPPublicKey.ELGAMAL_GENERAL
+                || algorithm == PGPPublicKey.ECDSA;
+    }
+
+    /** Returns true if the algorithm is of a type which is suitable for encryption. */
+    static boolean isEncryptionAlgo(int algorithm) {
+        return algorithm == PGPPublicKey.RSA_GENERAL
+                || algorithm == PGPPublicKey.RSA_ENCRYPT
+                || algorithm == PGPPublicKey.ELGAMAL_ENCRYPT
+                || algorithm == PGPPublicKey.ELGAMAL_GENERAL
+                || algorithm == PGPPublicKey.ECDH;
+    }
+
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java

@@ -18,9 +18,6 @@
 
 package org.sufficientlysecure.keychain.pgp;
 
-import org.spongycastle.asn1.ASN1ObjectIdentifier;
-import org.spongycastle.asn1.nist.NISTNamedCurves;
-import org.spongycastle.asn1.teletrust.TeleTrusTNamedCurves;
 import org.spongycastle.bcpg.ECPublicBCPGKey;
 import org.spongycastle.bcpg.SignatureSubpacketTags;
 import org.spongycastle.bcpg.sig.KeyFlags;
@@ -28,7 +25,6 @@ import org.spongycastle.openpgp.PGPPublicKey;
 import org.spongycastle.openpgp.PGPSignature;
 import org.spongycastle.openpgp.PGPSignatureSubpacketVector;
 import org.spongycastle.openpgp.operator.jcajce.JcaPGPContentVerifierBuilderProvider;
-import org.spongycastle.util.Strings;
 import org.sufficientlysecure.keychain.Constants;
 import org.sufficientlysecure.keychain.util.IterableIterator;
 import org.sufficientlysecure.keychain.util.Log;
@@ -232,92 +228,12 @@ public class UncachedPublicKey {
         return getAlgorithm() == PGPPublicKey.ECDH || getAlgorithm() == PGPPublicKey.ECDSA;
     }
 
-    /**
-     * Get all key usage flags.
-     * If at least one key flag subpacket is present return these.
-     * If no subpacket is present it returns null.
-     */
-    @SuppressWarnings("unchecked")
-    public Integer getKeyUsage() {
-        if (mCacheUsage == null) {
-            for (PGPSignature sig : new IterableIterator<PGPSignature>(mPublicKey.getSignatures())) {
-                if (mPublicKey.isMasterKey() && sig.getKeyID() != mPublicKey.getKeyID()) {
-                    continue;
-                }
-
-                PGPSignatureSubpacketVector hashed = sig.getHashedSubPackets();
-                if (hashed != null && hashed.getSubpacket(SignatureSubpacketTags.KEY_FLAGS) != null) {
-                    // init if at least one key flag subpacket has been found
-                    if (mCacheUsage == null) {
-                        mCacheUsage = 0;
-                    }
-                    mCacheUsage |= hashed.getKeyFlags();
-                }
-            }
-        }
-        return mCacheUsage;
-    }
-
-    public boolean canCertify() {
-        // if key flags subpacket is available, honor it!
-        if (getKeyUsage() != null) {
-            return (getKeyUsage() & KeyFlags.CERTIFY_OTHER) != 0;
-        }
-
-        if (mPublicKey.getAlgorithm() == PGPPublicKey.RSA_GENERAL
-                || mPublicKey.getAlgorithm() == PGPPublicKey.RSA_SIGN
-                || mPublicKey.getAlgorithm() == PGPPublicKey.ECDSA) {
-            return true;
-        }
-
-        return false;
-    }
-
-    public boolean canSign() {
-        // if key flags subpacket is available, honor it!
-        if (getKeyUsage() != null) {
-            return (getKeyUsage() & KeyFlags.SIGN_DATA) != 0;
-        }
-
-        if (mPublicKey.getAlgorithm() == PGPPublicKey.RSA_GENERAL
-                || mPublicKey.getAlgorithm() == PGPPublicKey.RSA_SIGN
-                || mPublicKey.getAlgorithm() == PGPPublicKey.ECDSA) {
-            return true;
-        }
-
-        return false;
-    }
-
-    public boolean canEncrypt() {
-        // if key flags subpacket is available, honor it!
-        if (getKeyUsage() != null) {
-            return (getKeyUsage() & (KeyFlags.ENCRYPT_COMMS | KeyFlags.ENCRYPT_STORAGE)) != 0;
-        }
-
-        // RSA_GENERAL, RSA_ENCRYPT, ELGAMAL_ENCRYPT, ELGAMAL_GENERAL, ECDH
-        if (mPublicKey.isEncryptionKey()) {
-            return true;
-        }
-
-        return false;
-    }
-
-    public boolean canAuthenticate() {
-        // if key flags subpacket is available, honor it!
-        if (getKeyUsage() != null) {
-            return (getKeyUsage() & KeyFlags.AUTHENTICATION) != 0;
-        }
-
-        return false;
-    }
-
     public byte[] getFingerprint() {
         return mPublicKey.getFingerprint();
     }
 
-    // TODO This method should have package visibility - no access outside the pgp package!
     // (It's still used in ProviderHelper at this point)
-    public PGPPublicKey getPublicKey() {
+    PGPPublicKey getPublicKey() {
         return mPublicKey;
     }
 
@@ -355,4 +271,33 @@ public class UncachedPublicKey {
         }
     }
 
+    /** Get all key usage flags.
+     * If at least one key flag subpacket is present return these. If no
+     * subpacket is present it returns null.
+     *
+     * Note that this method has package visiblity because it is used in test
+     * cases. Certificates of UncachedPublicKey instances can NOT be assumed to
+     * be verified, so the result of this method should not be used in other
+     * places!
+     */
+    @SuppressWarnings("unchecked")
+    Integer getKeyUsage() {
+        if (mCacheUsage == null) {
+            for (PGPSignature sig : new IterableIterator<PGPSignature>(mPublicKey.getSignatures())) {
+                if (mPublicKey.isMasterKey() && sig.getKeyID() != mPublicKey.getKeyID()) {
+                    continue;
+                }
+
+                PGPSignatureSubpacketVector hashed = sig.getHashedSubPackets();
+                if (hashed != null && hashed.getSubpacket(SignatureSubpacketTags.KEY_FLAGS) != null) {
+                    // init if at least one key flag subpacket has been found
+                    if (mCacheUsage == null) {
+                        mCacheUsage = 0;
+                    }
+                    mCacheUsage |= hashed.getKeyFlags();
+                }
+            }
+        }
+        return mCacheUsage;
+    }
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java

@@ -341,6 +341,8 @@ public abstract class OperationResult implements Parcelable {
         MSG_KC_SUB_REVOKE_BAD (LogLevel.WARN, R.string.msg_kc_sub_revoke_bad),
         MSG_KC_SUB_REVOKE_DUP (LogLevel.DEBUG, R.string.msg_kc_sub_revoke_dup),
         MSG_KC_SUB_UNKNOWN_ALGO (LogLevel.WARN, R.string.msg_kc_sub_unknown_algo),
+        MSG_KC_SUB_ALGO_BAD_ENCRYPT (LogLevel.WARN, R.string.msg_kc_sub_algo_bad_encrpyt),
+        MSG_KC_SUB_ALGO_BAD_SIGN (LogLevel.WARN, R.string.msg_kc_sub_algo_bad_sign),
         MSG_KC_SUCCESS_BAD (LogLevel.OK, R.plurals.msg_kc_success_bad),
         MSG_KC_SUCCESS_BAD_AND_RED (LogLevel.OK, R.string.msg_kc_success_bad_and_red),
         MSG_KC_SUCCESS_REDUNDANT (LogLevel.OK, R.plurals.msg_kc_success_redundant),

OpenKeychain/src/main/res/values/strings.xml

@@ -684,6 +684,8 @@
     <string name="msg_kc_sub_revoke_bad">"Removing bad subkey revocation certificate"</string>
     <string name="msg_kc_sub_revoke_dup">"Removing redundant subkey revocation certificate"</string>
     <string name="msg_kc_sub_unknown_algo">"Subkey uses an unknown algorithm, not importing…"</string>
+    <string name="msg_kc_sub_algo_bad_encrpyt">"Subkey has encryption usage flag, but algorithm is not suitable for encryption."</string>
+    <string name="msg_kc_sub_algo_bad_sign">"Subkey has signing usage flag, but algorithm is not suitable for signing."</string>
     <string name="msg_kc_success">"Keyring canonicalization successful, no changes"</string>
     <plurals name="msg_kc_success_bad">
         <item quantity="one">"Keyring canonicalization successful, removed one erroneous certificate"</item>