From d188529cab5dc6cb6c0446387979c61139b410cd Mon Sep 17 00:00:00 2001 From: matt-edmondson <19528727+matt-edmondson@users.noreply.github.com> Date: Wed, 11 Nov 2026 05:26:49 +0000 Subject: [PATCH] Zero the native plaintext TryGet loads before freeing it On Windows and macOS, TryLoad copied the decrypted credential out of the buffer CredRead / SecKeychainFindGenericPassword returned and scrubbed only that managed copy. CredFree and SecKeychainItemFreeContent then released the native block unscrubbed, leaving a plaintext copy in freed heap on every load. NativeSecretBuffer.ReadCredentialAndZero now reads, deserializes and scrubs the managed copy, and zeroes the native bytes on every path out while they are still allocated. Both stores read through it, and a wiring test pins them to it, since neither store body runs on the Linux CI leg. Fixes ktsu-dev/CredentialCache#173 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW5TgsM2XbL48rHXNad --- .../NativeStoreScrubbingWiringTests.cs | 30 ++++++++++++- CredentialCache.Test/SecretScrubbingTests.cs | 40 +++++++++++++++++ .../Storage/MacOsCredentialStore.cs | 6 +-- CredentialCache/Storage/NativeSecretBuffer.cs | 45 +++++++++++++++++++ .../Storage/WindowsCredentialStore.cs | 6 +-- 5 files changed, 119 insertions(+), 8 deletions(-) diff --git a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs index 1230e26..7b12885 100644 --- a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs +++ b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs @@ -35,6 +35,7 @@ public class NativeStoreScrubbingWiringTests private static readonly MethodInfo SerializeToString = Helper(nameof(CredentialSerialization.SerializeToString)); private static readonly MethodInfo ReadCredential = Buffer(nameof(NativeSecretBuffer.ReadCredential)); + private static readonly MethodInfo ReadCredentialAndZero = Buffer(nameof(NativeSecretBuffer.ReadCredentialAndZero)); private static readonly MethodInfo OfCredential = Buffer(nameof(NativeSecretBuffer.OfCredential)); private static readonly MethodInfo CopyWithinLimit = Buffer(nameof(NativeSecretBuffer.CopyWithinLimit)); @@ -106,9 +107,9 @@ public void EveryNativeStoreLoadsThroughTheScrubbingDeserializer() // reads through that wrapper so the length-finding is covered by tests rather // than sitting in a method body that only runs on one operating system. Assert.IsTrue( - Calls(tryLoad, DeserializeAndScrub) || Calls(tryLoad, ReadCredential), + Calls(tryLoad, DeserializeAndScrub) || Calls(tryLoad, ReadCredential) || Calls(tryLoad, ReadCredentialAndZero), $"{store.Name}.TryLoad must deserialize through DeserializeAndScrub, directly or via " - + "NativeSecretBuffer.ReadCredential, so the plaintext copy it read is zeroed."); + + "NativeSecretBuffer.ReadCredential or ReadCredentialAndZero, so the plaintext copy it read is zeroed."); Assert.IsFalse( Calls(tryLoad, DeserializeFromString), $"{store.Name}.TryLoad must not deserialize from a string: an immutable managed string " @@ -195,6 +196,24 @@ public void TheMacOsStoreDecryptsASecretOnlyToLoadIt() } } + /// + /// CredFree and SecKeychainItemFreeContent release the plaintext the OS handed + /// back without scrubbing it. Scrubbing only the managed copy left a full plaintext copy of the + /// credential in freed native heap on every load (#173). Linux is exempt: libsecret's + /// secret_password_free wipes its own copy. + /// + [TestMethod] + public void TheWindowsAndMacOsStoresZeroTheNativePlaintextTheyLoad() + { + foreach (Type store in new[] { typeof(WindowsCredentialStore), typeof(MacOsCredentialStore) }) + { + Assert.IsTrue( + Calls(Method(store, nameof(ICredentialStore.TryLoad)), ReadCredentialAndZero), + $"{store.Name}.TryLoad must read through NativeSecretBuffer.ReadCredentialAndZero, which zeroes the " + + "plaintext the OS returned before the store frees it."); + } + } + /// /// The second hop of the two assertions above. Accepting the wrapper as equivalent to a /// direct call is only sound while the wrapper itself uses the scrubbing helper, so that is @@ -211,6 +230,13 @@ public void TheSharedCredentialWrappersUseTheScrubbingHelpers() Calls(ReadCredential, DeserializeFromString), "NativeSecretBuffer.ReadCredential must not deserialize from a string."); + Assert.IsTrue( + Calls(ReadCredentialAndZero, DeserializeAndScrub), + "NativeSecretBuffer.ReadCredentialAndZero must deserialize through DeserializeAndScrub."); + Assert.IsFalse( + Calls(ReadCredentialAndZero, DeserializeFromString), + "NativeSecretBuffer.ReadCredentialAndZero must not deserialize from a string."); + Assert.IsTrue( Calls(OfCredential, Serialize), "NativeSecretBuffer.OfCredential must serialize to bytes it can zero; the store assertions " diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs index 6cab047..ee7c290 100644 --- a/CredentialCache.Test/SecretScrubbingTests.cs +++ b/CredentialCache.Test/SecretScrubbingTests.cs @@ -235,6 +235,46 @@ public void NulTerminatedCopyOfHandlesAnEmptySource() Assert.AreSequenceEqual(new byte[] { 0 }, ReadUnmanaged(buffer)); } + [TestMethod] + public void ReadCredentialAndZeroReturnsTheCredentialAndZeroesTheNativeCopy() + { + // Stands in for the block CredRead or SecKeychainFindGenericPassword returns: writable + // native memory holding the plaintext, which the store frees afterwards. + byte[] blob = SerializedCredential(); + using NativeSecretBuffer native = NativeSecretBuffer.CopyOf(blob); + Assert.AreSequenceEqual(blob, ReadUnmanaged(native), "Precondition: the native copy is plaintext."); + + Credential? credential = NativeSecretBuffer.ReadCredentialAndZero(native.Pointer, native.Length); + + CredentialWithToken? typed = credential as CredentialWithToken; + Assert.IsNotNull(typed); + Assert.AreEqual("plaintext-token-to-scrub", typed!.Token.ToString()); + Assert.AreSequenceEqual(new byte[blob.Length], ReadUnmanaged(native), + "The native plaintext must be zeroed before the store frees it."); + } + + [TestMethod] + public void ReadCredentialAndZeroZeroesTheNativeCopyForUnparseableBytes() + { + byte[] blob = [.. "{ not a credential"u8]; + using NativeSecretBuffer native = NativeSecretBuffer.CopyOf(blob); + + Credential? credential = NativeSecretBuffer.ReadCredentialAndZero(native.Pointer, native.Length); + + Assert.IsNull(credential); + Assert.AreSequenceEqual(new byte[blob.Length], ReadUnmanaged(native)); + } + + [TestMethod] + public void ReadCredentialAndZeroReturnsNullForNothing() + { + Assert.IsNull(NativeSecretBuffer.ReadCredentialAndZero(IntPtr.Zero, 10)); + + using NativeSecretBuffer native = NativeSecretBuffer.CopyOf([1]); + Assert.IsNull(NativeSecretBuffer.ReadCredentialAndZero(native.Pointer, 0)); + Assert.AreSequenceEqual(new byte[] { 1 }, ReadUnmanaged(native), "A zero length touches nothing."); + } + [TestMethod] public void ReadNulTerminatedCopiesUpToTheTerminatorOnly() { diff --git a/CredentialCache/Storage/MacOsCredentialStore.cs b/CredentialCache/Storage/MacOsCredentialStore.cs index a9e11a7..d97e7da 100644 --- a/CredentialCache/Storage/MacOsCredentialStore.cs +++ b/CredentialCache/Storage/MacOsCredentialStore.cs @@ -55,9 +55,9 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) try { - byte[] blob = new byte[length]; - Marshal.Copy(passwordPtr, blob, 0, (int)length); - credential = CredentialSerialization.DeserializeAndScrub(blob); + // SecKeychainItemFreeContent releases the password data without scrubbing it, so the + // plaintext is zeroed here first, along with the managed copy it was read into. + credential = NativeSecretBuffer.ReadCredentialAndZero(passwordPtr, checked((int)length)); return credential is not null; } finally diff --git a/CredentialCache/Storage/NativeSecretBuffer.cs b/CredentialCache/Storage/NativeSecretBuffer.cs index 91d1b84..f1b0f28 100644 --- a/CredentialCache/Storage/NativeSecretBuffer.cs +++ b/CredentialCache/Storage/NativeSecretBuffer.cs @@ -181,6 +181,51 @@ internal static NativeSecretBuffer OfCredential(Credential credential) return blob.Length == 0 ? null : CredentialSerialization.DeserializeAndScrub(blob); } + /// + /// Deserializes the plaintext bytes a native credential API + /// returned at , then zeroes both the managed copy and the + /// native memory itself. + /// + /// + /// Writable memory the native API allocated and the caller will free, holding the + /// plaintext credential blob. + /// + /// The number of plaintext bytes at . + /// + /// The credential, or when addresses + /// nothing, is zero, or the bytes are not a known credential. + /// + /// + /// The read path for a store whose native API hands back a pointer and a length, such as + /// CredRead or SecKeychainFindGenericPassword. Neither CredFree nor + /// SecKeychainItemFreeContent scrubs what it releases, so a store that only + /// scrubbed its managed copy left a full plaintext copy in freed native heap on every + /// load (#173). The native bytes are zeroed on every path out, the unparseable one + /// included, while the memory is still allocated; the caller frees it afterwards. + /// + internal static Credential? ReadCredentialAndZero(IntPtr pointer, int length) + { + ArgumentOutOfRangeException.ThrowIfNegative(length); + + if (pointer == IntPtr.Zero || length == 0) + { + return null; + } + + try + { + byte[] blob = new byte[length]; + Marshal.Copy(pointer, blob, 0, length); + return CredentialSerialization.DeserializeAndScrub(blob); + } + finally + { + // As in Zero(): Marshal.Copy into unmanaged memory is an opaque interop call the + // runtime cannot elide as a dead store. + Marshal.Copy(new byte[length], 0, pointer, length); + } + } + /// /// Copies the nul-terminated bytes at into a managed array, /// excluding the terminator. diff --git a/CredentialCache/Storage/WindowsCredentialStore.cs b/CredentialCache/Storage/WindowsCredentialStore.cs index 3baf6d6..b06659e 100644 --- a/CredentialCache/Storage/WindowsCredentialStore.cs +++ b/CredentialCache/Storage/WindowsCredentialStore.cs @@ -55,9 +55,9 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) return false; } - byte[] blob = new byte[cred.CredentialBlobSize]; - Marshal.Copy(cred.CredentialBlob, blob, 0, blob.Length); - credential = CredentialSerialization.DeserializeAndScrub(blob); + // CredFree releases the block without scrubbing it, so the plaintext blob inside it + // is zeroed here first, along with the managed copy it was read into. + credential = NativeSecretBuffer.ReadCredentialAndZero(cred.CredentialBlob, cred.CredentialBlobSize); return credential is not null; } finally