Severity: Info / Code Quality
File: src/Servy.Core/Security/SecureData.cs lines 403-419
Finding
SecureData.Dispose(bool disposing) performs CryptographicOperations.ZeroMemory on the four key arrays before flipping _disposed = true:
protected virtual void Dispose(bool disposing)
{
if (_disposed) return;
if (disposing)
{
if (_v1MasterKey != null) CryptographicOperations.ZeroMemory(_v1MasterKey);
if (_v1StaticIv != null) CryptographicOperations.ZeroMemory(_v1StaticIv);
if (_v2EncryptionKey != null) CryptographicOperations.ZeroMemory(_v2EncryptionKey);
if (_v2HmacKey != null) CryptographicOperations.ZeroMemory(_v2HmacKey);
}
_disposed = true;
}
The _disposed field is not volatile, so two threads calling Dispose() simultaneously can both read false, both pass the guard, and both walk through the zeroing block. ZeroMemory is idempotent, so the practical fallout is benign (extra work, no corruption). However:
Encrypt/Decrypt only check _disposed via ThrowIfDisposed() (lines 87, 166) — there's a small but non-zero window in which one thread is mid-zeroing the keys while another thread is in the middle of aes.Key = _v2EncryptionKey; / HMACSHA256.HashData(_v2HmacKey, ...). The reader could end up using a partially-zeroed key, producing garbage ciphertext or HMAC mismatches without an exception.
- The class XML doc claims it's "Designed for a Singleton lifetime; internal keys are immutable after construction" — the immutability claim doesn't hold once Dispose can be raced against a live operation.
Suggested fix
Make _disposed volatile (or use Interlocked.Exchange(ref _disposed, true) returning the previous value) and flip it BEFORE the zeroing pass:
protected virtual void Dispose(bool disposing)
{
if (Interlocked.Exchange(ref _disposedInt, 1) != 0) return; // assume int field
if (disposing)
{
// zeroing here, no other thread will pass the guard
}
}
Or more conservatively, lock around Dispose and ThrowIfDisposed together so the live operation either completes fully or fails fast.
Severity: Info / Code Quality
File:
src/Servy.Core/Security/SecureData.cslines 403-419Finding
SecureData.Dispose(bool disposing)performsCryptographicOperations.ZeroMemoryon the four key arrays before flipping_disposed = true:The
_disposedfield is notvolatile, so two threads callingDispose()simultaneously can both readfalse, both pass the guard, and both walk through the zeroing block.ZeroMemoryis idempotent, so the practical fallout is benign (extra work, no corruption). However:Encrypt/Decryptonly check_disposedviaThrowIfDisposed()(lines 87, 166) — there's a small but non-zero window in which one thread is mid-zeroing the keys while another thread is in the middle ofaes.Key = _v2EncryptionKey;/HMACSHA256.HashData(_v2HmacKey, ...). The reader could end up using a partially-zeroed key, producing garbage ciphertext or HMAC mismatches without an exception.Suggested fix
Make
_disposedvolatile (or useInterlocked.Exchange(ref _disposed, true)returning the previous value) and flip it BEFORE the zeroing pass:Or more conservatively, lock around
DisposeandThrowIfDisposedtogether so the live operation either completes fully or fails fast.