diff --git a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs
index 137eb25..1230e26 100644
--- a/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs
+++ b/CredentialCache.Test/NativeStoreScrubbingWiringTests.cs
@@ -147,6 +147,54 @@ public void TheWindowsStoreChecksItsBlobLimitInsideTheScrubbing() =>
"WindowsCredentialStore.Save must copy its blob through NativeSecretBuffer.CopyWithinLimit, "
+ "which zeroes the serialized plaintext on the over-limit rejection path as well.");
+ ///
+ /// 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)]