Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove the spending PIN encryption before backup #1133

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove the spending PIN encryption before backup #1133

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3035a3
Remove spending PIN before backup
May 17, 2024
736109d
rework backup logic to create a wallet clone, remove spending PIN and…
May 21, 2024
48dc908
change walletToBackup to MutableLiveData
oliveraemmer Jun 5, 2024
7c0aafb
Merge pull request #20 from HW-Backed-Bitcoin-Wallet/oa/simplified_ba…
iSavicx Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions wallet/res/layout/backup_wallet_dialog.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,5 +91,36 @@
android:text="@string/backup_wallet_dialog_warning_encrypted"
android:textColor="@color/fg_less_significant"
android:textSize="@dimen/font_size_small" />

<LinearLayout
android:id="@+id/backup_wallet_dialog_spending_pin_group"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_marginBottom="@dimen/list_entry_padding_vertical"
android:divider="@drawable/divider_field"
android:orientation="horizontal"
android:showDividers="middle"
android:visibility="gone">

<EditText
android:id="@+id/backup_wallet_dialog_spending_pin"
android:layout_width="0px"
android:layout_height="wrap_content"
android:layout_weight="1"
android:hint="@string/private_key_password"
android:imeOptions="flagNoExtractUi"
android:inputType="numberPassword"
android:singleLine="true" />

<TextView
android:id="@+id/backup_wallet_dialog_bad_spending_pin"
android:layout_width="0px"
android:layout_height="wrap_content"
android:layout_weight="1"
android:text="@string/private_key_bad_password"
android:textColor="@color/fg_error"
android:textStyle="bold"
android:visibility="invisible" />
</LinearLayout>
</LinearLayout>
</ScrollView>
3 changes: 2 additions & 1 deletion wallet/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,8 @@
<string name="import_keys_dialog_failure">Wallet could not be restored:\n\n%s\n\nBad password?</string>
<string name="export_keys_dialog_title">Back up wallet</string>
<string name="backup_wallet_dialog_message">Your backup will be encrypted with the chosen password and written to external storage.</string>
<string name="backup_wallet_dialog_warning_encrypted">Your wallet is protected by a spending PIN. Make sure you remember the PIN in addition to the backup password!</string>
<string name="backup_wallet_dialog_warning_encrypted">Your spending PIN protection won\'t be present in the backup. Make sure you set the PIN again if you restore the wallet!</string>
<string name="backup_wallet_dialog_state_verifying">Verifying...</string>
<string name="export_keys_dialog_button_export">Back up</string>
<string name="export_keys_dialog_success"><![CDATA[<p>Your wallet has been backed up to <tt>%s</tt></p><p><b>If the only place your backup exists is on your device, you run the risk of losing both at the same time!</b></p><p>In any case, make sure you remember your backup password.</p>]]></string>
<string name="export_keys_dialog_failure">Your wallet could not be backed up:\n%s</string>
Expand Down
167 changes: 146 additions & 21 deletions wallet/src/de/schildbach/wallet/ui/backup/BackupWalletDialogFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,13 @@
import de.schildbach.wallet.util.Crypto;
import de.schildbach.wallet.util.Toast;
import de.schildbach.wallet.util.WalletUtils;

import org.bitcoinj.crypto.KeyCrypter;
import org.bitcoinj.wallet.Protos;
import org.bitcoinj.wallet.UnreadableWalletException;
import org.bitcoinj.wallet.Wallet;
import org.bitcoinj.wallet.WalletProtobufSerializer;
import org.bouncycastle.crypto.params.KeyParameter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -64,12 +68,12 @@
import java.io.Reader;
import java.io.Writer;
import java.nio.charset.StandardCharsets;
import java.text.DateFormat;
import java.time.Instant;
import java.time.format.DateTimeFormatter;
import java.util.Arrays;
import java.util.Date;
import java.util.TimeZone;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;

import static com.google.common.base.Preconditions.checkState;

Expand All @@ -92,11 +96,14 @@ public static void show(final FragmentManager fm) {
private View passwordMismatchView;
private CheckBox showView;
private TextView warningView;
private View spendingPINViewGroup;
private EditText spendingPINView;
private TextView badSpendingPINView;
private Button positiveButton, negativeButton;

private Executor executor = Executors.newSingleThreadExecutor();
private Wallet backupWallet;
Copy link
Collaborator

@schildbach schildbach May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the (decrypted) backupWallet should be kept in a MutableLiveData<Wallet> in BackupWalletViewModel, rather than here where it can be collected at any time (e.g. while you're picking the file).

And I'd use the name walletToBackUp, maybe even walletToBeBackedUp or decryptedWalletToBeBackedUp.

private AbstractWalletActivityViewModel walletActivityViewModel;
private BackupWalletViewModel viewModel;

private static final Logger log = LoggerFactory.getLogger(BackupWalletDialogFragment.class);

private final ActivityResultLauncher<String> createDocumentLauncher =
Expand All @@ -111,6 +118,7 @@ public void onChanged(final Wallet wallet) {
final String targetProvider = WalletUtils.uriToProvider(uri);
final String password = passwordView.getText().toString().trim();
checkState(!password.isEmpty());
checkState(backupWallet != null);
Copy link
Collaborator

@schildbach schildbach May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line 113 above, you'd observe that walletToBackUp which is then guaranteed to be non-null so you can remove this assert.

wipePasswords();
dismiss();

Expand All @@ -119,7 +127,7 @@ public void onChanged(final Wallet wallet) {
activity.getContentResolver().openOutputStream(uri),
StandardCharsets.UTF_8)) {
final Protos.Wallet walletProto =
new WalletProtobufSerializer().walletToProto(wallet);
new WalletProtobufSerializer().walletToProto(backupWallet);
final ByteArrayOutputStream baos = new ByteArrayOutputStream();
walletProto.writeTo(baos);
baos.close();
Expand Down Expand Up @@ -169,7 +177,7 @@ public void onChanged(final Wallet wallet) {
}
});

private final TextWatcher textWatcher = new TextWatcher() {
private final TextWatcher passwordTextWatcher = new TextWatcher() {
@Override
public void onTextChanged(final CharSequence s, final int start, final int before, final int count) {
viewModel.password.postValue(s.toString().trim());
Expand All @@ -184,6 +192,21 @@ public void afterTextChanged(final Editable s) {
}
};

private final TextWatcher spendingPINTextWatcher = new TextWatcher() {
@Override
public void onTextChanged(final CharSequence s, final int start, final int before, final int count) {
viewModel.spendingPIN.postValue(s.toString().trim());
}

@Override
public void beforeTextChanged(final CharSequence s, final int start, final int count, final int after) {
}

@Override
public void afterTextChanged(final Editable s) {
}
};

@Override
public void onAttach(final Context context) {
super.onAttach(context);
Expand Down Expand Up @@ -216,6 +239,10 @@ public Dialog onCreateDialog(final Bundle savedInstanceState) {

showView = view.findViewById(R.id.backup_wallet_dialog_show);

spendingPINViewGroup = view.findViewById(R.id.backup_wallet_dialog_spending_pin_group);
spendingPINView = view.findViewById(R.id.backup_wallet_dialog_spending_pin);
badSpendingPINView = view.findViewById(R.id.backup_wallet_dialog_bad_spending_pin);

warningView = view.findViewById(R.id.backup_wallet_dialog_warning_encrypted);

final DialogBuilder builder = DialogBuilder.custom(activity, R.string.export_keys_dialog_title, view);
Expand All @@ -237,13 +264,28 @@ public Dialog onCreateDialog(final Bundle savedInstanceState) {
activity.finish();
});

passwordView.addTextChangedListener(textWatcher);
passwordAgainView.addTextChangedListener(textWatcher);

passwordView.addTextChangedListener(passwordTextWatcher);
passwordAgainView.addTextChangedListener(passwordTextWatcher);
spendingPINView.addTextChangedListener(spendingPINTextWatcher);
showView.setOnCheckedChangeListener(new ShowPasswordCheckListener(passwordView, passwordAgainView));

walletActivityViewModel.wallet.observe(BackupWalletDialogFragment.this,
wallet -> warningView.setVisibility(wallet.isEncrypted() ? View.VISIBLE : View.GONE));
walletActivityViewModel.wallet.observe(BackupWalletDialogFragment.this, wallet ->{
warningView.setVisibility(wallet.isEncrypted() ? View.VISIBLE : View.GONE);
spendingPINViewGroup.setVisibility(wallet.isEncrypted() ? View.VISIBLE : View.GONE);
});

viewModel.spendingPIN.observe(BackupWalletDialogFragment.this, spendingPIN -> {
badSpendingPINView.setVisibility(View.INVISIBLE);
if (positiveButton != null) {
positiveButton.setEnabled(preConditionsSatisfied());
}

});

viewModel.state.observe(BackupWalletDialogFragment.this, state -> {
updateView();
});

viewModel.password.observe(BackupWalletDialogFragment.this, password -> {
passwordMismatchView.setVisibility(View.INVISIBLE);

Expand All @@ -268,10 +310,7 @@ public Dialog onCreateDialog(final Bundle savedInstanceState) {
}

if (positiveButton != null) {
final Wallet wallet = walletActivityViewModel.wallet.getValue();
final boolean hasPassword = !password.isEmpty();
final boolean hasPasswordAgain = !passwordAgainView.getText().toString().trim().isEmpty();
positiveButton.setEnabled(wallet != null && hasPassword && hasPasswordAgain);
positiveButton.setEnabled(preConditionsSatisfied());
}
});
});
Expand All @@ -281,8 +320,9 @@ public Dialog onCreateDialog(final Bundle savedInstanceState) {

@Override
public void onDismiss(final DialogInterface dialog) {
passwordView.removeTextChangedListener(textWatcher);
passwordAgainView.removeTextChangedListener(textWatcher);
passwordView.removeTextChangedListener(passwordTextWatcher);
passwordAgainView.removeTextChangedListener(passwordTextWatcher);
spendingPINView.removeTextChangedListener(spendingPINTextWatcher);

showView.setOnCheckedChangeListener(null);

Expand All @@ -302,21 +342,106 @@ private void handleGo() {
final String passwordAgain = passwordAgainView.getText().toString().trim();

if (passwordAgain.equals(password)) {
backupWallet();
final Wallet wallet = walletActivityViewModel.wallet.getValue();
if (wallet.isEncrypted()) {
setState(BackupWalletViewModel.State.CRYPTING);
executor.execute(() -> {
final String inputPIN = spendingPINView.getText().toString().trim();

try {
backupWallet = new WalletProtobufSerializer()
.readWallet(Constants.NETWORK_PARAMETERS, null,
new WalletProtobufSerializer().walletToProto(wallet));

final KeyCrypter keyCrypter = backupWallet.getKeyCrypter();
final KeyParameter derivedAesKey = keyCrypter.deriveKey(inputPIN);
backupWallet.decrypt(derivedAesKey);
log.info("backup wallet successfully decrypted for back up");
setState(BackupWalletViewModel.State.EXPORTING);
backupWallet();

} catch (final Wallet.BadWalletEncryptionKeyException e) {
log.info("wallet decryption failed, bad spending password: " + e.getMessage());
backupWallet = null;
setState(BackupWalletViewModel.State.BADPIN);
} catch (UnreadableWalletException e) {
log.info("wallet deserialization failed: " + e.getMessage());
ErrorDialogFragment.showDialog(getParentFragmentManager(), e.toString());
}
});
} else {
setState(BackupWalletViewModel.State.EXPORTING);
backupWallet = walletActivityViewModel.wallet.getValue();
backupWallet();
backupWallet = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the removal of the reference to the decrypted wallet is needed in general. We need to trust the garbage collector anyway. If you prefer to keep it, I'd move it towards the end of the CreateDocument callback (around line 171) because that's where we're done with it.

setState(BackupWalletViewModel.State.INPUT);
}
} else {
passwordMismatchView.setVisibility(View.VISIBLE);
setState(BackupWalletViewModel.State.INPUT);
}
}

private void setState(final BackupWalletViewModel.State state) {
viewModel.state.postValue(state);
}

private void updateView() {

BackupWalletViewModel.State currentState = viewModel.state.getValue();

if (currentState == BackupWalletViewModel.State.INPUT || currentState ==BackupWalletViewModel.State.BADPIN) {
showView.setEnabled(true);
negativeButton.setEnabled(true);
positiveButton.setEnabled(preConditionsSatisfied());
positiveButton.setText(R.string.export_keys_dialog_button_export);
spendingPINView.setEnabled(true);
passwordView.setEnabled(true);
passwordAgainView.setEnabled(true);
if (currentState ==BackupWalletViewModel.State.BADPIN) {
badSpendingPINView.setVisibility(View.VISIBLE);
}
} else if (currentState == BackupWalletViewModel.State.CRYPTING) {
negativeButton.setEnabled(true);
showView.setEnabled(false);
positiveButton.setEnabled(false);
positiveButton.setText(R.string.backup_wallet_dialog_state_verifying);
spendingPINView.setEnabled(false);
passwordView.setEnabled(false);
passwordAgainView.setEnabled(false);
} else if (currentState == BackupWalletViewModel.State.EXPORTING) {
negativeButton.setEnabled(true);
positiveButton.setEnabled(false);
positiveButton.setText(R.string.backup_wallet_dialog_state_verifying);
spendingPINView.setEnabled(false);
passwordView.setEnabled(false);
passwordAgainView.setEnabled(false);
}
}

private boolean isSpendingPINPlausible() {
final Wallet wallet = walletActivityViewModel.wallet.getValue();
if (wallet == null)
return false;
if (!wallet.isEncrypted())
return true;
return !spendingPINView.getText().toString().trim().isEmpty();
}

private boolean preConditionsSatisfied() {
final Wallet wallet = walletActivityViewModel.wallet.getValue();
final boolean hasPassword = !passwordView.getText().toString().trim().isEmpty();
final boolean hasPasswordAgain = !passwordAgainView.getText().toString().trim().isEmpty();
return wallet != null && hasPassword && hasPasswordAgain && isSpendingPINPlausible();
}

private void wipePasswords() {
passwordView.setText(null);
passwordAgainView.setText(null);
spendingPINView.setText(null);
}

private void backupWallet() {
passwordView.setEnabled(false);
passwordAgainView.setEnabled(false);

final DateTimeFormatter dateFormat = DateTimeFormatter.ofPattern("yyyy-MM-dd-HH-mm");
final StringBuilder filename = new StringBuilder(Constants.Files.EXTERNAL_WALLET_BACKUP);
filename.append('-');
Expand Down
9 changes: 9 additions & 0 deletions wallet/src/de/schildbach/wallet/ui/backup/BackupWalletViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,14 @@
* @author Andreas Schildbach
*/
public class BackupWalletViewModel extends ViewModel {

public enum State {
INPUT, CRYPTING, BADPIN, EXPORTING
}


public final MutableLiveData<State> state = new MutableLiveData<>(State.INPUT);

public final MutableLiveData<String> password = new MutableLiveData<>();
public final MutableLiveData<String> spendingPIN = new MutableLiveData<>();
}