From c94da7b24be32e7443df302fc327fd56fa74bb00 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 5 Oct 2026 23:24:13 +0000 Subject: [PATCH] Zero an over-limit credential blob in the Windows store [patch] WindowsCredentialStore.Save checked the 2560-byte blob limit before its try/finally, so an oversize credential was rejected with its serialized plaintext left on the managed heap unscrubbed. The check now lives in NativeSecretBuffer.CopyWithinLimit, which zeroes the managed copy on every path, rejection included, and runs under test on every operating system. Fixes ktsu-dev/CredentialCache#165 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018fjR4mduqAKqmuCXhfcCbx --- .../NativeStoreScrubbingWiringTests.cs | 14 +++++ CredentialCache.Test/SecretScrubbingTests.cs | 31 +++++++++++ CredentialCache/Storage/NativeSecretBuffer.cs | 35 +++++++++++++ .../Storage/WindowsCredentialStore.cs | 51 ++++++++----------- 4 files changed, 100 insertions(+), 31 deletions(-) diff --git a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs index 2f2409e..137eb25 100644 --- a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs +++ b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs @@ -36,6 +36,7 @@ public class NativeStoreScrubbingWiringTests private static readonly MethodInfo ReadCredential = Buffer(nameof(NativeSecretBuffer.ReadCredential)); private static readonly MethodInfo OfCredential = Buffer(nameof(NativeSecretBuffer.OfCredential)); + private static readonly MethodInfo CopyWithinLimit = Buffer(nameof(NativeSecretBuffer.CopyWithinLimit)); private static MethodInfo Helper(string name) => typeof(CredentialSerialization).GetMethod(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static) @@ -133,6 +134,19 @@ public void EveryNativeStoreSavesFromScrubbableBytes() } } + /// + /// The Windows store is the one with a blob size limit. Checking it in the store body put + /// the throw ahead of the store's own try, so an over-limit credential was rejected + /// without its serialized plaintext ever being zeroed (#165). The shared helper owns the + /// check inside its scrubbing, where exercises it. + /// + [TestMethod] + public void TheWindowsStoreChecksItsBlobLimitInsideTheScrubbing() => + Assert.IsTrue( + Calls(Method(typeof(WindowsCredentialStore), nameof(ICredentialStore.Save)), CopyWithinLimit), + "WindowsCredentialStore.Save must copy its blob through NativeSecretBuffer.CopyWithinLimit, " + + "which zeroes the serialized plaintext on the over-limit rejection path as well."); + /// /// 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 diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs index ea3da40..6cab047 100644 --- a/CredentialCache.Test/SecretScrubbingTests.cs +++ b/CredentialCache.Test/SecretScrubbingTests.cs @@ -158,6 +158,37 @@ public void NativeSecretBufferHandlesAnEmptySource() public void NativeSecretBufferRejectsANullSource() => Assert.ThrowsExactly(() => NativeSecretBuffer.CopyOf(null!)); + [TestMethod] + public void CopyWithinLimitCopiesASourceAtTheLimitAndZeroesTheManagedCopy() + { + byte[] blob = SerializedCredential(); + byte[] expected = (byte[])blob.Clone(); + + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyWithinLimit(blob, blob.Length, "Test store"); + + Assert.AreSequenceEqual(expected, ReadUnmanaged(buffer)); + Assert.AreSequenceEqual(new byte[blob.Length], blob, + "The managed plaintext must be zeroed once it has been copied out."); + } + + [TestMethod] + public void CopyWithinLimitRejectsAnOversizeSourceAndStillZeroesIt() + { + byte[] blob = SerializedCredential(); + + CredentialStoreException ex = Assert.ThrowsExactly( + () => NativeSecretBuffer.CopyWithinLimit(blob, blob.Length - 1, "Test store")); + + StringAssert.Contains(ex.Message, "Test store"); + StringAssert.Contains(ex.Message, $"(was {blob.Length})"); + Assert.AreSequenceEqual(new byte[blob.Length], blob, + "An over-limit credential must be scrubbed on the rejection path too."); + } + + [TestMethod] + public void CopyWithinLimitRejectsANullSource() => + Assert.ThrowsExactly(() => NativeSecretBuffer.CopyWithinLimit(null!, 1, "Test store")); + [TestMethod] public void NulTerminatedCopyOfAppendsTheTerminator() { diff --git a/CredentialCache/Storage/NativeSecretBuffer.cs b/CredentialCache/Storage/NativeSecretBuffer.cs index 746c4d7..91d1b84 100644 --- a/CredentialCache/Storage/NativeSecretBuffer.cs +++ b/CredentialCache/Storage/NativeSecretBuffer.cs @@ -60,6 +60,41 @@ internal static NativeSecretBuffer CopyOf(byte[] source) return new NativeSecretBuffer(pointer, source.Length); } + /// + /// Copies into newly allocated unmanaged memory, provided it fits + /// within , and zeroes before returning. + /// + /// The plaintext bytes to copy. Always zeroed, whether or not this throws. + /// The largest blob the native store accepts, in bytes. + /// The store's name, for the message of an over-limit exception. + /// A buffer owning the unmanaged copy, which the caller must dispose. + /// is longer than . + /// + /// A store that checks the limit itself is tempted to do it before its own try, which + /// leaves the plaintext on the managed heap unscrubbed whenever the check throws. Taking the + /// managed copy over here keeps the rejection path inside the scrubbing, and keeps it + /// exercised by tests on every operating system rather than only the store's own. + /// + internal static NativeSecretBuffer CopyWithinLimit(byte[] source, int maxLength, string storeName) + { + ArgumentNullException.ThrowIfNull(source); + + try + { + if (source.Length > maxLength) + { + throw new CredentialStoreException( + $"Credential exceeds the {storeName} blob size limit of {maxLength} bytes (was {source.Length})."); + } + + return CopyOf(source); + } + finally + { + CredentialSerialization.Zero(source); + } + } + /// /// Copies into newly allocated unmanaged memory followed by a /// single nul byte, for a native API that takes a C string rather than a pointer and a diff --git a/CredentialCache/Storage/WindowsCredentialStore.cs b/CredentialCache/Storage/WindowsCredentialStore.cs index ba3526b..3baf6d6 100644 --- a/CredentialCache/Storage/WindowsCredentialStore.cs +++ b/CredentialCache/Storage/WindowsCredentialStore.cs @@ -72,39 +72,28 @@ public void Save(PersonaGUID persona, Credential credential) ArgumentNullException.ThrowIfNull(persona); ArgumentNullException.ThrowIfNull(credential); - byte[] blob = CredentialSerialization.Serialize(credential); - if (blob.Length > NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE) + // CopyWithinLimit zeroes the serialized bytes on every path, the over-limit rejection + // included. The unmanaged copy handed to CredWriteW is plaintext too; NativeSecretBuffer + // zeroes it before freeing it, which Marshal.FreeHGlobal alone does not. + using NativeSecretBuffer nativeBlob = NativeSecretBuffer.CopyWithinLimit( + CredentialSerialization.Serialize(credential), + NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE, + Name); + + NativeMethods.CREDENTIAL native = new() { - throw new CredentialStoreException( - $"Credential exceeds the Windows Credential Manager blob size limit of " + - $"{NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE} bytes (was {blob.Length})."); - } - - try + Type = NativeMethods.CRED_TYPE_GENERIC, + TargetName = TargetFor(persona), + CredentialBlob = nativeBlob.Pointer, + CredentialBlobSize = nativeBlob.Length, + Persist = NativeMethods.CRED_PERSIST_LOCAL_MACHINE, + UserName = Environment.UserName, + }; + + if (!NativeMethods.CredWrite(ref native, 0)) { - // 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 = nativeBlob.Pointer, - CredentialBlobSize = nativeBlob.Length, - Persist = NativeMethods.CRED_PERSIST_LOCAL_MACHINE, - UserName = Environment.UserName, - }; - - if (!NativeMethods.CredWrite(ref native, 0)) - { - int err = Marshal.GetLastWin32Error(); - throw new CredentialStoreException($"CredWrite failed for '{persona}'.", new Win32Exception(err)); - } - } - finally - { - CredentialSerialization.Zero(blob); + int err = Marshal.GetLastWin32Error(); + throw new CredentialStoreException($"CredWrite failed for '{persona}'.", new Win32Exception(err)); } }