Hold temporary keychain refs for certificates imported with X509Certificate2Collection.Import

[filipnavara]

Fixes #80176

SafeSecCertificateHandle explictly tracks the associated temporary keychain through calls to SafeTemporaryKeychainHandle.TrackItem and SafeTemporaryKeychainHandle.UntrackItem. UntrackItem is called from ReleaseHandle when the certificate handle is being disposed/finalized. TrackItem is called on all the places where the certificate/identity handle is created.

X509MoveToKeychain takes a SafeSecCertificateHandle as input parameter. The native API could change the keychain associated with this certificate. This was not accounted for and it resulted in mismatched tracking of reference count on the temporary keychain. For example, the handle could initially be without a keychain, so the first SafeTemporaryKeychainHandle.TrackItem call on it was a no-op. When the certificate is moved into temporary keychain and the handle is later disposed, SafeTemporaryKeychainHandle.UntrackItem gets called and decrements a reference count that was not previously incremented.

Additionally, several temporary AppleCertificatePal objects were not disposed during PKCS#12 import. When they eventually get garbage collected it causes the associated SafeSecCertificateHandle objects to be released, and this results in the unbalanced reference counts releasing the native keychain object for the temporary keychain.

This affects APIs like new X509Certificate2(...) and X509Certificate2Collection.Import when importing PKCS#12 certificates.

PR #41787 previously tried to fix the issue, but it did so only partially and largely by accident. It introduced an additional explicit keychain reference in the new X509Certificate2(...) code path through DangerousAddRef. This negated the effect of the incorrect tracking in X509MoveToKeychain. The additional keychain reference was then released through Dispose instead of DangerousReleaseRef which silently failed since the reference count was already zero at that point. Effectively it meant the new X509Certificate2(...) code path succeeded but only as a side-effect of the extra DangerousAddRef. The same workaround was not applied for the X509Certificate2Collection.Import though.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #80176

Author:	filipnavara
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[bartonjs]

I believe this makes the "temporary" keychain more or less "permanent", because all calls to Dispose, collectively, only call DangerousRelease once.

You'd need to tell the newPal that it should DangerousRelease instead of Dispose in order to get it to ever tick down to 0 and actually release; and then have the collection import call Dispose() on the temp handle here (after the loop ends).

[filipnavara]

I believe this makes the "temporary" keychain more or less "permanent", because all calls to Dispose, collectively, only call DangerousRelease once.
You'd need to tell the newPal that it should DangerousRelease instead of Dispose in order to get it to ever tick down to 0 and actually release;

I was about to agree with you but then I went to compare it with my observations:

• The DangerousAddRef /Dispose pair is a pre-existing construct introduced in hold ref to temp keychain on OSX to avoid premature cleanup #41787.
• I definitely do see AppleCryptoNative_SecKeychainDelete getting called and that's called only from the final release of the temporary keychain. This can be verified by taking the repro from the issue and adding certificateToImport.Dispose() at the end of the for loop block.
• If DangerousRelease is used instead of Dispose in AppleCertificatePal.DisposeTempKeychain then the same test code above actually fails with the following exception:

System.ObjectDisposedException: Safe handle has been closed.
Object name: 'SafeHandle'.
   at System.Runtime.InteropServices.SafeHandle.InternalRelease(Boolean disposeOrFinalizeOperation)
   at System.Runtime.InteropServices.SafeHandle.DangerousRelease()
   at Internal.Cryptography.Pal.AppleCertificatePal.DisposeTempKeychain()
   at Internal.Cryptography.Pal.AppleCertificatePal.Dispose()
   at System.Security.Cryptography.X509Certificates.X509Certificate.Reset()
   at System.Security.Cryptography.X509Certificates.X509Certificate2.Reset()
   at System.Security.Cryptography.X509Certificates.X509Certificate.Dispose(Boolean disposing)
   at System.Security.Cryptography.X509Certificates.X509Certificate.Dispose()

and then have the collection import call Dispose() on the temp handle here (after the loop ends).

That's already happening in ApplePkcs12CertLoader.Dispose.

(Not saying this sounds correct; I just don't fully grasp what is going on)

[filipnavara]

Looks like the mismatched references could be caused by a missing SafeTemporaryKeychainHandle.TrackItem somewhere which causes additional release from SafeTemporaryKeychainHandle.UntrackItem (called from SafeKeychainItemHandle.ReleaseHandle which is itself called from AppleCertificatePal.Dispose).

[filipnavara]

I think I untangled the mess. X509MoveToKeychain takes a certificate as an input parameter but it changes its keychain. While SafeTemporaryKeychainHandle.TrackItem was previously called on the input certificate it was no-op since the item had no associated keychain. However, by the time the handle is disposed, it has the associated keychain, and thus SafeTemporaryKeychainHandle.UntrackItem would release one too many references.

It is plausible that fixing this would make the AppleCertificatePal._tempKeychain logic entirely unnecessary.

[filipnavara]

I updated the PR description to explain what is going on.

[filipnavara]

Note: With this object disposed in deterministic manner the existing tests should reliably trip on miscounted references without a GC being triggered.

[bartonjs]

The only failure is known, #81249.