Merge dashpay#7127: feat: tidy-up mnemonic dialog by removing useless code and reducing exposure of sensitive data

b847bae fix: restore eager clearing of parsed words and improve secure memory handling (UdjinM6)
b8b0c1c fmt: fix formatting for mnemonicverificatinodialog (Konstantin Akimov)
1edec05 fix: add mnemonicverificationdialog to non-backported list (Konstantin Akimov)
f5d8b74 fix: avoid extra allocation of secured data in temporary std::string for mnemonic (Konstantin Akimov)
c7e3b46 refactor: SecureString already does zeroeing, remove duplicated code (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  Firstly, current implementation of mnemonic dialog has zeroing of SecureString's objects with std::fill which is useless, and most likely even removed by optimizing compiler.

  For reference, `SecureString`'s implementation of it, see [src/support/cleanse.cpp](https://github.com/dashpay/dash/blob/develop/src/support/cleanse.cpp) for details:

      void deallocate(T* p, std::size_t n)
      {
          if (p != nullptr) {
              memory_cleanse(p, sizeof(T) * n); // <- safe memory cleaning
          }
          LockedPoolManager::Instance().free(p);
      }

  Secondly, current implementation causes creating extra temporary object with sensitive data:

      QString mnemonicStr = QString::fromStdString(std::string(m_mnemonic.begin(), m_mnemonic.end()));

  This std::string object maybe omitted, see PR

  ## What was done?
  This PR tidy-up a bit mnemonic dialog by fixing these issues and some minor improvements for formatting.
  Though, using `memory_cleanse` should be considered to use for QStrings.

  This PR conflicts to dashpay#7126 because the same function is changed; I will prefer dashpay#7126 to be merged first because 7126 is meant to be backported.

  ## How Has This Been Tested?
  Tested as an extra changes to dashpay#7126 locally in the same branch, splitted to 2 PR after that.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)

ACKs for top commit:
  UdjinM6:
    utACK b847bae

Tree-SHA512: c843a39b49f76c6647d004e1044fd4d24ecc4a82ccc311c50d4a88989bfcd62ac1cec09079c3ac40e5efd9e3790115625e65eedb10fc03bfd6475272cb0df72e

Author: PastaPastaPasta

src/qt/mnemonicverificationdialog.cpp

@@ -78,7 +78,7 @@ MnemonicVerificationDialog::MnemonicVerificationDialog(const SecureString& mnemo
 
     // Connections
     connect(ui->showMnemonicButton, &QPushButton::clicked, this, &MnemonicVerificationDialog::onShowMnemonicClicked);
-    connect(ui->hideMnemonicButton,  &QPushButton::clicked, this, &MnemonicVerificationDialog::onHideMnemonicClicked);
+    connect(ui->hideMnemonicButton, &QPushButton::clicked, this, &MnemonicVerificationDialog::onHideMnemonicClicked);
 
     if (!m_view_only) {
         connect(ui->writtenDownCheckbox, &QCheckBox::toggled, this, [this](bool checked) {
@@ -96,8 +96,6 @@ MnemonicVerificationDialog::MnemonicVerificationDialog(const SecureString& mnemo
 
 MnemonicVerificationDialog::~MnemonicVerificationDialog()
 {
-    clearWordsSecurely();
-    clearMnemonic();
     delete ui;
 }
 
@@ -278,16 +276,15 @@ void MnemonicVerificationDialog::onHideMnemonicClicked()
     ui->hideMnemonicButton->hide();
     ui->showMnemonicButton->show();
     m_mnemonic_revealed = false;
-    // Clear words from non-secure memory immediately when hiding
-    clearWordsSecurely();
+    // Clear parsed words from memory immediately when hiding
+    m_words.clear();
 }
 
 void MnemonicVerificationDialog::reject()
 {
-    // Clear words when going back to step 1 (unless mnemonic is revealed)
-    if (!m_mnemonic_revealed) {
-        clearWordsSecurely();
-    }
+    // Eagerly clear parsed words to minimize exposure time in memory.
+    // They will be re-parsed on demand via parseWords() if needed.
+    m_words.clear();
     // close dialog for step-1; return back to step-1 for step-2
     if (ui->stackedWidget->currentIndex() == 0) {
         QDialog::reject();
@@ -310,8 +307,8 @@ bool MnemonicVerificationDialog::validateWord(const QString& word, int position)
         return false;
     }
     // Convert SecureString to QString temporarily for comparison
-    QString secureWord = QString::fromStdString(std::string(words[position - 1].begin(), words[position - 1].end()));
-    bool result = word == secureWord.toLower();
+    QString secureWord{QString::fromUtf8(words[position - 1].data(), words[position - 1].size()).toLower()};
+    const bool result{word == secureWord};
     // Clear temporary QString immediately
     secureWord.fill(QChar(0));
     secureWord.clear();
@@ -361,12 +358,6 @@ void MnemonicVerificationDialog::accept()
     QDialog::accept();
 }
 
-void MnemonicVerificationDialog::clearMnemonic()
-{
-    clearWordsSecurely();
-    m_mnemonic.assign(m_mnemonic.size(), 0);
-}
-
 std::vector<SecureString> MnemonicVerificationDialog::parseWords()
 {
     // If words are already parsed, reuse them (for step 2 validation or step 1 display)
@@ -375,19 +366,18 @@ std::vector<SecureString> MnemonicVerificationDialog::parseWords()
     }
 
     // Parse words from secure mnemonic string
-    QString mnemonicStr = QString::fromStdString(std::string(m_mnemonic.begin(), m_mnemonic.end()));
+    QString mnemonicStr{QString::fromUtf8(m_mnemonic.data(), m_mnemonic.size())};
     QStringList wordList = mnemonicStr.split(QRegularExpression("\\s+"), Qt::SkipEmptyParts);
 
     // Convert to SecureString vector for secure storage
     m_words.clear();
     m_words.reserve(wordList.size());
     for (const QString& word : wordList) {
-        std::string wordStd = word.toStdString();
+        QByteArray utf8 = word.toUtf8();
         SecureString secureWord;
-        secureWord.assign(std::string_view{wordStd});
-        m_words.push_back(secureWord);
-        // Clear temporary std::string
-        wordStd.assign(wordStd.size(), 0);
+        secureWord.assign(utf8.constData(), utf8.size());
+        m_words.push_back(std::move(secureWord));
+        utf8.fill(0);
     }
 
     // Clear the temporary QString immediately after parsing
@@ -399,23 +389,12 @@ std::vector<SecureString> MnemonicVerificationDialog::parseWords()
     return m_words;
 }
 
-void MnemonicVerificationDialog::clearWordsSecurely()
-{
-    // Securely clear each word string by overwriting before clearing
-    for (SecureString& word : m_words) {
-        // Overwrite with zeros before clearing
-        word.assign(word.size(), 0);
-        word.clear();
-    }
-    m_words.clear();
-}
-
 int MnemonicVerificationDialog::getWordCount() const
 {
     // Count words without parsing them into vector
     // This avoids storing words in non-secure memory unnecessarily
     if (m_words.empty()) {
-        QString mnemonicStr = QString::fromStdString(std::string(m_mnemonic.begin(), m_mnemonic.end()));
+        QString mnemonicStr{QString::fromUtf8(m_mnemonic.data(), m_mnemonic.size())};
         QStringList words = mnemonicStr.split(QRegularExpression("\\s+"), Qt::SkipEmptyParts);
         int count = words.size();
         // Clear immediately
@@ -480,7 +459,7 @@ void MnemonicVerificationDialog::buildMnemonicGrid(bool reveal)
         for (int c = 0; c < columns; ++c) {
             int idx = r * columns + c; if (idx >= n) break;
             // Convert SecureString to QString temporarily for display
-            QString wordStr = QString::fromStdString(std::string(words[idx].begin(), words[idx].end()));
+            QString wordStr{QString::fromUtf8(words[idx].data(), words[idx].size())};
             const QString text = QString("%1. %2").arg(idx + 1, 2).arg(wordStr);
             QLabel* lbl = new QLabel(text);
             lbl->setTextInteractionFlags(Qt::TextSelectableByMouse);
@@ -494,5 +473,3 @@ void MnemonicVerificationDialog::buildMnemonicGrid(bool reveal)
 
     m_gridLayout->setRowMinimumHeight(rows, 12);
 }
-
-

src/qt/mnemonicverificationdialog.h

@@ -41,10 +41,8 @@ private Q_SLOTS:
     void generateRandomPositions();
     void updateWordValidation();
     bool validateWord(const QString& word, int position);
-    void clearMnemonic();
     void buildMnemonicGrid(bool reveal);
     std::vector<SecureString> parseWords();
-    void clearWordsSecurely();
     int getWordCount() const;
 
     Ui::MnemonicVerificationDialog *ui;
@@ -59,4 +57,3 @@ private Q_SLOTS:
 };
 
 #endif // BITCOIN_QT_MNEMONICVERIFICATIONDIALOG_H
-

test/util/data/non-backported.txt

@@ -33,6 +33,7 @@ src/qt/governancelist.*
 src/qt/guiutil_font.*
 src/qt/masternodelist.*
 src/qt/masternodemodel.*
+src/qt/mnemonicverificationdialog.*
 src/qt/proposalmodel.*
 src/qt/proposalwizard.*
 src/rpc/coinjoin.cpp