Use direct burn in BufferPtr::Erase and guard token keyfile data

[damianrickard]

Problem

BufferPtr::Erase() currently delegates to Zero(), which uses memset(). Since sensitive buffers are not subsequently read, that wipe may be removed as a dead store.

The four GUI and text-mode token import/export paths also install their cleanup guards only after GetKeyfileData() or ReadCompleteBuffer(). If either operation throws after allocating or partially populating the vector, its contents can be released without being wiped.

Changes

• Expand burn() directly inside BufferPtr::Erase(), following VeraCrypt’s established direct-erasure design from commit 885cc1d0.
• Leave Memory::Zero() and the existing direct-burn implementation of Buffer::Erase() unchanged.
• Install vector-referencing cleanup guards before the sensitive operations in the four GUI/text token paths.
• Reject unexpectedly empty token exports before accessing vector::front().

The existing password cleanup guards are already installed before their sensitive operations and remain unchanged.

Validation

• macOS arm64 GUI build
• VeraCrypt --text --test
• Linux GUI and console GitHub Actions build and test workflow

[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.

[damianrickard]

Thanks for the detailed follow-up. I’ve amended the existing commit and rebased it onto current master.

• Removed Memory::SecureErase; Memory.h and Memory.cpp are unchanged from master.
• Restored Buffer::Erase() to its existing direct-burn implementation.
• Implemented the guarded direct burn(DataPtr, DataSize) inside BufferPtr::Erase().
• Installed vector-referencing cleanup guards before the sensitive operation in all four GUI/text token import and export paths.
• Added an empty check before front() in both export paths.
• Left the two existing password cleanup guards unchanged.
• Updated the PR title, description, and commit message.

Validation completed:

• macOS arm64 GUI build passed.
• VeraCrypt --text --test passed with “Self-tests of all algorithms passed”.
• The Linux “Build and test Linux” workflow passed for both GUI and console variants.

Could you please re-review when convenient? I’ve left the existing review thread unresolved for you.