WindowsCredentialStore doesn't clear decrypted credential bytes from memory after use

[matt-edmondson]

What's wrong

WindowsCredentialStore.TryLoad and WindowsCredentialStore.Save (CredentialCache/Storage/WindowsCredentialStore.cs) copy the plaintext, serialized Credential JSON into managed and unmanaged buffers and then let those buffers go stale without zeroing them — unlike MacOsCredentialStore, which clears its equivalent transient buffer in both TryLoad and Save.

• TryLoad:

  byte[] blob = new byte[cred.CredentialBlobSize];
  Marshal.Copy(cred.CredentialBlob, blob, 0, blob.Length);
  credential = CredentialSerialization.Deserialize(blob);
  return credential is not null;

  blob (the raw plaintext bytes of the stored secret) is never cleared. It stays on the managed heap, subject to GC promotion/compaction, until eventually collected — potentially for a long time — and can end up in a crash/memory dump. Compare with MacOsCredentialStore.TryLoad, which calls Array.Clear(blob, 0, blob.Length); right after deserializing.

• Save:

  IntPtr blobPtr = Marshal.AllocHGlobal(blob.Length);
  try
  {
      Marshal.Copy(blob, 0, blobPtr, blob.Length);
      // ... CredWrite(...) ...
  }
  finally
  {
      Marshal.FreeHGlobal(blobPtr);
      Array.Clear(blob, 0, blob.Length);
  }

  The managed copy (blob) is cleared, but the unmanaged copy at blobPtr — holding the identical plaintext bytes handed to CredWriteW — is freed via Marshal.FreeHGlobal without being zeroed first. Freed unmanaged memory is not scrubbed by the CLR or the OS heap allocator, so the plaintext credential remains in the process's heap until that memory page happens to be reused, and can be recovered by memory-scanning tools or found in a crash dump/hibernation file/page file in the meantime.

Why it matters

This is the platform-native store whose entire purpose is to keep secrets out of ordinary memory/disk exposure. Leaving plaintext copies un-scrubbed defeats part of that goal on Windows specifically, while the macOS implementation in the same codebase already does the right thing — so this is an inconsistency with a known-good pattern already present in the repo, not a speculative hardening request.

Suggested fix

• In TryLoad, call Array.Clear(blob, 0, blob.Length); after CredentialSerialization.Deserialize(blob) (mirroring MacOsCredentialStore.TryLoad).
• In Save, zero the unmanaged buffer before freeing it, e.g.:

  finally
  {
      if (blobPtr != IntPtr.Zero)
      {
          for (int i = 0; i < blob.Length; i++)
          {
              Marshal.WriteByte(blobPtr, i, 0);
          }
          Marshal.FreeHGlobal(blobPtr);
      }
      Array.Clear(blob, 0, blob.Length);
  }

Acceptance criteria

• No path in WindowsCredentialStore.TryLoad/Save frees or abandons a buffer holding plaintext credential bytes without zeroing it first.
• Existing round-trip tests continue to pass.

[matt-edmondson]

Triage

Category: Bug (security hardening)
Priority: High
Area: CredentialCache/Storage/WindowsCredentialStore.cs — TryLoad, Save

This is defence-in-depth rather than a directly exploitable flaw, which normally means Medium — but it is High here for two reasons. First, it is in the component whose entire purpose is keeping secrets out of ordinary memory exposure, so the gap undercuts the security property the library is sold on. Second, the correct pattern already exists in this repo: MacOsCredentialStore clears its equivalent buffers in both methods, so this is a platform inconsistency with a known-good reference implementation, not a speculative request.

The unmanaged half of the Save leak is the more interesting one — the managed blob is already cleared, so only the Marshal.AllocHGlobal copy handed to CredWriteW survives un-scrubbed, and freed unmanaged memory is not zeroed by the CLR or the OS heap.

Not covered by an open PR. No duplicate found in this repo or elsewhere in the org.

Suggested next step: small, self-contained change; both fixes are given in the issue. Consider CryptographicOperations.ZeroMemory over a hand-rolled Marshal.WriteByte loop so the zeroing is not optimized away, and audit the other platform stores for the same pattern while in here.

---

Generated by Claude Code