From 38da2af0e89ca05f2a01ed08801dee635784168e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Wed, 13 Aug 2014 16:37:28 +0200 Subject: [PATCH] Better error handling for passphrase cache if key is missing --- .../keychain/pgp/PgpDecryptVerify.java | 4 +- .../keychain/remote/OpenPgpService.java | 16 +-- .../service/KeychainIntentService.java | 20 ++-- .../service/PassphraseCacheService.java | 101 ++++++++++-------- .../keychain/ui/CertifyKeyActivity.java | 9 +- .../keychain/ui/EditKeyFragment.java | 10 +- .../keychain/ui/EncryptActivity.java | 26 +++-- 7 files changed, 115 insertions(+), 71 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java index 6fa380357..7d113241b 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerify.java @@ -46,6 +46,7 @@ import org.sufficientlysecure.keychain.R; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; import org.sufficientlysecure.keychain.provider.KeychainContract.KeyRings; import org.sufficientlysecure.keychain.provider.ProviderHelper; +import org.sufficientlysecure.keychain.service.PassphraseCacheService; import org.sufficientlysecure.keychain.util.InputData; import org.sufficientlysecure.keychain.util.Log; import org.sufficientlysecure.keychain.util.ProgressScaler; @@ -163,7 +164,8 @@ public class PgpDecryptVerify { } public interface PassphraseCache { - public String getCachedPassphrase(long masterKeyId); + public String getCachedPassphrase(long masterKeyId) + throws NoSecretKeyException; } public static class InvalidDataException extends Exception { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java index 50c8fcf3b..1d3d7d02f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java @@ -168,8 +168,7 @@ public class OpenPgpService extends RemoteService { } if (passphrase == null) { // get PendingIntent for passphrase input, add it to given params and return to client - Intent passphraseBundle = getPassphraseBundleIntent(data, accSettings.getKeyId()); - return passphraseBundle; + return getPassphraseBundleIntent(data, accSettings.getKeyId()); } // Get Input- and OutputStream from ParcelFileDescriptor @@ -289,8 +288,7 @@ public class OpenPgpService extends RemoteService { } if (passphrase == null) { // get PendingIntent for passphrase input, add it to given params and return to client - Intent passphraseBundle = getPassphraseBundleIntent(data, accSettings.getKeyId()); - return passphraseBundle; + return getPassphraseBundleIntent(data, accSettings.getKeyId()); } // sign and encrypt @@ -358,9 +356,13 @@ public class OpenPgpService extends RemoteService { new ProviderHelper(this), new PgpDecryptVerify.PassphraseCache() { @Override - public String getCachedPassphrase(long masterKeyId) { - return PassphraseCacheService.getCachedPassphrase( - OpenPgpService.this, masterKeyId); + public String getCachedPassphrase(long masterKeyId) throws PgpDecryptVerify.NoSecretKeyException { + try { + return PassphraseCacheService.getCachedPassphrase( + OpenPgpService.this, masterKeyId); + } catch (PassphraseCacheService.KeyNotFoundException e) { + throw new PgpDecryptVerify.NoSecretKeyException(); + } } }, inputData, os diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java index 54895973b..443f66d58 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java @@ -307,9 +307,13 @@ public class KeychainIntentService extends IntentService new ProviderHelper(this), new PgpDecryptVerify.PassphraseCache() { @Override - public String getCachedPassphrase(long masterKeyId) { - return PassphraseCacheService.getCachedPassphrase( - KeychainIntentService.this, masterKeyId); + public String getCachedPassphrase(long masterKeyId) throws PgpDecryptVerify.NoSecretKeyException { + try { + return PassphraseCacheService.getCachedPassphrase( + KeychainIntentService.this, masterKeyId); + } catch (PassphraseCacheService.KeyNotFoundException e) { + throw new PgpDecryptVerify.NoSecretKeyException(); + } } }, inputData, outStream @@ -351,9 +355,13 @@ public class KeychainIntentService extends IntentService new ProviderHelper(this), new PgpDecryptVerify.PassphraseCache() { @Override - public String getCachedPassphrase(long masterKeyId) { - return PassphraseCacheService.getCachedPassphrase( - KeychainIntentService.this, masterKeyId); + public String getCachedPassphrase(long masterKeyId) throws PgpDecryptVerify.NoSecretKeyException { + try { + return PassphraseCacheService.getCachedPassphrase( + KeychainIntentService.this, masterKeyId); + } catch (PassphraseCacheService.KeyNotFoundException e) { + throw new PgpDecryptVerify.NoSecretKeyException(); + } } }, inputData, null diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/PassphraseCacheService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/PassphraseCacheService.java index e95e737d5..ae1b026a5 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/PassphraseCacheService.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/PassphraseCacheService.java @@ -76,12 +76,24 @@ public class PassphraseCacheService extends Service { private static final int NOTIFICATION_ID = 1; + private static final int MSG_PASSPHRASE_CACHE_GET_OKAY = 1; + private static final int MSG_PASSPHRASE_CACHE_GET_KEY_NO_FOUND = 2; + private BroadcastReceiver mIntentReceiver; private LongSparseArray mPassphraseCache = new LongSparseArray(); Context mContext; + public static class KeyNotFoundException extends Exception { + public KeyNotFoundException() { + } + + public KeyNotFoundException(String name) { + super(name); + } + } + /** * This caches a new passphrase in memory by sending a new command to the service. An android * service is only run once. Thus, when the service is already started, new commands just add @@ -114,24 +126,23 @@ public class PassphraseCacheService extends Service { * @param keyId * @return passphrase or null (if no passphrase is cached for this keyId) */ - public static String getCachedPassphrase(Context context, long keyId) { + public static String getCachedPassphrase(Context context, long keyId) throws KeyNotFoundException { Log.d(Constants.TAG, "PassphraseCacheService.getCachedPassphrase() get masterKeyId for " + keyId); Intent intent = new Intent(context, PassphraseCacheService.class); intent.setAction(ACTION_PASSPHRASE_CACHE_GET); final Object mutex = new Object(); - final Bundle returnBundle = new Bundle(); + final Message returnMessage = Message.obtain(); HandlerThread handlerThread = new HandlerThread("getPassphraseThread"); handlerThread.start(); Handler returnHandler = new Handler(handlerThread.getLooper()) { @Override public void handleMessage(Message message) { - if (message.obj != null) { - String passphrase = ((Bundle) message.obj).getString(EXTRA_PASSPHRASE); - returnBundle.putString(EXTRA_PASSPHRASE, passphrase); - } + // copy over result to handle after mutex.wait + returnMessage.what = message.what; + returnMessage.copyFrom(message); synchronized (mutex) { mutex.notify(); } @@ -155,10 +166,13 @@ public class PassphraseCacheService extends Service { } } - if (returnBundle.containsKey(EXTRA_PASSPHRASE)) { - return returnBundle.getString(EXTRA_PASSPHRASE); - } else { - return null; + switch (returnMessage.what) { + case MSG_PASSPHRASE_CACHE_GET_OKAY: + return returnMessage.getData().getString(EXTRA_PASSPHRASE); + case MSG_PASSPHRASE_CACHE_GET_KEY_NO_FOUND: + throw new KeyNotFoundException(); + default: + throw new KeyNotFoundException("should not happen!"); } } @@ -168,7 +182,7 @@ public class PassphraseCacheService extends Service { * @param keyId * @return */ - private String getCachedPassphraseImpl(long keyId) { + private String getCachedPassphraseImpl(long keyId) throws ProviderHelper.NotFoundException { // passphrase for symmetric encryption? if (keyId == Constants.key.symmetric) { Log.d(Constants.TAG, "PassphraseCacheService.getCachedPassphraseImpl() for symmetric encryption"); @@ -181,39 +195,33 @@ public class PassphraseCacheService extends Service { } // try to get master key id which is used as an identifier for cached passphrases - try { - Log.d(Constants.TAG, "PassphraseCacheService.getCachedPassphraseImpl() for masterKeyId " + keyId); - CanonicalizedSecretKeyRing key = new ProviderHelper(this).getCanonicalizedSecretKeyRing( - KeychainContract.KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(keyId)); - // no passphrase needed? just add empty string and return it, then - if (!key.hasPassphrase()) { - Log.d(Constants.TAG, "Key has no passphrase! Caches and returns empty passphrase!"); + Log.d(Constants.TAG, "PassphraseCacheService.getCachedPassphraseImpl() for masterKeyId " + keyId); + CanonicalizedSecretKeyRing key = new ProviderHelper(this).getCanonicalizedSecretKeyRing( + KeychainContract.KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(keyId)); + // no passphrase needed? just add empty string and return it, then + if (!key.hasPassphrase()) { + Log.d(Constants.TAG, "Key has no passphrase! Caches and returns empty passphrase!"); - try { - addCachedPassphrase(this, keyId, "", key.getPrimaryUserIdWithFallback()); - } catch (PgpGeneralException e) { - Log.d(Constants.TAG, "PgpGeneralException occured"); - } - return ""; + try { + addCachedPassphrase(this, keyId, "", key.getPrimaryUserIdWithFallback()); + } catch (PgpGeneralException e) { + Log.d(Constants.TAG, "PgpGeneralException occured"); } + return ""; + } - // get cached passphrase - CachedPassphrase cachedPassphrase = mPassphraseCache.get(keyId); - if (cachedPassphrase == null) { - Log.d(Constants.TAG, "PassphraseCacheService: Passphrase not (yet) cached, returning null"); - // not really an error, just means the passphrase is not cached but not empty either - return null; - } - - // set it again to reset the cache life cycle - Log.d(Constants.TAG, "PassphraseCacheService: Cache passphrase again when getting it!"); - addCachedPassphrase(this, keyId, cachedPassphrase.getPassphrase(), cachedPassphrase.getPrimaryUserID()); - return cachedPassphrase.getPassphrase(); - - } catch (ProviderHelper.NotFoundException e) { - Log.e(Constants.TAG, "PassphraseCacheService: Passphrase for unknown key was requested!"); + // get cached passphrase + CachedPassphrase cachedPassphrase = mPassphraseCache.get(keyId); + if (cachedPassphrase == null) { + Log.d(Constants.TAG, "PassphraseCacheService: Passphrase not (yet) cached, returning null"); + // not really an error, just means the passphrase is not cached but not empty either return null; } + + // set it again to reset the cache life cycle + Log.d(Constants.TAG, "PassphraseCacheService: Cache passphrase again when getting it!"); + addCachedPassphrase(this, keyId, cachedPassphrase.getPassphrase(), cachedPassphrase.getPrimaryUserID()); + return cachedPassphrase.getPassphrase(); } /** @@ -295,12 +303,19 @@ public class PassphraseCacheService extends Service { long keyId = intent.getLongExtra(EXTRA_KEY_ID, -1); Messenger messenger = intent.getParcelableExtra(EXTRA_MESSENGER); - String passphrase = getCachedPassphraseImpl(keyId); Message msg = Message.obtain(); - Bundle bundle = new Bundle(); - bundle.putString(EXTRA_PASSPHRASE, passphrase); - msg.obj = bundle; + try { + String passphrase = getCachedPassphraseImpl(keyId); + msg.what = MSG_PASSPHRASE_CACHE_GET_OKAY; + Bundle bundle = new Bundle(); + bundle.putString(EXTRA_PASSPHRASE, passphrase); + msg.setData(bundle); + } catch (ProviderHelper.NotFoundException e) { + Log.e(Constants.TAG, "PassphraseCacheService: Passphrase for unknown key was requested!"); + msg.what = MSG_PASSPHRASE_CACHE_GET_KEY_NO_FOUND; + } + try { messenger.send(msg); } catch (RemoteException e) { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/CertifyKeyActivity.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/CertifyKeyActivity.java index 467e0c14a..c1986825c 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/CertifyKeyActivity.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/CertifyKeyActivity.java @@ -231,7 +231,14 @@ public class CertifyKeyActivity extends ActionBarActivity implements */ private void initiateCertifying() { // get the user's passphrase for this key (if required) - String passphrase = PassphraseCacheService.getCachedPassphrase(this, mMasterKeyId); + String passphrase = null; + try { + passphrase = PassphraseCacheService.getCachedPassphrase(this, mMasterKeyId); + } catch (PassphraseCacheService.KeyNotFoundException e) { + Log.e(Constants.TAG, "Key not found!", e); + finish(); + return; + } if (passphrase == null) { PassphraseDialogFragment.show(this, mMasterKeyId, new Handler() { 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 ae294547f..03074bb6a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java @@ -475,8 +475,14 @@ public class EditKeyFragment extends LoaderFragment implements } private void cachePassphraseForEdit() { - mCurrentPassphrase = PassphraseCacheService.getCachedPassphrase(getActivity(), - mSaveKeyringParcel.mMasterKeyId); + try { + mCurrentPassphrase = PassphraseCacheService.getCachedPassphrase(getActivity(), + mSaveKeyringParcel.mMasterKeyId); + } catch (PassphraseCacheService.KeyNotFoundException e) { + Log.e(Constants.TAG, "Key not found!", e); + getActivity().finish(); + return; + } if (mCurrentPassphrase == null) { PassphraseDialogFragment.show(getActivity(), mSaveKeyringParcel.mMasterKeyId, new Handler() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EncryptActivity.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EncryptActivity.java index 33deddd81..7e08f6b7c 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EncryptActivity.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EncryptActivity.java @@ -450,20 +450,24 @@ public class EncryptActivity extends DrawerActivity implements EncryptActivityIn return false; } - if (mSigningKeyId != 0 && PassphraseCacheService.getCachedPassphrase(this, mSigningKeyId) == null) { - PassphraseDialogFragment.show(this, mSigningKeyId, - new Handler() { - @Override - public void handleMessage(Message message) { - if (message.what == PassphraseDialogFragment.MESSAGE_OKAY) { - // restart - startEncrypt(); + try { + if (mSigningKeyId != 0 && PassphraseCacheService.getCachedPassphrase(this, mSigningKeyId) == null) { + PassphraseDialogFragment.show(this, mSigningKeyId, + new Handler() { + @Override + public void handleMessage(Message message) { + if (message.what == PassphraseDialogFragment.MESSAGE_OKAY) { + // restart + startEncrypt(); + } } } - } - ); + ); - return false; + return false; + } + } catch (PassphraseCacheService.KeyNotFoundException e) { + Log.e(Constants.TAG, "Key not found!", e); } } return true;