Skip to content

[Code Quality] SecureData.Dispose — _disposed flag set after ZeroMemory; concurrent Dispose calls can race through the guard #1004

@Christophe-Rogiers

Description

@Christophe-Rogiers

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:

  1. 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.
  2. 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.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions