Zero an over-limit credential blob in the Windows store [patch]

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()
 		}
 	}
 
+	/// <summary>
+	/// 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 <c>try</c>, 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 <see cref="SecretScrubbingTests"/> exercises it.
+	/// </summary>
+	[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.");
+
 	/// <summary>
 	/// 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

CredentialCache.Test/SecretScrubbingTests.cs

@@ -158,6 +158,37 @@
 	public void NativeSecretBufferRejectsANullSource() =>
 		Assert.ThrowsExactly<ArgumentNullException>(() => 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<CredentialStoreException>(
+			() => 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<ArgumentNullException>(() => NativeSecretBuffer.CopyWithinLimit(null!, 1, "Test store"));
+
 	[TestMethod]
 	public void NulTerminatedCopyOfAppendsTheTerminator()
 	{

CredentialCache/Storage/NativeSecretBuffer.cs

@@ -60,6 +60,41 @@ internal static NativeSecretBuffer CopyOf(byte[] source)
 		return new NativeSecretBuffer(pointer, source.Length);
 	}
 
+	/// <summary>
+	/// Copies <paramref name="source"/> into newly allocated unmanaged memory, provided it fits
+	/// within <paramref name="maxLength"/>, and zeroes <paramref name="source"/> before returning.
+	/// </summary>
+	/// <param name="source">The plaintext bytes to copy. Always zeroed, whether or not this throws.</param>
+	/// <param name="maxLength">The largest blob the native store accepts, in bytes.</param>
+	/// <param name="storeName">The store's name, for the message of an over-limit exception.</param>
+	/// <returns>A buffer owning the unmanaged copy, which the caller must dispose.</returns>
+	/// <exception cref="CredentialStoreException"><paramref name="source"/> is longer than <paramref name="maxLength"/>.</exception>
+	/// <remarks>
+	/// A store that checks the limit itself is tempted to do it before its own <c>try</c>, 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.
+	/// </remarks>
+	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);
+		}
+	}
+
 	/// <summary>
 	/// Copies <paramref name="source"/> 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

CredentialCache/Storage/WindowsCredentialStore.cs

@@ -72,39 +72,28 @@
 		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));
 		}
 	}
 
@@ -172,7 +161,7 @@
 		internal const int CRED_MAX_CREDENTIAL_BLOB_SIZE = 5 * 512;
 
 		[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
 		internal struct CREDENTIAL
 		{
 			public int Flags;
 			public int Type;