From 720f2dbef15ed342dd3f7ab1ad10b58aa6c5be5b Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 14 Feb 2017 17:55:06 +0100 Subject: [PATCH] remember the specific security problems of keys and symmetric algos during decryption --- .../results/DecryptVerifyResult.java | 35 ++++++++++ .../keychain/pgp/CanonicalizedPublicKey.java | 19 +++--- .../pgp/OpenPgpDecryptionResultBuilder.java | 31 ++++++--- .../pgp/OpenPgpSignatureResultBuilder.java | 21 +++--- .../pgp/PgpDecryptVerifyOperation.java | 26 ++++--- .../keychain/pgp/PgpSecurityConstants.java | 68 +++++++++++++++---- .../keychain/pgp/PgpSignatureChecker.java | 29 +++++--- .../keychain/pgp/SecurityProblem.java | 67 ++++++++++++++++++ 8 files changed, 241 insertions(+), 55 deletions(-) create mode 100644 OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/SecurityProblem.java diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/DecryptVerifyResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/DecryptVerifyResult.java index 0ebcb321d..65f36cc70 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/DecryptVerifyResult.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/DecryptVerifyResult.java @@ -18,11 +18,17 @@ package org.sufficientlysecure.keychain.operations.results; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + import android.os.Parcel; import org.openintents.openpgp.OpenPgpDecryptionResult; import org.openintents.openpgp.OpenPgpMetadata; import org.openintents.openpgp.OpenPgpSignatureResult; +import org.sufficientlysecure.keychain.pgp.SecurityProblem; import org.sufficientlysecure.keychain.service.input.CryptoInputParcel; import org.sufficientlysecure.keychain.service.input.RequiredInputParcel; @@ -34,6 +40,7 @@ public class DecryptVerifyResult extends InputPendingResult { OpenPgpSignatureResult mSignatureResult; OpenPgpDecryptionResult mDecryptionResult; OpenPgpMetadata mDecryptionMetadata; + ArrayList mSecurityProblems; CryptoInputParcel mCachedCryptoInputParcel; @@ -65,6 +72,8 @@ public class DecryptVerifyResult extends InputPendingResult { mDecryptionMetadata = source.readParcelable(OpenPgpMetadata.class.getClassLoader()); mCachedCryptoInputParcel = source.readParcelable(CryptoInputParcel.class.getClassLoader()); mSkippedDisallowedKeys = source.createLongArray(); + + mSecurityProblems = (ArrayList) source.readSerializable(); } @@ -127,6 +136,8 @@ public class DecryptVerifyResult extends InputPendingResult { dest.writeParcelable(mDecryptionMetadata, flags); dest.writeParcelable(mCachedCryptoInputParcel, flags); dest.writeLongArray(mSkippedDisallowedKeys); + + dest.writeSerializable(mSecurityProblems); } public static final Creator CREATOR = new Creator() { @@ -139,4 +150,28 @@ public class DecryptVerifyResult extends InputPendingResult { } }; + public void addSecurityProblem(SecurityProblem securityProblem) { + if (securityProblem == null) { + return; + } + if (mSecurityProblems == null) { + mSecurityProblems = new ArrayList<>(); + } + mSecurityProblems.add(securityProblem); + } + + public void addSecurityProblems(List securityProblems) { + if (securityProblems == null) { + return; + } + if (mSecurityProblems == null) { + mSecurityProblems = new ArrayList<>(); + } + mSecurityProblems.addAll(securityProblems); + } + + public List getSecurityProblems() { + return mSecurityProblems != null ? + Collections.unmodifiableList(mSecurityProblems) : Collections.emptyList(); + } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java index d95cf9a31..fc0baaf07 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java @@ -18,6 +18,15 @@ package org.sufficientlysecure.keychain.pgp; + +import java.io.IOException; +import java.security.PublicKey; +import java.security.interfaces.ECPublicKey; +import java.util.Calendar; +import java.util.Date; +import java.util.GregorianCalendar; +import java.util.Iterator; + import org.bouncycastle.asn1.ASN1ObjectIdentifier; import org.bouncycastle.asn1.nist.NISTObjectIdentifiers; import org.bouncycastle.bcpg.ECDHPublicBCPGKey; @@ -36,14 +45,6 @@ import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; import org.sufficientlysecure.keychain.util.IterableIterator; import org.sufficientlysecure.keychain.util.Log; -import java.io.IOException; -import java.security.PublicKey; -import java.security.interfaces.ECPublicKey; -import java.util.Calendar; -import java.util.Date; -import java.util.GregorianCalendar; -import java.util.Iterator; - /** Wrapper for a PGPPublicKey. * * The methods implemented in this class are a thin layer over @@ -141,7 +142,7 @@ public class CanonicalizedPublicKey extends UncachedPublicKey { } public boolean isSecure() { - return PgpSecurityConstants.isSecureKey(this); + return PgpSecurityConstants.checkForSecurityProblems(this) == null; } public long getValidSeconds() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpDecryptionResultBuilder.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpDecryptionResultBuilder.java index 31a3f91b6..e2c0b6b63 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpDecryptionResultBuilder.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpDecryptionResultBuilder.java @@ -17,35 +17,49 @@ package org.sufficientlysecure.keychain.pgp; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + import org.openintents.openpgp.OpenPgpDecryptionResult; import org.sufficientlysecure.keychain.Constants; +import org.sufficientlysecure.keychain.pgp.SecurityProblem.SymmetricAlgorithmProblem; import org.sufficientlysecure.keychain.util.Log; public class OpenPgpDecryptionResultBuilder { // builder - private boolean mInsecure = false; - private boolean mEncrypted = false; + private boolean isEncrypted = false; private byte[] sessionKey; private byte[] decryptedSessionKey; + private ArrayList securityProblems; - public void setInsecure(boolean insecure) { - this.mInsecure = insecure; + public void addSecurityProblem(SecurityProblem securityProblem) { + if (securityProblems == null) { + securityProblems = new ArrayList<>(); + } + securityProblems.add(securityProblem); + } + + public List getKeySecurityProblems() { + return securityProblems != null ? Collections.unmodifiableList(securityProblems) : null; } public void setEncrypted(boolean encrypted) { - this.mEncrypted = encrypted; + this.isEncrypted = encrypted; } public OpenPgpDecryptionResult build() { - if (mInsecure) { + if (securityProblems != null && !securityProblems.isEmpty()) { Log.d(Constants.TAG, "RESULT_INSECURE"); return new OpenPgpDecryptionResult(OpenPgpDecryptionResult.RESULT_INSECURE, sessionKey, decryptedSessionKey); } - if (mEncrypted) { + if (isEncrypted) { Log.d(Constants.TAG, "RESULT_ENCRYPTED"); - return new OpenPgpDecryptionResult(OpenPgpDecryptionResult.RESULT_ENCRYPTED, sessionKey, decryptedSessionKey); + return new OpenPgpDecryptionResult( + OpenPgpDecryptionResult.RESULT_ENCRYPTED, sessionKey, decryptedSessionKey); } Log.d(Constants.TAG, "RESULT_NOT_ENCRYPTED"); @@ -60,4 +74,5 @@ public class OpenPgpDecryptionResultBuilder { this.sessionKey = sessionKey; this.decryptedSessionKey = decryptedSessionKey; } + } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java index 9c04c5394..83b99776f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java @@ -19,7 +19,9 @@ package org.sufficientlysecure.keychain.pgp; import java.util.ArrayList; +import java.util.Collections; import java.util.Date; +import java.util.List; import org.openintents.openpgp.OpenPgpSignatureResult; import org.openintents.openpgp.OpenPgpSignatureResult.SenderStatusResult; @@ -53,9 +55,9 @@ public class OpenPgpSignatureResultBuilder { private boolean mIsSignatureKeyCertified = false; private boolean mIsKeyRevoked = false; private boolean mIsKeyExpired = false; - private boolean mInsecure = false; private String mSenderAddress; private Date mSignatureTimestamp; + private ArrayList mSecurityProblems; public OpenPgpSignatureResultBuilder(KeyRepository keyRepository) { this.mKeyRepository = keyRepository; @@ -81,8 +83,15 @@ public class OpenPgpSignatureResultBuilder { this.mValidSignature = validSignature; } - public void setInsecure(boolean insecure) { - this.mInsecure = insecure; + public void addSecurityProblem(SecurityProblem securityProblem) { + if (mSecurityProblems == null) { + mSecurityProblems = new ArrayList<>(); + } + mSecurityProblems.add(securityProblem); + } + + public List getSecurityProblems() { + return mSecurityProblems != null ? Collections.unmodifiableList(mSecurityProblems) : null; } public void setSignatureKeyCertified(boolean isSignatureKeyCertified) { @@ -106,10 +115,6 @@ public class OpenPgpSignatureResultBuilder { this.mConfirmedUserIds = confirmedUserIds; } - public boolean isInsecure() { - return mInsecure; - } - public void initValid(CanonicalizedPublicKey signingKey) { setSignatureAvailable(true); setKnownKey(true); @@ -184,7 +189,7 @@ public class OpenPgpSignatureResultBuilder { } else if (mIsKeyExpired) { Log.d(Constants.TAG, "RESULT_INVALID_KEY_EXPIRED"); signatureStatus = OpenPgpSignatureResult.RESULT_INVALID_KEY_EXPIRED; - } else if (mInsecure) { + } else if (mSecurityProblems != null && !mSecurityProblems.isEmpty()) { Log.d(Constants.TAG, "RESULT_INVALID_INSECURE"); signatureStatus = OpenPgpSignatureResult.RESULT_INVALID_KEY_INSECURE; } else if (mIsSignatureKeyCertified) { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java index 743295007..d728b185c 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -68,6 +68,9 @@ import org.sufficientlysecure.keychain.operations.results.DecryptVerifyResult; import org.sufficientlysecure.keychain.operations.results.OperationResult.LogType; import org.sufficientlysecure.keychain.operations.results.OperationResult.OperationLog; import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey.SecretKeyType; +import org.sufficientlysecure.keychain.pgp.SecurityProblem.KeySecurityProblem; +import org.sufficientlysecure.keychain.pgp.SecurityProblem.MissingMdc; +import org.sufficientlysecure.keychain.pgp.SecurityProblem.SymmetricAlgorithmProblem; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; import org.sufficientlysecure.keychain.pgp.exception.PgpKeyNotFoundException; import org.sufficientlysecure.keychain.provider.CachedPublicKeyRing; @@ -211,7 +214,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation skippedDisallowedEncryptionKeys = new HashSet<>(); - boolean insecureEncryptionKey = false; + KeySecurityProblem encryptionKeySecurityProblem = null; // convenience method to return with error public EncryptStreamResult with(DecryptVerifyResult result) { @@ -317,15 +320,17 @@ public class PgpDecryptVerifyOperation extends BaseOperation= 2048); + if (bitStrength < 2048) { + return new InsecureBitStrength(masterKeyId, subKeyId, algorithm, bitStrength); + } + return null; } // RSA_ENCRYPT, RSA_SIGN: deprecated in RFC 4880, use RSA_GENERAL with key flags case PublicKeyAlgorithmTags.ELGAMAL_ENCRYPT: { - return (key.getBitStrength() >= 2048); + if (bitStrength < 2048) { + return new InsecureBitStrength(masterKeyId, subKeyId, algorithm, bitStrength); + } + return null; } case PublicKeyAlgorithmTags.DSA: { - return (key.getBitStrength() >= 2048); + if (bitStrength < 2048) { + return new InsecureBitStrength(masterKeyId, subKeyId, algorithm, bitStrength); + } + return null; } case PublicKeyAlgorithmTags.ECDH: case PublicKeyAlgorithmTags.ECDSA: { - return PgpSecurityConstants.sCurveWhitelist.contains(key.getCurveOid()); + if (!PgpSecurityConstants.sCurveWhitelist.contains(curveOid)) { + return new NotWhitelistedCurve(masterKeyId, subKeyId, curveOid, algorithm); + } + return null; } // ELGAMAL_GENERAL: deprecated in RFC 4880, use ELGAMAL_ENCRYPT // DIFFIE_HELLMAN: unsure + // TODO specialize all cases! default: - return false; + return new UnidentifiedKeyProblem(masterKeyId, subKeyId, algorithm); } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java index 2f83e9518..679dcc5d4 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java @@ -25,18 +25,21 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.security.SignatureException; +import java.util.List; -import org.openintents.openpgp.OpenPgpSignatureResult; import org.bouncycastle.openpgp.PGPException; import org.bouncycastle.openpgp.PGPOnePassSignature; import org.bouncycastle.openpgp.PGPOnePassSignatureList; import org.bouncycastle.openpgp.PGPSignature; import org.bouncycastle.openpgp.PGPSignatureList; import org.bouncycastle.openpgp.operator.jcajce.JcaPGPContentVerifierBuilderProvider; +import org.openintents.openpgp.OpenPgpSignatureResult; import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.operations.results.OperationResult.LogType; import org.sufficientlysecure.keychain.operations.results.OperationResult.OperationLog; import org.sufficientlysecure.keychain.provider.KeyWritableRepository; +import org.sufficientlysecure.keychain.pgp.SecurityProblem.InsecureHashAlgorithm; +import org.sufficientlysecure.keychain.pgp.SecurityProblem.KeySecurityProblem; import org.sufficientlysecure.keychain.provider.KeychainContract.KeyRings; import org.sufficientlysecure.keychain.provider.KeyRepository; import org.sufficientlysecure.keychain.util.Log; @@ -134,10 +137,12 @@ class PgpSignatureChecker { } private void checkKeySecurity(OperationLog log, int indent) { - // TODO: checks on signingRing ? - if (!PgpSecurityConstants.isSecureKey(signingKey)) { + // TODO check primary key as well, not only the signing key + KeySecurityProblem keySecurityProblem = + PgpSecurityConstants.checkForSecurityProblems(signingKey); + if (keySecurityProblem != null) { log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1); - signatureResultBuilder.setInsecure(true); + signatureResultBuilder.addSecurityProblem(keySecurityProblem); } } @@ -233,9 +238,11 @@ class PgpSignatureChecker { } // check for insecure hash algorithms - if (!PgpSecurityConstants.isSecureHashAlgorithm(signature.getHashAlgorithm())) { + InsecureHashAlgorithm signatureSecurityProblem = + PgpSecurityConstants.checkSignatureAlgorithmForSecurityProblems(signature.getHashAlgorithm()); + if (signatureSecurityProblem != null) { log.add(LogType.MSG_DC_INSECURE_HASH_ALGO, indent + 1); - signatureResultBuilder.setInsecure(true); + signatureResultBuilder.addSecurityProblem(signatureSecurityProblem); } signatureResultBuilder.setSignatureTimestamp(signature.getCreationTime()); @@ -268,9 +275,11 @@ class PgpSignatureChecker { } // check for insecure hash algorithms - if (!PgpSecurityConstants.isSecureHashAlgorithm(onePassSignature.getHashAlgorithm())) { + InsecureHashAlgorithm signatureSecurityProblem = + PgpSecurityConstants.checkSignatureAlgorithmForSecurityProblems(onePassSignature.getHashAlgorithm()); + if (signatureSecurityProblem != null) { log.add(LogType.MSG_DC_INSECURE_HASH_ALGO, indent + 1); - signatureResultBuilder.setInsecure(true); + signatureResultBuilder.addSecurityProblem(signatureSecurityProblem); } signatureResultBuilder.setSignatureTimestamp(messageSignature.getCreationTime()); @@ -288,6 +297,10 @@ class PgpSignatureChecker { return signatureResultBuilder.build(); } + public List getSecurityProblems() { + return signatureResultBuilder.getSecurityProblems(); + } + /** * Mostly taken from ClearSignedFileProcessor in Bouncy Castle */ diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/SecurityProblem.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/SecurityProblem.java new file mode 100644 index 000000000..d7e985861 --- /dev/null +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/SecurityProblem.java @@ -0,0 +1,67 @@ +package org.sufficientlysecure.keychain.pgp; + + +import java.io.Serializable; + +public abstract class SecurityProblem implements Serializable { + + public static abstract class KeySecurityProblem extends SecurityProblem { + public final long masterKeyId; + public final long subKeyId; + public final int algorithm; + + private KeySecurityProblem(long masterKeyId, long subKeyId, int algorithm) { + this.masterKeyId = masterKeyId; + this.subKeyId = subKeyId; + this.algorithm = algorithm; + } + } + + public static abstract class SymmetricAlgorithmProblem extends SecurityProblem { + + } + + public static class InsecureBitStrength extends KeySecurityProblem { + public final int bitStrength; + + InsecureBitStrength(long masterKeyId, long subKeyId, int algorithm, int bitStrength) { + super(masterKeyId, subKeyId, algorithm); + this.bitStrength = bitStrength; + } + } + + public static class NotWhitelistedCurve extends KeySecurityProblem { + public final String curveOid; + + NotWhitelistedCurve(long masterKeyId, long subKeyId, String curveOid, int algorithm) { + super(masterKeyId, subKeyId, algorithm); + this.curveOid = curveOid; + } + } + + public static class UnidentifiedKeyProblem extends KeySecurityProblem { + UnidentifiedKeyProblem(long masterKeyId, long subKeyId, int algorithm) { + super(masterKeyId, subKeyId, algorithm); + } + } + + public static class InsecureHashAlgorithm extends SecurityProblem { + public final int hashAlgorithm; + + InsecureHashAlgorithm(int hashAlgorithm) { + this.hashAlgorithm = hashAlgorithm; + } + } + + public static class InsecureSymmetricAlgorithm extends SymmetricAlgorithmProblem { + public final int symmetricAlgorithm; + + InsecureSymmetricAlgorithm(int symmetricAlgorithm) { + this.symmetricAlgorithm = symmetricAlgorithm; + } + } + + public static class MissingMdc extends SymmetricAlgorithmProblem { + + } +}