remember the specific security problems of keys and symmetric algos during decryption

This commit is contained in:
Vincent Breitmoser 2017-02-14 17:55:06 +01:00
parent c59a570a27
commit 720f2dbef1
8 changed files with 241 additions and 55 deletions

View file

@ -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<SecurityProblem> 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<SecurityProblem>) 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<DecryptVerifyResult> CREATOR = new Creator<DecryptVerifyResult>() {
@ -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<SecurityProblem> securityProblems) {
if (securityProblems == null) {
return;
}
if (mSecurityProblems == null) {
mSecurityProblems = new ArrayList<>();
}
mSecurityProblems.addAll(securityProblems);
}
public List<SecurityProblem> getSecurityProblems() {
return mSecurityProblems != null ?
Collections.unmodifiableList(mSecurityProblems) : Collections.<SecurityProblem>emptyList();
}
}

View file

@ -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() {

View file

@ -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<SecurityProblem> 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<SecurityProblem> 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;
}
}

View file

@ -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<SecurityProblem> 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<SecurityProblem> 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) {

View file

@ -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<PgpDecryptVerifyInp
int symmetricEncryptionAlgo = 0;
HashSet<Long> 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<PgpDecryptVerifyInp
decryptionResultBuilder.setSessionKey(esResult.sessionKey, esResult.decryptedSessionKey);
}
if (esResult.insecureEncryptionKey) {
if (esResult.encryptionKeySecurityProblem != null) {
log.add(LogType.MSG_DC_INSECURE_SYMMETRIC_ENCRYPTION_ALGO, indent + 1);
decryptionResultBuilder.setInsecure(true);
decryptionResultBuilder.addSecurityProblem(esResult.encryptionKeySecurityProblem);
}
// Check for insecure encryption algorithms!
if (!PgpSecurityConstants.isSecureSymmetricAlgorithm(esResult.symmetricEncryptionAlgo)) {
SymmetricAlgorithmProblem symmetricSecurityProblem =
PgpSecurityConstants.checkSecureSymmetricAlgorithm(esResult.symmetricEncryptionAlgo);
if (symmetricSecurityProblem != null) {
log.add(LogType.MSG_DC_INSECURE_SYMMETRIC_ENCRYPTION_ALGO, indent + 1);
decryptionResultBuilder.setInsecure(true);
decryptionResultBuilder.addSecurityProblem(symmetricSecurityProblem);
}
plainFact = new JcaSkipMarkerPGPObjectFactory(esResult.cleartextStream);
@ -531,7 +536,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
// Handle missing integrity protection like failed integrity protection!
// The MDC packet can be stripped by an attacker!
log.add(LogType.MSG_DC_INSECURE_MDC_MISSING, indent);
decryptionResultBuilder.setInsecure(true);
decryptionResultBuilder.addSecurityProblem(new MissingMdc());
}
}
@ -541,10 +546,11 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
// Return a positive result, with metadata and verification info
DecryptVerifyResult result = new DecryptVerifyResult(DecryptVerifyResult.RESULT_OK, log);
result.setCachedCryptoInputParcel(cryptoInput);
result.setSignatureResult(signatureChecker.getSignatureResult());
result.setDecryptionResult(decryptionResultBuilder.build());
result.addSecurityProblems(signatureChecker.getSecurityProblems());
result.addSecurityProblems(decryptionResultBuilder.getKeySecurityProblems());
result.setDecryptionMetadata(metadata);
result.mOperationTime = opTime;
@ -659,9 +665,11 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
}
// check for insecure encryption key
if ( ! PgpSecurityConstants.isSecureKey(candidateDecryptionKey)) {
KeySecurityProblem keySecurityProblem =
PgpSecurityConstants.checkForSecurityProblems(candidateDecryptionKey);
if (keySecurityProblem != null) {
log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1);
result.insecureEncryptionKey = true;
result.encryptionKeySecurityProblem = keySecurityProblem;
}
// we're good, write down the data for later

View file

@ -17,6 +17,12 @@
package org.sufficientlysecure.keychain.pgp;
import java.util.Arrays;
import java.util.HashSet;
import android.support.annotation.Nullable;
import org.bouncycastle.asn1.nist.NISTNamedCurves;
import org.bouncycastle.asn1.teletrust.TeleTrusTNamedCurves;
import org.bouncycastle.bcpg.CompressionAlgorithmTags;
@ -24,9 +30,14 @@ import org.bouncycastle.bcpg.HashAlgorithmTags;
import org.bouncycastle.bcpg.PublicKeyAlgorithmTags;
import org.bouncycastle.bcpg.SymmetricKeyAlgorithmTags;
import org.bouncycastle.crypto.ec.CustomNamedCurves;
import org.sufficientlysecure.keychain.pgp.SecurityProblem.InsecureBitStrength;
import org.sufficientlysecure.keychain.pgp.SecurityProblem.InsecureHashAlgorithm;
import org.sufficientlysecure.keychain.pgp.SecurityProblem.InsecureSymmetricAlgorithm;
import org.sufficientlysecure.keychain.pgp.SecurityProblem.KeySecurityProblem;
import org.sufficientlysecure.keychain.pgp.SecurityProblem.NotWhitelistedCurve;
import org.sufficientlysecure.keychain.pgp.SecurityProblem.SymmetricAlgorithmProblem;
import org.sufficientlysecure.keychain.pgp.SecurityProblem.UnidentifiedKeyProblem;
import java.util.Arrays;
import java.util.HashSet;
/**
* NIST requirements for 2011-2030 (http://www.keylength.com/en/4/):
@ -63,8 +74,11 @@ public class PgpSecurityConstants {
// CAMELLIA_256: not used widely
));
public static boolean isSecureSymmetricAlgorithm(int id) {
return sSymmetricAlgorithmsWhitelist.contains(id);
public static SymmetricAlgorithmProblem checkSecureSymmetricAlgorithm(int id) {
if (!sSymmetricAlgorithmsWhitelist.contains(id)) {
return new InsecureSymmetricAlgorithm(id);
}
return null;
}
/**
@ -93,8 +107,11 @@ public class PgpSecurityConstants {
// SHA224: Not used widely, Yahoo argues against it
));
public static boolean isSecureHashAlgorithm(int id) {
return sHashAlgorithmsWhitelist.contains(id);
public static InsecureHashAlgorithm checkSignatureAlgorithmForSecurityProblems(int hashAlgorithm) {
if (!sHashAlgorithmsWhitelist.contains(hashAlgorithm)) {
return new InsecureHashAlgorithm(hashAlgorithm);
}
return null;
}
/**
@ -117,26 +134,51 @@ public class PgpSecurityConstants {
TeleTrusTNamedCurves.getOID("brainpoolP512r1").getId()
));
public static boolean isSecureKey(CanonicalizedPublicKey key) {
switch (key.getAlgorithm()) {
static KeySecurityProblem checkForSecurityProblems(CanonicalizedPublicKey key) {
long masterKeyId = key.getKeyRing().getMasterKeyId();
long subKeyId = key.getKeyId();
int algorithm = key.getAlgorithm();
Integer bitStrength = key.getBitStrength();
String curveOid = key.getCurveOid();
return getKeySecurityProblem(masterKeyId, subKeyId, algorithm, bitStrength, curveOid);
}
@Nullable
private static KeySecurityProblem getKeySecurityProblem(long masterKeyId, long subKeyId, int algorithm,
Integer bitStrength, String curveOid) {
switch (algorithm) {
case PublicKeyAlgorithmTags.RSA_GENERAL: {
return (key.getBitStrength() >= 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);
}
}

View file

@ -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<SecurityProblem> getSecurityProblems() {
return signatureResultBuilder.getSecurityProblems();
}
/**
* Mostly taken from ClearSignedFileProcessor in Bouncy Castle
*/

View file

@ -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 {
}
}