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)]