From 04efa9e66d0d49af7adfabdee2546a02759faec7 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 23 Nov 2017 22:19:44 +0100 Subject: [PATCH] check all requested keys in PassphraseDialogActivity --- .../pgp/PgpDecryptVerifyOperation.java | 4 +- .../keychain/pgp/PgpKeyOperation.java | 4 +- .../keychain/remote/OpenPgpService.java | 6 +- .../service/input/CryptoInputParcel.java | 48 +++-- .../keychain/ui/PassphraseDialogActivity.java | 186 ++++++++++-------- .../AuthenticationOperationTest.java | 12 +- 6 files changed, 139 insertions(+), 121 deletions(-) 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 8057f82a8..240e6cb56 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java @@ -649,7 +649,7 @@ public class PgpDecryptVerifyOperation extends BaseOperationemptyMap()); + return new AutoValue_CryptoInputParcel(null, null, null, true, null, Collections.emptyMap()); } public static CryptoInputParcel createCryptoInputParcel(Date signatureTime, Passphrase passphrase) { if (signatureTime == null) { signatureTime = new Date(); } - return new AutoValue_CryptoInputParcel(signatureTime, passphrase, true, null, + return new AutoValue_CryptoInputParcel(signatureTime, passphrase, null, true, null, Collections.emptyMap()); } public static CryptoInputParcel createCryptoInputParcel(Passphrase passphrase) { - return new AutoValue_CryptoInputParcel(null, passphrase, true, null, Collections.emptyMap()); + return new AutoValue_CryptoInputParcel(null, passphrase, null, true, null, Collections.emptyMap()); } public static CryptoInputParcel createCryptoInputParcel(Date signatureTime) { if (signatureTime == null) { signatureTime = new Date(); } - return new AutoValue_CryptoInputParcel(signatureTime, null, true, null, + return new AutoValue_CryptoInputParcel(signatureTime, null, null, true, null, Collections.emptyMap()); } public static CryptoInputParcel createCryptoInputParcel(ParcelableProxy parcelableProxy) { - return new AutoValue_CryptoInputParcel(null, null, true, parcelableProxy, new HashMap()); + return new AutoValue_CryptoInputParcel(null, null, null, true, parcelableProxy, new HashMap()); } public static CryptoInputParcel createCryptoInputParcel(Date signatureTime, boolean cachePassphrase) { if (signatureTime == null) { signatureTime = new Date(); } - return new AutoValue_CryptoInputParcel(signatureTime, null, cachePassphrase, null, + return new AutoValue_CryptoInputParcel(signatureTime, null, null, cachePassphrase, null, new HashMap()); } public static CryptoInputParcel createCryptoInputParcel(boolean cachePassphrase) { - return new AutoValue_CryptoInputParcel(null, null, cachePassphrase, null, new HashMap()); + return new AutoValue_CryptoInputParcel(null, null, null, cachePassphrase, null, new HashMap()); } // TODO get rid of this! @@ -105,8 +111,8 @@ public abstract class CryptoInputParcel implements Parcelable { newCryptoData.put(ByteBuffer.wrap(hash), signedHash); newCryptoData = Collections.unmodifiableMap(newCryptoData); - return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), isCachePassphrase(), - getParcelableProxy(), newCryptoData); + return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), getPassphraseSubkey(), + isCachePassphrase(), getParcelableProxy(), newCryptoData); } @CheckResult @@ -115,32 +121,32 @@ public abstract class CryptoInputParcel implements Parcelable { newCryptoData.putAll(cachedSessionKeys); newCryptoData = Collections.unmodifiableMap(newCryptoData); - return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), isCachePassphrase(), - getParcelableProxy(), newCryptoData); + return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), getPassphraseSubkey(), + isCachePassphrase(), getParcelableProxy(), newCryptoData); } @CheckResult - public CryptoInputParcel withPassphrase(Passphrase passphrase) { - return new AutoValue_CryptoInputParcel(getSignatureTime(), passphrase, isCachePassphrase(), + public CryptoInputParcel withPassphrase(Passphrase passphrase, Long subKeyId) { + return new AutoValue_CryptoInputParcel(getSignatureTime(), passphrase, subKeyId, isCachePassphrase(), getParcelableProxy(), getCryptoData()); } @CheckResult public CryptoInputParcel withNoCachePassphrase() { - return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), false, getParcelableProxy(), - getCryptoData()); + return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), getPassphraseSubkey(), + false, getParcelableProxy(), getCryptoData()); } @CheckResult public CryptoInputParcel withSignatureTime(Date signatureTime) { - return new AutoValue_CryptoInputParcel(signatureTime, getPassphrase(), isCachePassphrase(), - getParcelableProxy(), getCryptoData()); + return new AutoValue_CryptoInputParcel(signatureTime, getPassphrase(), getPassphraseSubkey(), + isCachePassphrase(), getParcelableProxy(), getCryptoData()); } @CheckResult public CryptoInputParcel withParcelableProxy(ParcelableProxy parcelableProxy) { - return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), isCachePassphrase(), - parcelableProxy, getCryptoData()); + return new AutoValue_CryptoInputParcel(getSignatureTime(), getPassphrase(), getPassphraseSubkey(), + isCachePassphrase(), parcelableProxy, getCryptoData()); } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/PassphraseDialogActivity.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/PassphraseDialogActivity.java index 7ba4f9945..2a5d49214 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/PassphraseDialogActivity.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/PassphraseDialogActivity.java @@ -25,6 +25,7 @@ import android.content.DialogInterface; import android.content.Intent; import android.os.AsyncTask; import android.os.Bundle; +import android.os.SystemClock; import android.support.annotation.NonNull; import android.support.v4.app.DialogFragment; import android.support.v4.app.FragmentActivity; @@ -117,7 +118,7 @@ public class PassphraseDialogActivity extends FragmentActivity { if (pubRing.getSecretKeyType(requiredInput.getSubKeyId()) == SecretKeyType.PASSPHRASE_EMPTY) { // also return passphrase back to activity Intent returnIntent = new Intent(); - cryptoInputParcel = cryptoInputParcel.withPassphrase(new Passphrase("")); + cryptoInputParcel = cryptoInputParcel.withPassphrase(new Passphrase(""), requiredInput.getSubKeyId()); returnIntent.putExtra(RESULT_CRYPTO_INPUT, cryptoInputParcel); setResult(RESULT_OK, returnIntent); finish(); @@ -260,7 +261,7 @@ public class PassphraseDialogActivity extends FragmentActivity { break; // special case: empty passphrase just returns the empty passphrase case PASSPHRASE_EMPTY: - finishCaching(new Passphrase("")); + finishCaching(new Passphrase(""), subKeyId); default: throw new AssertionError("Unhandled SecretKeyType (should not happen)"); } @@ -420,7 +421,7 @@ public class PassphraseDialogActivity extends FragmentActivity { backupCodeInput.deleteCharAt(backupCodeInput.length() - 1); Passphrase passphrase = new Passphrase(backupCodeInput.toString()); - finishCaching(passphrase); + finishCaching(passphrase, null); return; } @@ -438,96 +439,107 @@ public class PassphraseDialogActivity extends FragmentActivity { getString(R.string.passp_cache_notif_pwd), timeToLiveSeconds); } - finishCaching(passphrase); + finishCaching(passphrase, null); return; } - mLayout.setDisplayedChild(1); - positive.setEnabled(false); - - new AsyncTask() { - - @Override - protected CanonicalizedSecretKey doInBackground(Void... params) { - try { - long timeBeforeOperation = System.currentTimeMillis(); - - Long subKeyId = mRequiredInput.getSubKeyId(); - CanonicalizedSecretKeyRing secretKeyRing = - KeyRepository.create(getContext()).getCanonicalizedSecretKeyRing( - KeychainContract.KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(subKeyId)); - CanonicalizedSecretKey secretKeyToUnlock = - secretKeyRing.getSecretKey(subKeyId); - - // this is the operation may take a very long time (100ms to several seconds!) - boolean unlockSucceeded = secretKeyToUnlock.unlock(passphrase); - - // if it didn't take that long, give the user time to appreciate the progress bar - long operationTime = System.currentTimeMillis() - timeBeforeOperation; - if (operationTime < 100) { - try { - Thread.sleep(100 - operationTime); - } catch (InterruptedException e) { - // ignore - } - } - - return unlockSucceeded ? secretKeyToUnlock : null; - } catch (NotFoundException | PgpGeneralException e) { - Toast.makeText(getActivity(), R.string.error_could_not_extract_private_key, - Toast.LENGTH_SHORT).show(); - - getActivity().setResult(RESULT_CANCELED); - dismiss(); - getActivity().finish(); - return null; - } - } - - /** Handle a good or bad passphrase. This happens in the UI thread! */ - @Override - protected void onPostExecute(CanonicalizedSecretKey result) { - super.onPostExecute(result); - - // if we were cancelled in the meantime, the result isn't relevant anymore - if (mIsCancelled) { - return; - } - - // if the passphrase was wrong, reset and re-enable the dialogue - if (result == null) { - mPassphraseEditText.setText(""); - mPassphraseEditText.setError(getString(R.string.wrong_passphrase)); - mLayout.setDisplayedChild(0); - positive.setEnabled(true); - return; - } - - // cache the new passphrase as specified in CryptoInputParcel - Log.d(Constants.TAG, "Everything okay!"); - - if (mRequiredInput.mSkipCaching) { - Log.d(Constants.TAG, "Not caching entered passphrase!"); - } else { - Log.d(Constants.TAG, "Caching entered passphrase"); - - try { - PassphraseCacheService.addCachedPassphrase(getActivity(), - mRequiredInput.getMasterKeyId(), mRequiredInput.getSubKeyId(), passphrase, - result.getRing().getPrimaryUserIdWithFallback(), timeToLiveSeconds); - } catch (PgpKeyNotFoundException e) { - Log.e(Constants.TAG, "adding of a passphrase failed", e); - } - } - - finishCaching(passphrase); - } - }.execute(); + checkPassphraseAndFinishCaching(positive, passphrase, timeToLiveSeconds); } + }); } - private void finishCaching(Passphrase passphrase) { + private void checkPassphraseAndFinishCaching(final Button positive, final Passphrase passphrase, + final int timeToLiveSeconds) { + mLayout.setDisplayedChild(1); + positive.setEnabled(false); + + new AsyncTask() { + + @Override + protected CanonicalizedSecretKey doInBackground(Void... params) { + try { + long timeBeforeOperation = SystemClock.elapsedRealtime(); + + CanonicalizedSecretKey canonicalizedSecretKey = null; + for (long subKeyId : mRequiredInput.getSubKeyIds()) { + CanonicalizedSecretKeyRing secretKeyRing = + KeyRepository.create(getContext()).getCanonicalizedSecretKeyRing( + KeychainContract.KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(subKeyId)); + CanonicalizedSecretKey secretKeyToUnlock = + secretKeyRing.getSecretKey(subKeyId); + + // this is the operation may take a very long time (100ms to several seconds!) + boolean unlockSucceeded = secretKeyToUnlock.unlock(passphrase); + if (unlockSucceeded) { + canonicalizedSecretKey = secretKeyToUnlock; + } + } + + // if it didn't take that long, give the user time to appreciate the progress bar + long operationTime = SystemClock.elapsedRealtime() - timeBeforeOperation; + if (operationTime < 100) { + try { + Thread.sleep(100 - operationTime); + } catch (InterruptedException e) { + // ignore + } + } + + return canonicalizedSecretKey; + } catch (NotFoundException | PgpGeneralException e) { + Toast.makeText(getActivity(), R.string.error_could_not_extract_private_key, + Toast.LENGTH_SHORT).show(); + + getActivity().setResult(RESULT_CANCELED); + dismiss(); + getActivity().finish(); + return null; + } + } + + /** Handle a good or bad passphrase. This happens in the UI thread! */ + @Override + protected void onPostExecute(CanonicalizedSecretKey unlockedKey) { + super.onPostExecute(unlockedKey); + + // if we were cancelled in the meantime, the result isn't relevant anymore + if (mIsCancelled) { + return; + } + + // if the passphrase was wrong, reset and re-enable the dialogue + if (unlockedKey == null) { + mPassphraseEditText.setText(""); + mPassphraseEditText.setError(getString(R.string.wrong_passphrase)); + mLayout.setDisplayedChild(0); + positive.setEnabled(true); + return; + } + + // cache the new passphrase as specified in CryptoInputParcel + Log.d(Constants.TAG, "Everything okay!"); + + if (mRequiredInput.mSkipCaching) { + Log.d(Constants.TAG, "Not caching entered passphrase!"); + } else { + Log.d(Constants.TAG, "Caching entered passphrase"); + + try { + PassphraseCacheService.addCachedPassphrase(getActivity(), + unlockedKey.getRing().getMasterKeyId(), unlockedKey.getKeyId(), passphrase, + unlockedKey.getRing().getPrimaryUserIdWithFallback(), timeToLiveSeconds); + } catch (PgpKeyNotFoundException e) { + Log.e(Constants.TAG, "adding of a passphrase failed", e); + } + } + + finishCaching(passphrase, unlockedKey.getKeyId()); + } + }.execute(); + } + + private void finishCaching(Passphrase passphrase, Long subKeyId) { // any indication this isn't needed anymore, don't do it. if (mIsCancelled || getActivity() == null) { return; @@ -535,7 +547,7 @@ public class PassphraseDialogActivity extends FragmentActivity { CryptoInputParcel inputParcel = getArguments().getParcelable(EXTRA_CRYPTO_INPUT); // noinspection ConstantConditions, we handle the non-null case in PassphraseDialogActivity.onCreate() - inputParcel = inputParcel.withPassphrase(passphrase); + inputParcel = inputParcel.withPassphrase(passphrase, subKeyId); ((PassphraseDialogActivity) getActivity()).handleResult(inputParcel); diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/operations/AuthenticationOperationTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/operations/AuthenticationOperationTest.java index 898e2a91d..8f3726895 100644 --- a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/operations/AuthenticationOperationTest.java +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/operations/AuthenticationOperationTest.java @@ -175,7 +175,7 @@ public class AuthenticationOperationTest { .createAuthenticationParcel(authData.build(), challenge); CryptoInputParcel inputParcel = CryptoInputParcel.createCryptoInputParcel(); - inputParcel = inputParcel.withPassphrase(mKeyPhrase); + inputParcel = inputParcel.withPassphrase(mKeyPhrase, authSubKeyId); AuthenticationResult result = op.execute(authData.build(), inputParcel, authenticationParcel); @@ -221,7 +221,7 @@ public class AuthenticationOperationTest { .createAuthenticationParcel(authData.build(), challenge); CryptoInputParcel inputParcel = CryptoInputParcel.createCryptoInputParcel(); - inputParcel = inputParcel.withPassphrase(mKeyPhrase); + inputParcel = inputParcel.withPassphrase(mKeyPhrase, authSubKeyId); AuthenticationResult result = op.execute(authData.build(), inputParcel, authenticationParcel); @@ -267,7 +267,7 @@ public class AuthenticationOperationTest { .createAuthenticationParcel(authData.build(), challenge); CryptoInputParcel inputParcel = CryptoInputParcel.createCryptoInputParcel(); - inputParcel = inputParcel.withPassphrase(mKeyPhrase); + inputParcel = inputParcel.withPassphrase(mKeyPhrase, authSubKeyId); AuthenticationResult result = op.execute(authData.build(), inputParcel, authenticationParcel); @@ -315,7 +315,7 @@ public class AuthenticationOperationTest { .createAuthenticationParcel(authData.build(), challenge); CryptoInputParcel inputParcel = CryptoInputParcel.createCryptoInputParcel(); - inputParcel = inputParcel.withPassphrase(mKeyPhrase); + inputParcel = inputParcel.withPassphrase(mKeyPhrase, authSubKeyId); AuthenticationResult result = op.execute(authData.build(), inputParcel, authenticationParcel); @@ -364,7 +364,7 @@ public class AuthenticationOperationTest { .createAuthenticationParcel(authData.build(), challenge); CryptoInputParcel inputParcel = CryptoInputParcel.createCryptoInputParcel(); - inputParcel = inputParcel.withPassphrase(mKeyPhrase); + inputParcel = inputParcel.withPassphrase(mKeyPhrase, authSubKeyId); AuthenticationResult result = op.execute(authData.build(), inputParcel, authenticationParcel); @@ -387,7 +387,7 @@ public class AuthenticationOperationTest { .createAuthenticationParcel(authData.build(), challenge); CryptoInputParcel inputParcel = CryptoInputParcel.createCryptoInputParcel(); - inputParcel = inputParcel.withPassphrase(mKeyPhrase); + inputParcel = inputParcel.withPassphrase(mKeyPhrase, authSubKeyId); AuthenticationResult result = op.execute(authData.build(), inputParcel, authenticationParcel);