diff options
9 files changed, 148 insertions, 61 deletions
diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java index 0288d2937..1da69308c 100644 --- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java +++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java @@ -231,7 +231,7 @@ public class PgpKeyOperationTest { ring.getPublicKey().getCreationTime().after(new Date(new Date().getTime()-1000*120))); Assert.assertNull("key ring should not expire", - ring.getPublicKey().getExpiryTime()); + ring.getPublicKey().getUnsafeExpiryTimeForTesting()); Assert.assertEquals("first (master) key can certify", KeyFlags.CERTIFY_OTHER, (long) subkeys.get(0).getKeyUsage()); @@ -342,9 +342,9 @@ public class PgpKeyOperationTest { Assert.assertNotNull("new key is not null", newKey); Assert.assertNotNull("added key must have an expiry date", - newKey.getExpiryTime()); + newKey.getUnsafeExpiryTimeForTesting()); Assert.assertEquals("added key must have expected expiry date", - expiry, newKey.getExpiryTime().getTime()/1000); + expiry, newKey.getUnsafeExpiryTimeForTesting().getTime()/1000); Assert.assertEquals("added key must have expected flags", flags, (long) newKey.getKeyUsage()); Assert.assertEquals("added key must have expected bitsize", @@ -403,9 +403,9 @@ public class PgpKeyOperationTest { ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID()); Assert.assertNotNull("modified key must have an expiry date", - modified.getPublicKey(keyId).getExpiryTime()); + modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting()); Assert.assertEquals("modified key must have expected expiry date", - expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000); + expiry, modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting().getTime()/1000); Assert.assertEquals("modified key must have same flags as before", ring.getPublicKey(keyId).getKeyUsage(), modified.getPublicKey(keyId).getKeyUsage()); } @@ -417,9 +417,9 @@ public class PgpKeyOperationTest { modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB); Assert.assertNotNull("modified key must have an expiry date", - modified.getPublicKey(keyId).getExpiryTime()); + modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting()); Assert.assertEquals("modified key must have expected expiry date", - expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000); + expiry, modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting().getTime()/1000); Assert.assertEquals("modified key must have same flags as before", ring.getPublicKey(keyId).getKeyUsage(), modified.getPublicKey(keyId).getKeyUsage()); } @@ -443,9 +443,9 @@ public class PgpKeyOperationTest { Assert.assertEquals("modified key must have expected flags", flags, (long) modified.getPublicKey(keyId).getKeyUsage()); Assert.assertNotNull("key must retain its expiry", - modified.getPublicKey(keyId).getExpiryTime()); + modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting()); Assert.assertEquals("key expiry must be unchanged", - expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000); + expiry, modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting().getTime()/1000); } { // expiry of 0 should be "no expiry" @@ -463,7 +463,7 @@ public class PgpKeyOperationTest { Assert.assertEquals("signature must have been created by master key", ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID()); - Assert.assertNull("key must not expire anymore", modified.getPublicKey(keyId).getExpiryTime()); + Assert.assertNull("key must not expire anymore", modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting()); } { // a past expiry should fail @@ -517,9 +517,9 @@ public class PgpKeyOperationTest { PacketTags.SIGNATURE, onlyB.get(1).tag); Assert.assertNotNull("modified key must have an expiry date", - modified.getPublicKey().getExpiryTime()); + modified.getPublicKey().getUnsafeExpiryTimeForTesting()); Assert.assertEquals("modified key must have expected expiry date", - expiry, modified.getPublicKey().getExpiryTime().getTime() / 1000); + expiry, modified.getPublicKey().getUnsafeExpiryTimeForTesting().getTime() / 1000); Assert.assertEquals("modified key must have same flags as before", ring.getPublicKey().getKeyUsage(), modified.getPublicKey().getKeyUsage()); } @@ -531,9 +531,9 @@ public class PgpKeyOperationTest { modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB); Assert.assertNotNull("modified key must have an expiry date", - modified.getPublicKey(keyId).getExpiryTime()); + modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting()); Assert.assertEquals("modified key must have expected expiry date", - expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000); + expiry, modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting().getTime()/1000); Assert.assertEquals("modified key must have same flags as before", ring.getPublicKey(keyId).getKeyUsage(), modified.getPublicKey(keyId).getKeyUsage()); } @@ -547,9 +547,9 @@ public class PgpKeyOperationTest { Assert.assertEquals("modified key must have expected flags", flags, (long) modified.getPublicKey(keyId).getKeyUsage()); Assert.assertNotNull("key must retain its expiry", - modified.getPublicKey(keyId).getExpiryTime()); + modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting()); Assert.assertEquals("key expiry must be unchanged", - expiry, modified.getPublicKey(keyId).getExpiryTime().getTime()/1000); + expiry, modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting().getTime()/1000); } { // expiry of 0 should be "no expiry" @@ -557,7 +557,7 @@ public class PgpKeyOperationTest { parcel.mChangeSubKeys.add(new SubkeyChange(keyId, null, 0L)); modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB); - Assert.assertNull("key must not expire anymore", modified.getPublicKey(keyId).getExpiryTime()); + Assert.assertNull("key must not expire anymore", modified.getPublicKey(keyId).getUnsafeExpiryTimeForTesting()); } { // if we revoke everything, nothing is left to properly sign... @@ -609,7 +609,7 @@ public class PgpKeyOperationTest { ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID()); Assert.assertTrue("subkey must actually be revoked", - modified.getPublicKey().isRevoked()); + modified.getPublicKey().isMaybeRevoked()); } @@ -653,7 +653,7 @@ public class PgpKeyOperationTest { ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID()); Assert.assertTrue("subkey must actually be revoked", - modified.getPublicKey(keyId).isRevoked()); + modified.getPublicKey(keyId).isMaybeRevoked()); } { // re-add second subkey @@ -691,7 +691,7 @@ public class PgpKeyOperationTest { ring.getMasterKeyId(), ((SignaturePacket) p).getKeyID()); Assert.assertFalse("subkey must no longer be revoked", - modified.getPublicKey(keyId).isRevoked()); + modified.getPublicKey(keyId).isMaybeRevoked()); Assert.assertEquals("subkey must have the same usage flags as before", flags, (long) modified.getPublicKey(keyId).getKeyUsage()); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ImportKeysListEntry.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ImportKeysListEntry.java index 591408c8b..79065604a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ImportKeysListEntry.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ImportKeysListEntry.java @@ -294,8 +294,8 @@ public class ImportKeysListEntry implements Serializable, Parcelable { mKeyId = key.getKeyId(); mKeyIdHex = KeyFormattingUtils.convertKeyIdToHex(mKeyId); - mRevoked = key.isRevoked(); - mExpired = key.isExpired(); + mRevoked = key.isMaybeRevoked(); + mExpired = key.isMaybeExpired(); mFingerprintHex = KeyFormattingUtils.convertFingerprintToHex(key.getFingerprint()); mBitStrength = key.getBitStrength(); mCurveOid = key.getCurveOid(); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java index bbf136dac..4adacaf23 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java @@ -79,9 +79,8 @@ public abstract class CanonicalizedKeyRing extends KeyRing { public boolean isExpired() { // Is the master key expired? - Date creationDate = getRing().getPublicKey().getCreationTime(); - Date expiryDate = getRing().getPublicKey().getValidSeconds() > 0 - ? new Date(creationDate.getTime() + getRing().getPublicKey().getValidSeconds() * 1000) : null; + Date creationDate = getPublicKey().getCreationTime(); + Date expiryDate = getPublicKey().getExpiryTime(); Date now = new Date(); return creationDate.after(now) || (expiryDate != null && expiryDate.before(now)); 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 b026d9257..303070333 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java @@ -20,8 +20,16 @@ package org.sufficientlysecure.keychain.pgp; import org.spongycastle.bcpg.sig.KeyFlags; import org.spongycastle.openpgp.PGPPublicKey; +import org.spongycastle.openpgp.PGPSignature; import org.spongycastle.openpgp.operator.jcajce.JcePublicKeyKeyEncryptionMethodGenerator; +import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.util.IterableIterator; +import org.sufficientlysecure.keychain.util.Log; + +import java.util.Calendar; +import java.util.Date; +import java.util.GregorianCalendar; +import java.util.Iterator; /** Wrapper for a PGPPublicKey. * @@ -100,6 +108,72 @@ public class CanonicalizedPublicKey extends UncachedPublicKey { return false; } + public boolean isRevoked() { + return mPublicKey.getSignaturesOfType(isMasterKey() + ? PGPSignature.KEY_REVOCATION + : PGPSignature.SUBKEY_REVOCATION).hasNext(); + } + + public boolean isExpired () { + Date expiry = getExpiryTime(); + return expiry != null && expiry.before(new Date()); + } + + public long getValidSeconds() { + + long seconds; + + // the getValidSeconds method is unreliable for master keys. we need to iterate all + // user ids, then use the most recent certification from a non-revoked user id + if (isMasterKey()) { + Date latestCreation = null; + seconds = 0; + + for (byte[] rawUserId : getUnorderedRawUserIds()) { + Iterator<WrappedSignature> sigs = getSignaturesForRawId(rawUserId); + + // there is always a certification, so this call is safe + WrappedSignature sig = sigs.next(); + + // we know a user id has at most two sigs: one certification, one revocation. + // if the sig is a revocation, or there is another sig (which is a revocation), + // the data in this uid is not relevant + if (sig.isRevocation() || sigs.hasNext()) { + continue; + } + + // this is our revocation, UNLESS there is a newer certificate! + if (latestCreation == null || latestCreation.before(sig.getCreationTime())) { + latestCreation = sig.getCreationTime(); + seconds = sig.getKeyExpirySeconds(); + } + } + } else { + seconds = mPublicKey.getValidSeconds(); + } + + return seconds; + } + + public Date getExpiryTime() { + long seconds = getValidSeconds(); + + if (seconds > Integer.MAX_VALUE) { + Log.e(Constants.TAG, "error, expiry time too large"); + return null; + } + if (seconds == 0) { + // no expiry + return null; + } + Date creationDate = getCreationTime(); + Calendar calendar = GregorianCalendar.getInstance(); + calendar.setTime(creationDate); + calendar.add(Calendar.SECOND, (int) seconds); + + return calendar.getTime(); + } + /** Same method as superclass, but we make it public. */ public Integer getKeyUsage() { return super.getKeyUsage(); 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 ed4715681..46defebf7 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/OpenPgpSignatureResultBuilder.java @@ -104,8 +104,8 @@ public class OpenPgpSignatureResultBuilder { setUserIds(signingRing.getUnorderedUserIds()); // either master key is expired/revoked or this specific subkey is expired/revoked - setKeyExpired(signingRing.isExpired() || signingKey.isExpired()); - setKeyRevoked(signingRing.isRevoked() || signingKey.isRevoked()); + setKeyExpired(signingRing.isExpired() || signingKey.isMaybeExpired()); + setKeyRevoked(signingRing.isRevoked() || signingKey.isMaybeRevoked()); } public OpenPgpSignatureResult build() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java index aebb52a03..1a251eb79 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -439,8 +439,8 @@ public class PgpKeyOperation { // since this is the master key, this contains at least CERTIFY_OTHER PGPPublicKey masterPublicKey = masterSecretKey.getPublicKey(); int masterKeyFlags = readKeyFlags(masterPublicKey) | KeyFlags.CERTIFY_OTHER; - long masterKeyExpiry = masterPublicKey.getValidSeconds() == 0L ? 0L : - masterPublicKey.getCreationTime().getTime() / 1000 + masterPublicKey.getValidSeconds(); + Date expiryTime = wsKR.getPublicKey().getExpiryTime(); + long masterKeyExpiry = expiryTime != null ? expiryTime.getTime() / 1000 : 0L; return internal(sKR, masterSecretKey, masterKeyFlags, masterKeyExpiry, saveParcel, passphrase, log); 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 0fe1ccdb6..9276cba10 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java @@ -50,7 +50,7 @@ public class UncachedPublicKey { } /** The revocation signature is NOT checked here, so this may be false! */ - public boolean isRevoked() { + public boolean isMaybeRevoked() { return mPublicKey.getSignaturesOfType(isMasterKey() ? PGPSignature.KEY_REVOCATION : PGPSignature.SUBKEY_REVOCATION).hasNext(); @@ -60,25 +60,8 @@ public class UncachedPublicKey { return mPublicKey.getCreationTime(); } - public Date getExpiryTime() { - long seconds = mPublicKey.getValidSeconds(); - if (seconds > Integer.MAX_VALUE) { - Log.e(Constants.TAG, "error, expiry time too large"); - return null; - } - if (seconds == 0) { - // no expiry - return null; - } - Date creationDate = getCreationTime(); - Calendar calendar = GregorianCalendar.getInstance(); - calendar.setTime(creationDate); - calendar.add(Calendar.SECOND, (int) seconds); - - return calendar.getTime(); - } - - public boolean isExpired() { + /** The revocation signature is NOT checked here, so this may be false! */ + public boolean isMaybeExpired() { Date creationDate = mPublicKey.getCreationTime(); Date expiryDate = mPublicKey.getValidSeconds() > 0 ? new Date(creationDate.getTime() + mPublicKey.getValidSeconds() * 1000) : null; @@ -358,4 +341,24 @@ public class UncachedPublicKey { return mCacheUsage; } + // this method relies on UNSAFE assumptions about the keyring, and should ONLY be used for + // TEST CASES!! + Date getUnsafeExpiryTimeForTesting () { + long valid = mPublicKey.getValidSeconds(); + + if (valid > Integer.MAX_VALUE) { + Log.e(Constants.TAG, "error, expiry time too large"); + return null; + } + if (valid == 0) { + // no expiry + return null; + } + Date creationDate = getCreationTime(); + Calendar calendar = GregorianCalendar.getInstance(); + calendar.setTime(creationDate); + calendar.add(Calendar.SECOND, (int) valid); + + return calendar.getTime(); + } } 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 ade075d55..c6fad1a73 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java @@ -78,6 +78,10 @@ public class WrappedSignature { return mSig.getCreationTime(); } + public long getKeyExpirySeconds() { + return mSig.getHashedSubPackets().getKeyExpirationTime(); + } + public ArrayList<WrappedSignature> getEmbeddedSignatures() { ArrayList<WrappedSignature> sigs = new ArrayList<>(); if (!mSig.hasSubpackets()) { 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 18efa2b80..d947ae053 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java @@ -473,7 +473,7 @@ public class ProviderHelper { item.selfCert = cert; item.isPrimary = cert.isPrimaryUserId(); } else { - item.isRevoked = true; + item.selfRevocation = cert; log(LogType.MSG_IP_UID_REVOKED); } continue; @@ -569,10 +569,11 @@ public class ProviderHelper { // NOTE self-certificates are already verified during canonicalization, // AND we know there is at most one cert plus at most one revocation + // AND the revocation only exists if there is no newer certification if (!cert.isRevocation()) { item.selfCert = cert; } else { - item.isRevoked = true; + item.selfRevocation = cert; log(LogType.MSG_IP_UAT_REVOKED); } continue; @@ -643,16 +644,21 @@ public class ProviderHelper { for (int userIdRank = 0; userIdRank < uids.size(); userIdRank++) { UserPacketItem item = uids.get(userIdRank); operations.add(buildUserIdOperations(masterKeyId, item, userIdRank)); - if (item.selfCert != null) { - // TODO get rid of "self verified" status? this cannot even happen anymore! - operations.add(buildCertOperations(masterKeyId, userIdRank, item.selfCert, - selfCertsAreTrusted ? Certs.VERIFIED_SECRET : Certs.VERIFIED_SELF)); + + if (item.selfCert == null) { + throw new AssertionError("User ids MUST be self-certified at this point!!"); } - // don't bother with trusted certs if the uid is revoked, anyways - if (item.isRevoked) { + + if (item.selfRevocation != null) { + operations.add(buildCertOperations(masterKeyId, userIdRank, item.selfRevocation, + Certs.VERIFIED_SELF)); + // don't bother with trusted certs if the uid is revoked, anyways continue; } + operations.add(buildCertOperations(masterKeyId, userIdRank, item.selfCert, + selfCertsAreTrusted ? Certs.VERIFIED_SECRET : Certs.VERIFIED_SELF)); + // iterate over signatures for (int i = 0; i < item.trustedCerts.size() ; i++) { WrappedSignature sig = item.trustedCerts.valueAt(i); @@ -711,15 +717,16 @@ public class ProviderHelper { String userId; byte[] attributeData; boolean isPrimary = false; - boolean isRevoked = false; WrappedSignature selfCert; + WrappedSignature selfRevocation; LongSparseArray<WrappedSignature> trustedCerts = new LongSparseArray<>(); @Override public int compareTo(UserPacketItem o) { // revoked keys always come last! - if (isRevoked != o.isRevoked) { - return isRevoked ? 1 : -1; + //noinspection DoubleNegation + if ( (selfRevocation != null) != (o.selfRevocation != null)) { + return selfRevocation != null ? 1 : -1; } // if one is a user id, but the other isn't, the user id always comes first. // we compare for null values here, so != is the correct operator! @@ -1353,7 +1360,7 @@ public class ProviderHelper { values.put(UserPackets.USER_ID, item.userId); values.put(UserPackets.ATTRIBUTE_DATA, item.attributeData); values.put(UserPackets.IS_PRIMARY, item.isPrimary); - values.put(UserPackets.IS_REVOKED, item.isRevoked); + values.put(UserPackets.IS_REVOKED, item.selfRevocation != null); values.put(UserPackets.RANK, rank); Uri uri = UserPackets.buildUserIdsUri(masterKeyId); |