Wipe memory reliably in Memory::Zero

[damianrickard]

Memory::Zero() used a plain memset(), which the compiler may remove as a dead store when the buffer is not read afterwards. Memory::Zero backs BufferPtr::Erase() and BufferPtr::Zero(), which are used to clear key material and other secrets, so an elided memset can leave secrets in memory.

Use burn() instead — the same volatile-write wipe already used by Buffer::Erase(). Memory::Zero is not called on the bulk encryption/I/O path, so this has no throughput impact.

[idrassi]

Thank you for looking into this.

I can't merge this PR as-is. Memory::Zero is a generic zero initialization primitive, not only a secret wiping primitive. It is used in several contexts where the intent is normal zero-fill, including non secret structures and larger working buffers.

In particular, the statement that this is not on a bulk path is not correct. During non quick volume creation, src/Core/VolumeCreator.cpp calls outputBuffer.Zero on a File::GetOptimalWriteSize buffer before encryption and writing. Changing Memory::Zero globally to burn would put the volatile byte-wise wipe loop in a repeated large buffer path and can have a measurable performance impact.

For me, the right approach is to introduce a dedicated secure erasure API, e.g. Memory::Erase / Memory::SecureErase, and then use it explicitly where the data is sensitive. BufferPtr::Erase should use that secure erasure primitive, while generic Zero should remain normal zero initialization. Other call sites should then be reviewed case by case.

So I agree with the goal, but not with changing Memory::Zero globally.

[idrassi]

I don't think Memory::Zero should call burn globally. This function is a generic zero fill primitive and is used outside secret erasure contexts, including repeated large buffer paths such as non quick volume creation via outputBuffer.Zero in VolumeCreator.cpp. Please introduce a dedicated secure erasure API instead, use it from BufferPtr::Erase, and leave generic Zero as normal zero initialization.

[damianrickard]

Thanks — that's a fair objection, and you're right that the "not on a bulk path" claim was wrong: VolumeCreator's outputBuffer.Zero() before each encrypt pass goes through Memory::Zero.

I've reworked this along the lines you suggested. Memory::Zero is back to a plain memset for generic zero-initialization. There's a new dedicated Memory::SecureErase (the same volatile-write burn loop Buffer::Erase already used), and both BufferPtr::Erase and Buffer::Erase now go through it.

Going through the call sites case by case, this turned out to fix a real gap rather than just harden one: every BufferPtr::Erase() caller is wiping secrets — typed passwords (TextUserInterface, VolumePasswordPanel) and security-token keyfile data (TextUserInterface, SecurityTokenKeyfilesDialog) — but BufferPtr::Erase was an alias for Zero()/memset, so those wipes were the ones actually at risk of being elided. They now reach burn in every case. The remaining .Zero() sites are all genuine zero-init (header buffers before serialization, keyfile-pool padding, RNG self-test reset, FAT scratch sectors), so they stay on memset and the bulk paths are unaffected.

Rebased onto current master. Builds clean (C++03 and C++11); --text --test passes.

[idrassi]

Thanks for reworking the PR.

First, I initially suggested introducing Memory::SecureErase, but after reviewing the project history I realized that VeraCrypt previously had essentially the same abstraction as Memory::Erase. That method itself called burn, but I deliberately removed it in commit 885cc1d 6 years ago because of the risk that a compiler could eliminate the separate Memory::Erase call when the erased memory was no longer subsequently used.

The affected sensitive call sites were changed to expand burn directly. This puts the volatile writes at the erasure site and is the explicit guarantee used by VeraCrypt against dead-store elimination, without depending on preservation of an out-of-line wrapper call.

Reintroducing Memory::SecureErase and routing Buffer::Erase through it would recreate the same abstraction boundary that I intentionally removed and would undo that established hardening design.

Please therefore:

• Leave Buffer::Erase as its existing direct burn(DataPtr, DataSize) implementation.
• Don't add Memory::SecureErase to Memory.h/Memory.cpp.
• Implement BufferPtr::Erase directly:

  void Erase () const
  {
      if (DataSize > 0)
          burn (DataPtr, DataSize);
  }

Second, the review of the current callers exposed an exception handling gap in the token keyfile cleanup.

In four token keyfile paths, the cleanup guard is installed only after the sensitive vector has been populated:

1. GUI token export installs it after GetKeyfileData.
2. GUI token import installs it after ReadCompleteBuffer.
3. Text mode token export installs it after GetKeyfileData.
4. Text mode token import installs it after ReadCompleteBuffer.

If either operation throws after allocating or partially filling the vector, the cleanup guard is never constructed. The vector is then released without wiping its contents.

Please install the cleanup guard immediately after declaring or allocating the vector. The guard should reference the vector itself instead of capturing a BufferPtr containing its current address. For example:

vector <uint8> keyfileData;

finally_do_arg (
      vector <uint8> *, &keyfileData,
      { if (!finally_arg->empty()) burn (&finally_arg->front(), finally_arg->size()); }
);

keyfile->GetKeyfileData (keyfileData);

For the import paths, use the same pattern immediately after constructing the sized vector and before calling ReadCompleteBuffer.

Referencing the vector ensures that the cleanup uses its current storage if the vector was reallocated.
The empty check also avoids calling front() when there is no data.

After a successful GetKeyfileData call, please reject an unexpectedly empty vector before constructing any BufferPtr from keyfileData.front.

The cleanup guards for the password arrays are already installed before the sensitive operations and don't require changes.

Finally, please update the PR title and description. They still describe changing Memory::Zero, while the revised implementation correctly leaves Memory::Zero unchanged. The commit message should also avoid claiming that every cleanup path is covered until the exception handling issue described above has been addressed.