From: tobtoht Date: Fri, 8 Mar 2024 16:35:57 +0000 (+0100) Subject: receive: improve wallet file corruption detection X-Git-Url: https://git.nutra.tk/v1?a=commitdiff_plain;h=f97fa90521a1a098d34dac4fdb4f2213e3f14d38;p=gamesguru%2Ffeather.git receive: improve wallet file corruption detection --- diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp index decf6e5f..0f4047d7 100644 --- a/src/MainWindow.cpp +++ b/src/MainWindow.cpp @@ -1591,7 +1591,12 @@ void MainWindow::onInitiateTransaction() { void MainWindow::onKeysCorrupted() { if (!m_criticalWarningShown) { m_criticalWarningShown = true; - Utils::showError(this, "Wallet keys are corrupted", "WARNING!\n\nTo prevent LOSS OF FUNDS do NOT continue to use this wallet file.\n\nRestore your wallet from seed.\n\nPlease report this incident to the Feather developers.\n\nWARNING!"); + Utils::showError(this, "Potential wallet file corruption detected", + "WARNING!\n\n" + "To prevent LOSS OF FUNDS do NOT continue to use this wallet file.\n\n" + "Restore your wallet from seed, keys, or device.\n\n" + "Please report this incident to the Feather developers.\n\n" + "WARNING!", {}, "report_an_issue"); m_sendWidget->disallowSending(); } } diff --git a/src/ReceiveWidget.cpp b/src/ReceiveWidget.cpp index edd4f103..c57e5c9f 100644 --- a/src/ReceiveWidget.cpp +++ b/src/ReceiveWidget.cpp @@ -30,7 +30,7 @@ ReceiveWidget::ReceiveWidget(Wallet *wallet, QWidget *parent) ui->addresses->header()->setSectionResizeMode(SubaddressModel::Address, QHeaderView::ResizeToContents); ui->addresses->header()->setSectionResizeMode(SubaddressModel::Label, QHeaderView::Stretch); - connect(ui->addresses->selectionModel(), &QItemSelectionModel::currentChanged, [=](QModelIndex current, QModelIndex prev){ + connect(ui->addresses->selectionModel(), &QItemSelectionModel::selectionChanged, [=](const QItemSelection &selected, const QItemSelection &deselected){ this->updateQrCode(); }); connect(m_model, &SubaddressModel::modelReset, [this](){ @@ -74,12 +74,12 @@ ReceiveWidget::ReceiveWidget(Wallet *wallet, QWidget *parent) m_showTransactionsAction = new QAction("Show transactions"); connect(m_showTransactionsAction, &QAction::triggered, this, &ReceiveWidget::onShowTransactions); connect(ui->addresses, &QTreeView::customContextMenuRequested, this, &ReceiveWidget::showContextMenu); - - connect(ui->btn_generateSubaddress, &QPushButton::clicked, this, &ReceiveWidget::generateSubaddress); + connect(ui->addresses, &SubaddressView::copyAddress, this, &ReceiveWidget::copyAddress); connect(ui->qrCode, &ClickableLabel::clicked, this, &ReceiveWidget::showQrCodeDialog); connect(ui->search, &QLineEdit::textChanged, this, &ReceiveWidget::setSearchFilter); + connect(ui->btn_generateSubaddress, &QPushButton::clicked, this, &ReceiveWidget::generateSubaddress); connect(ui->btn_createPaymentRequest, &QPushButton::clicked, this, &ReceiveWidget::createPaymentRequest); } @@ -104,9 +104,32 @@ void ReceiveWidget::focusSearchbar() { ui->search->setFocus(); } +QString ReceiveWidget::getAddress(quint32 minorIndex) { + bool ok; + QString reason; + QString address = m_wallet->getAddressSafe(m_wallet->currentSubaddressAccount(), minorIndex, ok, reason); + + if (!ok) { + Utils::showError(this, "Unable to get address", + QString("Reason: %1\n\n" + "WARNING!\n\n" + "Potential wallet file corruption detected.\n\n" + "To prevent LOSS OF FUNDS do NOT continue to use this wallet file.\n\n" + "Restore your wallet from seed, keys, or device.\n\n" + "Please report this incident to the Feather developers.\n\n" + "WARNING!").arg(reason), {}, "report_an_issue"); + return {}; + } + + return address; +} + void ReceiveWidget::copyAddress() { - QModelIndex index = ui->addresses->currentIndex(); - Utils::copyColumn(&index, SubaddressModel::Address); + SubaddressRow* row = this->currentEntry(); + if (!row) return; + + QString address = this->getAddress(row->getRow()); + Utils::copyToClipboard(address); } void ReceiveWidget::copyLabel() { @@ -115,7 +138,7 @@ void ReceiveWidget::copyLabel() { } void ReceiveWidget::editLabel() { - QModelIndex index = ui->addresses->currentIndex().siblingAtColumn(m_model->ModelColumn::Label); + QModelIndex index = ui->addresses->currentIndex().siblingAtColumn(SubaddressModel::ModelColumn::Label); ui->addresses->setCurrentIndex(index); ui->addresses->edit(index); } @@ -164,24 +187,20 @@ void ReceiveWidget::showContextMenu(const QPoint &point) { } void ReceiveWidget::createPaymentRequest() { - QModelIndex index = ui->addresses->currentIndex(); - if (!index.isValid()) { - return; - } + SubaddressRow* row = this->currentEntry(); + if (!row) return; - QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString(); + QString address = this->getAddress(row->getRow()); PaymentRequestDialog dialog{this, m_wallet, address}; dialog.exec(); } void ReceiveWidget::onShowTransactions() { - QModelIndex index = ui->addresses->currentIndex(); - if (!index.isValid()) { - return; - } + SubaddressRow* row = this->currentEntry(); + if (!row) return; - QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString(); + QString address = this->getAddress(row->getRow()); emit showTransactions(address); } @@ -209,14 +228,14 @@ void ReceiveWidget::generateSubaddress() { } void ReceiveWidget::updateQrCode(){ - QModelIndex index = ui->addresses->currentIndex(); - if (!index.isValid()) { + SubaddressRow* row = this->currentEntry(); + if (!row) { ui->qrCode->clear(); ui->btn_createPaymentRequest->hide(); return; } - QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString(); + QString address = this->getAddress(row->getRow()); const QrCode qrc(address, QrCode::Version::AUTO, QrCode::ErrorCorrectionLevel::MEDIUM); int width = ui->qrCode->width() - 4; @@ -227,11 +246,10 @@ void ReceiveWidget::updateQrCode(){ } void ReceiveWidget::showQrCodeDialog() { - QModelIndex index = ui->addresses->currentIndex(); - if (!index.isValid()) { - return; - } - QString address = index.model()->data(index.siblingAtColumn(SubaddressModel::Address), Qt::UserRole).toString(); + SubaddressRow* row = this->currentEntry(); + if (!row) return; + + QString address = this->getAddress(row->getRow()); QrCode qr(address, QrCode::Version::AUTO, QrCode::ErrorCorrectionLevel::HIGH); QrCodeDialog dialog{this, &qr, "Address"}; dialog.exec(); diff --git a/src/ReceiveWidget.h b/src/ReceiveWidget.h index 699b514a..43be717b 100644 --- a/src/ReceiveWidget.h +++ b/src/ReceiveWidget.h @@ -31,6 +31,7 @@ public: void focusSearchbar(); public slots: + QString getAddress(quint32 minorIndex); void copyAddress(); void copyLabel(); void editLabel(); diff --git a/src/libwalletqt/Subaddress.cpp b/src/libwalletqt/Subaddress.cpp index 2e89c579..d5a5bbcd 100644 --- a/src/libwalletqt/Subaddress.cpp +++ b/src/libwalletqt/Subaddress.cpp @@ -120,33 +120,53 @@ bool Subaddress::refresh(quint32 accountIndex) emit refreshStarted(); this->clearRows(); - for (qsizetype i = 0; i < m_wallet2->get_num_subaddresses(accountIndex); ++i) + + bool potentialWalletFileCorruption = false; + + for (quint32 i = 0; i < m_wallet2->get_num_subaddresses(accountIndex); ++i) { - QString address = QString::fromStdString(m_wallet2->get_subaddress_as_str({accountIndex, (uint32_t)i})); - + cryptonote::subaddress_index index = {accountIndex, i}; + cryptonote::account_public_address address = m_wallet2->get_subaddress(index); + + // Make sure we have previously generated Di + auto idx = m_wallet2->get_subaddress_index(address); + if (!idx) { + potentialWalletFileCorruption = true; + break; + } + + // Verify mapping + if (idx != index) { + potentialWalletFileCorruption = true; + break; + } + + QString addressStr = QString::fromStdString(cryptonote::get_account_address_as_str(m_wallet2->nettype(), !index.is_zero(), address)); + auto* row = new SubaddressRow{this, i, - address, - QString::fromStdString(m_wallet2->get_subaddress_label({accountIndex, (uint32_t)i})), + addressStr, + QString::fromStdString(m_wallet2->get_subaddress_label(index)), m_wallet2->get_subaddress_used({accountIndex, (uint32_t)i}), - this->isHidden(address), - this->isPinned(address) + this->isHidden(addressStr), + this->isPinned(addressStr) }; m_rows.append(row); } // Make sure keys are intact. We NEVER want to display incorrect addresses in case of memory corruption. - bool keysCorrupt = m_wallet2->get_device_type() == hw::device::SOFTWARE && !m_wallet2->verify_keys(); + potentialWalletFileCorruption = potentialWalletFileCorruption || (m_wallet2->get_device_type() == hw::device::SOFTWARE && !m_wallet2->verify_keys()); - if (keysCorrupt) { - clearRows(); + if (potentialWalletFileCorruption) { LOG_ERROR("KEY INCONSISTENCY DETECTED, WALLET IS IN CORRUPT STATE."); + clearRows(); + emit corrupted(); } emit refreshFinished(); - return !keysCorrupt; + return !potentialWalletFileCorruption; } qsizetype Subaddress::count() const diff --git a/src/libwalletqt/Subaddress.h b/src/libwalletqt/Subaddress.h index 7007baa9..fe7571ca 100644 --- a/src/libwalletqt/Subaddress.h +++ b/src/libwalletqt/Subaddress.h @@ -46,6 +46,7 @@ public: signals: void refreshStarted() const; void refreshFinished() const; + void corrupted() const; private: explicit Subaddress(Wallet *wallet, tools::wallet2 *wallet2, QObject *parent); diff --git a/src/libwalletqt/Wallet.cpp b/src/libwalletqt/Wallet.cpp index 1b485e15..bf6f1c58 100644 --- a/src/libwalletqt/Wallet.cpp +++ b/src/libwalletqt/Wallet.cpp @@ -82,6 +82,10 @@ Wallet::Wallet(Monero::Wallet *wallet, QObject *parent) connect(this, &Wallet::updated, this, &Wallet::onUpdated); connect(this, &Wallet::heightsRefreshed, this, &Wallet::onHeightsRefreshed); connect(this, &Wallet::transactionCommitted, this, &Wallet::onTransactionCommitted); + + connect(m_subaddress, &Subaddress::corrupted, [this]{ + emit keysCorrupted(); + }); } // #################### Status #################### @@ -185,6 +189,85 @@ QString Wallet::address(quint32 accountIndex, quint32 addressIndex) const { return QString::fromStdString(m_wallet2->get_subaddress_as_str({accountIndex, addressIndex})); } +QString Wallet::getAddressSafe(quint32 accountIndex, quint32 addressIndex, bool &ok, QString &reason) const { + ok = false; + + // If we copy an address to clipboard or create a QR code, there must not be a spark of doubt that + // the address belongs to our wallet. + + // This function does a number of sanity checks, some seemingly unnecessary or redundant to ensure + // that, yes, we own this address. It provides basic protection against memory corruption errors. + + if (accountIndex >= this->numSubaddressAccounts()) { + reason = "Account index exceeds number of pre-computed subaddress accounts"; + return {}; + } + + if (addressIndex >= this->numSubaddresses(accountIndex)) { + reason = "Address index exceeds number of pre-computed subaddresses"; + return {}; + } + + // Realistically, nobody will have more than 1M subaddresses or accounts in their wallet. + if (accountIndex >= 1000000) { + reason = "Account index exceeds safety limit"; + return {}; + } + + if (addressIndex >= 1000000) { + reason = "Address index exceeds safety limit"; + return {}; + } + + // subaddress public spendkey (Di) = Hs(secret viewkey || subaddress index)G + primary address public spendkey (B) + // subaddress public viewkey (Ci) = D * secret viewkey (a) + + if (!m_wallet2->verify_keys()) { + reason = "Unable to verify viewkey"; + return {}; + } + + cryptonote::subaddress_index index = {accountIndex, addressIndex}; + cryptonote::account_public_address address = m_wallet2->get_subaddress(index); + + // Make sure we have previously generated Di + auto idx = m_wallet2->get_subaddress_index(address); + if (!idx) { + reason = "No mapping found for this subaddress public spendkey"; + return {}; + } + + // Verify mapping + if (idx != index) { + reason = "Invalid subaddress public spendkey mapping"; + return {}; + } + + // Recompute address + cryptonote::account_public_address address2 = m_wallet2->get_subaddress(idx.value()); + if (address != address2) { + reason = "Recomputed address does not match original address"; + return {}; + } + + std::string address_str = m_wallet2->get_subaddress_as_str(index); + + // Make sure address is parseable + cryptonote::address_parse_info info; + if (!cryptonote::get_account_address_from_str(info, m_wallet2->nettype(), address_str)) { + reason = "Unable to parse address"; + return {}; + } + + if (info.address != address) { + reason = "Parsed address does not match original address"; + return {}; + } + + ok = true; + return QString::fromStdString(address_str); +} + SubaddressIndex Wallet::subaddressIndex(const QString &address) const { std::pair i; if (!m_walletImpl->subaddressIndex(address.toStdString(), i)) { @@ -479,14 +562,7 @@ void Wallet::onRefreshed(bool success, const QString &message) { void Wallet::refreshModels() { m_history->refresh(); m_coins->refresh(); - bool r = this->subaddress()->refresh(this->currentSubaddressAccount()); - - if (!r) { - // This should only happen if wallet keys got corrupted or were tampered with - // The list of subaddresses is wiped to prevent loss of funds - // Notify MainWindow to display an error message - emit keysCorrupted(); - } + this->subaddress()->refresh(this->currentSubaddressAccount()); } // #################### Hardware wallet #################### diff --git a/src/libwalletqt/Wallet.h b/src/libwalletqt/Wallet.h index efe82a27..d24e4020 100644 --- a/src/libwalletqt/Wallet.h +++ b/src/libwalletqt/Wallet.h @@ -154,8 +154,8 @@ public: void updateBalance(); // ##### Subaddresses and Accounts ##### - //! returns wallet's public address QString address(quint32 accountIndex, quint32 addressIndex) const; + QString getAddressSafe(quint32 accountIndex, quint32 addressIndex, bool &ok, QString &reason) const; //! returns the subaddress index of the address SubaddressIndex subaddressIndex(const QString &address) const; diff --git a/src/libwalletqt/rows/SubaddressRow.cpp b/src/libwalletqt/rows/SubaddressRow.cpp index d60d1173..e47cee71 100644 --- a/src/libwalletqt/rows/SubaddressRow.cpp +++ b/src/libwalletqt/rows/SubaddressRow.cpp @@ -3,7 +3,7 @@ #include "SubaddressRow.h" -qsizetype SubaddressRow::getRow() const { +quint32 SubaddressRow::getRow() const { return m_row; } diff --git a/src/libwalletqt/rows/SubaddressRow.h b/src/libwalletqt/rows/SubaddressRow.h index fb634c3b..f48e8b50 100644 --- a/src/libwalletqt/rows/SubaddressRow.h +++ b/src/libwalletqt/rows/SubaddressRow.h @@ -11,7 +11,7 @@ class SubaddressRow : public QObject Q_OBJECT public: - SubaddressRow(QObject *parent, qsizetype row, const QString& address, const QString &label, bool used, bool hidden, bool pinned) + SubaddressRow(QObject *parent, quint32 row, const QString& address, const QString &label, bool used, bool hidden, bool pinned) : QObject(parent) , m_row(row) , m_address(address) @@ -20,7 +20,7 @@ public: , m_hidden(hidden) , m_pinned(pinned) {} - qsizetype getRow() const; + [[nodiscard]] quint32 getRow() const; const QString& getAddress() const; const QString& getLabel() const; bool isUsed() const; @@ -28,7 +28,7 @@ public: bool isPinned() const; private: - qsizetype m_row; + quint32 m_row; QString m_address; QString m_label; bool m_used = false; diff --git a/src/model/SubaddressView.cpp b/src/model/SubaddressView.cpp index 1fe4a9b9..54565b8a 100644 --- a/src/model/SubaddressView.cpp +++ b/src/model/SubaddressView.cpp @@ -4,6 +4,7 @@ #include "SubaddressView.h" #include "utils/Utils.h" +#include "SubaddressModel.h" SubaddressView::SubaddressView(QWidget *parent) : QTreeView(parent) { @@ -15,7 +16,11 @@ void SubaddressView::keyPressEvent(QKeyEvent *event){ if(!selectedIndexes().isEmpty()){ if(event->matches(QKeySequence::Copy)){ QModelIndex index = this->currentIndex(); - Utils::copyColumn(&index, index.column()); + if (index.column() == SubaddressModel::ModelColumn::Address) { + emit copyAddress(); + } else { + Utils::copyColumn(&index, index.column()); + } } else QTreeView::keyPressEvent(event); diff --git a/src/model/SubaddressView.h b/src/model/SubaddressView.h index 5a4ba178..a89a60ac 100644 --- a/src/model/SubaddressView.h +++ b/src/model/SubaddressView.h @@ -10,10 +10,14 @@ class SubaddressView : public QTreeView { +Q_OBJECT public: SubaddressView(QWidget* parent = nullptr); +signals: + void copyAddress(); + protected: void keyPressEvent(QKeyEvent *event); };