From 7c5cd45f135e860a95cbcf707317a7610d59df36 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Thu, 17 Sep 2026 09:28:04 +0000 Subject: [PATCH 1/2] fix: scrub plaintext credential buffers before releasing them [patch] WindowsCredentialStore left two plaintext copies of a credential behind: TryLoad never cleared the managed byte[] it deserialized from, and Save handed its Marshal.AllocHGlobal copy to Marshal.FreeHGlobal without zeroing it first, so the plaintext stayed in the process heap until that allocation happened to be reused. Both now route through shared primitives that the tests can observe on any platform: CredentialSerialization.DeserializeAndScrub zeroes the managed copy on both the success and the failure path, and the new NativeSecretBuffer zeroes an unmanaged copy before freeing it. MacOsCredentialStore was audited at the same time. It already cleared its buffers, but TryLoad skipped the clear if deserialization threw; it now uses the same two helpers. Zeroing goes through CryptographicOperations.ZeroMemory (managed) and Marshal.Copy of a zero-filled array (unmanaged) rather than Array.Clear, so the runtime cannot elide it as a dead store. Fixes ktsu-dev/CredentialCache#144 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ENSG6H3g5YSd1mQMPD8EGd --- CredentialCache.Test/SecretScrubbingTests.cs | 177 ++++++++++++++++++ .../Storage/CredentialSerialization.cs | 44 +++++ .../Storage/MacOsCredentialStore.cs | 5 +- CredentialCache/Storage/NativeSecretBuffer.cs | 94 ++++++++++ .../Storage/WindowsCredentialStore.cs | 14 +- 5 files changed, 324 insertions(+), 10 deletions(-) create mode 100644 CredentialCache.Test/SecretScrubbingTests.cs create mode 100644 CredentialCache/Storage/NativeSecretBuffer.cs diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs new file mode 100644 index 0000000..d3a0389 --- /dev/null +++ b/CredentialCache.Test/SecretScrubbingTests.cs @@ -0,0 +1,177 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.CredentialCache.Test; + +using System.Runtime.InteropServices; +using ktsu.CredentialCache.Storage; +using ktsu.Semantics.Strings; + +/// +/// Covers the two primitives the platform-native stores use to keep plaintext +/// credential bytes from outliving the call that read or wrote them: +/// for the managed +/// copy a store deserializes from, and for the +/// unmanaged copy a store hands to a native API. +/// +/// These run on every platform. The native stores themselves are only reachable on +/// their own OS, so the scrubbing lives in these two shared primitives rather than +/// being re-implemented (and left untested) in each store. +/// +[TestClass] +public class SecretScrubbingTests +{ + private static byte[] SerializedCredential() => + CredentialSerialization.Serialize(new CredentialWithToken + { + Token = SemanticString.Create("plaintext-token-to-scrub"), + }); + + private static byte[] ReadUnmanaged(NativeSecretBuffer buffer) + { + byte[] copy = new byte[buffer.Length]; + Marshal.Copy(buffer.Pointer, copy, 0, buffer.Length); + return copy; + } + + [TestMethod] + public void DeserializeAndScrubReturnsTheCredential() + { + byte[] blob = SerializedCredential(); + + Credential? credential = CredentialSerialization.DeserializeAndScrub(blob); + + CredentialWithToken? typed = credential as CredentialWithToken; + Assert.IsNotNull(typed); + Assert.AreEqual("plaintext-token-to-scrub", typed!.Token.ToString()); + } + + [TestMethod] + public void DeserializeAndScrubZeroesTheManagedCopy() + { + byte[] blob = SerializedCredential(); + CollectionAssert.AreNotEqual(new byte[blob.Length], blob, "Precondition: the blob starts out as plaintext."); + + _ = CredentialSerialization.DeserializeAndScrub(blob); + + CollectionAssert.AreEqual(new byte[blob.Length], blob, + "The plaintext blob must be zeroed once it has been deserialized."); + } + + [TestMethod] + public void DeserializeAndScrubZeroesTheManagedCopyForUnparseableBytes() + { + // A blob that isn't a credential still came out of the platform store, so it + // is still secret-bearing and must be scrubbed on the failure path too. + byte[] blob = [.. "{ not a credential"u8]; + + Credential? credential = CredentialSerialization.DeserializeAndScrub(blob); + + Assert.IsNull(credential); + CollectionAssert.AreEqual(new byte[blob.Length], blob); + } + + [TestMethod] + public void NativeSecretBufferCopiesTheSourceBytes() + { + byte[] blob = SerializedCredential(); + + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(blob); + + Assert.AreEqual(blob.Length, buffer.Length); + Assert.AreNotEqual(IntPtr.Zero, buffer.Pointer); + CollectionAssert.AreEqual(blob, ReadUnmanaged(buffer)); + } + + [TestMethod] + public void NativeSecretBufferZeroOverwritesThePlaintextWhileStillAllocated() + { + byte[] blob = SerializedCredential(); + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(blob); + CollectionAssert.AreEqual(blob, ReadUnmanaged(buffer), "Precondition: the unmanaged copy is plaintext."); + + buffer.Zero(); + + // Read back before Dispose - reading freed memory would be undefined, so the + // scrub has to be observable while the allocation is still live. This is the + // exact ordering Dispose relies on: zero, then free. + CollectionAssert.AreEqual(new byte[blob.Length], ReadUnmanaged(buffer), + "The unmanaged copy must be zeroed before the memory is released."); + } + + [TestMethod] + public void NativeSecretBufferZeroIsIdempotent() + { + byte[] blob = SerializedCredential(); + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(blob); + + buffer.Zero(); + buffer.Zero(); + + CollectionAssert.AreEqual(new byte[blob.Length], ReadUnmanaged(buffer)); + } + + [TestMethod] + public void NativeSecretBufferDisposeReleasesThePointer() + { + NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(SerializedCredential()); + + buffer.Dispose(); + + Assert.AreEqual(IntPtr.Zero, buffer.Pointer, "A disposed buffer must not keep a dangling pointer."); + Assert.AreEqual(0, buffer.Pointer.ToInt64()); + } + + [TestMethod] + public void NativeSecretBufferDisposeIsIdempotent() + { + NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(SerializedCredential()); + + buffer.Dispose(); + buffer.Dispose(); + + Assert.AreEqual(IntPtr.Zero, buffer.Pointer); + } + + [TestMethod] + public void NativeSecretBufferZeroAfterDisposeDoesNotTouchFreedMemory() + { + NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(SerializedCredential()); + buffer.Dispose(); + + // Must be a no-op rather than a write through a freed pointer. + buffer.Zero(); + + Assert.AreEqual(IntPtr.Zero, buffer.Pointer); + } + + [TestMethod] + public void NativeSecretBufferHandlesAnEmptySource() + { + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf([]); + + Assert.AreEqual(0, buffer.Length); + Assert.AreNotEqual(IntPtr.Zero, buffer.Pointer); + buffer.Zero(); + } + + [TestMethod] + public void NativeSecretBufferRejectsANullSource() => + Assert.ThrowsExactly(() => NativeSecretBuffer.CopyOf(null!)); + + [TestMethod] + public void ZeroOverwritesTheBuffer() + { + byte[] blob = SerializedCredential(); + + CredentialSerialization.Zero(blob); + + CollectionAssert.AreEqual(new byte[blob.Length], blob); + } + + [TestMethod] + public void ZeroToleratesEmptyAndNullBuffers() + { + CredentialSerialization.Zero([]); + CredentialSerialization.Zero(null!); + } +} diff --git a/CredentialCache/Storage/CredentialSerialization.cs b/CredentialCache/Storage/CredentialSerialization.cs index 875f07f..1b2aba4 100644 --- a/CredentialCache/Storage/CredentialSerialization.cs +++ b/CredentialCache/Storage/CredentialSerialization.cs @@ -2,6 +2,7 @@ namespace ktsu.CredentialCache.Storage; +using System.Security.Cryptography; using System.Text.Json; using ktsu.RoundTripStringJsonConverter; @@ -60,6 +61,49 @@ public static string SerializeToString(Credential credential) => } } + /// + /// Deserializes a credential from a UTF-8 JSON byte array and then overwrites + /// with zeros, whether or not deserialization succeeds. + /// + /// + /// Platform stores copy the stored secret into a managed array in order to + /// deserialize it. That array holds plaintext, and left alone it lingers on the + /// managed heap - subject to GC promotion and compaction - until it is eventually + /// collected, where it can still be read out of a crash dump. Store implementations + /// should read through this helper rather than calling + /// directly, so no plaintext copy outlives the call. + /// + internal static Credential? DeserializeAndScrub(byte[] utf8Json) + { + if (utf8Json is null) + { + return null; + } + + try + { + return Deserialize(utf8Json); + } + finally + { + Zero(utf8Json); + } + } + + /// + /// Overwrites with zeros in a way the runtime cannot + /// discard as a dead store. + /// + internal static void Zero(byte[] buffer) + { + if (buffer is null || buffer.Length == 0) + { + return; + } + + CryptographicOperations.ZeroMemory(buffer); + } + /// /// Deserializes a credential from a UTF-8 JSON string. Returns null if the value /// does not represent a known credential. diff --git a/CredentialCache/Storage/MacOsCredentialStore.cs b/CredentialCache/Storage/MacOsCredentialStore.cs index ff8ffb4..242b601 100644 --- a/CredentialCache/Storage/MacOsCredentialStore.cs +++ b/CredentialCache/Storage/MacOsCredentialStore.cs @@ -57,8 +57,7 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) { byte[] blob = new byte[length]; Marshal.Copy(passwordPtr, blob, 0, (int)length); - credential = CredentialSerialization.Deserialize(blob); - Array.Clear(blob, 0, blob.Length); + credential = CredentialSerialization.DeserializeAndScrub(blob); return credential is not null; } finally @@ -131,7 +130,7 @@ public void Save(PersonaGUID persona, Credential credential) } finally { - Array.Clear(blob, 0, blob.Length); + CredentialSerialization.Zero(blob); } } diff --git a/CredentialCache/Storage/NativeSecretBuffer.cs b/CredentialCache/Storage/NativeSecretBuffer.cs new file mode 100644 index 0000000..289f880 --- /dev/null +++ b/CredentialCache/Storage/NativeSecretBuffer.cs @@ -0,0 +1,94 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.CredentialCache.Storage; + +using System.Runtime.InteropServices; + +/// +/// An unmanaged copy of a plaintext credential blob that is overwritten with zeros +/// before the memory is released. +/// +/// +/// hands memory back to the process heap +/// without scrubbing it, so plaintext passed to a native credential API survives in +/// the process image until that allocation happens to be reused - long enough to be +/// recovered by a memory scanner, or to land in a crash dump, hibernation file, or +/// page file. Owning the copy through this type zeroes the bytes first, on every path +/// out of the caller. +/// +internal sealed class NativeSecretBuffer : IDisposable +{ + private NativeSecretBuffer(IntPtr pointer, int length) + { + Pointer = pointer; + Length = length; + } + + /// + /// Gets the number of bytes copied into unmanaged memory. + /// + internal int Length { get; } + + /// + /// Gets a pointer to the unmanaged copy, or once the + /// buffer has been disposed. + /// + internal IntPtr Pointer { get; private set; } + + /// + /// Copies into newly allocated unmanaged memory. + /// + /// The plaintext bytes to copy. + /// A buffer owning the unmanaged copy, which the caller must dispose. + internal static NativeSecretBuffer CopyOf(byte[] source) + { + ArgumentNullException.ThrowIfNull(source); + + // AllocHGlobal(0) is implementation defined, so keep at least one byte and the + // pointer handed to native code is always valid. + IntPtr pointer = Marshal.AllocHGlobal(Math.Max(source.Length, 1)); + try + { + Marshal.Copy(source, 0, pointer, source.Length); + } + catch + { + Marshal.FreeHGlobal(pointer); + throw; + } + + return new NativeSecretBuffer(pointer, source.Length); + } + + /// + /// Overwrites the unmanaged copy with zeros while the memory is still allocated. + /// Idempotent, and a no-op once the buffer has been disposed. + /// + internal void Zero() + { + if (Pointer == IntPtr.Zero || Length == 0) + { + return; + } + + // Marshal.Copy into unmanaged memory is an opaque interop call, so unlike a + // managed Array.Clear the runtime cannot elide it as a dead store. The source + // is a fresh zero-filled array, which holds no secret of its own. + Marshal.Copy(new byte[Length], 0, Pointer, Length); + } + + /// + /// Zeroes the unmanaged copy and then releases it. + /// + public void Dispose() + { + if (Pointer == IntPtr.Zero) + { + return; + } + + Zero(); + Marshal.FreeHGlobal(Pointer); + Pointer = IntPtr.Zero; + } +} diff --git a/CredentialCache/Storage/WindowsCredentialStore.cs b/CredentialCache/Storage/WindowsCredentialStore.cs index 25b7ea6..ba3526b 100644 --- a/CredentialCache/Storage/WindowsCredentialStore.cs +++ b/CredentialCache/Storage/WindowsCredentialStore.cs @@ -57,7 +57,7 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) byte[] blob = new byte[cred.CredentialBlobSize]; Marshal.Copy(cred.CredentialBlob, blob, 0, blob.Length); - credential = CredentialSerialization.Deserialize(blob); + credential = CredentialSerialization.DeserializeAndScrub(blob); return credential is not null; } finally @@ -80,17 +80,18 @@ public void Save(PersonaGUID persona, Credential credential) $"{NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE} bytes (was {blob.Length})."); } - IntPtr blobPtr = Marshal.AllocHGlobal(blob.Length); try { - Marshal.Copy(blob, 0, blobPtr, blob.Length); + // The unmanaged copy handed to CredWriteW is plaintext; NativeSecretBuffer + // zeroes it before freeing it, which Marshal.FreeHGlobal alone does not. + using NativeSecretBuffer nativeBlob = NativeSecretBuffer.CopyOf(blob); NativeMethods.CREDENTIAL native = new() { Type = NativeMethods.CRED_TYPE_GENERIC, TargetName = TargetFor(persona), - CredentialBlob = blobPtr, - CredentialBlobSize = blob.Length, + CredentialBlob = nativeBlob.Pointer, + CredentialBlobSize = nativeBlob.Length, Persist = NativeMethods.CRED_PERSIST_LOCAL_MACHINE, UserName = Environment.UserName, }; @@ -103,8 +104,7 @@ public void Save(PersonaGUID persona, Credential credential) } finally { - Marshal.FreeHGlobal(blobPtr); - Array.Clear(blob, 0, blob.Length); + CredentialSerialization.Zero(blob); } } From c38fa404e3b310323a5a846dd956c028fa8dacf5 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Thu, 17 Sep 2026 09:40:38 +0000 Subject: [PATCH 2/2] test: address SonarCloud findings on the scrubbing tests MSTEST0068 (8x): use Assert.AreSequenceEqual / AreNotSequenceEqual rather than the legacy CollectionAssert equivalents. No other test file in the repo used CollectionAssert, so this was new drift. S2699 (blocker): ZeroToleratesEmptyAndNullBuffers had no assertion. Its subject is that Zero's guard clauses return instead of throwing, so it now names that in a comment and asserts the empty buffer it passed in. Re-verified the revert proof under the new assertion API: stubbing out both scrubs still fails the same 5 tests, so the converted asserts still compare contents rather than references. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ENSG6H3g5YSd1mQMPD8EGd --- CredentialCache.Test/SecretScrubbingTests.cs | 25 +++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs index d3a0389..a9ecb01 100644 --- a/CredentialCache.Test/SecretScrubbingTests.cs +++ b/CredentialCache.Test/SecretScrubbingTests.cs @@ -49,11 +49,11 @@ public void DeserializeAndScrubReturnsTheCredential() public void DeserializeAndScrubZeroesTheManagedCopy() { byte[] blob = SerializedCredential(); - CollectionAssert.AreNotEqual(new byte[blob.Length], blob, "Precondition: the blob starts out as plaintext."); + Assert.AreNotSequenceEqual(new byte[blob.Length], blob, "Precondition: the blob starts out as plaintext."); _ = CredentialSerialization.DeserializeAndScrub(blob); - CollectionAssert.AreEqual(new byte[blob.Length], blob, + Assert.AreSequenceEqual(new byte[blob.Length], blob, "The plaintext blob must be zeroed once it has been deserialized."); } @@ -67,7 +67,7 @@ public void DeserializeAndScrubZeroesTheManagedCopyForUnparseableBytes() Credential? credential = CredentialSerialization.DeserializeAndScrub(blob); Assert.IsNull(credential); - CollectionAssert.AreEqual(new byte[blob.Length], blob); + Assert.AreSequenceEqual(new byte[blob.Length], blob); } [TestMethod] @@ -79,7 +79,7 @@ public void NativeSecretBufferCopiesTheSourceBytes() Assert.AreEqual(blob.Length, buffer.Length); Assert.AreNotEqual(IntPtr.Zero, buffer.Pointer); - CollectionAssert.AreEqual(blob, ReadUnmanaged(buffer)); + Assert.AreSequenceEqual(blob, ReadUnmanaged(buffer)); } [TestMethod] @@ -87,14 +87,14 @@ public void NativeSecretBufferZeroOverwritesThePlaintextWhileStillAllocated() { byte[] blob = SerializedCredential(); using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(blob); - CollectionAssert.AreEqual(blob, ReadUnmanaged(buffer), "Precondition: the unmanaged copy is plaintext."); + Assert.AreSequenceEqual(blob, ReadUnmanaged(buffer), "Precondition: the unmanaged copy is plaintext."); buffer.Zero(); // Read back before Dispose - reading freed memory would be undefined, so the // scrub has to be observable while the allocation is still live. This is the // exact ordering Dispose relies on: zero, then free. - CollectionAssert.AreEqual(new byte[blob.Length], ReadUnmanaged(buffer), + Assert.AreSequenceEqual(new byte[blob.Length], ReadUnmanaged(buffer), "The unmanaged copy must be zeroed before the memory is released."); } @@ -107,7 +107,7 @@ public void NativeSecretBufferZeroIsIdempotent() buffer.Zero(); buffer.Zero(); - CollectionAssert.AreEqual(new byte[blob.Length], ReadUnmanaged(buffer)); + Assert.AreSequenceEqual(new byte[blob.Length], ReadUnmanaged(buffer)); } [TestMethod] @@ -165,13 +165,20 @@ public void ZeroOverwritesTheBuffer() CredentialSerialization.Zero(blob); - CollectionAssert.AreEqual(new byte[blob.Length], blob); + Assert.AreSequenceEqual(new byte[blob.Length], blob); } [TestMethod] public void ZeroToleratesEmptyAndNullBuffers() { - CredentialSerialization.Zero([]); + byte[] empty = []; + + // The guard clauses exist so a store can scrub whatever it has without + // length- or null-checking first. Both calls returning rather than throwing + // is the behaviour under test; an exception from either fails the test. + CredentialSerialization.Zero(empty); CredentialSerialization.Zero(null!); + + Assert.IsEmpty(empty); } }