From 344bc1736deabd343191080f78a9865283ea4703 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sat, 20 Sep 2014 00:12:50 +0200 Subject: respect user id revocation signatures Closes #836 --- .../keychain/pgp/WrappedSignature.java | 15 +++- .../keychain/provider/ProviderHelper.java | 91 ++++++++++++++-------- .../keychain/service/results/OperationResult.java | 6 +- OpenKeychain/src/main/res/values/strings.xml | 6 +- 4 files changed, 84 insertions(+), 34 deletions(-) (limited to 'OpenKeychain/src/main') diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java index 4d4c0e5d1..c0bc7769b 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java @@ -20,6 +20,8 @@ package org.sufficientlysecure.keychain.pgp; import org.spongycastle.bcpg.SignatureSubpacket; import org.spongycastle.bcpg.SignatureSubpacketTags; +import org.spongycastle.bcpg.sig.Exportable; +import org.spongycastle.bcpg.sig.Revocable; import org.spongycastle.bcpg.sig.RevocationReason; import org.spongycastle.openpgp.PGPException; import org.spongycastle.openpgp.PGPObjectFactory; @@ -218,12 +220,23 @@ public class WrappedSignature { return new WrappedSignature(signatures.get(0)); } + /** Returns true if this certificate is revocable in general. */ + public boolean isRevokable () { + // If nothing is specified, the packet is considered revocable + if (mSig.getHashedSubPackets() == null + || !mSig.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.REVOCABLE)) { + return true; + } + SignatureSubpacket p = mSig.getHashedSubPackets().getSubpacket(SignatureSubpacketTags.REVOCABLE); + return ((Revocable) p).isRevocable(); + } + public boolean isLocal() { if (mSig.getHashedSubPackets() == null || !mSig.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.EXPORTABLE)) { return false; } SignatureSubpacket p = mSig.getHashedSubPackets().getSubpacket(SignatureSubpacketTags.EXPORTABLE); - return p.getData()[0] == 0; + return ((Exportable) p).isExportable(); } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java index c29a67215..8d790110d 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java @@ -450,47 +450,67 @@ public class ProviderHelper { for (WrappedSignature cert : new IterableIterator( masterKey.getSignaturesForRawId(rawUserId))) { long certId = cert.getKeyId(); + // self signature + if (certId == masterKeyId) { + + // NOTE self-certificates are already verified during canonicalization, + // AND we know there is at most one cert plus at most one revocation + if (!cert.isRevocation()) { + item.selfCert = cert; + item.isPrimary = cert.isPrimaryUserId(); + } else { + item.isRevoked = true; + log(LogType.MSG_IP_UID_REVOKED); + } + continue; + + } + + // do we have a trusted key for this? + if (trustedKeys.indexOfKey(certId) < 0) { + unknownCerts += 1; + continue; + } + + // verify signatures from known private keys + CanonicalizedPublicKey trustedKey = trustedKeys.get(certId); + try { - // self signature - if (certId == masterKeyId) { - - // NOTE self-certificates are already verified during canonicalization, - // AND we know there is at most one cert plus at most one revocation - if (!cert.isRevocation()) { - item.selfCert = cert; - item.isPrimary = cert.isPrimaryUserId(); - } else { - item.isRevoked = true; - log(LogType.MSG_IP_UID_REVOKED); - } + cert.init(trustedKey); + // if it doesn't certify, leave a note and skip + if ( ! cert.verifySignature(masterKey, rawUserId)) { + log(LogType.MSG_IP_UID_CERT_BAD); continue; - } - // verify signatures from known private keys - if (trustedKeys.indexOfKey(certId) >= 0) { - CanonicalizedPublicKey trustedKey = trustedKeys.get(certId); - if (cert.isRevocation()) { - // skip for now + log(cert.isRevocation() + ? LogType.MSG_IP_UID_CERT_GOOD_REVOKE + : LogType.MSG_IP_UID_CERT_GOOD, + KeyFormattingUtils.convertKeyIdToHexShort(trustedKey.getKeyId()) + ); + + // check if there is a previous certificate + WrappedSignature prev = item.trustedCerts.get(cert.getKeyId()); + if (prev != null) { + // if it's newer, skip this one + if (prev.getCreationTime().after(cert.getCreationTime())) { + log(LogType.MSG_IP_UID_CERT_OLD); continue; } - cert.init(trustedKey); - if (cert.verifySignature(masterKey, rawUserId)) { - item.trustedCerts.add(cert); - log(LogType.MSG_IP_UID_CERT_GOOD, - KeyFormattingUtils.convertKeyIdToHexShort(trustedKey.getKeyId()) - ); - } else { - log(LogType.MSG_IP_UID_CERT_BAD); + // if the previous one was a non-revokable certification, no need to look further + if (!prev.isRevocation() && !prev.isRevokable()) { + log(LogType.MSG_IP_UID_CERT_NONREVOKE); + continue; } + log(LogType.MSG_IP_UID_CERT_NEW); } - - unknownCerts += 1; + item.trustedCerts.put(cert.getKeyId(), cert); } catch (PgpGeneralException e) { log(LogType.MSG_IP_UID_CERT_ERROR, KeyFormattingUtils.convertKeyIdToHex(cert.getKeyId())); } + } if (unknownCerts > 0) { @@ -518,9 +538,18 @@ public class ProviderHelper { if (item.isRevoked) { continue; } - for (int i = 0; i < item.trustedCerts.size(); i++) { + + // iterate over signatures + for (int i = 0; i < item.trustedCerts.size() ; i++) { + WrappedSignature sig = item.trustedCerts.valueAt(i); + // if it's a revocation + if (sig.isRevocation()) { + // don't further process it + continue; + } + // otherwise, build database operation operations.add(buildCertOperations( - masterKeyId, userIdRank, item.trustedCerts.get(i), Certs.VERIFIED_SECRET)); + masterKeyId, userIdRank, sig, Certs.VERIFIED_SECRET)); } } @@ -568,7 +597,7 @@ public class ProviderHelper { boolean isPrimary = false; boolean isRevoked = false; WrappedSignature selfCert; - List trustedCerts = new ArrayList(); + LongSparseArray trustedCerts = new LongSparseArray(); @Override public int compareTo(UserIdItem o) { 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 0e883c79b..cd9c29996 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 @@ -264,8 +264,12 @@ public abstract class OperationResult implements Parcelable { MSG_IP_SUCCESS (LogLevel.OK, R.string.msg_ip_success), MSG_IP_SUCCESS_IDENTICAL (LogLevel.OK, R.string.msg_ip_success_identical), MSG_IP_UID_CERT_BAD (LogLevel.WARN, R.string.msg_ip_uid_cert_bad), - MSG_IP_UID_CERT_ERROR (LogLevel.ERROR, R.string.msg_ip_uid_cert_error), + MSG_IP_UID_CERT_ERROR (LogLevel.WARN, R.string.msg_ip_uid_cert_error), + MSG_IP_UID_CERT_OLD (LogLevel.DEBUG, R.string.msg_ip_uid_cert_old), + MSG_IP_UID_CERT_NONREVOKE (LogLevel.DEBUG, R.string.msg_ip_uid_cert_nonrevoke), + MSG_IP_UID_CERT_NEW (LogLevel.DEBUG, R.string.msg_ip_uid_cert_new), MSG_IP_UID_CERT_GOOD (LogLevel.DEBUG, R.string.msg_ip_uid_cert_good), + MSG_IP_UID_CERT_GOOD_REVOKE (LogLevel.DEBUG, R.string.msg_ip_uid_cert_good_revoke), MSG_IP_UID_CERTS_UNKNOWN (LogLevel.DEBUG, R.plurals.msg_ip_uid_certs_unknown), MSG_IP_UID_CLASSIFYING_ZERO (LogLevel.DEBUG, R.string.msg_ip_uid_classifying_zero), MSG_IP_UID_CLASSIFYING (LogLevel.DEBUG, R.plurals.msg_ip_uid_classifying), diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index 63292ce71..5d79441cf 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -580,7 +580,11 @@ "Re-inserting secret key" "Encountered bad certificate!" "Error processing certificate!" - "User id is certified by %1$s" + "Already have a non-revokable certificate, skipping." + "Certificate is older than previous, skipping." + "Certificate is more recent, replacing previous." + "Found good certificate by %1$s" + "Found good certificate revocation by %1$s" "Ignoring one certificate issued by an unknown public key" "Ignoring %s certificates issued by unknown public keys" -- cgit v1.2.3