From bafc10896922a50fb32d3eb105c389d863b53d20 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 6 Oct 2015 17:34:47 +0200 Subject: pgpdecryptverify: refactor signature processing --- .../keychain/pgp/CanonicalizedPublicKey.java | 8 +- .../pgp/OpenPgpSignatureResultBuilder.java | 5 +- .../keychain/pgp/PgpDecryptVerifyOperation.java | 148 ++++++++++----------- 3 files changed, 82 insertions(+), 79 deletions(-) (limited to 'OpenKeychain/src/main/java/org') 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 412468a48..476b4e59c 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java @@ -44,13 +44,17 @@ import java.util.Iterator; public class CanonicalizedPublicKey extends UncachedPublicKey { // this is the parent key ring - final KeyRing mRing; + final CanonicalizedKeyRing mRing; - CanonicalizedPublicKey(KeyRing ring, PGPPublicKey key) { + CanonicalizedPublicKey(CanonicalizedKeyRing ring, PGPPublicKey key) { super(key); mRing = ring; } + public CanonicalizedKeyRing getKeyRing() { + return mRing; + } + public IterableIterator getUserIds() { return new IterableIterator(mPublicKey.getUserIDs()); } 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 9d059b58f..2dd1e2dde 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java @@ -91,11 +91,12 @@ public class OpenPgpSignatureResultBuilder { return mInsecure; } - public void initValid(CanonicalizedPublicKeyRing signingRing, - CanonicalizedPublicKey signingKey) { + public void initValid(CanonicalizedPublicKey signingKey) { setSignatureAvailable(true); setKnownKey(true); + CanonicalizedKeyRing signingRing = signingKey.getKeyRing(); + // from RING setKeyId(signingRing.getMasterKeyId()); try { 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 36b4f5e1e..3bb442143 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -214,32 +214,15 @@ public class PgpDecryptVerifyOperation extends BaseOperation return "unknown pub key" status including the first key id if (!sigList.isEmpty()) { @@ -482,7 +446,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation return "unknown pub key" status including the first key id if (!sigList.isEmpty()) { @@ -1189,7 +1137,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation Date: Wed, 7 Oct 2015 18:57:43 +0200 Subject: pgpdecryptverify: fix one pass signature check, actually use bracketed structure --- .../operations/results/OperationResult.java | 2 ++ .../keychain/pgp/PgpDecryptVerifyOperation.java | 33 +++++++++++++++++++--- 2 files changed, 31 insertions(+), 4 deletions(-) (limited to 'OpenKeychain/src/main/java/org') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java index 00c88089a..3b65d9fb1 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java @@ -661,6 +661,7 @@ public abstract class OperationResult implements Parcelable { MSG_DC_ERROR_INPUT (LogLevel.ERROR, R.string.msg_dc_error_input), MSG_DC_ERROR_NO_DATA (LogLevel.ERROR, R.string.msg_dc_error_no_data), MSG_DC_ERROR_NO_KEY (LogLevel.ERROR, R.string.msg_dc_error_no_key), + MSG_DC_ERROR_NO_SIGNATURE (LogLevel.ERROR, R.string.msg_dc_error_no_signature), MSG_DC_ERROR_PGP_EXCEPTION (LogLevel.ERROR, R.string.msg_dc_error_pgp_exception), MSG_DC_INTEGRITY_CHECK_OK (LogLevel.INFO, R.string.msg_dc_integrity_check_ok), MSG_DC_OK_META_ONLY (LogLevel.OK, R.string.msg_dc_ok_meta_only), @@ -687,6 +688,7 @@ public abstract class OperationResult implements Parcelable { MSG_VL_ERROR_MISSING_SIGLIST (LogLevel.ERROR, R.string.msg_vl_error_no_siglist), MSG_VL_ERROR_MISSING_LITERAL (LogLevel.ERROR, R.string.msg_vl_error_missing_literal), MSG_VL_ERROR_MISSING_KEY (LogLevel.ERROR, R.string.msg_vl_error_wrong_key), + MSG_VL_ERROR_NO_SIGNATURE (LogLevel.ERROR, R.string.msg_vl_error_no_signature), MSG_VL_CLEAR_SIGNATURE_CHECK (LogLevel.DEBUG, R.string.msg_vl_clear_signature_check), MSG_VL_ERROR_INTEGRITY_CHECK (LogLevel.ERROR, R.string.msg_vl_error_integrity_check), MSG_VL_OK (LogLevel.OK, R.string.msg_vl_ok), 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 3bb442143..4f3f323a5 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -264,8 +264,20 @@ public class PgpDecryptVerifyOperation extends BaseOperation Date: Thu, 8 Oct 2015 13:53:58 +0200 Subject: pgpdecryptverify: refactor signature verification state into SignatureChecker subclass --- .../keychain/pgp/PgpDecryptVerifyOperation.java | 275 +++++++++++---------- 1 file changed, 141 insertions(+), 134 deletions(-) (limited to 'OpenKeychain/src/main/java/org') 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 4f3f323a5..d613b08eb 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -207,22 +207,18 @@ public class PgpDecryptVerifyOperation extends BaseOperation 0) { out.write(buffer, 0, length); - signature.update(buffer, 0, length); + signatureChecker.updateSignatureData(buffer, 0, length); } updateProgress(R.string.progress_verifying_signature, 95, 100); log.add(LogType.MSG_VL_CLEAR_SIGNATURE_CHECK, indent + 1); o = pgpF.nextObject(); - if ( ! (o instanceof PGPSignatureList) ) { - log.add(LogType.MSG_VL_ERROR_NO_SIGNATURE, indent); - return new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log); - } - PGPSignatureList signatureList = (PGPSignatureList) o; - if (signatureList.size() <= signatureData.signatureIndex) { - log.add(LogType.MSG_VL_ERROR_NO_SIGNATURE, indent); + if ( ! signatureChecker.verifySignature(o, log, indent) ) { return new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log); } - // PGPOnePassSignature and PGPSignature packets are "bracketed", - // so we need to take the last-minus-index'th element here - PGPSignature messageSignature = signatureList.get(signatureList.size() -1 -signatureData.signatureIndex); - - // Verify signature and check binding signatures - boolean validSignature = signature.verify(messageSignature); - if (validSignature) { - log.add(LogType.MSG_DC_CLEAR_SIGNATURE_OK, indent + 1); - } else { - log.add(LogType.MSG_DC_CLEAR_SIGNATURE_BAD, indent + 1); - } - - signatureResultBuilder.setValidSignature(validSignature); - - OpenPgpSignatureResult signatureResult = signatureResultBuilder.build(); + OpenPgpSignatureResult signatureResult = signatureChecker.getSignatureResult(); if (signatureResult.getResult() != OpenPgpSignatureResult.RESULT_VALID_CONFIRMED && signatureResult.getResult() != OpenPgpSignatureResult.RESULT_VALID_UNCONFIRMED) { @@ -364,7 +330,6 @@ public class PgpDecryptVerifyOperation extends BaseOperation return "unknown pub key" status including the first key id - if (!sigList.isEmpty()) { - signatureResultBuilder.setSignatureAvailable(true); - signatureResultBuilder.setKnownKey(false); - signatureResultBuilder.setKeyId(sigList.get(0).getKeyID()); - } - } - - // check for insecure signing key - // TODO: checks on signingRing ? - if (signatureData != null && ! PgpSecurityConstants.isSecureKey(signatureData.signingKey)) { - log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1); - signatureResultBuilder.setInsecure(true); - } - + SignatureChecker signatureChecker = new SignatureChecker(); + if (signatureChecker.initializeOnePassSignature(dataChunk, log, indent +1)) { dataChunk = plainFact.nextObject(); } @@ -544,16 +474,8 @@ public class PgpDecryptVerifyOperation extends BaseOperation 0) { @@ -587,40 +507,17 @@ public class PgpDecryptVerifyOperation extends BaseOperation Date: Thu, 8 Oct 2015 14:36:20 +0200 Subject: pgpdecryptverify: move cleartext verification into SignatureChecker --- .../keychain/pgp/PgpDecryptVerifyOperation.java | 295 ++++++++++----------- 1 file changed, 139 insertions(+), 156 deletions(-) (limited to 'OpenKeychain/src/main/java/org') 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 d613b08eb..888869994 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -251,7 +251,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation 0) { long progress = 100 * alreadyWritten / wholeSize; // stop at 100% for wrong file sizes... @@ -501,7 +502,6 @@ public class PgpDecryptVerifyOperation extends BaseOperation 0) { @@ -1001,21 +964,8 @@ public class PgpDecryptVerifyOperation extends BaseOperation return "unknown pub key" status including the first key id - if (!sigList.isEmpty()) { - signatureResultBuilder.setSignatureAvailable(true); - signatureResultBuilder.setKnownKey(false); - signatureResultBuilder.setKeyId(sigList.get(0).getKeyID()); - } - } - - // check for insecure signing key - // TODO: checks on signingRing ? - if (signatureData != null && ! PgpSecurityConstants.isSecureKey(signatureData.signingKey)) { - log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1); - signatureResultBuilder.setInsecure(true); - } - - return signature; - } - /** * Mostly taken from ClearSignedFileProcessor in Bouncy Castle */ @@ -1162,20 +1074,6 @@ public class PgpDecryptVerifyOperation extends BaseOperation Date: Thu, 8 Oct 2015 18:02:17 +0200 Subject: pgpdecryptverify: externalize PgpSignatureChecker --- .../keychain/pgp/PgpDecryptVerifyOperation.java | 330 ++------------------ .../keychain/pgp/PgpSignatureChecker.java | 336 +++++++++++++++++++++ 2 files changed, 369 insertions(+), 297 deletions(-) create mode 100644 OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java (limited to 'OpenKeychain/src/main/java/org') 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 888869994..f31b6af59 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -18,6 +18,17 @@ package org.sufficientlysecure.keychain.pgp; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.security.SignatureException; +import java.util.Date; +import java.util.Iterator; + import android.content.Context; import android.support.annotation.NonNull; import android.webkit.MimeTypeMap; @@ -33,18 +44,14 @@ import org.spongycastle.openpgp.PGPEncryptedDataList; import org.spongycastle.openpgp.PGPException; import org.spongycastle.openpgp.PGPKeyValidationException; import org.spongycastle.openpgp.PGPLiteralData; -import org.spongycastle.openpgp.PGPOnePassSignature; -import org.spongycastle.openpgp.PGPOnePassSignatureList; import org.spongycastle.openpgp.PGPPBEEncryptedData; import org.spongycastle.openpgp.PGPPublicKeyEncryptedData; -import org.spongycastle.openpgp.PGPSignature; import org.spongycastle.openpgp.PGPSignatureList; import org.spongycastle.openpgp.PGPUtil; import org.spongycastle.openpgp.jcajce.JcaPGPObjectFactory; import org.spongycastle.openpgp.operator.PBEDataDecryptorFactory; import org.spongycastle.openpgp.operator.PGPDigestCalculatorProvider; import org.spongycastle.openpgp.operator.jcajce.CachingDataDecryptorFactory; -import org.spongycastle.openpgp.operator.jcajce.JcaPGPContentVerifierBuilderProvider; import org.spongycastle.openpgp.operator.jcajce.JcaPGPDigestCalculatorProviderBuilder; import org.spongycastle.openpgp.operator.jcajce.JcePBEDataDecryptorFactoryBuilder; import org.spongycastle.util.encoders.DecoderException; @@ -68,17 +75,6 @@ import org.sufficientlysecure.keychain.util.Log; import org.sufficientlysecure.keychain.util.Passphrase; import org.sufficientlysecure.keychain.util.ProgressScaler; -import java.io.BufferedInputStream; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.FileNotFoundException; -import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; -import java.security.SignatureException; -import java.util.Date; -import java.util.Iterator; - public class PgpDecryptVerifyOperation extends BaseOperation { public PgpDecryptVerifyOperation(Context context, ProviderHelper providerHelper, Progressable progressable) { @@ -207,7 +203,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation 0) { - sig.update(line, 0, length); - } - } - private static int readInputLine(ByteArrayOutputStream bOut, InputStream fIn) throws IOException { bOut.reset(); @@ -1055,253 +1035,9 @@ public class PgpDecryptVerifyOperation extends BaseOperation= 0 && isWhiteSpace(line[end])) { - end--; - } - - return end + 1; - } - - private static boolean isWhiteSpace(byte b) { - return b == '\r' || b == '\n' || b == '\t' || b == ' '; - } - private static byte[] getLineSeparator() { String nl = System.getProperty("line.separator"); return nl.getBytes(); } - private class SignatureChecker { - - OpenPgpSignatureResultBuilder signatureResultBuilder = new OpenPgpSignatureResultBuilder(); - - private CanonicalizedPublicKey signingKey; - // we use the signatureIndex instead of the signature itself here for two reasons: - // 1) the signature may be either of type PGPSignature or PGPOnePassSignature (which have no common ancestor) - // 2) in case of the latter, we need the signatureIndex to know which PGPSignature to use later on - private int signatureIndex; - - PGPOnePassSignature onePassSignature; - PGPSignature signature; - - ProviderHelper mProviderHelper; - - boolean initializeSignature(Object dataChunk, OperationLog log, int indent) throws PGPException { - - if ( ! (dataChunk instanceof PGPSignatureList) ) { - return false; - } - - PGPSignatureList sigList = (PGPSignatureList) dataChunk; - findAvailableSignature(sigList); - - if (signingKey != null) { - - // key found in our database! - signatureResultBuilder.initValid(signingKey); - - JcaPGPContentVerifierBuilderProvider contentVerifierBuilderProvider = - new JcaPGPContentVerifierBuilderProvider() - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); - signature.init(contentVerifierBuilderProvider, signingKey.getPublicKey()); - checkKeySecurity(log, indent); - - - } else if (!sigList.isEmpty()) { - - signatureResultBuilder.setSignatureAvailable(true); - signatureResultBuilder.setKnownKey(false); - signatureResultBuilder.setKeyId(sigList.get(0).getKeyID()); - - } - - return true; - - } - - boolean initializeOnePassSignature(Object dataChunk, OperationLog log, int indent) throws PGPException { - - if ( ! (dataChunk instanceof PGPOnePassSignatureList) ) { - return false; - } - - log.add(LogType.MSG_DC_CLEAR_SIGNATURE, indent + 1); - - PGPOnePassSignatureList sigList = (PGPOnePassSignatureList) dataChunk; - findAvailableSignature(sigList); - - if (signingKey != null) { - - // key found in our database! - signatureResultBuilder.initValid(signingKey); - - JcaPGPContentVerifierBuilderProvider contentVerifierBuilderProvider = - new JcaPGPContentVerifierBuilderProvider() - .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); - onePassSignature.init(contentVerifierBuilderProvider, signingKey.getPublicKey()); - - checkKeySecurity(log, indent); - - } else if (!sigList.isEmpty()) { - - signatureResultBuilder.setSignatureAvailable(true); - signatureResultBuilder.setKnownKey(false); - signatureResultBuilder.setKeyId(sigList.get(0).getKeyID()); - - } - - return true; - - } - - private void checkKeySecurity(OperationLog log, int indent) { - // TODO: checks on signingRing ? - if ( ! PgpSecurityConstants.isSecureKey(signingKey)) { - log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1); - signatureResultBuilder.setInsecure(true); - } - } - - public boolean isInitialized() { - return signingKey != null; - } - - private void findAvailableSignature(PGPOnePassSignatureList sigList) { - // go through all signatures (should be just one), make sure we have - // the key and it matches the one we’re looking for - for (int i = 0; i < sigList.size(); ++i) { - try { - long sigKeyId = sigList.get(i).getKeyID(); - CanonicalizedPublicKeyRing signingRing = mProviderHelper.getCanonicalizedPublicKeyRing( - KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(sigKeyId) - ); - signatureIndex = i; - signingKey = signingRing.getPublicKey(sigKeyId); - onePassSignature = sigList.get(i); - return; - } catch (ProviderHelper.NotFoundException e) { - Log.d(Constants.TAG, "key not found, trying next signature..."); - } - } - } - - public void findAvailableSignature(PGPSignatureList sigList) { - // go through all signatures (should be just one), make sure we have - // the key and it matches the one we’re looking for - for (int i = 0; i < sigList.size(); ++i) { - try { - long sigKeyId = sigList.get(i).getKeyID(); - CanonicalizedPublicKeyRing signingRing = mProviderHelper.getCanonicalizedPublicKeyRing( - KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(sigKeyId) - ); - signatureIndex = i; - signingKey = signingRing.getPublicKey(sigKeyId); - signature = sigList.get(i); - return; - } catch (ProviderHelper.NotFoundException e) { - Log.d(Constants.TAG, "key not found, trying next signature..."); - } - } - } - - public void updateSignatureWithCleartext(byte[] clearText) throws IOException, SignatureException { - - InputStream sigIn = new BufferedInputStream(new ByteArrayInputStream(clearText)); - - ByteArrayOutputStream outputBuffer = new ByteArrayOutputStream(); - - int lookAhead = readInputLine(outputBuffer, sigIn); - - processLine(signature, outputBuffer.toByteArray()); - - if (lookAhead != -1) { - do { - lookAhead = readInputLine(outputBuffer, lookAhead, sigIn); - - signature.update((byte) '\r'); - signature.update((byte) '\n'); - - processLine(signature, outputBuffer.toByteArray()); - } while (lookAhead != -1); - } - - } - - public void updateSignatureData(byte[] buf, int off, int len) { - if (onePassSignature != null) { - onePassSignature.update(buf, off, len); - } - } - - void verifySignature(OperationLog log, int indent) throws PGPException { - - log.add(LogType.MSG_DC_CLEAR_SIGNATURE_CHECK, indent); - - // Verify signature - boolean validSignature = signature.verify(); - if (validSignature) { - log.add(LogType.MSG_DC_CLEAR_SIGNATURE_OK, indent + 1); - } else { - log.add(LogType.MSG_DC_CLEAR_SIGNATURE_BAD, indent + 1); - } - - // check for insecure hash algorithms - if (!PgpSecurityConstants.isSecureHashAlgorithm(onePassSignature.getHashAlgorithm())) { - log.add(LogType.MSG_DC_INSECURE_HASH_ALGO, indent + 1); - signatureResultBuilder.setInsecure(true); - } - - signatureResultBuilder.setValidSignature(validSignature); - - } - - boolean verifySignatureOnePass(Object o, OperationLog log, int indent) throws PGPException { - - if ( ! (o instanceof PGPSignatureList) ) { - log.add(LogType.MSG_DC_ERROR_NO_SIGNATURE, indent); - return false; - } - PGPSignatureList signatureList = (PGPSignatureList) o; - if (signatureList.size() <= signatureIndex) { - log.add(LogType.MSG_DC_ERROR_NO_SIGNATURE, indent); - return false; - } - - // PGPOnePassSignature and PGPSignature packets are "bracketed", - // so we need to take the last-minus-index'th element here - PGPSignature messageSignature = signatureList.get(signatureList.size() -1 - signatureIndex); - - // Verify signature - boolean validSignature = onePassSignature.verify(messageSignature); - if (validSignature) { - log.add(LogType.MSG_DC_CLEAR_SIGNATURE_OK, indent + 1); - } else { - log.add(LogType.MSG_DC_CLEAR_SIGNATURE_BAD, indent + 1); - } - - // check for insecure hash algorithms - if (!PgpSecurityConstants.isSecureHashAlgorithm(onePassSignature.getHashAlgorithm())) { - log.add(LogType.MSG_DC_INSECURE_HASH_ALGO, indent + 1); - signatureResultBuilder.setInsecure(true); - } - - signatureResultBuilder.setValidSignature(validSignature); - - return true; - - } - - public byte[] getSigningFingerprint() { - return signingKey.getFingerprint(); - } - - public OpenPgpSignatureResult getSignatureResult() { - return signatureResultBuilder.build(); - } - - } - } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java new file mode 100644 index 000000000..2fccf2197 --- /dev/null +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java @@ -0,0 +1,336 @@ +package org.sufficientlysecure.keychain.pgp; + + +import java.io.BufferedInputStream; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.security.SignatureException; + +import org.openintents.openpgp.OpenPgpSignatureResult; +import org.spongycastle.openpgp.PGPException; +import org.spongycastle.openpgp.PGPOnePassSignature; +import org.spongycastle.openpgp.PGPOnePassSignatureList; +import org.spongycastle.openpgp.PGPSignature; +import org.spongycastle.openpgp.PGPSignatureList; +import org.spongycastle.openpgp.operator.jcajce.JcaPGPContentVerifierBuilderProvider; +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.KeychainContract.KeyRings; +import org.sufficientlysecure.keychain.provider.ProviderHelper; +import org.sufficientlysecure.keychain.util.Log; + + +/** This class is used to track the state of a single signature verification. + * + * + */ +class PgpSignatureChecker { + + OpenPgpSignatureResultBuilder signatureResultBuilder = new OpenPgpSignatureResultBuilder(); + + private CanonicalizedPublicKey signingKey; + + private int signatureIndex; + PGPOnePassSignature onePassSignature; + PGPSignature signature; + + ProviderHelper mProviderHelper; + + PgpSignatureChecker(ProviderHelper providerHelper) { + mProviderHelper = providerHelper; + } + + boolean initializeSignature(Object dataChunk, OperationLog log, int indent) throws PGPException { + + if (!(dataChunk instanceof PGPSignatureList)) { + return false; + } + + PGPSignatureList sigList = (PGPSignatureList) dataChunk; + findAvailableSignature(sigList); + + if (signingKey != null) { + + // key found in our database! + signatureResultBuilder.initValid(signingKey); + + JcaPGPContentVerifierBuilderProvider contentVerifierBuilderProvider = + new JcaPGPContentVerifierBuilderProvider() + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); + signature.init(contentVerifierBuilderProvider, signingKey.getPublicKey()); + checkKeySecurity(log, indent); + + + } else if (!sigList.isEmpty()) { + + signatureResultBuilder.setSignatureAvailable(true); + signatureResultBuilder.setKnownKey(false); + signatureResultBuilder.setKeyId(sigList.get(0).getKeyID()); + + } + + return true; + + } + + boolean initializeOnePassSignature(Object dataChunk, OperationLog log, int indent) throws PGPException { + + if (!(dataChunk instanceof PGPOnePassSignatureList)) { + return false; + } + + log.add(LogType.MSG_DC_CLEAR_SIGNATURE, indent + 1); + + PGPOnePassSignatureList sigList = (PGPOnePassSignatureList) dataChunk; + findAvailableSignature(sigList); + + if (signingKey != null) { + + // key found in our database! + signatureResultBuilder.initValid(signingKey); + + JcaPGPContentVerifierBuilderProvider contentVerifierBuilderProvider = + new JcaPGPContentVerifierBuilderProvider() + .setProvider(Constants.BOUNCY_CASTLE_PROVIDER_NAME); + onePassSignature.init(contentVerifierBuilderProvider, signingKey.getPublicKey()); + + checkKeySecurity(log, indent); + + } else if (!sigList.isEmpty()) { + + signatureResultBuilder.setSignatureAvailable(true); + signatureResultBuilder.setKnownKey(false); + signatureResultBuilder.setKeyId(sigList.get(0).getKeyID()); + + } + + return true; + + } + + private void checkKeySecurity(OperationLog log, int indent) { + // TODO: checks on signingRing ? + if (!PgpSecurityConstants.isSecureKey(signingKey)) { + log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1); + signatureResultBuilder.setInsecure(true); + } + } + + public boolean isInitialized() { + return signingKey != null; + } + + private void findAvailableSignature(PGPOnePassSignatureList sigList) { + // go through all signatures (should be just one), make sure we have + // the key and it matches the one we’re looking for + for (int i = 0; i < sigList.size(); ++i) { + try { + long sigKeyId = sigList.get(i).getKeyID(); + CanonicalizedPublicKeyRing signingRing = mProviderHelper.getCanonicalizedPublicKeyRing( + KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(sigKeyId) + ); + signatureIndex = i; + signingKey = signingRing.getPublicKey(sigKeyId); + onePassSignature = sigList.get(i); + return; + } catch (ProviderHelper.NotFoundException e) { + Log.d(Constants.TAG, "key not found, trying next signature..."); + } + } + } + + public void findAvailableSignature(PGPSignatureList sigList) { + // go through all signatures (should be just one), make sure we have + // the key and it matches the one we’re looking for + for (int i = 0; i < sigList.size(); ++i) { + try { + long sigKeyId = sigList.get(i).getKeyID(); + CanonicalizedPublicKeyRing signingRing = mProviderHelper.getCanonicalizedPublicKeyRing( + KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(sigKeyId) + ); + signatureIndex = i; + signingKey = signingRing.getPublicKey(sigKeyId); + signature = sigList.get(i); + return; + } catch (ProviderHelper.NotFoundException e) { + Log.d(Constants.TAG, "key not found, trying next signature..."); + } + } + } + + public void updateSignatureWithCleartext(byte[] clearText) throws IOException, SignatureException { + + InputStream sigIn = new BufferedInputStream(new ByteArrayInputStream(clearText)); + + ByteArrayOutputStream outputBuffer = new ByteArrayOutputStream(); + + int lookAhead = readInputLine(outputBuffer, sigIn); + + processLine(signature, outputBuffer.toByteArray()); + + while (lookAhead != -1) { + lookAhead = readInputLine(outputBuffer, lookAhead, sigIn); + + signature.update((byte) '\r'); + signature.update((byte) '\n'); + + processLine(signature, outputBuffer.toByteArray()); + } + + } + + public void updateSignatureData(byte[] buf, int off, int len) { + if (signature != null) { + signature.update(buf, off, len); + } else if (onePassSignature != null) { + onePassSignature.update(buf, off, len); + } + } + + void verifySignature(OperationLog log, int indent) throws PGPException { + + log.add(LogType.MSG_DC_CLEAR_SIGNATURE_CHECK, indent); + + // Verify signature + boolean validSignature = signature.verify(); + if (validSignature) { + log.add(LogType.MSG_DC_CLEAR_SIGNATURE_OK, indent + 1); + } else { + log.add(LogType.MSG_DC_CLEAR_SIGNATURE_BAD, indent + 1); + } + + // check for insecure hash algorithms + if (!PgpSecurityConstants.isSecureHashAlgorithm(onePassSignature.getHashAlgorithm())) { + log.add(LogType.MSG_DC_INSECURE_HASH_ALGO, indent + 1); + signatureResultBuilder.setInsecure(true); + } + + signatureResultBuilder.setValidSignature(validSignature); + + } + + boolean verifySignatureOnePass(Object o, OperationLog log, int indent) throws PGPException { + + if (!(o instanceof PGPSignatureList)) { + log.add(LogType.MSG_DC_ERROR_NO_SIGNATURE, indent); + return false; + } + PGPSignatureList signatureList = (PGPSignatureList) o; + if (signatureList.size() <= signatureIndex) { + log.add(LogType.MSG_DC_ERROR_NO_SIGNATURE, indent); + return false; + } + + // PGPOnePassSignature and PGPSignature packets are "bracketed", + // so we need to take the last-minus-index'th element here + PGPSignature messageSignature = signatureList.get(signatureList.size() - 1 - signatureIndex); + + // Verify signature + boolean validSignature = onePassSignature.verify(messageSignature); + if (validSignature) { + log.add(LogType.MSG_DC_CLEAR_SIGNATURE_OK, indent + 1); + } else { + log.add(LogType.MSG_DC_CLEAR_SIGNATURE_BAD, indent + 1); + } + + // check for insecure hash algorithms + if (!PgpSecurityConstants.isSecureHashAlgorithm(onePassSignature.getHashAlgorithm())) { + log.add(LogType.MSG_DC_INSECURE_HASH_ALGO, indent + 1); + signatureResultBuilder.setInsecure(true); + } + + signatureResultBuilder.setValidSignature(validSignature); + + return true; + + } + + public byte[] getSigningFingerprint() { + return signingKey.getFingerprint(); + } + + public OpenPgpSignatureResult getSignatureResult() { + return signatureResultBuilder.build(); + } + + /** + * Mostly taken from ClearSignedFileProcessor in Bouncy Castle + */ + + private static void processLine(PGPSignature sig, byte[] line) + throws SignatureException { + int length = getLengthWithoutWhiteSpace(line); + if (length > 0) { + sig.update(line, 0, length); + } + } + + private static int readInputLine(ByteArrayOutputStream bOut, InputStream fIn) + throws IOException { + bOut.reset(); + + int lookAhead = -1; + int ch; + + while ((ch = fIn.read()) >= 0) { + bOut.write(ch); + if (ch == '\r' || ch == '\n') { + lookAhead = readPastEOL(bOut, ch, fIn); + break; + } + } + + return lookAhead; + } + + private static int readInputLine(ByteArrayOutputStream bOut, int lookAhead, InputStream fIn) + throws IOException { + bOut.reset(); + + int ch = lookAhead; + + do { + bOut.write(ch); + if (ch == '\r' || ch == '\n') { + lookAhead = readPastEOL(bOut, ch, fIn); + break; + } + } while ((ch = fIn.read()) >= 0); + + if (ch < 0) { + lookAhead = -1; + } + + return lookAhead; + } + + private static int readPastEOL(ByteArrayOutputStream bOut, int lastCh, InputStream fIn) + throws IOException { + int lookAhead = fIn.read(); + + if (lastCh == '\r' && lookAhead == '\n') { + bOut.write(lookAhead); + lookAhead = fIn.read(); + } + + return lookAhead; + } + + private static int getLengthWithoutWhiteSpace(byte[] line) { + int end = line.length - 1; + + while (end >= 0 && isWhiteSpace(line[end])) { + end--; + } + + return end + 1; + } + + private static boolean isWhiteSpace(byte b) { + return b == '\r' || b == '\n' || b == '\t' || b == ' '; + } + +} -- cgit v1.2.3 From e29f9017f7a83198537df2e2f5279359e2d86118 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 8 Oct 2015 18:36:03 +0200 Subject: pgpsignencrypt: unsupported parameter combinations are a bug --- .../keychain/operations/results/OperationResult.java | 1 - .../org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) (limited to 'OpenKeychain/src/main/java/org') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java index 3b65d9fb1..f959ddd76 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java @@ -705,7 +705,6 @@ public abstract class OperationResult implements Parcelable { // pgpsignencrypt MSG_PSE_ASYMMETRIC (LogLevel.INFO, R.string.msg_pse_asymmetric), - MSG_PSE_CLEARSIGN_ONLY (LogLevel.DEBUG, R.string.msg_pse_clearsign_only), MSG_PSE_COMPRESSING (LogLevel.DEBUG, R.string.msg_pse_compressing), MSG_PSE_ENCRYPTING (LogLevel.DEBUG, R.string.msg_pse_encrypting), MSG_PSE_ERROR_BAD_PASSPHRASE (LogLevel.ERROR, R.string.msg_pse_error_bad_passphrase), diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java index 29b2ef727..406c64ae8 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java @@ -491,9 +491,7 @@ public class PgpSignEncryptOperation extends BaseOperation { literalGen.close(); } else { - pOut = null; - // TODO: Is this log right? - log.add(LogType.MSG_PSE_CLEARSIGN_ONLY, indent); + throw new AssertionError("cannot clearsign in non-ascii armored text, this is a bug!"); } if (enableSignature) { -- cgit v1.2.3 From cda1ba47d27c668e99fe212f2ce0977962eabb86 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 8 Oct 2015 18:37:30 +0200 Subject: pgpdecryptverify: fix non-onepass signature checking --- .../org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java | 2 ++ .../java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) (limited to 'OpenKeychain/src/main/java/org') 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 f31b6af59..39cd65671 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -963,6 +963,8 @@ public class PgpDecryptVerifyOperation extends BaseOperation Date: Thu, 8 Oct 2015 19:54:50 +0200 Subject: pgpdecryptverify: get rid of duplicate code path for binary signature verification --- .../operations/KeybaseVerificationOperation.java | 1 - .../keychain/pgp/PgpDecryptVerifyInputParcel.java | 14 +--- .../keychain/pgp/PgpDecryptVerifyOperation.java | 94 +--------------------- 3 files changed, 2 insertions(+), 107 deletions(-) (limited to 'OpenKeychain/src/main/java/org') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/KeybaseVerificationOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/KeybaseVerificationOperation.java index aaff0a07c..f3ceac681 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/KeybaseVerificationOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/KeybaseVerificationOperation.java @@ -151,7 +151,6 @@ public class KeybaseVerificationOperation extends BaseOperation CREATOR = new Creator() { public PgpDecryptVerifyInputParcel createFromParcel(final Parcel source) { return new PgpDecryptVerifyInputParcel(source); 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 39cd65671..d3c722761 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -35,7 +35,6 @@ import android.webkit.MimeTypeMap; import org.openintents.openpgp.OpenPgpDecryptionResult; import org.openintents.openpgp.OpenPgpMetadata; -import org.openintents.openpgp.OpenPgpSignatureResult; import org.spongycastle.bcpg.ArmoredInputStream; import org.spongycastle.openpgp.PGPCompressedData; import org.spongycastle.openpgp.PGPDataValidationException; @@ -149,9 +148,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation 0) { - out.write(buffer, 0, length); - signatureChecker.updateSignatureData(buffer, 0, length); - } - - updateProgress(R.string.progress_verifying_signature, 95, 100); - log.add(LogType.MSG_VL_CLEAR_SIGNATURE_CHECK, indent + 1); - - o = pgpF.nextObject(); - if ( ! signatureChecker.verifySignatureOnePass(o, log, indent) ) { - return new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log); - } - - OpenPgpSignatureResult signatureResult = signatureChecker.getSignatureResult(); - - if (signatureResult.getResult() != OpenPgpSignatureResult.RESULT_VALID_CONFIRMED - && signatureResult.getResult() != OpenPgpSignatureResult.RESULT_VALID_UNCONFIRMED) { - log.add(LogType.MSG_VL_ERROR_INTEGRITY_CHECK, indent); - return new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log); - } - - updateProgress(R.string.progress_done, 100, 100); - - log.add(LogType.MSG_VL_OK, indent); - - // Return a positive result, with metadata and verification info - DecryptVerifyResult result = new DecryptVerifyResult(DecryptVerifyResult.RESULT_OK, log); - result.setSignatureResult(signatureResult); - result.setDecryptionResult( - new OpenPgpDecryptionResult(OpenPgpDecryptionResult.RESULT_NOT_ENCRYPTED)); - return result; - } - private static class EncryptStreamResult { // this is non-null iff an error occured, return directly -- cgit v1.2.3 From 3bf653775b7fa668d11ea4a2369d90ad0c7f645d Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 8 Oct 2015 19:55:28 +0200 Subject: improve tests, get rid of some redundant checks --- .../keychain/pgp/PgpDecryptVerifyOperation.java | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'OpenKeychain/src/main/java/org') 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 d3c722761..0709d4f62 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -502,11 +502,6 @@ public class PgpDecryptVerifyOperation extends BaseOperation Date: Thu, 8 Oct 2015 20:01:04 +0200 Subject: pgpdecryptverify: only use keys for verification which are allowed to sign (OKC-01-013) --- .../sufficientlysecure/keychain/pgp/PgpSignatureChecker.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'OpenKeychain/src/main/java/org') 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 a892a8a0d..4067372a1 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignatureChecker.java @@ -132,8 +132,12 @@ class PgpSignatureChecker { CanonicalizedPublicKeyRing signingRing = mProviderHelper.getCanonicalizedPublicKeyRing( KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(sigKeyId) ); + CanonicalizedPublicKey keyCandidate = signingRing.getPublicKey(sigKeyId); + if ( ! signingKey.canSign()) { + continue; + } signatureIndex = i; - signingKey = signingRing.getPublicKey(sigKeyId); + signingKey = keyCandidate; onePassSignature = sigList.get(i); return; } catch (ProviderHelper.NotFoundException e) { @@ -151,8 +155,12 @@ class PgpSignatureChecker { CanonicalizedPublicKeyRing signingRing = mProviderHelper.getCanonicalizedPublicKeyRing( KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(sigKeyId) ); + CanonicalizedPublicKey keyCandidate = signingRing.getPublicKey(sigKeyId); + if ( ! signingKey.canSign()) { + continue; + } signatureIndex = i; - signingKey = signingRing.getPublicKey(sigKeyId); + signingKey = keyCandidate; signature = sigList.get(i); return; } catch (ProviderHelper.NotFoundException e) { -- cgit v1.2.3