Zero the native plaintext TryGet loads before freeing it [patch]

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()
 		}
 	}
 
+	/// <summary>
+	/// <c>CredFree</c> and <c>SecKeychainItemFreeContent</c> 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
+	/// <c>secret_password_free</c> wipes its own copy.
+	/// </summary>
+	[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.");
+		}
+	}
+
 	/// <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
@@ -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 "

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()
 	{

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

CredentialCache/Storage/NativeSecretBuffer.cs

@@ -181,6 +181,51 @@ internal static NativeSecretBuffer OfCredential(Credential credential)
 		return blob.Length == 0 ? null : CredentialSerialization.DeserializeAndScrub(blob);
 	}
 
+	/// <summary>
+	/// Deserializes the <paramref name="length"/> plaintext bytes a native credential API
+	/// returned at <paramref name="pointer"/>, then zeroes both the managed copy and the
+	/// native memory itself.
+	/// </summary>
+	/// <param name="pointer">
+	/// Writable memory the native API allocated and the caller will free, holding the
+	/// plaintext credential blob.
+	/// </param>
+	/// <param name="length">The number of plaintext bytes at <paramref name="pointer"/>.</param>
+	/// <returns>
+	/// The credential, or <see langword="null"/> when <paramref name="pointer"/> addresses
+	/// nothing, <paramref name="length"/> is zero, or the bytes are not a known credential.
+	/// </returns>
+	/// <remarks>
+	/// The read path for a store whose native API hands back a pointer and a length, such as
+	/// <c>CredRead</c> or <c>SecKeychainFindGenericPassword</c>. Neither <c>CredFree</c> nor
+	/// <c>SecKeychainItemFreeContent</c> 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.
+	/// </remarks>
+	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);
+		}
+	}
+
 	/// <summary>
 	/// Copies the nul-terminated bytes at <paramref name="pointer"/> into a managed array,
 	/// excluding the terminator.

CredentialCache/Storage/WindowsCredentialStore.cs

@@ -55,9 +55,9 @@
 				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
@@ -161,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;