diff options
8 files changed, 165 insertions, 18 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 73b5c4be5..0288d2937 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 @@ -702,7 +702,7 @@ public class PgpKeyOperationTest { public void testSubkeyStrip() throws Exception { long keyId = KeyringTestingHelper.getSubkeyId(ring, 1); - parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, false)); + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, null)); applyModificationWithChecks(parcel, ring, onlyA, onlyB); Assert.assertEquals("one extra packet in original", 1, onlyA.size()); @@ -728,7 +728,7 @@ public class PgpKeyOperationTest { public void testMasterStrip() throws Exception { long keyId = ring.getMasterKeyId(); - parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, false)); + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, null)); applyModificationWithChecks(parcel, ring, onlyA, onlyB); Assert.assertEquals("one extra packet in original", 1, onlyA.size()); @@ -747,6 +747,44 @@ public class PgpKeyOperationTest { Assert.assertEquals("new packet secret key data should have length zero", 0, ((SecretKeyPacket) p).getSecretKeyData().length); Assert.assertNull("new packet should have no iv data", ((SecretKeyPacket) p).getIV()); + } + + @Test + public void testRestrictedStrip() throws Exception { + + long keyId = KeyringTestingHelper.getSubkeyId(ring, 1); + UncachedKeyRing modified; + + { // we should be able to change the stripped/divert status of subkeys without passphrase + parcel.reset(); + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, null)); + modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB, null); + Assert.assertEquals("one extra packet in modified", 1, onlyB.size()); + Packet p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket(); + Assert.assertEquals("new packet should have GNU_DUMMY S2K type", + S2K.GNU_DUMMY_S2K, ((SecretKeyPacket) p).getS2K().getType()); + Assert.assertEquals("new packet should have GNU_DUMMY protection mode stripped", + S2K.GNU_PROTECTION_MODE_NO_PRIVATE_KEY, ((SecretKeyPacket) p).getS2K().getProtectionMode()); + } + + { // and again, changing to divert-to-card + parcel.reset(); + byte[] serial = new byte[] { + 0x6a, 0x6f, 0x6c, 0x6f, 0x73, 0x77, 0x61, 0x67, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + }; + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, false, serial)); + modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB, null); + Assert.assertEquals("one extra packet in modified", 1, onlyB.size()); + Packet p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket(); + Assert.assertEquals("new packet should have GNU_DUMMY S2K type", + S2K.GNU_DUMMY_S2K, ((SecretKeyPacket) p).getS2K().getType()); + Assert.assertEquals("new packet should have GNU_DUMMY protection mode divert-to-card", + S2K.GNU_PROTECTION_MODE_DIVERT_TO_CARD, ((SecretKeyPacket) p).getS2K().getProtectionMode()); + Assert.assertArrayEquals("new packet should have correct serial number as iv", + serial, ((SecretKeyPacket) p).getIV()); + + } } @@ -1093,6 +1131,17 @@ public class PgpKeyOperationTest { } + @Test + public void testRestricted () throws Exception { + + CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing(ring.getEncoded(), false, 0); + + parcel.mAddUserIds.add("discord"); + PgpKeyOperation op = new PgpKeyOperation(null); + PgpEditKeyResult result = op.modifySecretKeyRing(secretRing, parcel, null); + Assert.assertFalse("non-restricted operations should fail without passphrase", result.success()); + } + private static UncachedKeyRing applyModificationWithChecks(SaveKeyringParcel parcel, UncachedKeyRing ring, ArrayList<RawPacket> onlyA, 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 8c76ebb8a..f295d09a9 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 @@ -473,6 +473,7 @@ public abstract class OperationResult implements Parcelable { // secret key modify MSG_MF (LogLevel.START, R.string.msg_mr), + MSG_MF_ERROR_DIVERT_SERIAL (LogLevel.ERROR, R.string.msg_mf_error_divert_serial), MSG_MF_ERROR_ENCODE (LogLevel.ERROR, R.string.msg_mf_error_encode), MSG_MF_ERROR_FINGERPRINT (LogLevel.ERROR, R.string.msg_mf_error_fingerprint), MSG_MF_ERROR_KEYID (LogLevel.ERROR, R.string.msg_mf_error_keyid), @@ -485,6 +486,7 @@ public abstract class OperationResult implements Parcelable { MSG_MF_ERROR_PASSPHRASE_MASTER(LogLevel.ERROR, R.string.msg_mf_error_passphrase_master), MSG_MF_ERROR_PAST_EXPIRY(LogLevel.ERROR, R.string.msg_mf_error_past_expiry), MSG_MF_ERROR_PGP (LogLevel.ERROR, R.string.msg_mf_error_pgp), + MSG_MF_ERROR_RESTRICTED(LogLevel.ERROR, R.string.msg_mf_error_restricted), MSG_MF_ERROR_REVOKED_PRIMARY (LogLevel.ERROR, R.string.msg_mf_error_revoked_primary), MSG_MF_ERROR_SIG (LogLevel.ERROR, R.string.msg_mf_error_sig), MSG_MF_ERROR_SUBKEY_MISSING(LogLevel.ERROR, R.string.msg_mf_error_subkey_missing), 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 928a0f96b..bd7759d43 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -20,7 +20,6 @@ package org.sufficientlysecure.keychain.pgp; import org.spongycastle.bcpg.CompressionAlgorithmTags; import org.spongycastle.bcpg.HashAlgorithmTags; -import org.spongycastle.bcpg.S2K; import org.spongycastle.bcpg.SymmetricKeyAlgorithmTags; import org.spongycastle.bcpg.sig.Features; import org.spongycastle.bcpg.sig.KeyFlags; @@ -390,6 +389,9 @@ public class PgpKeyOperation { * with a passphrase fails, the operation will fail with an unlocking error. More specific * handling of errors should be done in UI code! * + * If the passphrase is null, only a restricted subset of operations will be available, + * namely stripping of subkeys and changing the protection mode of dummy keys. + * */ public PgpEditKeyResult modifySecretKeyRing(CanonicalizedSecretKeyRing wsKR, SaveKeyringParcel saveParcel, String passphrase) { @@ -430,6 +432,11 @@ public class PgpKeyOperation { return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } + // If we have no passphrase, only allow restricted operation + if (passphrase == null) { + return internalRestricted(sKR, saveParcel, log); + } + // read masterKeyFlags, and use the same as before. // since this is the master key, this contains at least CERTIFY_OTHER PGPPublicKey masterPublicKey = masterSecretKey.getPublicKey(); @@ -716,15 +723,18 @@ public class PgpKeyOperation { return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } - if (change.mDummyStrip || change.mDummyDivert) { + if (change.mDummyStrip || change.mDummyDivert != null) { // IT'S DANGEROUS~ // no really, it is. this operation irrevocably removes the private key data from the key if (change.mDummyStrip) { - sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey(), - S2K.GNU_PROTECTION_MODE_NO_PRIVATE_KEY); + sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey()); } else { - sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey(), - S2K.GNU_PROTECTION_MODE_DIVERT_TO_CARD); + // the serial number must be 16 bytes in length + if (change.mDummyDivert.length != 16) { + log.add(LogType.MSG_MF_ERROR_DIVERT_SERIAL, + indent + 1, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId)); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); + } } sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey); } @@ -932,6 +942,73 @@ public class PgpKeyOperation { } + /** This method does the actual modifications in a keyring just like internal, except it + * supports only the subset of operations which require no passphrase, and will error + * otherwise. + */ + private PgpEditKeyResult internalRestricted(PGPSecretKeyRing sKR, SaveKeyringParcel saveParcel, + OperationLog log) { + + int indent = 1; + + progress(R.string.progress_modify, 0); + + // Make sure the saveParcel includes only operations available without passphrae! + if (!saveParcel.isRestrictedOnly()) { + log.add(LogType.MSG_MF_ERROR_RESTRICTED, indent); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); + } + + // Check if we were cancelled + if (checkCancelled()) { + log.add(LogType.MSG_OPERATION_CANCELLED, indent); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_CANCELLED, log, null); + } + + // The only operation we can do here: + // 4a. Strip secret keys, or change their protection mode (stripped/divert-to-card) + subProgressPush(50, 60); + for (int i = 0; i < saveParcel.mChangeSubKeys.size(); i++) { + + progress(R.string.progress_modify_subkeychange, (i - 1) * (100 / saveParcel.mChangeSubKeys.size())); + SaveKeyringParcel.SubkeyChange change = saveParcel.mChangeSubKeys.get(i); + log.add(LogType.MSG_MF_SUBKEY_CHANGE, + indent, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId)); + + PGPSecretKey sKey = sKR.getSecretKey(change.mKeyId); + if (sKey == null) { + log.add(LogType.MSG_MF_ERROR_SUBKEY_MISSING, + indent + 1, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId)); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); + } + + if (change.mDummyStrip || change.mDummyDivert != null) { + // IT'S DANGEROUS~ + // no really, it is. this operation irrevocably removes the private key data from the key + if (change.mDummyStrip) { + sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey()); + } else { + // the serial number must be 16 bytes in length + if (change.mDummyDivert.length != 16) { + log.add(LogType.MSG_MF_ERROR_DIVERT_SERIAL, + indent + 1, KeyFormattingUtils.convertKeyIdToHex(change.mKeyId)); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); + } + sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey(), change.mDummyDivert); + } + sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey); + } + + } + + // And we're done! + progress(R.string.progress_done, 100); + log.add(LogType.MSG_MF_SUCCESS, indent); + return new PgpEditKeyResult(OperationResult.RESULT_OK, log, new UncachedKeyRing(sKR)); + + } + + private static PGPSecretKeyRing applyNewUnlock( PGPSecretKeyRing sKR, PGPPublicKey masterPublicKey, diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index df333553b..04fb955fa 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -20,7 +20,6 @@ package org.sufficientlysecure.keychain.pgp; import org.spongycastle.bcpg.ArmoredOutputStream; import org.spongycastle.bcpg.PublicKeyAlgorithmTags; -import org.spongycastle.bcpg.S2K; import org.spongycastle.bcpg.SignatureSubpacketTags; import org.spongycastle.bcpg.UserAttributeSubpacketTags; import org.spongycastle.bcpg.sig.KeyFlags; @@ -1222,8 +1221,7 @@ public class UncachedKeyRing { // if this is a secret key which does not yet occur in the secret ring if (sKey == null) { // generate a stripped secret (sub)key - sKey = PGPSecretKey.constructGnuDummyKey(key, - S2K.GNU_PROTECTION_MODE_NO_PRIVATE_KEY); + sKey = PGPSecretKey.constructGnuDummyKey(key); } sKey = PGPSecretKey.replacePublicKey(sKey, key); return PGPSecretKeyRing.insertSecretKey(secRing, sKey); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java index a8823cd5c..3c78f2c40 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java @@ -80,6 +80,23 @@ public class SaveKeyringParcel implements Parcelable { mRevokeSubKeys = new ArrayList<Long>(); } + /** Returns true iff this parcel does not contain any operations which require a passphrase. */ + public boolean isRestrictedOnly() { + if (mNewUnlock != null || !mAddUserIds.isEmpty() || !mAddUserAttribute.isEmpty() + || !mAddSubKeys.isEmpty() || mChangePrimaryUserId != null || !mRevokeSubKeys .isEmpty() + || !mRevokeSubKeys.isEmpty()) { + return false; + } + + for (SubkeyChange change : mChangeSubKeys) { + if (change.mRecertify || change.mFlags != null || change.mExpiry != null) { + return false; + } + } + + return true; + } + // performance gain for using Parcelable here would probably be negligible, // use Serializable instead. public static class SubkeyAdd implements Serializable { @@ -114,12 +131,14 @@ public class SaveKeyringParcel implements Parcelable { public Integer mFlags; // this is a long unix timestamp, in seconds (NOT MILLISECONDS!) public Long mExpiry; + // if this flag is true, the key will be recertified even if all above + // values are no-ops + public boolean mRecertify; // if this flag is true, the subkey should be changed to a stripped key public boolean mDummyStrip; - // if this flag is true, the subkey should be changed to a divert-to-card key - public boolean mDummyDivert; - // if this flag is true, the key will be recertified even if the above values are no-ops - public boolean mRecertify; + // if this is non-null, the subkey will be changed to a divert-to-card + // key for the given serial number + public byte[] mDummyDivert; public SubkeyChange(long keyId) { mKeyId = keyId; @@ -136,11 +155,11 @@ public class SaveKeyringParcel implements Parcelable { mExpiry = expiry; } - public SubkeyChange(long keyId, boolean dummyStrip, boolean dummyDivert) { + public SubkeyChange(long keyId, boolean dummyStrip, byte[] dummyDivert) { this(keyId, null, null); // these flags are mutually exclusive! - if (dummyStrip && dummyDivert) { + if (dummyStrip && dummyDivert != null) { throw new AssertionError( "cannot set strip and divert flags at the same time - this is a bug!"); } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java index 330589c7c..febda16c3 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java @@ -481,7 +481,7 @@ public class EditKeyFragment extends LoaderFragment implements case EditSubkeyDialogFragment.MESSAGE_STRIP: SubkeyChange change = mSaveKeyringParcel.getSubkeyChange(keyId); if (change == null) { - mSaveKeyringParcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, false)); + mSaveKeyringParcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, null)); break; } // toggle diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index 5bc3c0601..522013de3 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -826,6 +826,7 @@ <!-- modifySecretKeyRing --> <string name="msg_mr">"Modifying keyring %s"</string> + <string name="msg_mf_error_divert_serial">"The serial number of a divert-to-card key must be 16 bytes! This is a programming error, please file a bug report!"</string> <string name="msg_mf_error_encode">"Encoding exception!"</string> <string name="msg_mf_error_fingerprint">"Actual key fingerprint does not match the expected one!"</string> <string name="msg_mf_error_keyid">"No key ID. This is an internal error, please file a bug report!"</string> @@ -833,6 +834,7 @@ <string name="msg_mf_error_master_none">"No master certificate found to operate on! (All revoked?)"</string> <string name="msg_mf_error_noexist_primary">"Bad primary user ID specified!"</string> <string name="msg_mf_error_noexist_revoke">"Bad user ID for revocation specified!"</string> + <string name="msg_mf_error_restricted">"Tried to execute restricted operation without passphrase! This is a programming error, please file a bug report!"</string> <string name="msg_mf_error_revoked_primary">"Revoked user IDs cannot be primary!"</string> <string name="msg_mf_error_null_expiry">"Expiry time cannot be "same as before" on subkey creation. This is a programming error, please file a bug report!"</string> <string name="msg_mf_error_passphrase_master">"Fatal error decrypting master key! This is likely a programming error, please file a bug report!"</string> diff --git a/extern/spongycastle b/extern/spongycastle -Subproject c2a91166cef1a08c97ad2e988e368fe36babfc0 +Subproject 26c232f31b62404ecb5d3ae3d2c1730fd0a0a0e |