feat: tidy-up mnemonic dialog by removing useless code and reducing exposure of sensitive data#7127
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved private helpers clearMnemonic() and clearWordsSecurely() and their declarations. Mnemonic handling was migrated from std::string-based conversions to UTF-8-based QString/fromUtf8 paths across parsing, counting, grid building, display, and validation. Memory handling was adjusted: m_words is cleared eagerly in some flows, temporaries (QString/std::string) are cleared immediately after use, and SecureString words are constructed from UTF-8 bytes. Revealed grid now parses words via UTF-8; hidden view shows placeholder bullets. The dialog source path was added to the non-backported data list. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/qt/mnemonicverificationdialog.cpp (2)
298-314:⚠️ Potential issue | 🟡 MinorRemove redundant toLower() call.
Line 309 calls
toLower()onsecureWordwhich is already lowercased from line 308. Qt 5'sQString::toLower()returns a new allocated copy, creating an uncleared temporary containing sensitive data. Compare directly against the already-lowercasedsecureWord.🔧 Proposed fix
- QString secureWord{QString::fromUtf8(words[position - 1].data(), words[position - 1].size()).toLower()}; - const bool result{word == secureWord.toLower()}; + QString secureWord{QString::fromUtf8(words[position - 1].data(), words[position - 1].size()).toLower()}; + const bool result{word == secureWord};
392-405:⚠️ Potential issue | 🟡 MinorClear mnemonicStr buffer before releasing.
In Qt 5,
QString::clear()does not overwrite the buffer contents before deallocation—the UTF-16 data remains in freed heap memory until reused by the allocator. Usefill(QChar(0))beforeclear()to overwrite the buffer with null characters and reduce exposure of mnemonic text.🔧 Proposed fix
// Clear immediately - mnemonicStr.clear(); + mnemonicStr.fill(QChar(0)); + mnemonicStr.clear(); mnemonicStr.squeeze();
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-mnemonic-allocations
branch
from
660e89b to
df28c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/qt/mnemonicverificationdialog.cpp (2)
308-313:⚠️ Potential issue | 🟡 MinorRedundant
.toLower()creates an uncleared temporary containing sensitive data.
secureWordis already lowered on line 308 via.toLower(). Calling.toLower()again on line 309 is redundant and, more importantly, produces a temporaryQStringthat holds the mnemonic word but is never zeroed — contradicting the PR's goal of reducing sensitive data exposure.Proposed fix
QString secureWord{QString::fromUtf8(words[position - 1].data(), words[position - 1].size()).toLower()}; - const bool result{word == secureWord.toLower()}; + const bool result{word == secureWord};
396-404:⚠️ Potential issue | 🟡 MinorMissing
fill(QChar(0))leaves mnemonic data in freed memory.In
parseWords()(line 384),mnemonicStris zeroed withfill(QChar(0))beforeclear()/squeeze(). Here ingetWordCount(), the same sensitive mnemonic string is only cleared and squeezed — the underlying buffer is released without being zeroed first.Apply the same sanitization pattern for consistency.
Proposed fix
int count = words.size(); // Clear immediately + mnemonicStr.fill(QChar(0)); mnemonicStr.clear(); mnemonicStr.squeeze(); + for (auto& w : words) { + w.fill(QChar(0)); + } words.clear(); return count;
🧹 Nitpick comments (1)
src/qt/mnemonicverificationdialog.cpp (1)
380-381:wordStd.assign(size, 0)is a dead store the compiler may optimize away.Unlike
memory_cleanse(which uses volatile orOPENSSL_cleanseto resist optimization), a plainassignof zeros towordStdright before it goes out of scope is a classic dead-store that an optimizing compiler can elide. Additionally, this only clears up tosize(), notcapacity(), so remnants of earlier (possibly longer) words may linger in the slack buffer.Consider using
memory_cleansehere for consistency with the secure-deallocation guarantees the PR relies on forSecureString.Proposed fix
// Clear temporary std::string - wordStd.assign(wordStd.size(), 0); + memory_cleanse(wordStd.data(), wordStd.size()); + wordStd.clear(); + wordStd.shrink_to_fit();
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-mnemonic-allocations
branch
from
df28c17 to
540aca7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/mnemonicverificationdialog.cpp (1)
393-401:⚠️ Potential issue | 🟡 MinorInconsistent secure cleanup:
mnemonicStris not zeroed before clearing.In
parseWords()(line 381),mnemonicStr.fill(QChar(0))is called beforeclear()/squeeze()to wipe the sensitive content. Here ingetWordCount(), thefillstep is missing —clear()only resets the logical length, leaving the mnemonic text in the underlying buffer until the allocator reuses it.Proposed fix
int count = words.size(); // Clear immediately + mnemonicStr.fill(QChar(0)); mnemonicStr.clear(); mnemonicStr.squeeze(); words.clear();
🤖 Fix all issues with AI agents
In `@src/qt/mnemonicverificationdialog.cpp`:
- Around line 305-306: secureWord is already lowercased when constructed, so
remove the redundant .toLower() on the comparison line to avoid creating an
extra temporary containing sensitive data; change the comparison to use
secureWord directly (const bool result{word == secureWord}) and, if necessary,
ensure the incoming word is normalized to lowercase once before this comparison
so both sides are compared consistently without extra temporaries.
🧹 Nitpick comments (2)
src/qt/mnemonicverificationdialog.cpp (2)
370-378:wordStdzeroing only covers the last iteration's content.Because
wordStdis reused across loop iterations, overwriting with a shorter word leaves residual bytes from the previous (longer) word in the buffer. The post-loopassign(size, 0)only covers the final word's length. Additionally, sincewordStdis not read after the zero-fill, the compiler may optimize the call away (unlikememory_cleansewhich uses a volatile store).Consider scoping
wordStdinside the loop body and clearing it each iteration, or usingmemory_cleansefor the final wipe.Suggested approach
- std::string wordStd; for (const QString& word : wordList) { - wordStd = word.toStdString(); + std::string wordStd = word.toStdString(); SecureString secureWord; secureWord.assign(std::string_view{wordStd}); m_words.push_back(secureWord); + memory_cleanse(wordStd.data(), wordStd.size()); } - // Clear temporary std::string - wordStd.assign(wordStd.size(), 0);
295-311:parseWords()returns by value — eachvalidateWord()call copies the entire secure word vector.
validateWord()is called 3 times per keystroke (viaupdateWordValidation) and 3 more times fromaccept(). Each invocation copies everySecureStringin the vector just to access a single word. Returning aconstreference would avoid these copies and reduce sensitive-data duplication in memory.Suggested approach
Change the return type and call sites:
-std::vector<SecureString> MnemonicVerificationDialog::parseWords() +const std::vector<SecureString>& MnemonicVerificationDialog::parseWords() {Then in
validateWord:- std::vector<SecureString> words = parseWords(); + const auto& words = parseWords();And similarly in
buildMnemonicGrid:- std::vector<SecureString> words; - if (reveal) { - words = parseWords(); - } else { + if (!reveal) { // ... hidden view branch (unchanged, returns early) ... } - - // Revealed view - words are already parsed - const int n = words.size(); + const auto& words = parseWords(); + const int n = words.size();
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-mnemonic-allocations
branch
from
540aca7 to
b8b0c1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/mnemonicverificationdialog.cpp (1)
393-401:⚠️ Potential issue | 🟡 Minor
getWordCountdoesn't zero theQStringbuffer before releasing it.
parseWords()(line 381) usesmnemonicStr.fill(QChar(0))before.clear()/.squeeze(). Here thefillstep is missing, so the mnemonic content lingers in the freed Qt heap block.Proposed fix
int count = words.size(); // Clear immediately + mnemonicStr.fill(QChar(0)); mnemonicStr.clear(); mnemonicStr.squeeze(); words.clear();
🤖 Fix all issues with AI agents
In `@src/qt/mnemonicverificationdialog.cpp`:
- Around line 370-378: The temporary std::string buffer `wordStd` is reused in
the loop so only the last buffer is wiped; modify the loop in
mnemonicverificationdialog.cpp that iterates `for (const QString& word :
wordList)` so that you explicitly overwrite/zero `wordStd` before each new
assignment (and still zero it after the loop) to ensure each previous buffer is
scrubbed; retain construction of `SecureString secureWord` and push to `m_words`
as-is, but ensure `wordStd` is cleared (e.g., assign zeros or call a memory
cleanse routine) immediately before `wordStd = word.toStdString()` and after the
loop to avoid leaving plaintext in freed heap memory.
|
pls consider 65722be |
… handling Restore m_words.clear() calls in onHideMnemonicClicked() and reject() to minimize the exposure window of sensitive mnemonic data in memory. While secure_allocator cleanses on deallocation, keeping data in memory longer than necessary increases the attack surface. Also fix redundant double .toLower() in validateWord() and replace std::string intermediate in parseWords() loop with QByteArray to avoid leaking word data through the default (non-secure) allocator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/mnemonicverificationdialog.cpp (1)
396-404:⚠️ Potential issue | 🟡 Minor
getWordCountskips zeroingmnemonicStrbefore clearing — inconsistent withparseWords.
parseWords()(line 384) doesmnemonicStr.fill(QChar(0))before.clear(), but heremnemonicStris cleared without zeroing first. The mnemonic content remains in freed heap memory.Proposed fix
int count = words.size(); // Clear immediately + mnemonicStr.fill(QChar(0)); mnemonicStr.clear(); mnemonicStr.squeeze(); + for (QString& w : words) { + w.fill(QChar(0)); + } words.clear();
🧹 Nitpick comments (3)
src/qt/mnemonicverificationdialog.cpp (3)
369-390:mnemonicStris properly scrubbed, butwordListentries are not zeroed before clearing.Line 384-386 correctly fills
mnemonicStrwith zeros before releasing. However, line 387 callswordList.clear()without first zeroing each individualQStringin the list. EachQStringinwordListholds a mnemonic word in Qt's default (non-secure) allocator and will be freed without scrubbing.For consistency with the rest of the scrubbing in this function:
Proposed fix
// Clear the temporary QString immediately after parsing mnemonicStr.fill(QChar(0)); mnemonicStr.clear(); mnemonicStr.squeeze(); // Release memory + for (QString& w : wordList) { + w.fill(QChar(0)); + } wordList.clear();
462-470:texton line 463 contains the mnemonic word but is never scrubbed.
wordStris properly zeroed, but the formattedtextstring ("N. word") still holds the plaintext word and is freed without cleansing when it goes out of scope.Proposed fix — also scrub `text`
lbl->setTextInteractionFlags(Qt::TextSelectableByMouse); m_gridLayout->addWidget(lbl, r, c); // Clear temporary QString immediately after use wordStr.fill(QChar(0)); wordStr.clear(); wordStr.squeeze(); + // Note: `text` also contains the word; zero it too + // (the QLabel retains its own copy internally) }Though since
textisconst, you'd need to drop theconstor restructure slightly:- const QString text = QString("%1. %2").arg(idx + 1, 2).arg(wordStr); + QString text = QString("%1. %2").arg(idx + 1, 2).arg(wordStr); QLabel* lbl = new QLabel(text); ... + text.fill(QChar(0));
300-317:parseWords()returns by value — eachvalidateWordcall copies the entire word vector.
updateWordValidation()callsvalidateWord()three times per keystroke (lines 325-327). Each call invokesparseWords()on line 305, which returnsm_wordsby value, creating a fullstd::vector<SecureString>copy each time. WhileSecureStringusessecure_allocatorso copies are safely handled, it's 3 unnecessary deep copies per keystroke.Consider using
m_wordsdirectly (it's already a member), or havingparseWords()return aconstreference:Sketch
-std::vector<SecureString> MnemonicVerificationDialog::parseWords() +const std::vector<SecureString>& MnemonicVerificationDialog::parseWords() { ... return m_words; }Then in
validateWord:- std::vector<SecureString> words = parseWords(); + const auto& words = parseWords();
3 participants
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 for details:Secondly, current implementation causes creating extra temporary object with sensitive data:
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_cleanseshould be considered to use for QStrings.This PR conflicts to #7126 because the same function is changed; I will prefer #7126 to be merged first because 7126 is meant to be backported.
How Has This Been Tested?
Tested as an extra changes to #7126 locally in the same branch, splitted to 2 PR after that.
Breaking Changes
N/A
Checklist: