From 9a020936aa578ce2b41f13aa09317c92ac284760 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 5 Oct 2026 23:25:28 +0000 Subject: [PATCH] Look macOS items up by reference without decrypting the secret [patch] MacOsCredentialStore.Save and Remove only need the item reference, but asked SecKeychainFindGenericPassword for the password data too. The Keychain decrypted the old secret into the process, where SecKeychainItemFreeContent released it unscrubbed, and the read could raise an access prompt. The lookup now passes null for both password parameters, so TryLoad is the only path that decrypts a secret. Fixes ktsu-dev/CredentialCache#166 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018fjR4mduqAKqmuCXhfcCbx --- .../NativeStoreScrubbingWiringTests.cs | 48 +++++++++++++++++++ .../Storage/MacOsCredentialStore.cs | 26 +++++----- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs index 2f2409e..efc5066 100644 --- a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs +++ b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs @@ -133,6 +133,54 @@ public void EveryNativeStoreSavesFromScrubbableBytes() } } + /// + /// macOS Save and Remove look an item up only for its reference, to modify or + /// delete through. Asking SecKeychainFindGenericPassword for the password data as well + /// made the Keychain decrypt the old secret into this process, where + /// SecKeychainItemFreeContent released it unscrubbed (#166). The lookup they use must + /// have no way to receive the secret, and so nothing for them to free. + /// + [TestMethod] + public void TheMacOsStoreDecryptsASecretOnlyToLoadIt() + { + Type nativeMethods = typeof(MacOsCredentialStore).GetNestedType("NativeMethods", BindingFlags.NonPublic) + ?? throw new InvalidOperationException("MacOsCredentialStore.NativeMethods not found."); + MethodInfo[] lookups = [.. nativeMethods + .GetMethods(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static) + .Where(m => m.GetCustomAttribute()?.EntryPoint + == "SecKeychainFindGenericPassword")]; + MethodInfo freeContent = nativeMethods.GetMethod("SecKeychainItemFreeContent", BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static) + ?? throw new InvalidOperationException("SecKeychainItemFreeContent not found."); + + MethodInfo tryLoad = Method(typeof(MacOsCredentialStore), nameof(ICredentialStore.TryLoad)); + Assert.IsTrue( + Calls(tryLoad, freeContent), + "TryLoad is the one path that needs the secret, and must still release what the Keychain returned."); + + foreach (string name in new[] { nameof(ICredentialStore.Save), nameof(ICredentialStore.Remove) }) + { + MethodInfo method = Method(typeof(MacOsCredentialStore), name); + MethodInfo[] used = [.. lookups.Where(l => Calls(method, l))]; + + Assert.IsNotEmpty(used, $"MacOsCredentialStore.{name} must look the item up to modify or delete it."); + foreach (MethodInfo lookup in used) + { + foreach (string parameter in new[] { "passwordLength", "passwordData" }) + { + Assert.IsFalse( + lookup.GetParameters().Single(p => p.Name == parameter).ParameterType.IsByRef, + $"MacOsCredentialStore.{name} looks the item up through {lookup.Name}, whose {parameter} is an " + + "out parameter; it must be a plain pointer so it can be passed as null and the Keychain " + + "never decrypts the secret."); + } + } + + Assert.IsFalse( + Calls(method, freeContent), + $"MacOsCredentialStore.{name} must never receive password data, so it has nothing to free."); + } + } + /// /// 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 diff --git a/CredentialCache/Storage/MacOsCredentialStore.cs b/CredentialCache/Storage/MacOsCredentialStore.cs index 242b601..a9e11a7 100644 --- a/CredentialCache/Storage/MacOsCredentialStore.cs +++ b/CredentialCache/Storage/MacOsCredentialStore.cs @@ -78,11 +78,11 @@ public void Save(PersonaGUID persona, Credential credential) try { - int findStatus = NativeMethods.SecKeychainFindGenericPasswordWithRef( + int findStatus = NativeMethods.SecKeychainFindGenericPasswordItem( keychainOrArray: IntPtr.Zero, serviceNameLength: (uint)service.Length, serviceName: service, accountNameLength: (uint)account.Length, accountName: account, - passwordLength: out _, passwordData: out IntPtr existingPtr, + passwordLength: IntPtr.Zero, passwordData: IntPtr.Zero, itemRef: out IntPtr itemRef); if (findStatus == NativeMethods.errSecSuccess) @@ -100,10 +100,6 @@ public void Save(PersonaGUID persona, Credential credential) } finally { - if (existingPtr != IntPtr.Zero) - { - _ = NativeMethods.SecKeychainItemFreeContent(IntPtr.Zero, existingPtr); - } if (itemRef != IntPtr.Zero) { NativeMethods.CFRelease(itemRef); @@ -142,11 +138,11 @@ public bool Remove(PersonaGUID persona) byte[] account = Encoding.UTF8.GetBytes(persona.ToString()); byte[] service = Encoding.UTF8.GetBytes(_serviceName); - int findStatus = NativeMethods.SecKeychainFindGenericPasswordWithRef( + int findStatus = NativeMethods.SecKeychainFindGenericPasswordItem( keychainOrArray: IntPtr.Zero, serviceNameLength: (uint)service.Length, serviceName: service, accountNameLength: (uint)account.Length, accountName: account, - passwordLength: out _, passwordData: out IntPtr passwordPtr, + passwordLength: IntPtr.Zero, passwordData: IntPtr.Zero, itemRef: out IntPtr itemRef); if (findStatus == NativeMethods.errSecItemNotFound) @@ -169,10 +165,6 @@ public bool Remove(PersonaGUID persona) } finally { - if (passwordPtr != IntPtr.Zero) - { - _ = NativeMethods.SecKeychainItemFreeContent(IntPtr.Zero, passwordPtr); - } if (itemRef != IntPtr.Zero) { NativeMethods.CFRelease(itemRef); @@ -196,12 +188,18 @@ internal static extern int SecKeychainFindGenericPassword( out uint passwordLength, out IntPtr passwordData, IntPtr itemRef); + /// + /// Finds an item for its reference alone. Pass for both password + /// parameters: asking for the password data makes the Keychain decrypt the secret into this + /// process, where releases it without zeroing it, + /// and can raise an access prompt that modifying or deleting through the reference does not. + /// [DllImport(Security, EntryPoint = "SecKeychainFindGenericPassword")] - internal static extern int SecKeychainFindGenericPasswordWithRef( + internal static extern int SecKeychainFindGenericPasswordItem( IntPtr keychainOrArray, uint serviceNameLength, byte[] serviceName, uint accountNameLength, byte[] accountName, - out uint passwordLength, out IntPtr passwordData, + IntPtr passwordLength, IntPtr passwordData, out IntPtr itemRef); [DllImport(Security)]