From 97a49ab0107d3a3a0f5d12c112f2ae2a60ba7a13 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 20:49:14 +0000 Subject: [PATCH 1/2] fix: scrub Linux Secret Service plaintext like Windows and macOS [patch] The Linux store routed every credential through a managed string. TryLoad read the secret with Marshal.PtrToStringUTF8 and deserialized it with the non-scrubbing DeserializeFromString; Save built the plaintext JSON as a string and let the marshaller make a native copy it frees unscrubbed. A managed string is immutable, so neither copy could be zeroed - the plaintext sat on the GC heap, movable by compaction and readable from a crash dump, for an unbounded time after use. Windows got this treatment in #144 and macOS already had it; Linux was left behind. Both paths now use byte arrays and unmanaged buffers this code owns and zeroes, matching the other two stores. secret_password_store_sync takes the password as an IntPtr rather than a string, so the runtime cannot make an unscrubbed copy of its own. NativeSecretBuffer gains the two primitives that needs, because libsecret deals in nul-terminated C strings where the other platforms pass a pointer and a length: NulTerminatedCopyOf, which appends the terminator and covers it in Length so Zero scrubs the whole allocation, and ReadNulTerminated, which copies up to the terminator into an array the caller scrubs. Also pins all three stores to the scrubbing helpers by inspecting which one each is compiled against. That is what was missing: the primitives were tested, but nothing checked that a store called them, which is how one platform stayed on the string path through two releases. Fixes #160 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LLBd6CLjwb1r6wuDqDzW4J --- .../NativeStoreScrubbingWiringTests.cs | 123 ++++++++++++++++++ CredentialCache.Test/SecretScrubbingTests.cs | 84 ++++++++++++ .../LinuxSecretServiceCredentialStore.cs | 73 ++++++++--- CredentialCache/Storage/NativeSecretBuffer.cs | 78 +++++++++++ README.md | 1 + 5 files changed, 338 insertions(+), 21 deletions(-) create mode 100644 CredentialCache.Test/NativeStoreScrubbingWiringTests.cs diff --git a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs new file mode 100644 index 0000000..b0c5fed --- /dev/null +++ b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs @@ -0,0 +1,123 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.CredentialCache.Test; + +using System.Reflection; +using ktsu.CredentialCache.Storage; + +/// +/// Pins every platform-native store to the scrubbing helpers rather than the +/// string-based ones. +/// +/// +/// +/// proves the scrubbing primitives work. Nothing +/// proved that each store actually calls them, and that gap is what this class closes: +/// the Windows store was moved onto them in #144 and the Linux store was left behind +/// until #160, because a store can only be exercised on its own operating system and +/// two of the three are therefore unreachable from any single CI leg. +/// +/// +/// Whether a secret was routed through an immutable managed is not +/// observable at runtime — the plaintext's problem is precisely that it lingers where +/// nothing can see or reach it — so this inspects which helper each store is compiled +/// against instead. The "must call" assertions double as a check on the IL scan itself: +/// a scan that found nothing would fail them rather than quietly passing the "must not +/// call" half. +/// +/// +[TestClass] +public class NativeStoreScrubbingWiringTests +{ + private static readonly MethodInfo DeserializeAndScrub = Helper(nameof(CredentialSerialization.DeserializeAndScrub)); + private static readonly MethodInfo DeserializeFromString = Helper(nameof(CredentialSerialization.DeserializeFromString)); + private static readonly MethodInfo Serialize = Helper(nameof(CredentialSerialization.Serialize)); + private static readonly MethodInfo SerializeToString = Helper(nameof(CredentialSerialization.SerializeToString)); + + private static MethodInfo Helper(string name) => + typeof(CredentialSerialization).GetMethod(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static) + ?? throw new InvalidOperationException($"{nameof(CredentialSerialization)}.{name} not found."); + + private static IEnumerable NativeStores() => + [ + typeof(WindowsCredentialStore), + typeof(MacOsCredentialStore), + typeof(LinuxSecretServiceCredentialStore), + ]; + + private static MethodInfo Method(Type store, string name) => + store.GetMethod(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance) + ?? throw new InvalidOperationException($"{store.Name}.{name} not found."); + + /// + /// Reports whether 's body contains a call or + /// callvirt to . + /// + /// + /// Both live in this assembly, so the call site carries the target's own MethodDef + /// token and matching on it needs no opcode table. A byte sequence that happened to + /// look like such a call without being one could only make an assertion stricter, + /// never let a violation through. + /// + private static bool Calls(MethodInfo method, MethodInfo target) + { + byte[] il = method.GetMethodBody()?.GetILAsByteArray() + ?? throw new InvalidOperationException($"No IL available for {method.DeclaringType?.Name}.{method.Name}."); + Assert.AreEqual( + method.Module, + target.Module, + "The scan matches a MethodDef token, so both methods must live in one module."); + + byte[] token = BitConverter.GetBytes(target.MetadataToken); + + for (int i = 0; i + 5 <= il.Length; i++) + { + if (il[i] is not (0x28 or 0x6F)) + { + continue; + } + + if (il[i + 1] == token[0] && il[i + 2] == token[1] && il[i + 3] == token[2] && il[i + 4] == token[3]) + { + return true; + } + } + + return false; + } + + [TestMethod] + public void EveryNativeStoreLoadsThroughTheScrubbingDeserializer() + { + foreach (Type store in NativeStores()) + { + MethodInfo tryLoad = Method(store, nameof(ICredentialStore.TryLoad)); + + Assert.IsTrue( + Calls(tryLoad, DeserializeAndScrub), + $"{store.Name}.TryLoad must deserialize through DeserializeAndScrub so the plaintext " + + "copy it read is zeroed before the call returns."); + Assert.IsFalse( + Calls(tryLoad, DeserializeFromString), + $"{store.Name}.TryLoad must not deserialize from a string: an immutable managed string " + + "holds the plaintext until the GC happens to collect it, and cannot be scrubbed."); + } + } + + [TestMethod] + public void EveryNativeStoreSavesFromScrubbableBytes() + { + foreach (Type store in NativeStores()) + { + MethodInfo save = Method(store, nameof(ICredentialStore.Save)); + + Assert.IsTrue( + Calls(save, Serialize), + $"{store.Name}.Save must serialize to a byte array it can zero once the native call returns."); + Assert.IsFalse( + Calls(save, SerializeToString), + $"{store.Name}.Save must not serialize to a string: the plaintext would sit on the managed " + + "heap unscrubbable, and marshalling it would add a native copy freed without scrubbing."); + } + } +} diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs index a9ecb01..5870d29 100644 --- a/CredentialCache.Test/SecretScrubbingTests.cs +++ b/CredentialCache.Test/SecretScrubbingTests.cs @@ -158,6 +158,90 @@ public void NativeSecretBufferHandlesAnEmptySource() public void NativeSecretBufferRejectsANullSource() => Assert.ThrowsExactly(() => NativeSecretBuffer.CopyOf(null!)); + [TestMethod] + public void NulTerminatedCopyOfAppendsTheTerminator() + { + byte[] blob = SerializedCredential(); + + using NativeSecretBuffer buffer = NativeSecretBuffer.NulTerminatedCopyOf(blob); + + Assert.AreEqual(blob.Length + 1, buffer.Length, "The terminator has to be part of the allocation."); + Assert.AreSequenceEqual([.. blob, (byte)0], ReadUnmanaged(buffer)); + } + + [TestMethod] + public void NulTerminatedCopyOfZeroesTheWholeAllocationIncludingTheTerminator() + { + byte[] blob = SerializedCredential(); + using NativeSecretBuffer buffer = NativeSecretBuffer.NulTerminatedCopyOf(blob); + + buffer.Zero(); + + // Length covers the terminator, so a scrub that used the payload length would + // leave the last byte alone. Read back before Dispose, as above. + Assert.AreSequenceEqual(new byte[blob.Length + 1], ReadUnmanaged(buffer)); + } + + [TestMethod] + public void NulTerminatedCopyOfRejectsASourceCarryingItsOwnNul() + { + // A native C-string API would read a truncated secret and store it, which fails + // silently at save time and surfaces as an unparseable blob on the next load. + byte[] blob = [.. "{\"x\":1}"u8, 0, .. "tail"u8]; + + ArgumentException thrown = Assert.ThrowsExactly( + () => NativeSecretBuffer.NulTerminatedCopyOf(blob)); + + Assert.AreEqual("source", thrown.ParamName); + } + + [TestMethod] + public void NulTerminatedCopyOfHandlesAnEmptySource() + { + using NativeSecretBuffer buffer = NativeSecretBuffer.NulTerminatedCopyOf([]); + + Assert.AreEqual(1, buffer.Length); + Assert.AreSequenceEqual(new byte[] { 0 }, ReadUnmanaged(buffer)); + } + + [TestMethod] + public void ReadNulTerminatedCopiesUpToTheTerminatorOnly() + { + byte[] blob = SerializedCredential(); + // Trailing bytes past the terminator stand in for whatever else the native + // allocation happens to hold; none of it is part of the secret. + using NativeSecretBuffer stored = NativeSecretBuffer.CopyOf([.. blob, 0, .. "trailing"u8]); + + byte[] read = NativeSecretBuffer.ReadNulTerminated(stored.Pointer); + + Assert.AreSequenceEqual(blob, read); + } + + [TestMethod] + public void ReadNulTerminatedRoundTripsACredentialWithoutAManagedString() + { + // The exact composition a C-string store performs: read the bytes, then scrub + // the managed copy. Nothing in between is a string. + using NativeSecretBuffer stored = NativeSecretBuffer.NulTerminatedCopyOf(SerializedCredential()); + + byte[] read = NativeSecretBuffer.ReadNulTerminated(stored.Pointer); + Credential? credential = CredentialSerialization.DeserializeAndScrub(read); + + CredentialWithToken? typed = credential as CredentialWithToken; + Assert.IsNotNull(typed); + Assert.AreEqual("plaintext-token-to-scrub", typed!.Token.ToString()); + Assert.AreSequenceEqual(new byte[read.Length], read, "The managed copy must be zeroed once deserialized."); + } + + [TestMethod] + public void ReadNulTerminatedReturnsEmptyForAnEmptyStringAndForNull() + { + using NativeSecretBuffer empty = NativeSecretBuffer.CopyOf([0]); + + Assert.IsEmpty(NativeSecretBuffer.ReadNulTerminated(empty.Pointer)); + Assert.IsEmpty(NativeSecretBuffer.ReadNulTerminated(IntPtr.Zero)); + } + [TestMethod] public void ZeroOverwritesTheBuffer() { diff --git a/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs b/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs index 3b3076c..051bc9b 100644 --- a/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs +++ b/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs @@ -12,10 +12,19 @@ namespace ktsu.CredentialCache.Storage; /// service and account identifying it. /// /// +/// /// Requires libsecret to be installed on the host. On headless systems without /// a running secret-service implementation (e.g. minimal containers, build agents) /// this provider will fail at the first operation; consumers should detect this /// and fall back to if appropriate. +/// +/// +/// Plaintext credential bytes are scrubbed on the same terms as the Windows and macOS +/// stores: every copy this code owns is a array or a +/// that is zeroed on the way out, and no plaintext is +/// ever marshalled through a managed . libsecret scrubs its own +/// copy in secret_password_free. +/// /// [SupportedOSPlatform("linux")] internal sealed class LinuxSecretServiceCredentialStore : ICredentialStore @@ -57,16 +66,22 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) try { - string? value = Marshal.PtrToStringUTF8(passwordPtr); - if (string.IsNullOrEmpty(value)) + // The bytes go straight into a managed array rather than through + // Marshal.PtrToStringUTF8: an immutable string cannot be scrubbed, so the + // plaintext would outlive the call. This is the same byte-array-and-scrub path + // the Windows and macOS stores take; libsecret just hands back a C string + // instead of a pointer and a length. + byte[] blob = NativeSecretBuffer.ReadNulTerminated(passwordPtr); + if (blob.Length == 0) { return false; } - credential = CredentialSerialization.DeserializeFromString(value); + credential = CredentialSerialization.DeserializeAndScrub(blob); return credential is not null; } finally { + // secret_password_free wipes libsecret's own copy before releasing it. NativeMethods.secret_password_free(passwordPtr); } } @@ -77,26 +92,39 @@ public void Save(PersonaGUID persona, Credential credential) ArgumentNullException.ThrowIfNull(persona); ArgumentNullException.ThrowIfNull(credential); - string value = CredentialSerialization.SerializeToString(credential); string label = $"{_serviceName}:{persona}"; + byte[] blob = CredentialSerialization.Serialize(credential); - IntPtr error = IntPtr.Zero; - bool stored = NativeMethods.secret_password_store_sync( - Schema.Handle, - IntPtr.Zero, - label, - value, - IntPtr.Zero, - ref error, - "service", _serviceName, - "account", persona.ToString(), - IntPtr.Zero); - - ThrowIfError(error, "secret_password_store_sync"); - - if (!stored) + try + { + // The password is handed over as an unmanaged nul-terminated copy this code + // owns, not as a managed string. Marshalling a string would put the plaintext + // on the managed heap, where it cannot be scrubbed, and would leave the + // runtime's own native copy to be freed unscrubbed. + using NativeSecretBuffer nativeValue = NativeSecretBuffer.NulTerminatedCopyOf(blob); + + IntPtr error = IntPtr.Zero; + bool stored = NativeMethods.secret_password_store_sync( + Schema.Handle, + IntPtr.Zero, + label, + nativeValue.Pointer, + IntPtr.Zero, + ref error, + "service", _serviceName, + "account", persona.ToString(), + IntPtr.Zero); + + ThrowIfError(error, "secret_password_store_sync"); + + if (!stored) + { + throw new CredentialStoreException($"secret_password_store_sync returned false for '{persona}'."); + } + } + finally { - throw new CredentialStoreException($"secret_password_store_sync returned false for '{persona}'."); + CredentialSerialization.Zero(blob); } } @@ -175,12 +203,15 @@ internal static extern IntPtr secret_password_lookup_sync( [DllImport(Lib, CharSet = CharSet.Ansi)] internal static extern void secret_password_free(IntPtr password); + // password is an IntPtr, not a string, so the plaintext is never marshalled by the + // runtime into a copy it frees without scrubbing. The caller owns a + // NativeSecretBuffer for it. [DllImport(Lib, CharSet = CharSet.Ansi)] internal static extern bool secret_password_store_sync( IntPtr schema, IntPtr collection, string label, - string password, + IntPtr password, IntPtr cancellable, ref IntPtr error, string attribute1Name, string attribute1Value, diff --git a/CredentialCache/Storage/NativeSecretBuffer.cs b/CredentialCache/Storage/NativeSecretBuffer.cs index 289f880..dc13e12 100644 --- a/CredentialCache/Storage/NativeSecretBuffer.cs +++ b/CredentialCache/Storage/NativeSecretBuffer.cs @@ -60,6 +60,84 @@ internal static NativeSecretBuffer CopyOf(byte[] source) return new NativeSecretBuffer(pointer, source.Length); } + /// + /// Copies 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 + /// length. + /// + /// The plaintext bytes to copy. Must contain no nul byte of its own. + /// A buffer owning the unmanaged copy, which the caller must dispose. + /// + /// covers the terminator, so scrubs the whole + /// allocation. Marshalling a managed string would be the obvious alternative and is the + /// thing to avoid: the runtime frees the native copy it makes without scrubbing it, and + /// the immutable managed string it came from cannot be scrubbed at all. + /// + internal static NativeSecretBuffer NulTerminatedCopyOf(byte[] source) + { + ArgumentNullException.ThrowIfNull(source); + + if (Array.IndexOf(source, (byte)0) >= 0) + { + throw new ArgumentException( + "A nul-terminated copy cannot carry a nul byte of its own; the native call would read a truncated secret.", + nameof(source)); + } + + byte[] terminated = new byte[source.Length + 1]; + try + { + Buffer.BlockCopy(source, 0, terminated, 0, source.Length); + return CopyOf(terminated); + } + finally + { + // terminated is a second plaintext copy on the managed heap and has served its + // purpose by here, whether CopyOf succeeded or threw. + CredentialSerialization.Zero(terminated); + } + } + + /// + /// Copies the nul-terminated bytes at into a managed array, + /// excluding the terminator. + /// + /// A pointer to nul-terminated plaintext in unmanaged memory. + /// + /// The bytes before the first nul, or an empty array when is + /// or addresses an empty string. + /// + /// + /// The caller owns the returned array and is expected to hand it to + /// , which scrubs it. + /// This exists so a store whose native API returns a C string can take the same + /// byte-array-and-scrub path as one that returns a pointer and a length, instead of + /// going through and stranding the + /// plaintext in an immutable managed string. + /// + internal static byte[] ReadNulTerminated(IntPtr pointer) + { + if (pointer == IntPtr.Zero) + { + return []; + } + + int length = 0; + while (Marshal.ReadByte(pointer, length) != 0) + { + length++; + } + + if (length == 0) + { + return []; + } + + byte[] copy = new byte[length]; + Marshal.Copy(pointer, copy, 0, length); + return copy; + } + /// /// Overwrites the unmanaged copy with zeros while the memory is still allocated. /// Idempotent, and a no-op once the buffer has been disposed. diff --git a/README.md b/README.md index 98aadc7..6e09489 100644 --- a/README.md +++ b/README.md @@ -152,6 +152,7 @@ else - **macOS** uses the user's default login keychain. The first access from an application prompts the user for permission, as with any keychain client. - **Linux** requires `libsecret-1` plus an active Secret Service. Headless CI agents typically have neither — use `InMemoryCredentialStore` there, or set up `dbus-run-session` + `gnome-keyring-daemon` as the `cross-platform.yml` workflow does. - All native calls happen on the thread the API is invoked from. The library's in-memory cache is thread-safe (`ConcurrentDictionary`); the native APIs themselves are documented as thread-safe by their respective platform owners, but blocking calls (especially libsecret) are not cheap — treat `Save` / `Remove` as I/O, not as cheap accessors. +- All three native stores handle plaintext on the same terms: every copy the library owns is a byte array or an unmanaged buffer that is overwritten with zeros before it is released, and no secret is ever routed through a managed `string`, which is immutable and so cannot be scrubbed at all. A credential you hold yourself is a different matter — anything you read out of `TryGet` is an ordinary object on the managed heap and its lifetime is yours. ## API summary From d00757d1d018a328719b40733f160d2057d105f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 21:05:30 +0000 Subject: [PATCH 2/2] test: cover the Linux store's scrub path in the shared layer [patch] SonarCloud's quality gate failed on #162: 70.4% coverage on new code against a required 80%. Measured locally with cobertura, all 20 uncovered new lines were in LinuxSecretServiceCredentialStore's own body, which cannot execute without a Secret Service, while the new NativeSecretBuffer primitives were fully covered. Two causes, both addressed. Most of those 20 lines were the pre-existing secret_password_store_sync call, which the try/finally wrapper re-indented and so marked as new code. Save now takes a using declaration instead, so the call block is untouched except for the one argument that had to change. The rest was composition logic sitting in a method body no test can reach. NativeSecretBuffer.OfCredential and ReadCredential now own serialize-copy-scrub and read-deserialize-scrub, so the store's two methods are one call each and the logic is exercised by tests on every platform - the principle SecretScrubbingTests already states, applied to the part that was still in the store. New uncovered lines in the store drop from 20 to 3, and new-code line coverage measured locally rises from 58.7% to 90.6%. The remaining three are the store's own two statements and one call argument, which need libsecret to run. The wiring guard accepts either the direct helper or the new wrapper, and a third case pins the wrappers themselves to the scrubbing helpers, so the indirection it now allows cannot hide a string-based path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LLBd6CLjwb1r6wuDqDzW4J --- .../NativeStoreScrubbingWiringTests.cs | 47 +++++++++++-- CredentialCache.Test/SecretScrubbingTests.cs | 46 +++++++++++++ .../LinuxSecretServiceCredentialStore.cs | 66 +++++++------------ CredentialCache/Storage/NativeSecretBuffer.cs | 48 ++++++++++++++ 4 files changed, 161 insertions(+), 46 deletions(-) diff --git a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs index b0c5fed..2f2409e 100644 --- a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs +++ b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs @@ -34,10 +34,17 @@ public class NativeStoreScrubbingWiringTests private static readonly MethodInfo Serialize = Helper(nameof(CredentialSerialization.Serialize)); private static readonly MethodInfo SerializeToString = Helper(nameof(CredentialSerialization.SerializeToString)); + private static readonly MethodInfo ReadCredential = Buffer(nameof(NativeSecretBuffer.ReadCredential)); + private static readonly MethodInfo OfCredential = Buffer(nameof(NativeSecretBuffer.OfCredential)); + private static MethodInfo Helper(string name) => typeof(CredentialSerialization).GetMethod(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static) ?? throw new InvalidOperationException($"{nameof(CredentialSerialization)}.{name} not found."); + private static MethodInfo Buffer(string name) => + typeof(NativeSecretBuffer).GetMethod(name, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static) + ?? throw new InvalidOperationException($"{nameof(NativeSecretBuffer)}.{name} not found."); + private static IEnumerable NativeStores() => [ typeof(WindowsCredentialStore), @@ -93,10 +100,14 @@ public void EveryNativeStoreLoadsThroughTheScrubbingDeserializer() { MethodInfo tryLoad = Method(store, nameof(ICredentialStore.TryLoad)); + // Either directly, or through NativeSecretBuffer.ReadCredential, which the test + // below pins to the same helper. A store whose native API hands back a C string + // 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), - $"{store.Name}.TryLoad must deserialize through DeserializeAndScrub so the plaintext " - + "copy it read is zeroed before the call returns."); + Calls(tryLoad, DeserializeAndScrub) || Calls(tryLoad, ReadCredential), + $"{store.Name}.TryLoad must deserialize through DeserializeAndScrub, directly or via " + + "NativeSecretBuffer.ReadCredential, 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 " @@ -112,12 +123,38 @@ public void EveryNativeStoreSavesFromScrubbableBytes() MethodInfo save = Method(store, nameof(ICredentialStore.Save)); Assert.IsTrue( - Calls(save, Serialize), - $"{store.Name}.Save must serialize to a byte array it can zero once the native call returns."); + Calls(save, Serialize) || Calls(save, OfCredential), + $"{store.Name}.Save must serialize to bytes it can zero, directly or via " + + "NativeSecretBuffer.OfCredential."); Assert.IsFalse( Calls(save, SerializeToString), $"{store.Name}.Save must not serialize to a string: the plaintext would sit on the managed " + "heap unscrubbable, and marshalling it would add a native copy freed without scrubbing."); } } + + /// + /// 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 + /// asserted rather than assumed. + /// + [TestMethod] + public void TheSharedCredentialWrappersUseTheScrubbingHelpers() + { + Assert.IsTrue( + Calls(ReadCredential, DeserializeAndScrub), + "NativeSecretBuffer.ReadCredential must deserialize through DeserializeAndScrub; the store " + + "assertions above accept it as a stand-in for calling that helper directly."); + Assert.IsFalse( + Calls(ReadCredential, DeserializeFromString), + "NativeSecretBuffer.ReadCredential must not deserialize from a string."); + + Assert.IsTrue( + Calls(OfCredential, Serialize), + "NativeSecretBuffer.OfCredential must serialize to bytes it can zero; the store assertions " + + "above accept it as a stand-in for calling that helper directly."); + Assert.IsFalse( + Calls(OfCredential, SerializeToString), + "NativeSecretBuffer.OfCredential must not serialize to a string."); + } } diff --git a/CredentialCache.Test/SecretScrubbingTests.cs b/CredentialCache.Test/SecretScrubbingTests.cs index 5870d29..ea3da40 100644 --- a/CredentialCache.Test/SecretScrubbingTests.cs +++ b/CredentialCache.Test/SecretScrubbingTests.cs @@ -242,6 +242,52 @@ public void ReadNulTerminatedReturnsEmptyForAnEmptyStringAndForNull() Assert.IsEmpty(NativeSecretBuffer.ReadNulTerminated(IntPtr.Zero)); } + [TestMethod] + public void OfCredentialProducesTheSerializedCredentialNulTerminated() + { + using NativeSecretBuffer buffer = NativeSecretBuffer.OfCredential(new CredentialWithToken + { + Token = SemanticString.Create("plaintext-token-to-scrub"), + }); + + // Byte-for-byte what a C-string native API should receive: the same JSON the other two + // stores hand over as a pointer and a length, plus the terminator. + Assert.AreSequenceEqual([.. SerializedCredential(), (byte)0], ReadUnmanaged(buffer)); + } + + [TestMethod] + public void OfCredentialAndReadCredentialRoundTripACredential() + { + using NativeSecretBuffer buffer = NativeSecretBuffer.OfCredential(new CredentialWithToken + { + Token = SemanticString.Create("plaintext-token-to-scrub"), + }); + + Credential? credential = NativeSecretBuffer.ReadCredential(buffer.Pointer); + + CredentialWithToken? typed = credential as CredentialWithToken; + Assert.IsNotNull(typed); + Assert.AreEqual("plaintext-token-to-scrub", typed!.Token.ToString()); + } + + [TestMethod] + public void OfCredentialRejectsANullCredential() => + Assert.ThrowsExactly(() => NativeSecretBuffer.OfCredential(null!)); + + [TestMethod] + public void ReadCredentialReturnsNullWhereThereIsNoCredentialToRead() + { + using NativeSecretBuffer empty = NativeSecretBuffer.CopyOf([0]); + using NativeSecretBuffer garbage = NativeSecretBuffer.NulTerminatedCopyOf([.. "{ not a credential"u8]); + + // A store treats null as "nothing stored for this persona", so all three of these have to + // answer null rather than throwing: no entry, an empty entry, and an entry that is not + // parseable as a credential. + Assert.IsNull(NativeSecretBuffer.ReadCredential(IntPtr.Zero)); + Assert.IsNull(NativeSecretBuffer.ReadCredential(empty.Pointer)); + Assert.IsNull(NativeSecretBuffer.ReadCredential(garbage.Pointer)); + } + [TestMethod] public void ZeroOverwritesTheBuffer() { diff --git a/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs b/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs index 051bc9b..6e1b930 100644 --- a/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs +++ b/CredentialCache/Storage/LinuxSecretServiceCredentialStore.cs @@ -66,17 +66,11 @@ public bool TryLoad(PersonaGUID persona, out Credential? credential) try { - // The bytes go straight into a managed array rather than through - // Marshal.PtrToStringUTF8: an immutable string cannot be scrubbed, so the - // plaintext would outlive the call. This is the same byte-array-and-scrub path - // the Windows and macOS stores take; libsecret just hands back a C string - // instead of a pointer and a length. - byte[] blob = NativeSecretBuffer.ReadNulTerminated(passwordPtr); - if (blob.Length == 0) - { - return false; - } - credential = CredentialSerialization.DeserializeAndScrub(blob); + // Read as bytes and scrubbed, never through Marshal.PtrToStringUTF8: an immutable + // string cannot be scrubbed, so the plaintext would outlive the call. Same + // byte-array-and-scrub path the Windows and macOS stores take; libsecret just hands + // back a C string rather than a pointer and a length. + credential = NativeSecretBuffer.ReadCredential(passwordPtr); return credential is not null; } finally @@ -92,39 +86,29 @@ public void Save(PersonaGUID persona, Credential credential) ArgumentNullException.ThrowIfNull(persona); ArgumentNullException.ThrowIfNull(credential); + // An unmanaged nul-terminated copy this code owns and zeroes, not a managed string: + // marshalling a string would put the plaintext on the managed heap where it cannot be + // scrubbed, and leave the runtime's own native copy to be freed unscrubbed. + using NativeSecretBuffer value = NativeSecretBuffer.OfCredential(credential); string label = $"{_serviceName}:{persona}"; - byte[] blob = CredentialSerialization.Serialize(credential); - try - { - // The password is handed over as an unmanaged nul-terminated copy this code - // owns, not as a managed string. Marshalling a string would put the plaintext - // on the managed heap, where it cannot be scrubbed, and would leave the - // runtime's own native copy to be freed unscrubbed. - using NativeSecretBuffer nativeValue = NativeSecretBuffer.NulTerminatedCopyOf(blob); - - IntPtr error = IntPtr.Zero; - bool stored = NativeMethods.secret_password_store_sync( - Schema.Handle, - IntPtr.Zero, - label, - nativeValue.Pointer, - IntPtr.Zero, - ref error, - "service", _serviceName, - "account", persona.ToString(), - IntPtr.Zero); - - ThrowIfError(error, "secret_password_store_sync"); - - if (!stored) - { - throw new CredentialStoreException($"secret_password_store_sync returned false for '{persona}'."); - } - } - finally + IntPtr error = IntPtr.Zero; + bool stored = NativeMethods.secret_password_store_sync( + Schema.Handle, + IntPtr.Zero, + label, + value.Pointer, + IntPtr.Zero, + ref error, + "service", _serviceName, + "account", persona.ToString(), + IntPtr.Zero); + + ThrowIfError(error, "secret_password_store_sync"); + + if (!stored) { - CredentialSerialization.Zero(blob); + throw new CredentialStoreException($"secret_password_store_sync returned false for '{persona}'."); } } diff --git a/CredentialCache/Storage/NativeSecretBuffer.cs b/CredentialCache/Storage/NativeSecretBuffer.cs index dc13e12..746c4d7 100644 --- a/CredentialCache/Storage/NativeSecretBuffer.cs +++ b/CredentialCache/Storage/NativeSecretBuffer.cs @@ -98,6 +98,54 @@ internal static NativeSecretBuffer NulTerminatedCopyOf(byte[] source) } } + /// + /// Serializes and copies it into unmanaged memory as a + /// nul-terminated UTF-8 JSON string, for a native API that takes a C string. + /// + /// The credential to persist. + /// A buffer owning the unmanaged copy, which the caller must dispose. + /// + /// Both managed copies made along the way — the serialized bytes and the nul-terminated + /// array — are zeroed before this returns, on the throwing path as well. Owning the whole + /// sequence here rather than in each store is what keeps it exercised by tests: a store's + /// own body only runs on its own operating system. + /// + internal static NativeSecretBuffer OfCredential(Credential credential) + { + ArgumentNullException.ThrowIfNull(credential); + + byte[] blob = CredentialSerialization.Serialize(credential); + try + { + return NulTerminatedCopyOf(blob); + } + finally + { + CredentialSerialization.Zero(blob); + } + } + + /// + /// Reads the nul-terminated UTF-8 JSON at and deserializes it, + /// scrubbing the managed copy it read. + /// + /// A pointer to a nul-terminated plaintext credential blob. + /// + /// The credential, or when addresses + /// nothing, an empty string, or bytes that are not a known credential. + /// + /// + /// The counterpart of , and the read path a store whose native API + /// returns a C string should use. Nothing here is ever a managed : one + /// would hold the plaintext until the GC happened to collect it and could not be scrubbed + /// at all. + /// + internal static Credential? ReadCredential(IntPtr pointer) + { + byte[] blob = ReadNulTerminated(pointer); + return blob.Length == 0 ? null : CredentialSerialization.DeserializeAndScrub(blob); + } + /// /// Copies the nul-terminated bytes at into a managed array, /// excluding the terminator.