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