From cb12d64e5f647d2a32bfe622f673c593717a7eed Mon Sep 17 00:00:00 2001 From: m2049r Date: Thu, 10 May 2018 13:48:11 +0200 Subject: [PATCH] Various Fixes (#279) * load password only if it's passed * cancel fingerprint if password entered * rework fingerprint code * cleanup unused params * new version code --- app/build.gradle | 4 +- .../xmrwallet/GenerateReviewFragment.java | 35 +++-- .../com/m2049r/xmrwallet/LoginActivity.java | 17 +-- .../com/m2049r/xmrwallet/WalletActivity.java | 3 +- .../xmrwallet/util/FingerprintHelper.java | 17 +-- .../com/m2049r/xmrwallet/util/Helper.java | 27 ++-- .../m2049r/xmrwallet/util/KeyStoreHelper.java | 122 +++++++++++------- 7 files changed, 125 insertions(+), 100 deletions(-) diff --git a/app/build.gradle b/app/build.gradle index 14e1e69..1eb2d32 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -7,8 +7,8 @@ android { applicationId "com.m2049r.xmrwallet" minSdkVersion 21 targetSdkVersion 25 - versionCode 92 - versionName "1.5.2 'CrAzY Nacho'" + versionCode 93 + versionName "1.5.3 'CrAzY Nacho'" testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner" externalNativeBuild { cmake { diff --git a/app/src/main/java/com/m2049r/xmrwallet/GenerateReviewFragment.java b/app/src/main/java/com/m2049r/xmrwallet/GenerateReviewFragment.java index 418436a..aff2226 100644 --- a/app/src/main/java/com/m2049r/xmrwallet/GenerateReviewFragment.java +++ b/app/src/main/java/com/m2049r/xmrwallet/GenerateReviewFragment.java @@ -53,7 +53,6 @@ import com.m2049r.xmrwallet.util.MoneroThreadPoolExecutor; import com.m2049r.xmrwallet.widget.Toolbar; import java.io.File; -import java.security.KeyStoreException; import timber.log.Timber; @@ -392,19 +391,19 @@ public class GenerateReviewFragment extends Fragment { @Override protected Boolean doInBackground(String... params) { - if (params.length != 4) return false; - File walletFile = Helper.getWalletFile(getActivity(), params[0]); - String oldPassword = params[1]; - String userPassword = params[2]; - boolean fingerprintAuthAllowed = Boolean.valueOf(params[3]); + if (params.length != 2) return false; + final String userPassword = params[0]; + final boolean fingerPassValid = Boolean.valueOf(params[1]); newPassword = KeyStoreHelper.getCrazyPass(getActivity(), userPassword); - boolean success = changeWalletPassword(newPassword); + final boolean success = changeWalletPassword(newPassword); if (success) { - if (fingerprintAuthAllowed) { - KeyStoreHelper.saveWalletUserPass(getActivity(), walletName, userPassword); - } else { - KeyStoreHelper.removeWalletUserPass(getActivity(), walletName); - } + Context ctx = getActivity(); + if (ctx != null) + if (fingerPassValid) { + KeyStoreHelper.saveWalletUserPass(ctx, walletName, userPassword); + } else { + KeyStoreHelper.removeWalletUserPass(ctx, walletName); + } } return success; } @@ -412,7 +411,7 @@ public class GenerateReviewFragment extends Fragment { @Override protected void onPostExecute(Boolean result) { super.onPostExecute(result); - if (getActivity().isDestroyed()) { + if ((getActivity() == null) || getActivity().isDestroyed()) { return; } if (progressCallback != null) @@ -467,11 +466,7 @@ public class GenerateReviewFragment extends Fragment { } }); - try { - swFingerprintAllowed.setChecked(FingerprintHelper.isFingerprintAuthAllowed(walletName)); - } catch (KeyStoreException ex) { - ex.printStackTrace(); - } + swFingerprintAllowed.setChecked(FingerprintHelper.isFingerPassValid(getActivity(), walletName)); } etPasswordA.getEditText().addTextChangedListener(new TextWatcher() { @@ -547,7 +542,7 @@ public class GenerateReviewFragment extends Fragment { } else if (!newPasswordA.equals(newPasswordB)) { etPasswordB.setError(getString(R.string.generate_bad_passwordB)); } else if (newPasswordA.equals(newPasswordB)) { - new AsyncChangePassword().execute(walletName, getPassword(), newPasswordA, Boolean.toString(swFingerprintAllowed.isChecked())); + new AsyncChangePassword().execute(newPasswordA, Boolean.toString(swFingerprintAllowed.isChecked())); Helper.hideKeyboardAlways(getActivity()); openDialog.dismiss(); openDialog = null; @@ -569,7 +564,7 @@ public class GenerateReviewFragment extends Fragment { } else if (!newPasswordA.equals(newPasswordB)) { etPasswordB.setError(getString(R.string.generate_bad_passwordB)); } else if (newPasswordA.equals(newPasswordB)) { - new AsyncChangePassword().execute(walletName, getPassword(), newPasswordA, Boolean.toString(swFingerprintAllowed.isChecked())); + new AsyncChangePassword().execute(newPasswordA, Boolean.toString(swFingerprintAllowed.isChecked())); Helper.hideKeyboardAlways(getActivity()); openDialog.dismiss(); openDialog = null; diff --git a/app/src/main/java/com/m2049r/xmrwallet/LoginActivity.java b/app/src/main/java/com/m2049r/xmrwallet/LoginActivity.java index b4473ca..39d7ce7 100644 --- a/app/src/main/java/com/m2049r/xmrwallet/LoginActivity.java +++ b/app/src/main/java/com/m2049r/xmrwallet/LoginActivity.java @@ -50,7 +50,6 @@ import com.m2049r.xmrwallet.model.NetworkType; import com.m2049r.xmrwallet.model.Wallet; import com.m2049r.xmrwallet.model.WalletManager; import com.m2049r.xmrwallet.service.WalletService; -import com.m2049r.xmrwallet.util.FingerprintHelper; import com.m2049r.xmrwallet.util.Helper; import com.m2049r.xmrwallet.util.KeyStoreHelper; import com.m2049r.xmrwallet.util.MoneroThreadPoolExecutor; @@ -63,7 +62,6 @@ import java.io.IOException; import java.net.Socket; import java.net.SocketAddress; import java.nio.channels.FileChannel; -import java.security.KeyStoreException; import java.util.Date; import timber.log.Timber; @@ -229,17 +227,20 @@ public class LoginActivity extends SecureActivity @Override protected Boolean doInBackground(String... params) { if (params.length != 2) return false; - File walletFile = Helper.getWalletFile(LoginActivity.this, params[0]); + String oldName = params[0]; String newName = params[1]; + File walletFile = Helper.getWalletFile(LoginActivity.this, oldName); boolean success = renameWallet(walletFile, newName); try { - if (success && FingerprintHelper.isFingerprintAuthAllowed(params[0])) { - String savedPass = KeyStoreHelper.loadWalletUserPass(LoginActivity.this, params[0]); + if (success) { + String savedPass = KeyStoreHelper.loadWalletUserPass(LoginActivity.this, oldName); KeyStoreHelper.saveWalletUserPass(LoginActivity.this, newName, savedPass); - KeyStoreHelper.removeWalletUserPass(LoginActivity.this, params[0]); } - } catch (KeyStoreException ex) { - ex.printStackTrace(); + } catch (KeyStoreHelper.BrokenPasswordStoreException ex) { + Timber.w(ex); + } finally { + // we have either set a new password or it is broken - kill the old one either way + KeyStoreHelper.removeWalletUserPass(LoginActivity.this, oldName); } return success; } diff --git a/app/src/main/java/com/m2049r/xmrwallet/WalletActivity.java b/app/src/main/java/com/m2049r/xmrwallet/WalletActivity.java index 2c38937..21bea51 100644 --- a/app/src/main/java/com/m2049r/xmrwallet/WalletActivity.java +++ b/app/src/main/java/com/m2049r/xmrwallet/WalletActivity.java @@ -133,6 +133,7 @@ public class WalletActivity extends SecureActivity implements WalletFragment.Lis acquireWakeLock(); String walletId = extras.getString(REQUEST_ID); needVerifyIdentity = extras.getBoolean(REQUEST_FINGERPRINT_USED); + password = extras.getString(REQUEST_PW); connectWalletService(walletId, password); } else { finish(); @@ -259,8 +260,6 @@ public class WalletActivity extends SecureActivity implements WalletFragment.Lis .add(R.id.fragment_container, walletFragment, WalletFragment.class.getName()).commit(); Timber.d("fragment added"); - password = getIntent().getExtras().getString(REQUEST_PW); - startWalletService(); Timber.d("onCreate() done."); } diff --git a/app/src/main/java/com/m2049r/xmrwallet/util/FingerprintHelper.java b/app/src/main/java/com/m2049r/xmrwallet/util/FingerprintHelper.java index 46de036..b0d480c 100644 --- a/app/src/main/java/com/m2049r/xmrwallet/util/FingerprintHelper.java +++ b/app/src/main/java/com/m2049r/xmrwallet/util/FingerprintHelper.java @@ -5,8 +5,7 @@ import android.content.Context; import android.support.v4.hardware.fingerprint.FingerprintManagerCompat; import android.support.v4.os.CancellationSignal; -import java.security.KeyStore; -import java.security.KeyStoreException; +import timber.log.Timber; public class FingerprintHelper { @@ -20,15 +19,14 @@ public class FingerprintHelper { fingerprintManager.hasEnrolledFingerprints(); } - public static boolean isFingerprintAuthAllowed(String wallet) throws KeyStoreException { - KeyStore keyStore = KeyStore.getInstance(KeyStoreHelper.SecurityConstants.KEYSTORE_PROVIDER_ANDROID_KEYSTORE); + public static boolean isFingerPassValid(Context context, String wallet) { try { - keyStore.load(null); - } catch (Exception ex) { - throw new IllegalStateException("Could not load KeyStore", ex); + KeyStoreHelper.loadWalletUserPass(context, wallet); + return true; + } catch (KeyStoreHelper.BrokenPasswordStoreException ex) { + Timber.w(ex); + return false; } - - return keyStore.containsAlias(KeyStoreHelper.SecurityConstants.WALLET_PASS_KEY_PREFIX + wallet); } public static void authenticate(Context context, CancellationSignal cancelSignal, @@ -36,5 +34,4 @@ public class FingerprintHelper { FingerprintManagerCompat manager = FingerprintManagerCompat.from(context); manager.authenticate(null, 0, cancelSignal, callback, null); } - } diff --git a/app/src/main/java/com/m2049r/xmrwallet/util/Helper.java b/app/src/main/java/com/m2049r/xmrwallet/util/Helper.java index 510134d..6abc6c6 100644 --- a/app/src/main/java/com/m2049r/xmrwallet/util/Helper.java +++ b/app/src/main/java/com/m2049r/xmrwallet/util/Helper.java @@ -64,7 +64,6 @@ import java.math.BigInteger; import java.net.MalformedURLException; import java.net.SocketTimeoutException; import java.net.URL; -import java.security.KeyStoreException; import java.util.Locale; import javax.net.ssl.HttpsURLConnection; @@ -368,12 +367,7 @@ public class Helper { final TextInputLayout etPassword = (TextInputLayout) promptsView.findViewById(R.id.etPassword); etPassword.setHint(context.getString(R.string.prompt_password, wallet)); - boolean fingerprintAuthCheck; - try { - fingerprintAuthCheck = FingerprintHelper.isFingerprintAuthAllowed(wallet); - } catch (KeyStoreException ex) { - fingerprintAuthCheck = false; - } + final boolean fingerprintAuthCheck = FingerprintHelper.isFingerPassValid(context, wallet); final boolean fingerprintAuthAllowed = !fingerprintDisabled && fingerprintAuthCheck; final CancellationSignal cancelSignal = new CancellationSignal(); @@ -425,13 +419,18 @@ public class Helper { @Override public void onAuthenticationSucceeded(FingerprintManagerCompat.AuthenticationResult result) { - String userPass = KeyStoreHelper.loadWalletUserPass(context, wallet); - if (Helper.processPasswordEntry(context, wallet, userPass, true, action)) { - Helper.hideKeyboardAlways((Activity) context); - openDialog.dismiss(); - openDialog = null; - } else { + try { + String userPass = KeyStoreHelper.loadWalletUserPass(context, wallet); + if (Helper.processPasswordEntry(context, wallet, userPass, true, action)) { + Helper.hideKeyboardAlways((Activity) context); + openDialog.dismiss(); + openDialog = null; + } else { + etPassword.setError(context.getString(R.string.bad_password)); + } + } catch (KeyStoreHelper.BrokenPasswordStoreException ex) { etPassword.setError(context.getString(R.string.bad_password)); + // TODO: better errror message here - what would it be? } } @@ -455,6 +454,7 @@ public class Helper { String pass = etPassword.getEditText().getText().toString(); if (processPasswordEntry(context, wallet, pass, false, action)) { Helper.hideKeyboardAlways((Activity) context); + cancelSignal.cancel(); openDialog.dismiss(); openDialog = null; } else { @@ -472,6 +472,7 @@ public class Helper { String pass = etPassword.getEditText().getText().toString(); if (processPasswordEntry(context, wallet, pass, false, action)) { Helper.hideKeyboardAlways((Activity) context); + cancelSignal.cancel(); openDialog.dismiss(); openDialog = null; } else { diff --git a/app/src/main/java/com/m2049r/xmrwallet/util/KeyStoreHelper.java b/app/src/main/java/com/m2049r/xmrwallet/util/KeyStoreHelper.java index 281fd25..4afd9c4 100644 --- a/app/src/main/java/com/m2049r/xmrwallet/util/KeyStoreHelper.java +++ b/app/src/main/java/com/m2049r/xmrwallet/util/KeyStoreHelper.java @@ -23,8 +23,10 @@ import android.os.Build; import android.security.KeyPairGeneratorSpec; import android.security.keystore.KeyGenParameterSpec; import android.security.keystore.KeyProperties; +import android.support.annotation.NonNull; import android.util.Base64; +import java.io.IOException; import java.math.BigInteger; import java.nio.charset.StandardCharsets; import java.security.InvalidAlgorithmParameterException; @@ -39,10 +41,15 @@ import java.security.PrivateKey; import java.security.PublicKey; import java.security.Signature; import java.security.SignatureException; +import java.security.UnrecoverableEntryException; +import java.security.cert.CertificateException; import java.util.Calendar; import java.util.GregorianCalendar; +import javax.crypto.BadPaddingException; import javax.crypto.Cipher; +import javax.crypto.IllegalBlockSizeException; +import javax.crypto.NoSuchPaddingException; import javax.security.auth.x500.X500Principal; import timber.log.Timber; @@ -58,43 +65,54 @@ public class KeyStoreHelper { static final private String RSA_ALIAS = "MonerujoRSA"; public static String getCrazyPass(Context context, String password) { - // TODO we should check Locale.getDefault().getLanguage() here but for now we default to English - return getCrazyPass(context, password, "English"); - } - - public static String getCrazyPass(Context context, String password, String language) { byte[] data = password.getBytes(StandardCharsets.UTF_8); byte[] sig = null; try { KeyStoreHelper.createKeys(context, RSA_ALIAS); sig = KeyStoreHelper.signData(RSA_ALIAS, data); - } catch (Exception ex) { + } catch (NoSuchProviderException | NoSuchAlgorithmException | + InvalidAlgorithmParameterException | KeyStoreException | + InvalidKeyException | SignatureException ex) { throw new IllegalStateException(ex); } return CrazyPassEncoder.encode(cnSlowHash(sig)); } - public static void saveWalletUserPass(Context context, String wallet, String password) { + public static boolean saveWalletUserPass(@NonNull Context context, String wallet, String password) { String walletKeyAlias = SecurityConstants.WALLET_PASS_KEY_PREFIX + wallet; byte[] data = password.getBytes(StandardCharsets.UTF_8); try { KeyStoreHelper.createKeys(context, walletKeyAlias); byte[] encrypted = KeyStoreHelper.encrypt(walletKeyAlias, data); + if (encrypted == null) return false; context.getSharedPreferences(SecurityConstants.WALLET_PASS_PREFS_NAME, Context.MODE_PRIVATE).edit() .putString(wallet, Base64.encodeToString(encrypted, Base64.DEFAULT)) .apply(); - } catch (Exception ex) { - throw new IllegalStateException(ex); + return true; + } catch (NoSuchProviderException | NoSuchAlgorithmException | + InvalidAlgorithmParameterException | KeyStoreException ex) { + Timber.w(ex); + return false; } } - public static String loadWalletUserPass(Context context, String wallet) { + static public class BrokenPasswordStoreException extends Exception { + BrokenPasswordStoreException() { + super(); + } + + BrokenPasswordStoreException(Throwable cause) { + super(cause); + } + } + + public static String loadWalletUserPass(@NonNull Context context, String wallet) throws BrokenPasswordStoreException { String walletKeyAlias = SecurityConstants.WALLET_PASS_KEY_PREFIX + wallet; String encoded = context.getSharedPreferences(SecurityConstants.WALLET_PASS_PREFS_NAME, Context.MODE_PRIVATE) .getString(wallet, ""); byte[] data = Base64.decode(encoded, Base64.DEFAULT); byte[] decrypted = KeyStoreHelper.decrypt(walletKeyAlias, data); - + if (decrypted == null) throw new BrokenPasswordStoreException(); return new String(decrypted, StandardCharsets.UTF_8); } @@ -102,23 +120,23 @@ public class KeyStoreHelper { String walletKeyAlias = SecurityConstants.WALLET_PASS_KEY_PREFIX + wallet; try { KeyStoreHelper.deleteKeys(walletKeyAlias); - context.getSharedPreferences(SecurityConstants.WALLET_PASS_PREFS_NAME, Context.MODE_PRIVATE).edit() - .remove(wallet).apply(); - } catch (Exception ex) { - throw new IllegalStateException(ex); + } catch (KeyStoreException ex) { + Timber.w(ex); } + context.getSharedPreferences(SecurityConstants.WALLET_PASS_PREFS_NAME, Context.MODE_PRIVATE).edit() + .remove(wallet).apply(); } /** * Creates a public and private key and stores it using the Android Key * Store, so that only this application will be able to access the keys. */ - public static void createKeys(Context context, String alias) throws NoSuchProviderException, + private static void createKeys(Context context, String alias) throws NoSuchProviderException, NoSuchAlgorithmException, InvalidAlgorithmParameterException, KeyStoreException { KeyStore keyStore = KeyStore.getInstance(SecurityConstants.KEYSTORE_PROVIDER_ANDROID_KEYSTORE); try { keyStore.load(null); - } catch (Exception ex) { // don't care why it failed + } catch (IOException | CertificateException ex) { throw new IllegalStateException("Could not load KeySotre", ex); } if (!keyStore.containsAlias(alias)) { @@ -130,13 +148,25 @@ public class KeyStoreHelper { } } - public static void deleteKeys(String alias) throws KeyStoreException { + private static boolean deleteKeys(String alias) throws KeyStoreException { KeyStore keyStore = KeyStore.getInstance(SecurityConstants.KEYSTORE_PROVIDER_ANDROID_KEYSTORE); try { keyStore.load(null); keyStore.deleteEntry(alias); - } catch (Exception ex) { // don't care why it failed - throw new IllegalStateException("Could not load KeySotre", ex); + return true; + } catch (IOException | NoSuchAlgorithmException | CertificateException ex) { + Timber.w(ex); + return false; + } + } + + public static boolean keyExists(String wallet) throws BrokenPasswordStoreException { + try { + KeyStore keyStore = KeyStore.getInstance(KeyStoreHelper.SecurityConstants.KEYSTORE_PROVIDER_ANDROID_KEYSTORE); + keyStore.load(null); + return keyStore.containsAlias(KeyStoreHelper.SecurityConstants.WALLET_PASS_KEY_PREFIX + wallet); + } catch (IOException | NoSuchAlgorithmException | CertificateException | KeyStoreException ex) { + throw new BrokenPasswordStoreException(ex); } } @@ -164,22 +194,19 @@ public class KeyStoreHelper { } @TargetApi(Build.VERSION_CODES.M) - private static void createKeysM(String alias) { - try { - KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance( - KeyProperties.KEY_ALGORITHM_RSA, SecurityConstants.KEYSTORE_PROVIDER_ANDROID_KEYSTORE); - keyPairGenerator.initialize( - new KeyGenParameterSpec.Builder( - alias, KeyProperties.PURPOSE_SIGN | KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) - .setDigests(KeyProperties.DIGEST_SHA256) - .setSignaturePaddings(KeyProperties.SIGNATURE_PADDING_RSA_PKCS1) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1) - .build()); - KeyPair keyPair = keyPairGenerator.generateKeyPair(); - Timber.d("M Keys created"); - } catch (NoSuchProviderException | NoSuchAlgorithmException | InvalidAlgorithmParameterException e) { - throw new RuntimeException(e); - } + private static void createKeysM(String alias) throws NoSuchProviderException, + NoSuchAlgorithmException, InvalidAlgorithmParameterException { + KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance( + KeyProperties.KEY_ALGORITHM_RSA, SecurityConstants.KEYSTORE_PROVIDER_ANDROID_KEYSTORE); + keyPairGenerator.initialize( + new KeyGenParameterSpec.Builder( + alias, KeyProperties.PURPOSE_SIGN | KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) + .setDigests(KeyProperties.DIGEST_SHA256) + .setSignaturePaddings(KeyProperties.SIGNATURE_PADDING_RSA_PKCS1) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1) + .build()); + KeyPair keyPair = keyPairGenerator.generateKeyPair(); + Timber.d("M Keys created"); } private static KeyStore.PrivateKeyEntry getPrivateKeyEntry(String alias) { @@ -199,33 +226,38 @@ public class KeyStoreHelper { return null; } return (KeyStore.PrivateKeyEntry) entry; - } catch (Exception ex) { + } catch (IOException | NoSuchAlgorithmException | CertificateException + | UnrecoverableEntryException | KeyStoreException ex) { Timber.e(ex); return null; } } - public static byte[] encrypt(String alias, byte[] data) { + private static byte[] encrypt(String alias, byte[] data) { try { PublicKey publicKey = getPrivateKeyEntry(alias).getCertificate().getPublicKey(); Cipher cipher = Cipher.getInstance(SecurityConstants.CIPHER_RSA_ECB_PKCS1); cipher.init(Cipher.ENCRYPT_MODE, publicKey); return cipher.doFinal(data); - } catch (Exception ex) { - throw new IllegalStateException("Could not initialize RSA cipher", ex); + } catch (InvalidKeyException | IllegalBlockSizeException | BadPaddingException + | NoSuchAlgorithmException | NoSuchPaddingException ex) { + Timber.e(ex); + return null; } } - public static byte[] decrypt(String alias, byte[] data) { + private static byte[] decrypt(String alias, byte[] data) { try { PrivateKey privateKey = getPrivateKeyEntry(alias).getPrivateKey(); Cipher cipher = Cipher.getInstance(SecurityConstants.CIPHER_RSA_ECB_PKCS1); cipher.init(Cipher.DECRYPT_MODE, privateKey); return cipher.doFinal(data); - } catch (Exception ex) { - throw new IllegalStateException("Could not initialize RSA cipher", ex); + } catch (NoSuchAlgorithmException | NoSuchPaddingException | InvalidKeyException | + IllegalBlockSizeException | BadPaddingException ex) { + Timber.e(ex); + return null; } } @@ -236,7 +268,7 @@ public class KeyStoreHelper { * * @return The data signature generated */ - public static byte[] signData(String alias, byte[] data) throws NoSuchAlgorithmException, + private static byte[] signData(String alias, byte[] data) throws NoSuchAlgorithmException, InvalidKeyException, SignatureException { PrivateKey privateKey = getPrivateKeyEntry(alias).getPrivateKey(); @@ -256,7 +288,7 @@ public class KeyStoreHelper { * @return A boolean value telling you whether the signature is valid or * not. */ - public static boolean verifyData(String alias, byte[] data, byte[] signature) + private static boolean verifyData(String alias, byte[] data, byte[] signature) throws NoSuchAlgorithmException, InvalidKeyException, SignatureException { // Make sure the signature string exists