Wipe transient mnemonic and seed material from wallet init memory

[ohcee]

Problem

The HD mnemonic phrase and the derived master seed, the root of every wallet, stealth, RingCT and zerocoin key, were left in ordinary process memory after wallet creation, where a swap file, core dump, or local attacker could recover them.

Description

During wallet creation the mnemonic phrase and the 512 bit master seed pass through plain std::string, std::vector and uint512 buffers. Those buffers were never cleansed, so the secret material lingered in memory and could be paged to disk.

Root Cause

The transient copies of the phrase and seed in GenerateNewMnemonicSeed and MnemonicWalletInit::Open were normal heap buffers with no cleanse on the way out, so their contents survived until the memory was eventually reused, and were swappable in the meantime.

Why broke?

Wallet creation was written for correctness, not for secret hygiene, so the transient phrase and seed copies were treated like any other local. They are correct functionally; they simply were not wiped, which only matters for secret material.

Solution

Cleanse the transient mnemonic and seed copies once they are no longer needed.

How fix?

memory_cleanse is called on the raw private key copy and the intermediate seed in GenerateNewMnemonicSeed, and on the mnemonic phrase strings, the decoded seed and the staged master key seed in MnemonicWalletInit::Open, the last wiped right after CreateWalletFromFile has taken it. memory_cleanse is a compiler barrier, so the wipes are not optimised away. This is the pragmatic main path fix; a fuller change would hold the phrase in SecureString and the seed in a secure_allocator or LockedPool so it is never swappable and every error path is covered too, and that is noted for a follow up.

Unit Testing Results

src/test/mnemonic_cleanse_tests.cpp confirms seed derivation is unchanged by the cleanse, so the wipes do not alter wallet creation behaviour. 2 of 2 test cases pass, exit 0, no regressions. The cleanse itself is a memory property rather than an observable output, so the test guards behaviour rather than asserting the wipe directly. Results captured during the security validation; the Boost 1.85 build shim used locally on Apple Silicon is not part of this PR.

How to test?

Build with tests and run test_veil --run_test=mnemonic_cleanse_tests, and confirm a wallet created from a known mnemonic still derives the same addresses as before, since the cleanse must not change any derived key.

[seanPhill]

ACK

Code review of PR #1083 (current state after your conflict resolution)

Status

• (I have resolved a conflict in src/veil/mnemonic/mnemonicwalletinit.cpp where an addition from PR 1079 was not in the same line used in this PR 1083.)
  Conflict correctly resolved (3 commits total, including the merge of master).
• The fRestoredSeed / extra argument from #1079 is preserved.
• The security cleanses are present.
• No remaining conflict markers.
• Ready for formal review.

Overall assessment

Concept ACK + Approach ACK. (ACK from me any NITS can be address later if necessary.)

This is a useful, low-risk security hardening. Leaving the HD mnemonic and the 512-bit master seed in ordinary heap buffers after wallet creation is a real (if local) exposure. Using memory_cleanse on the transient copies is the pragmatic, correct first step. The author is also honest about the limitations (no SecureString / LockedPool yet, error-path coverage incomplete).

The regression tests are appropriate — they correctly guard the functional behaviour rather than trying to assert a memory property that is hard to observe from user space.

File-by-file notes

src/veil/mnemonic/generateseed.cpp
Clean and correct. The two memory_cleanse calls after the seed has been copied into the return value are in the right place. Good comment.

src/veil/mnemonic/mnemonicwalletinit.cpp (post-merge)
The important call is now correct:

std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(..., fNewSeed ? &hashMasterKey : nullptr, fNewSeed && fRestoredSeed);
// The wallet has taken the seed; wipe our copy...
memory_cleanse(hashMasterKey.begin(), hashMasterKey.size());

All the other cleanses (mnemonic string, hashRet, strSeedPhraseArg, strMessage) are also present and look good.

Minor nits (optional, not blocking):

1. Empty-string safety
   These lines are technically undefined behaviour if the string is empty:

   memory_cleanse(&strSeedPhraseArg[0], strSeedPhraseArg.size());
   memory_cleanse(&strMessage[0], strMessage.size());
   memory_cleanse(&mnemonic[0], mnemonic.size());

   Safer pattern:

   if (!strSeedPhraseArg.empty())
       memory_cleanse(&strSeedPhraseArg[0], strSeedPhraseArg.size());

   (Same for the others.) In practice it almost never hits an empty string, but it’s cheap insurance.

2. Unnecessary cleanse when not a new seed
   memory_cleanse on hashMasterKey runs even when fNewSeed == false (i.e. the pointer passed to CreateWalletFromFile was nullptr). Harmless, but you could guard it with if (fNewSeed).

3. Tiny trailing whitespace on the long CreateWalletFromFile line after the merge.

src/test/mnemonic_cleanse_tests.cpp
Good. Two focused tests that confirm:

• The wipe does not change the seed that is returned / re-derived.
• decode_mnemonic remains pure and non-zero.

Exactly what is needed for this kind of change.

src/Makefile.test.include
Trivial and correct.

Recommendation

ACK

The only changes I would still like to see are the three tiny empty-string guards; everything else is solid. The PR is in good shape to merge.

[Veil-Project]

utACK

Looks good!