From 706e60474d15f833ad5ead519518775812978bac Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sun, 28 Sep 2014 14:25:54 +0200 Subject: be more mindful of algorithm and usage flag interaction Fixes #895 --- .../keychain/pgp/UncachedKeyRing.java | 40 ++++++++++++++++++++-- .../keychain/pgp/UncachedPublicKey.java | 16 ++------- .../keychain/service/results/OperationResult.java | 2 ++ OpenKeychain/src/main/res/values/strings.xml | 2 ++ 4 files changed, 44 insertions(+), 16 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 0e9377890..b4842b0a5 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -619,8 +619,7 @@ public class UncachedKeyRing { boolean needsPrimaryBinding = false; // If the algorithm is even suitable for signing - if (key.getAlgorithm() != PublicKeyAlgorithmTags.ELGAMAL_ENCRYPT - && key.getAlgorithm() != PublicKeyAlgorithmTags.RSA_ENCRYPT) { + if (isSigningAlgo(key.getAlgorithm())) { // If this certificate says it allows signing for the key if (zert.getHashedSubPackets() != null && @@ -722,6 +721,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 & PGPKeyFlags.CAN_SIGN) == PGPKeyFlags.CAN_SIGN) { + 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 & PGPKeyFlags.CAN_ENCRYPT_COMMS) == PGPKeyFlags.CAN_ENCRYPT_COMMS + || (flags & PGPKeyFlags.CAN_ENCRYPT_STORAGE) == PGPKeyFlags.CAN_ENCRYPT_STORAGE + )) { + log.add(LogType.MSG_KC_SUB_ALGO_BAD_ENCRYPT, indent); + } + } + // re-add certification modified = PGPPublicKey.addCertification(modified, selfCert); // add revocation, if any @@ -953,4 +970,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; + } + } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java index 7f08d121e..74af9fc97 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java +++ b/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; @@ -264,12 +260,6 @@ public class UncachedPublicKey { 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; } @@ -279,9 +269,7 @@ public class UncachedPublicKey { return (getKeyUsage() & KeyFlags.SIGN_DATA) != 0; } - if (mPublicKey.getAlgorithm() == PGPPublicKey.RSA_GENERAL - || mPublicKey.getAlgorithm() == PGPPublicKey.RSA_SIGN - || mPublicKey.getAlgorithm() == PGPPublicKey.ECDSA) { + if (UncachedKeyRing.isSigningAlgo(mPublicKey.getAlgorithm())) { return true; } @@ -295,7 +283,7 @@ public class UncachedPublicKey { } // RSA_GENERAL, RSA_ENCRYPT, ELGAMAL_ENCRYPT, ELGAMAL_GENERAL, ECDH - if (mPublicKey.isEncryptionKey()) { + if (UncachedKeyRing.isEncryptionAlgo(mPublicKey.getAlgorithm())) { return true; } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java index b0a255162..0a4d9649f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java +++ b/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), diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index 14d0eba9b..f9ae6f029 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -684,6 +684,8 @@ "Removing bad subkey revocation certificate" "Removing redundant subkey revocation certificate" "Subkey uses an unknown algorithm, not importing…" + "Subkey has encryption usage flag, but algorithm is not suitable for encryption." + "Subkey has signing usage flag, but algorithm is not suitable for signing." "Keyring canonicalization successful, no changes" "Keyring canonicalization successful, removed one erroneous certificate" -- cgit v1.2.3