Skip to content

Apply password during onboarding and prompt for password when needed#548

Open
johnny9 wants to merge 11 commits into
bitcoin-core:qt6from
johnny9:wallet-password
Open

Apply password during onboarding and prompt for password when needed#548
johnny9 wants to merge 11 commits into
bitcoin-core:qt6from
johnny9:wallet-password

Conversation

@johnny9
Copy link
Copy Markdown
Collaborator

@johnny9 johnny9 commented Apr 15, 2026

image

Closes #517

@johnny9 johnny9 force-pushed the wallet-password branch 2 times, most recently from fa75e31 to 4863b3e Compare April 15, 2026 18:55
@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented Apr 15, 2026

Cleaned up the commits and added unittest coverage

@johnny9 johnny9 force-pushed the wallet-password branch 5 times, most recently from 89b5190 to 6d3488a Compare May 9, 2026 15:24
@johnny9 johnny9 force-pushed the wallet-password branch from 6d3488a to 07e88a8 Compare May 11, 2026 12:41
Copy link
Copy Markdown
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

07e88a8

at migration, the dialog always shows up with error

Screencast.from.2026-05-11.10-55-04.webm

@johnny9 johnny9 force-pushed the wallet-password branch from 07e88a8 to fa67236 Compare May 11, 2026 21:17
@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented May 11, 2026

07e88a8

at migration, the dialog always shows up with error
Screencast.from.2026-05-11.10-55-04.webm

Removed the error message from showing on the first prompt

@MarnixCroes
Copy link
Copy Markdown
Contributor

fa67236

The password prompt does not show up on the send flow. Just throws an error.

image

@johnny9 johnny9 force-pushed the wallet-password branch from fa67236 to 3135d62 Compare May 12, 2026 14:30
@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented May 12, 2026

The password prompt does not show up on the send flow. Just throws an error.

In an attempt to improve on the signing flow I made the process too fragile. I have reverted back to a simpler design that aligns with the current Qt widgets flow. It should now correctly prompt and sign before moving to the review page. Then the review page will broadcast that prepared transaction when send is pressed.

@pseudoramdom
Copy link
Copy Markdown
Contributor

I realized that PR for RBF got merged before this. I had left a //FIXME: Unlock wallet before confirming (PR #548) in SpeedUpOverlay.qml.
Do you want to implement that as part of this PR or I can track that separately.

@pseudoramdom
Copy link
Copy Markdown
Contributor

Otherwise ACK 3135d62
Screenshot 2026-05-12 at 9 09 07 PM

Comment thread qml/pages/wallet/SendReview.qml Outdated
ContinueButton {
id: confirmationButton
objectName: "sendReviewSendButton"
objectName: "sendTransactionButton"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this impact any functional test? qml_test_send_receive.py:304 uses it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes, need to fix and also make sure all of the functional tests are running in CI

Copy link
Copy Markdown
Collaborator Author

@johnny9 johnny9 May 13, 2026

Choose a reason for hiding this comment

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

updated the selectors and added all of the functional tests into the workflow so they should run and catch any remaining issues.

@johnny9
Copy link
Copy Markdown
Collaborator Author

johnny9 commented May 13, 2026

I realized that PR for RBF got merged before this. I had left a //FIXME: Unlock wallet before confirming (PR #548) in SpeedUpOverlay.qml. Do you want to implement that as part of this PR or I can track that separately.

good point. I added a new issue to track this as this PR has become too large already (#692)

@johnny9 johnny9 force-pushed the wallet-password branch from 1cf486c to 4ab2785 Compare May 14, 2026 18:42
}

void WalletQmlController::startWalletMigration(const QString& path)
void WalletQmlController::startWalletMigration(const QString& path, const QString& passphrase)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like what we are currently doing now is determining that it needs a passphrase by first attempting without a passphrase, getting an error, then using that to learn that this wallet will need a passphrase

But, we should be using the isEncrypted call from the wallet to determine if the wallet migration will need a passphrase

The following diff should express what I mean:

Details
diff --git a/qml/pages/wallet/DesktopWallets.qml b/qml/pages/wallet/DesktopWallets.qml
index 1d638d850..fcab36cfd 100644
--- a/qml/pages/wallet/DesktopWallets.qml
+++ b/qml/pages/wallet/DesktopWallets.qml
@@ -66,6 +66,13 @@ Page {
             migrationRequiredPopup.errorText = ""
             migrationRequiredPopup.open()
         }
+        function onWalletMigrationPassphraseRequired(path) {
+            root.pendingMigrationPath = path
+            migrationRequiredPopup.close()
+            migrationPassphrasePopup.busy = false
+            migrationPassphrasePopup.errorText = ""
+            migrationPassphrasePopup.open()
+        }
         function onWalletMigrationSucceeded() {
             root.pendingMigrationPath = ""
             migrationRequiredPopup.close()
@@ -82,6 +89,8 @@ Page {
                 migrationPassphrasePopup.errorText = showPassphraseError ? walletController.walletMigrationError : ""
                 migrationPassphrasePopup.open()
             } else {
+                migrationPassphrasePopup.busy = false
+                migrationPassphrasePopup.close()
                 migrationRequiredPopup.busy = false
                 migrationRequiredPopup.errorText = walletController.walletMigrationError
                 migrationRequiredPopup.open()
diff --git a/qml/walletqmlcontroller.cpp b/qml/walletqmlcontroller.cpp
index 4f8381eaf..b85e94e0e 100644
--- a/qml/walletqmlcontroller.cpp
+++ b/qml/walletqmlcontroller.cpp
@@ -740,6 +740,11 @@ void WalletQmlController::startWalletMigration(const QString& path, const QStrin
         return;
     }
 
+    if (passphrase.isEmpty() && m_node.walletLoader().isEncrypted(wallet_reference.toStdString())) {
+        Q_EMIT walletMigrationPassphraseRequired(wallet_reference);
+        return;
+    }
+
     setWalletMigrationInProgress(true);
 
     QTimer::singleShot(0, m_worker, [this, wallet_reference, passphrase]() {
diff --git a/qml/walletqmlcontroller.h b/qml/walletqmlcontroller.h
index bb4a91e48..58095f327 100644
--- a/qml/walletqmlcontroller.h
+++ b/qml/walletqmlcontroller.h
@@ -94,6 +94,7 @@ Q_SIGNALS:
     void walletMigrationInProgressChanged();
     void walletMigrationErrorChanged();
     void walletMigrationRequired(const QString& path);
+    void walletMigrationPassphraseRequired(const QString& path);
     void walletMigrationSucceeded();
     void walletMigrationFailed();
     void lastImportedWalletInfoChanged();
diff --git a/test/test_walletqmlcontroller.cpp b/test/test_walletqmlcontroller.cpp
index b5d8a5b5c..1d3aaeb27 100644
--- a/test/test_walletqmlcontroller.cpp
+++ b/test/test_walletqmlcontroller.cpp
@@ -52,6 +52,7 @@ class FakeWalletLoader : public interfaces::WalletLoader
 public:
     int create_wallet_calls{0};
     int migrate_wallet_calls{0};
+    int is_encrypted_calls{0};
     int handle_load_wallet_calls{0};
     int get_wallets_calls{0};
     int list_wallet_dir_calls{0};
@@ -65,6 +66,9 @@ public:
         migrate_wallet_fn = [](const std::string&, const SecureString&) {
             return util::Error{Untranslated("Unexpected migrateWallet call")};
         };
+    std::function<bool(const std::string&)> is_encrypted_fn = [](const std::string&) {
+        return false;
+    };
     std::function<std::vector<std::unique_ptr<interfaces::Wallet>>()> get_wallets_fn = [] {
         return std::vector<std::unique_ptr<interfaces::Wallet>>{};
     };
@@ -101,7 +105,11 @@ public:
         ++migrate_wallet_calls;
         return migrate_wallet_fn(name, passphrase);
     }
-    bool isEncrypted(const std::string&) override { return false; }
+    bool isEncrypted(const std::string& name) override
+    {
+        ++is_encrypted_calls;
+        return is_encrypted_fn(name);
+    }
     std::vector<std::pair<std::string, std::string>> listWalletDir() override
     {
         ++list_wallet_dir_calls;
@@ -152,6 +160,8 @@ private Q_SLOTS:
     void selectWalletBeforeInitializationSetsLoadError();
     void initializedControllerPropagatesCreateErrors();
     void initializedControllerForwardsMigrationPassphrase();
+    void initializedControllerRequestsPassphraseBeforeEncryptedMigration();
+    void initializedControllerMigratesUnencryptedWalletWithoutPassphrase();
 };
 
 void WalletQmlControllerTests::externalSignerCreationRequiresConfiguredPath()
@@ -351,6 +361,65 @@ void WalletQmlControllerTests::initializedControllerForwardsMigrationPassphrase(
     QVERIFY(!controller.walletMigrationInProgress());
 }
 
+void WalletQmlControllerTests::initializedControllerRequestsPassphraseBeforeEncryptedMigration()
+{
+    using ::testing::StrictMock;
+
+    StrictMock<MockNode> node;
+    FakeWalletLoader loader;
+    loader.wallet_dir_entries = {{"legacy_wallet", "bdb"}};
+    loader.is_encrypted_fn = [](const std::string& name) {
+        return name == "legacy_wallet";
+    };
+    ExpectControllerInitialization(node, loader);
+
+    WalletQmlController controller(node);
+    controller.initialize();
+
+    QSignalSpy passphrase_spy(&controller, &WalletQmlController::walletMigrationPassphraseRequired);
+
+    controller.migrateWallet("legacy_wallet", "");
+    QCOMPARE(passphrase_spy.count(), 1);
+    QCOMPARE(passphrase_spy.takeFirst().at(0).toString(), QString{"legacy_wallet"});
+    QCOMPARE(loader.is_encrypted_calls, 1);
+    QCOMPARE(loader.migrate_wallet_calls, 0);
+    QVERIFY(!controller.walletMigrationInProgress());
+    QVERIFY(controller.walletMigrationError().isEmpty());
+}
+
+void WalletQmlControllerTests::initializedControllerMigratesUnencryptedWalletWithoutPassphrase()
+{
+    using ::testing::StrictMock;
+
+    StrictMock<MockNode> node;
+    FakeWalletLoader loader;
+    loader.wallet_dir_entries = {{"legacy_wallet", "bdb"}};
+    loader.is_encrypted_fn = [](const std::string&) {
+        return false;
+    };
+    ExpectControllerInitialization(node, loader);
+
+    WalletQmlController controller(node);
+    controller.initialize();
+
+    QSignalSpy failed_spy(&controller, &WalletQmlController::walletMigrationFailed);
+
+    bool saw_empty_passphrase{false};
+    loader.migrate_wallet_fn = [&](const std::string& name, const SecureString& passphrase) {
+        saw_empty_passphrase = (name == "legacy_wallet" && passphrase.empty());
+        return util::Result<interfaces::WalletMigrationResult>{
+            util::Error{Untranslated("Migration failed.")}};
+    };
+
+    controller.migrateWallet("legacy_wallet", "");
+    QVERIFY(failed_spy.wait(5000));
+    QVERIFY(saw_empty_passphrase);
+    QCOMPARE(loader.is_encrypted_calls, 1);
+    QCOMPARE(loader.migrate_wallet_calls, 1);
+    QCOMPARE(controller.walletMigrationError(), QString{"Migration failed."});
+    QVERIFY(!controller.walletMigrationInProgress());
+}
+
 #ifdef BITCOINQML_NO_TEST_MAIN
 #include <test/qt_test_registry.h>
 BITCOINQML_REGISTER_QT_TEST(WalletQmlControllerTests)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes, we need that

Comment thread qml/models/walletqmlmodel.cpp Outdated
m_current_transaction->reassignAmounts(nChangePosRet);
}
m_current_transaction->setDisplayUnit(m_display_unit);
if (relock) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

instead of the manual relocks everywhere, we should RAII this?

like so for example:

Details
diff --git a/qml/models/walletqmlmodel.cpp b/qml/models/walletqmlmodel.cpp
index 1f9a86bd8..5fff868c1 100644
--- a/qml/models/walletqmlmodel.cpp
+++ b/qml/models/walletqmlmodel.cpp
@@ -35,7 +35,9 @@
 #include <QRegularExpression>
 
 #include <array>
+#include <functional>
 #include <optional>
+#include <utility>
 
 namespace {
 constexpr unsigned int DEFAULT_STANDARD_FEE_TARGET{2};
@@ -254,6 +256,38 @@ QString LocalizedString(const bilingual_str& value)
     return QString::fromStdString(value.translated.empty() ? value.original : value.translated);
 }
 
+class WalletRelockGuard
+{
+public:
+    WalletRelockGuard(interfaces::Wallet& wallet, std::function<void()> refresh_security_state, bool active)
+        : m_wallet{wallet}, m_refresh_security_state{std::move(refresh_security_state)}, m_active{active}
+    {
+    }
+
+    ~WalletRelockGuard()
+    {
+        relock();
+    }
+
+    WalletRelockGuard(const WalletRelockGuard&) = delete;
+    WalletRelockGuard& operator=(const WalletRelockGuard&) = delete;
+
+    void relock()
+    {
+        if (!m_active) {
+            return;
+        }
+        m_wallet.lock();
+        m_refresh_security_state();
+        m_active = false;
+    }
+
+private:
+    interfaces::Wallet& m_wallet;
+    std::function<void()> m_refresh_security_state;
+    bool m_active;
+};
+
 } // namespace
 
 WalletQmlModel::WalletQmlModel(std::unique_ptr<interfaces::Wallet> wallet, QObject *parent)
@@ -694,6 +728,7 @@ bool WalletQmlModel::prepareTransactionInternal(const std::optional<QString>& pa
     if (!unlockForAction(passphrase, relock)) {
         return false;
     }
+    WalletRelockGuard relock_guard{*m_wallet, [this] { refreshSecurityState(); }, relock};
 
     const auto vec_send = BuildRecipients(*m_send_recipients);
     if (!vec_send.has_value()) {
@@ -727,10 +762,7 @@ bool WalletQmlModel::prepareTransactionInternal(const std::optional<QString>& pa
 
     CAmount balance = m_wallet->getBalance();
     if (balance < total) {
-        if (relock) {
-            m_wallet->lock();
-            refreshSecurityState();
-        }
+        relock_guard.relock();
         setTransactionStatus(tr("The wallet does not have enough balance for this transaction."));
         return false;
     }
@@ -751,18 +783,12 @@ bool WalletQmlModel::prepareTransactionInternal(const std::optional<QString>& pa
             m_current_transaction->reassignAmounts(nChangePosRet);
         }
         m_current_transaction->setDisplayUnit(m_display_unit);
-        if (relock) {
-            m_wallet->lock();
-            refreshSecurityState();
-        }
+        relock_guard.relock();
         Q_EMIT currentTransactionChanged();
         return true;
     }
 
-    if (relock) {
-        m_wallet->lock();
-        refreshSecurityState();
-    }
+    relock_guard.relock();
     setTransactionStatus(LocalizedString(util::ErrorString(result)));
     return false;
 }
diff --git a/test/test_walletqmlmodel.cpp b/test/test_walletqmlmodel.cpp
index 736b727b3..9aed28a67 100644
--- a/test/test_walletqmlmodel.cpp
+++ b/test/test_walletqmlmodel.cpp
@@ -213,6 +213,8 @@ private Q_SLOTS:
     void transactionChangedEmitsBalanceChanged();
     void prepareTransactionOnLockedWalletRequiresPassword();
     void prepareTransactionWithPrivateKeysDisabledDoesNotRequirePassword();
+    void prepareTransactionWithPassphraseRelocksWhenRecipientsInvalid();
+    void prepareTransactionWithPassphraseRelocksWhenCustomFeeInvalid();
     void sendTransactionCommitsPreparedTransactionWithoutUnlockingAgain();
     void sendTransactionWithPrivateKeysDisabledDoesNotCommit();
 };
@@ -719,6 +721,37 @@ void WalletQmlModelTests::prepareTransactionWithPrivateKeysDisabledDoesNotRequir
     QCOMPARE(wallet->lock_calls, 0);
 }
 
+void WalletQmlModelTests::prepareTransactionWithPassphraseRelocksWhenRecipientsInvalid()
+{
+    FakePasswordWallet* wallet{nullptr};
+    auto model = MakeWalletModel(wallet);
+    auto* recipient = model->sendRecipientList()->currentRecipient();
+    QVERIFY(recipient != nullptr);
+    recipient->amount()->setSatoshi(1'000);
+    QVERIFY(!recipient->isValid());
+
+    QVERIFY(!model->prepareTransactionWithPassphrase("secret"));
+    QVERIFY(wallet->locked);
+    QCOMPARE(wallet->unlock_calls, 1);
+    QCOMPARE(wallet->lock_calls, 1);
+    QVERIFY(wallet->create_transaction_sign_args.empty());
+}
+
+void WalletQmlModelTests::prepareTransactionWithPassphraseRelocksWhenCustomFeeInvalid()
+{
+    FakePasswordWallet* wallet{nullptr};
+    auto model = MakeWalletModel(wallet);
+    SetPasswordRecipient(*model, 1'000);
+    model->setCustomFeeEnabled(true);
+    model->setCustomFeeRate(QStringLiteral("not-a-fee"));
+
+    QVERIFY(!model->prepareTransactionWithPassphrase("secret"));
+    QVERIFY(wallet->locked);
+    QCOMPARE(wallet->unlock_calls, 1);
+    QCOMPARE(wallet->lock_calls, 1);
+    QVERIFY(wallet->create_transaction_sign_args.empty());
+}
+
 void WalletQmlModelTests::sendTransactionCommitsPreparedTransactionWithoutUnlockingAgain()
 {
     FakePasswordWallet* wallet{nullptr};

@johnny9 johnny9 requested a review from jarolrod May 14, 2026 22:03
implicitHeight: columnLayout.implicitHeight
anchors.centerIn: parent

onOpened: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to be more secure about the lifetime of the passphrase and how we treat it.

This is worth tightening as a security fix. The passphrase popup currently emits passphraseField.text directly and leaves the hidden CoreTextField populated until the popup is reopened or destroyed. That means the passphrase remains in memory after migration completed.

We should treat it as sensitive information and use SecureString, then overwrite and clear the UI immediately, and also clear again on close for redundancy.

Here is a diff that should accomplish this (without tests)

Details
diff --git a/qml/components/WalletPassphrasePopup.qml b/qml/components/WalletPassphrasePopup.qml
index 3539d95f4..f66dc6304 100644
--- a/qml/components/WalletPassphrasePopup.qml
+++ b/qml/components/WalletPassphrasePopup.qml
@@ -32,10 +32,25 @@ Popup {
     implicitHeight: columnLayout.implicitHeight
     anchors.centerIn: parent
 
+    function clearPassphraseField() {
+        if (passphraseField.text.length > 0) {
+            passphraseField.text = Array(passphraseField.text.length + 1).join(" ")
+            passphraseField.text = ""
+        }
+    }
+
+    function submitPassphrase() {
+        const passphrase = passphraseField.text
+        clearPassphraseField()
+        root.submitted(passphrase)
+    }
+
     onOpened: {
-        passphraseField.text = ""
+        clearPassphraseField()
         passphraseField.forceActiveFocus()
     }
+    onClosed: clearPassphraseField()
+    Component.onDestruction: clearPassphraseField()
 
     background: Rectangle {
         color: Theme.color.background
@@ -116,7 +131,7 @@ Popup {
                 Layout.minimumWidth: 120
                 enabled: !root.busy && passphraseField.text.length > 0
                 text: root.busy ? root.busyConfirmText : root.confirmText
-                onClicked: root.submitted(passphraseField.text)
+                onClicked: root.submitPassphrase()
             }
         }
     }
diff --git a/qml/models/walletqmlmodel.cpp b/qml/models/walletqmlmodel.cpp
index 5fff868c1..4abeea542 100644
--- a/qml/models/walletqmlmodel.cpp
+++ b/qml/models/walletqmlmodel.cpp
@@ -30,6 +30,7 @@
 #include <wallet/coincontrol.h>
 #include <wallet/wallet.h>
 
+#include <QByteArray>
 #include <QDateTime>
 #include <QMetaObject>
 #include <QRegularExpression>
@@ -256,6 +257,26 @@ QString LocalizedString(const bilingual_str& value)
     return QString::fromStdString(value.translated.empty() ? value.original : value.translated);
 }
 
+SecureString SecureStringFromQString(const QString& value)
+{
+    QByteArray bytes{value.toUtf8()};
+    SecureString secure;
+    secure.assign(bytes.constData(), bytes.constData() + bytes.size());
+    if (!bytes.isEmpty()) {
+        memory_cleanse(bytes.data(), static_cast<size_t>(bytes.size()));
+    }
+    return secure;
+}
+
+void ClearSecureString(SecureString& value)
+{
+    if (value.empty()) {
+        return;
+    }
+    memory_cleanse(value.data(), value.size());
+    value.clear();
+}
+
 class WalletRelockGuard
 {
 public:
@@ -708,10 +729,10 @@ bool WalletQmlModel::prepareTransaction()
 
 bool WalletQmlModel::prepareTransactionWithPassphrase(const QString& passphrase)
 {
-    return prepareTransactionInternal(passphrase);
+    return prepareTransactionInternal(std::optional<SecureString>{SecureStringFromQString(passphrase)});
 }
 
-bool WalletQmlModel::prepareTransactionInternal(const std::optional<QString>& passphrase)
+bool WalletQmlModel::prepareTransactionInternal(std::optional<SecureString> passphrase)
 {
     clearTransactionStatus();
     if (!m_wallet || !m_send_recipients || m_send_recipients->recipients().empty()) {
@@ -1057,18 +1078,24 @@ void WalletQmlModel::refreshSecurityState()
     }
 }
 
-bool WalletQmlModel::unlockForAction(const std::optional<QString>& passphrase, bool& relock)
+bool WalletQmlModel::unlockForAction(std::optional<SecureString>& passphrase, bool& relock)
 {
     relock = false;
     if (!m_wallet || !m_wallet->isCrypted() || !m_wallet->isLocked()) {
+        if (passphrase.has_value()) {
+            ClearSecureString(*passphrase);
+            passphrase.reset();
+        }
         return true;
     }
     if (!passphrase.has_value()) {
         return true;
     }
 
-    const SecureString secure_passphrase{passphrase->toStdString()};
-    if (!m_wallet->unlock(secure_passphrase)) {
+    const bool unlocked = m_wallet->unlock(*passphrase);
+    ClearSecureString(*passphrase);
+    passphrase.reset();
+    if (!unlocked) {
         setTransactionStatus(tr("The wallet password you entered was incorrect."));
         return false;
     }
diff --git a/qml/models/walletqmlmodel.h b/qml/models/walletqmlmodel.h
index 45951f93b..fe5077d3d 100644
--- a/qml/models/walletqmlmodel.h
+++ b/qml/models/walletqmlmodel.h
@@ -15,6 +15,7 @@
 #include <consensus/amount.h>
 #include <interfaces/handler.h>
 #include <interfaces/wallet.h>
+#include <support/allocators/secure.h>
 #include <wallet/coincontrol.h>
 
 #include <memory>
@@ -154,9 +155,9 @@ private:
     void subscribeToWalletSignals();
     void unsubscribeFromWalletSignals();
     void refreshSecurityState();
-    bool prepareTransactionInternal(const std::optional<QString>& passphrase);
+    bool prepareTransactionInternal(std::optional<SecureString> passphrase);
     bool sendTransactionInternal();
-    bool unlockForAction(const std::optional<QString>& passphrase, bool& relock);
+    bool unlockForAction(std::optional<SecureString>& passphrase, bool& relock);
     void clearTransactionStatus();
     void setTransactionStatus(const QString& error, bool needs_unlock = false);
 
diff --git a/qml/walletqmlcontroller.cpp b/qml/walletqmlcontroller.cpp
index b85e94e0e..f052231f0 100644
--- a/qml/walletqmlcontroller.cpp
+++ b/qml/walletqmlcontroller.cpp
@@ -18,7 +18,9 @@
 #include <util/threadnames.h>
 
 #include <stdexcept>
+#include <utility>
 
+#include <QByteArray>
 #include <QDir>
 #include <QFileInfo>
 #include <QMetaObject>
@@ -39,6 +41,26 @@ QString JoinWarnings(const std::vector<bilingual_str>& warnings)
     return lines.join('\n');
 }
 
+SecureString SecureStringFromQString(const QString& value)
+{
+    QByteArray bytes{value.toUtf8()};
+    SecureString secure;
+    secure.assign(bytes.constData(), bytes.constData() + bytes.size());
+    if (!bytes.isEmpty()) {
+        memory_cleanse(bytes.data(), static_cast<size_t>(bytes.size()));
+    }
+    return secure;
+}
+
+void ClearSecureString(SecureString& value)
+{
+    if (value.empty()) {
+        return;
+    }
+    memory_cleanse(value.data(), value.size());
+    value.clear();
+}
+
 bool IsBackupLikeFile(const QFileInfo& file_info)
 {
     const QString file_name = file_info.fileName().toLower();
@@ -145,9 +167,10 @@ bool WalletQmlController::createSingleSigWallet(const QString &name, const QStri
         setWalletCreateError(tr("Wallets are still loading. Try again in a moment."));
         return false;
     }
-    const SecureString secure_passphrase{passphrase.toStdString()};
+    SecureString secure_passphrase{SecureStringFromQString(passphrase)};
     const std::string wallet_name{name.toStdString()};
     auto wallet{m_node.walletLoader().createWallet(wallet_name, secure_passphrase, wallet::WALLET_FLAG_DESCRIPTORS, m_warning_messages)};
+    ClearSecureString(secure_passphrase);
     setWalletLoadWarnings(JoinWarnings(m_warning_messages));
     QMutexLocker locker(&m_wallets_mutex);
     if (wallet) {
@@ -262,7 +285,7 @@ void WalletQmlController::migrateWallet(const QString& path, const QString& pass
         setWalletMigrationError(tr("Wallets are still loading. Try again in a moment."));
         return;
     }
-    startWalletMigration(path, passphrase);
+    startWalletMigration(path, SecureStringFromQString(passphrase));
 }
 
 void WalletQmlController::clearWalletMigrationStatus()
@@ -722,12 +745,13 @@ void WalletQmlController::startWalletLoad(const QString& path, const QString& wa
     });
 }
 
-void WalletQmlController::startWalletMigration(const QString& path, const QString& passphrase)
+void WalletQmlController::startWalletMigration(const QString& path, SecureString passphrase)
 {
     clearWalletLoadStatus();
     clearWalletMigrationStatus();
 
     if (path.trimmed().isEmpty()) {
+        ClearSecureString(passphrase);
         setWalletMigrationError(tr("Choose a wallet to update."));
         Q_EMIT walletMigrationFailed();
         return;
@@ -735,21 +759,22 @@ void WalletQmlController::startWalletMigration(const QString& path, const QStrin
 
     const QString wallet_reference = resolveManagedWalletReference(path);
     if (wallet_reference.isEmpty()) {
+        ClearSecureString(passphrase);
         setWalletMigrationError(tr("The selected wallet is not available in the wallet directory."));
         Q_EMIT walletMigrationFailed();
         return;
     }
 
-    if (passphrase.isEmpty() && m_node.walletLoader().isEncrypted(wallet_reference.toStdString())) {
+    if (passphrase.empty() && m_node.walletLoader().isEncrypted(wallet_reference.toStdString())) {
         Q_EMIT walletMigrationPassphraseRequired(wallet_reference);
         return;
     }
 
     setWalletMigrationInProgress(true);
 
-    QTimer::singleShot(0, m_worker, [this, wallet_reference, passphrase]() {
-        const SecureString secure_passphrase{passphrase.toStdString()};
-        auto result = m_node.walletLoader().migrateWallet(wallet_reference.toStdString(), secure_passphrase);
+    QTimer::singleShot(0, m_worker, [this, wallet_reference, passphrase = std::move(passphrase)]() mutable {
+        auto result = m_node.walletLoader().migrateWallet(wallet_reference.toStdString(), passphrase);
+        ClearSecureString(passphrase);
 
         if (!result) {
             const QString error = QString::fromStdString(util::ErrorString(result).translated);
diff --git a/qml/walletqmlcontroller.h b/qml/walletqmlcontroller.h
index 58095f327..f0a30cd30 100644
--- a/qml/walletqmlcontroller.h
+++ b/qml/walletqmlcontroller.h
@@ -10,6 +10,7 @@
 #include <interfaces/handler.h>
 #include <interfaces/node.h>
 #include <interfaces/wallet.h>
+#include <support/allocators/secure.h>
 
 #include <memory>
 
@@ -114,7 +115,7 @@ private:
     void handleLoadWallet(std::unique_ptr<interfaces::Wallet> wallet);
     void startWalletImport(const QString& path);
     void startWalletLoad(const QString& path, const QString& wallet_format = QString());
-    void startWalletMigration(const QString& path, const QString& passphrase);
+    void startWalletMigration(const QString& path, SecureString passphrase);
     QString resolveManagedWalletReference(const QString& path, QString* wallet_format = nullptr) const;
     QString inferWalletLoadTarget(const QString& normalized_path) const;
     QString inferRestoreWalletName(const QString& normalized_path) const;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legacy Wallet Migration Flow

5 participants