fix: scrub plaintext credential buffers before releasing them [patch]

[matt-edmondson]

Fixes #144

The problem

WindowsCredentialStore left two plaintext copies of a credential behind:

• TryLoad copied the stored secret into a managed byte[], deserialized it, and returned — never clearing the array. It lingered on the managed heap, subject to GC promotion and compaction, until eventually collected, where it could still be read out of a crash dump. MacOsCredentialStore already did the right thing here.
• Save cleared its managed blob but handed the Marshal.AllocHGlobal copy — the identical plaintext bytes passed to CredWriteW — straight to Marshal.FreeHGlobal. Freed unmanaged memory is not scrubbed by the CLR or the OS heap, so the plaintext stayed in the process heap until that allocation happened to be reused.

The change

Both paths now go through shared primitives rather than each store re-implementing (and each store's copy going untested):

• CredentialSerialization.DeserializeAndScrub(byte[]) — deserializes, then zeroes the managed copy in a finally, so the plaintext is scrubbed on the failure path too. A blob that fails to parse still came out of the platform store, so it is still secret-bearing.
• NativeSecretBuffer (new, internal) — owns an unmanaged copy and zeroes it before FreeHGlobal. Zero() is separate from Dispose() so the scrub is observable while the allocation is still live; Dispose() is Zero() then free.
• CredentialSerialization.Zero(byte[]) — the one place managed scrubbing happens.

Zeroing goes through CryptographicOperations.ZeroMemory (managed) and Marshal.Copy of a zero-filled array (unmanaged) rather than Array.Clear, per the triage note, so the runtime cannot discard it as a dead store. The unmanaged path avoids unsafe/Span because AllowUnsafeBlocks is off in this project; Marshal.Copy into unmanaged memory is an opaque interop call and is not elidable either, and its source array is freshly zeroed so it holds no secret of its own.

Audit of the other platform stores

The triage asked for this while in here.

• MacOsCredentialStore — already cleared both buffers, but TryLoad skipped its Array.Clear if deserialization threw. Now uses the same two helpers, so the failure path is covered and the clears can't drift apart again.
• LinuxSecretServiceCredentialStore — not changed here. It never holds a plaintext byte[]: it round-trips through SerializeToString/DeserializeFromString and hands libsecret a string. A .NET string is immutable and interned-adjacent, so it cannot be scrubbed at all — fixing that means moving that store off the string-based API, which is a separate design change rather than a missing Zero() call. Worth its own issue; flagging rather than widening this PR.

Testing

New CredentialCache.Test/SecretScrubbingTests.cs — 13 tests, running on every platform. The native stores are only reachable on their own OS, which is exactly why the scrubbing lives in these two shared primitives: they are testable where the stores are not.

The key one, NativeSecretBufferZeroOverwritesThePlaintextWhileStillAllocated, reads the unmanaged bytes back before Dispose — reading freed memory would be undefined, so the scrub has to be observable while the allocation is live. That is the exact ordering Dispose relies on.

Proven to actually catch the bug: with Zero() and the DeserializeAndScrub scrub temporarily stubbed out, 5 of the new tests fail (Expected: 0 / Actual: 123 — the leading { of the credential JSON still sitting in the buffer), covering both the managed-copy and unmanaged-buffer paths. With the fix restored they pass.

Full suite, both configurations:

• Debug: total: 37, failed: 0, succeeded: 32, skipped: 5
• Release: total: 37, failed: 0, succeeded: 32, skipped: 5

The 5 skips are the pre-existing NativeCredentialStoreTests, which report Inconclusive without a Secret Service daemon — unchanged from the baseline on main (19 passing → 19 passing, +13 new). Release matters here specifically because dead-store elision is a Release-JIT concern, and the scrub assertions hold there.

No public API change; both new helpers are internal (the test project already has InternalsVisibleTo).

🤖 Generated with Claude Code

https://claude.ai/code/session_01ENSG6H3g5YSd1mQMPD8EGd

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud