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