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.