diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs new file mode 100644 index 0000000..a9ecb01 --- /dev/null +++ b/CredentialCache.Test/SecretScrubbingTests.cs @@ -0,0 +1,184 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.CredentialCache.Test; + +using System.Runtime.InteropServices; +using ktsu.CredentialCache.Storage; +using ktsu.Semantics.Strings; + +/// +/// Covers the two primitives the platform-native stores use to keep plaintext +/// credential bytes from outliving the call that read or wrote them: +/// for the managed +/// copy a store deserializes from, and for the +/// unmanaged copy a store hands to a native API. +/// +/// These run on every platform. The native stores themselves are only reachable on +/// their own OS, so the scrubbing lives in these two shared primitives rather than +/// being re-implemented (and left untested) in each store. +/// +[TestClass] +public class SecretScrubbingTests +{ + private static byte[] SerializedCredential() => + CredentialSerialization.Serialize(new CredentialWithToken + { + Token = SemanticString.Create("plaintext-token-to-scrub"), + }); + + private static byte[] ReadUnmanaged(NativeSecretBuffer buffer) + { + byte[] copy = new byte[buffer.Length]; + Marshal.Copy(buffer.Pointer, copy, 0, buffer.Length); + return copy; + } + + [TestMethod] + public void DeserializeAndScrubReturnsTheCredential() + { + byte[] blob = SerializedCredential(); + + Credential? credential = CredentialSerialization.DeserializeAndScrub(blob); + + CredentialWithToken? typed = credential as CredentialWithToken; + Assert.IsNotNull(typed); + Assert.AreEqual("plaintext-token-to-scrub", typed!.Token.ToString()); + } + + [TestMethod] + public void DeserializeAndScrubZeroesTheManagedCopy() + { + byte[] blob = SerializedCredential(); + Assert.AreNotSequenceEqual(new byte[blob.Length], blob, "Precondition: the blob starts out as plaintext."); + + _ = CredentialSerialization.DeserializeAndScrub(blob); + + Assert.AreSequenceEqual(new byte[blob.Length], blob, + "The plaintext blob must be zeroed once it has been deserialized."); + } + + [TestMethod] + public void DeserializeAndScrubZeroesTheManagedCopyForUnparseableBytes() + { + // A blob that isn't a credential still came out of the platform store, so it + // is still secret-bearing and must be scrubbed on the failure path too. + byte[] blob = [.. "{ not a credential"u8]; + + Credential? credential = CredentialSerialization.DeserializeAndScrub(blob); + + Assert.IsNull(credential); + Assert.AreSequenceEqual(new byte[blob.Length], blob); + } + + [TestMethod] + public void NativeSecretBufferCopiesTheSourceBytes() + { + byte[] blob = SerializedCredential(); + + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(blob); + + Assert.AreEqual(blob.Length, buffer.Length); + Assert.AreNotEqual(IntPtr.Zero, buffer.Pointer); + Assert.AreSequenceEqual(blob, ReadUnmanaged(buffer)); + } + + [TestMethod] + public void NativeSecretBufferZeroOverwritesThePlaintextWhileStillAllocated() + { + byte[] blob = SerializedCredential(); + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(blob); + Assert.AreSequenceEqual(blob, ReadUnmanaged(buffer), "Precondition: the unmanaged copy is plaintext."); + + buffer.Zero(); + + // Read back before Dispose - reading freed memory would be undefined, so the + // scrub has to be observable while the allocation is still live. This is the + // exact ordering Dispose relies on: zero, then free. + Assert.AreSequenceEqual(new byte[blob.Length], ReadUnmanaged(buffer), + "The unmanaged copy must be zeroed before the memory is released."); + } + + [TestMethod] + public void NativeSecretBufferZeroIsIdempotent() + { + byte[] blob = SerializedCredential(); + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(blob); + + buffer.Zero(); + buffer.Zero(); + + Assert.AreSequenceEqual(new byte[blob.Length], ReadUnmanaged(buffer)); + } + + [TestMethod] + public void NativeSecretBufferDisposeReleasesThePointer() + { + NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(SerializedCredential()); + + buffer.Dispose(); + + Assert.AreEqual(IntPtr.Zero, buffer.Pointer, "A disposed buffer must not keep a dangling pointer."); + Assert.AreEqual(0, buffer.Pointer.ToInt64()); + } + + [TestMethod] + public void NativeSecretBufferDisposeIsIdempotent() + { + NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(SerializedCredential()); + + buffer.Dispose(); + buffer.Dispose(); + + Assert.AreEqual(IntPtr.Zero, buffer.Pointer); + } + + [TestMethod] + public void NativeSecretBufferZeroAfterDisposeDoesNotTouchFreedMemory() + { + NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf(SerializedCredential()); + buffer.Dispose(); + + // Must be a no-op rather than a write through a freed pointer. + buffer.Zero(); + + Assert.AreEqual(IntPtr.Zero, buffer.Pointer); + } + + [TestMethod] + public void NativeSecretBufferHandlesAnEmptySource() + { + using NativeSecretBuffer buffer = NativeSecretBuffer.CopyOf([]); + + Assert.AreEqual(0, buffer.Length); + Assert.AreNotEqual(IntPtr.Zero, buffer.Pointer); + buffer.Zero(); + } + + [TestMethod] + public void NativeSecretBufferRejectsANullSource() => + Assert.ThrowsExactly(() => NativeSecretBuffer.CopyOf(null!)); + + [TestMethod] + public void ZeroOverwritesTheBuffer() + { + byte[] blob = SerializedCredential(); + + CredentialSerialization.Zero(blob); + + Assert.AreSequenceEqual(new byte[blob.Length], blob); + } + + [TestMethod] + public void ZeroToleratesEmptyAndNullBuffers() + { + byte[] empty = []; + + // The guard clauses exist so a store can scrub whatever it has without + // length- or null-checking first. Both calls returning rather than throwing + // is the behaviour under test; an exception from either fails the test. + CredentialSerialization.Zero(empty); + CredentialSerialization.Zero(null!); + + Assert.IsEmpty(empty); + } +} diff --git a/CredentialCache/Storage/CredentialSerialization.cs b/CredentialCache/Storage/CredentialSerialization.cs index 875f07f..1b2aba4 100644 --- a/CredentialCache/Storage/CredentialSerialization.cs +++ b/CredentialCache/Storage/CredentialSerialization.cs @@ -2,6 +2,7 @@ namespace ktsu.CredentialCache.Storage; +using System.Security.Cryptography; using System.Text.Json; using ktsu.RoundTripStringJsonConverter; @@ -60,6 +61,49 @@ public static string SerializeToString(Credential credential) => } } + /// + /// Deserializes a credential from a UTF-8 JSON byte array and then overwrites + /// with zeros, whether or not deserialization succeeds. + /// + /// + /// Platform stores copy the stored secret into a managed array in order to + /// deserialize it. That array holds plaintext, and left alone it lingers on the + /// managed heap - subject to GC promotion and compaction - until it is eventually + /// collected, where it can still be read out of a crash dump. Store implementations + /// should read through this helper rather than calling + /// directly, so no plaintext copy outlives the call. + /// + internal static Credential? DeserializeAndScrub(byte[] utf8Json) + { + if (utf8Json is null) + { + return null; + } + + try + { + return Deserialize(utf8Json); + } + finally + { + Zero(utf8Json); + } + } + + /// + /// Overwrites with zeros in a way the runtime cannot + /// discard as a dead store. + /// + internal static void Zero(byte[] buffer) + { + if (buffer is null || buffer.Length == 0) + { + return; + } + + CryptographicOperations.ZeroMemory(buffer); + } + /// /// Deserializes a credential from a UTF-8 JSON string. Returns null if the value /// does not represent a known credential. diff --git a/CredentialCache/Storage/MacOsCredentialStore.cs b/CredentialCache/Storage/MacOsCredentialStore.cs index ff8ffb4..242b601 100644 --- a/CredentialCache/Storage/MacOsCredentialStore.cs +++ b/CredentialCache/Storage/MacOsCredentialStore.cs @@ -57,8 +57,7 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) { byte[] blob = new byte[length]; Marshal.Copy(passwordPtr, blob, 0, (int)length); - credential = CredentialSerialization.Deserialize(blob); - Array.Clear(blob, 0, blob.Length); + credential = CredentialSerialization.DeserializeAndScrub(blob); return credential is not null; } finally @@ -131,7 +130,7 @@ public void Save(PersonaGUID persona, Credential credential) } finally { - Array.Clear(blob, 0, blob.Length); + CredentialSerialization.Zero(blob); } } diff --git a/CredentialCache/Storage/NativeSecretBuffer.cs b/CredentialCache/Storage/NativeSecretBuffer.cs new file mode 100644 index 0000000..289f880 --- /dev/null +++ b/CredentialCache/Storage/NativeSecretBuffer.cs @@ -0,0 +1,94 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.CredentialCache.Storage; + +using System.Runtime.InteropServices; + +/// +/// An unmanaged copy of a plaintext credential blob that is overwritten with zeros +/// before the memory is released. +/// +/// +/// hands memory back to the process heap +/// without scrubbing it, so plaintext passed to a native credential API survives in +/// the process image until that allocation happens to be reused - long enough to be +/// recovered by a memory scanner, or to land in a crash dump, hibernation file, or +/// page file. Owning the copy through this type zeroes the bytes first, on every path +/// out of the caller. +/// +internal sealed class NativeSecretBuffer : IDisposable +{ + private NativeSecretBuffer(IntPtr pointer, int length) + { + Pointer = pointer; + Length = length; + } + + /// + /// Gets the number of bytes copied into unmanaged memory. + /// + internal int Length { get; } + + /// + /// Gets a pointer to the unmanaged copy, or once the + /// buffer has been disposed. + /// + internal IntPtr Pointer { get; private set; } + + /// + /// Copies into newly allocated unmanaged memory. + /// + /// The plaintext bytes to copy. + /// A buffer owning the unmanaged copy, which the caller must dispose. + internal static NativeSecretBuffer CopyOf(byte[] source) + { + ArgumentNullException.ThrowIfNull(source); + + // AllocHGlobal(0) is implementation defined, so keep at least one byte and the + // pointer handed to native code is always valid. + IntPtr pointer = Marshal.AllocHGlobal(Math.Max(source.Length, 1)); + try + { + Marshal.Copy(source, 0, pointer, source.Length); + } + catch + { + Marshal.FreeHGlobal(pointer); + throw; + } + + return new NativeSecretBuffer(pointer, source.Length); + } + + /// + /// Overwrites the unmanaged copy with zeros while the memory is still allocated. + /// Idempotent, and a no-op once the buffer has been disposed. + /// + internal void Zero() + { + if (Pointer == IntPtr.Zero || Length == 0) + { + return; + } + + // Marshal.Copy into unmanaged memory is an opaque interop call, so unlike a + // managed Array.Clear the runtime cannot elide it as a dead store. The source + // is a fresh zero-filled array, which holds no secret of its own. + Marshal.Copy(new byte[Length], 0, Pointer, Length); + } + + /// + /// Zeroes the unmanaged copy and then releases it. + /// + public void Dispose() + { + if (Pointer == IntPtr.Zero) + { + return; + } + + Zero(); + Marshal.FreeHGlobal(Pointer); + Pointer = IntPtr.Zero; + } +} diff --git a/CredentialCache/Storage/WindowsCredentialStore.cs b/CredentialCache/Storage/WindowsCredentialStore.cs index 25b7ea6..ba3526b 100644 --- a/CredentialCache/Storage/WindowsCredentialStore.cs +++ b/CredentialCache/Storage/WindowsCredentialStore.cs @@ -57,7 +57,7 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) byte[] blob = new byte[cred.CredentialBlobSize]; Marshal.Copy(cred.CredentialBlob, blob, 0, blob.Length); - credential = CredentialSerialization.Deserialize(blob); + credential = CredentialSerialization.DeserializeAndScrub(blob); return credential is not null; } finally @@ -80,17 +80,18 @@ public void Save(PersonaGUID persona, Credential credential) $"{NativeMethods.CRED_MAX_CREDENTIAL_BLOB_SIZE} bytes (was {blob.Length})."); } - IntPtr blobPtr = Marshal.AllocHGlobal(blob.Length); try { - Marshal.Copy(blob, 0, blobPtr, blob.Length); + // 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 = blobPtr, - CredentialBlobSize = blob.Length, + CredentialBlob = nativeBlob.Pointer, + CredentialBlobSize = nativeBlob.Length, Persist = NativeMethods.CRED_PERSIST_LOCAL_MACHINE, UserName = Environment.UserName, }; @@ -103,8 +104,7 @@ public void Save(PersonaGUID persona, Credential credential) } finally { - Marshal.FreeHGlobal(blobPtr); - Array.Clear(blob, 0, blob.Length); + CredentialSerialization.Zero(blob); } }