Look macOS items up by reference without decrypting the secret [patch]

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.");
 
+	/// <summary>
+	/// macOS <c>Save</c> and <c>Remove</c> look an item up only for its reference, to modify or
+	/// delete through. Asking <c>SecKeychainFindGenericPassword</c> for the password data as well
+	/// made the Keychain decrypt the old secret into this process, where
+	/// <c>SecKeychainItemFreeContent</c> released it unscrubbed (#166). The lookup they use must
+	/// have no way to receive the secret, and so nothing for them to free.
+	/// </summary>
+	[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<System.Runtime.InteropServices.DllImportAttribute>()?.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.");
+		}
+	}
+
 	/// <summary>
 	/// 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

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);
 
+		/// <summary>
+		/// Finds an item for its reference alone. Pass <see cref="IntPtr.Zero"/> for both password
+		/// parameters: asking for the password data makes the Keychain decrypt the secret into this
+		/// process, where <see cref="SecKeychainItemFreeContent"/> releases it without zeroing it,
+		/// and can raise an access prompt that modifying or deleting through the reference does not.
+		/// </summary>
 		[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)]