Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

macOS: Cannot add to X509Store #80176

Closed
filipnavara opened this issue Jan 4, 2023 · 9 comments · Fixed by #82205
Closed

macOS: Cannot add to X509Store #80176

filipnavara opened this issue Jan 4, 2023 · 9 comments · Fixed by #82205
Labels
area-System.Security

Comments

@filipnavara
Copy link
Member

This is essentially regression of #39603 (which was supposed to be fixed by #41787) in .NET 6. We get the following error on our CI tests couple of times per day:

Interop+AppleCrypto+AppleCommonCryptoCryptographicException : UNIX[Undefined error: 0]
   at Interop.AppleCrypto.X509StoreAddCertificate(SafeKeychainItemHandle certOrIdentity, SafeKeychainHandle keychain)
   at Internal.Cryptography.Pal.StorePal.AppleKeychainStore.Add(ICertificatePal cert)
   at System.Security.Cryptography.X509Certificates.X509Store.Add(X509Certificate2 certificate)
   at MailClient.Abstractions.Test.Security.Integration.SMimeTest.CertificateInstaller..ctor(Nullable`1 windowsStore, String resourcePath, SecurityManager securityManager, String password) in /Users/teamcity/actions-runner/_work/emclient/emclient/MailClient.Abstractions.Test/Security.Integration/SMimeTest.cs:line 338
   at MailClient.Abstractions.Test.Security.Integration.SMimeTest.SMimeUsualSigningEncryptionTest(TestCaseParameters data) in /Users/teamcity/actions-runner/_work/emclient/emclient/MailClient.Abstractions.Test/Security.Integration/SMimeTest.cs:line 65
   at NUnit.Framework.Internal.TaskAwaitAdapter.GenericAdapter`1.GetResult()
   at NUnit.Framework.Internal.AsyncToSyncAdapter.Await(Func`1 invoke)
   at NUnit.Framework.Internal.Commands.TestMethodCommand.RunTestMethod(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.TestMethodCommand.Execute(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.BeforeTestCommand.Execute(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.TimeoutCommand.<>c__DisplayClass5_0.<RunTestOnSeparateThread>b__0()
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__272_0(Object obj)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location ---
   at NUnit.Framework.Internal.Commands.TimeoutCommand.Execute(TestExecutionContext context)

The tests run on two different machines - Intel and ARM Mac Mini - and it only fails on the ARM64 (ie. osx-arm64). There may be other differences between the machines and the keychains on those machines.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 4, 2023
@ghost
Copy link

ghost commented Jan 4, 2023

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

Issue Details

This is essentially regression of #39603 (which was supposed to be fixed by #41787) in .NET 6. We get the following error on our CI tests couple of times per day:

Interop+AppleCrypto+AppleCommonCryptoCryptographicException : UNIX[Undefined error: 0]
   at Interop.AppleCrypto.X509StoreAddCertificate(SafeKeychainItemHandle certOrIdentity, SafeKeychainHandle keychain)
   at Internal.Cryptography.Pal.StorePal.AppleKeychainStore.Add(ICertificatePal cert)
   at System.Security.Cryptography.X509Certificates.X509Store.Add(X509Certificate2 certificate)
   at MailClient.Abstractions.Test.Security.Integration.SMimeTest.CertificateInstaller..ctor(Nullable`1 windowsStore, String resourcePath, SecurityManager securityManager, String password) in /Users/teamcity/actions-runner/_work/emclient/emclient/MailClient.Abstractions.Test/Security.Integration/SMimeTest.cs:line 338
   at MailClient.Abstractions.Test.Security.Integration.SMimeTest.SMimeUsualSigningEncryptionTest(TestCaseParameters data) in /Users/teamcity/actions-runner/_work/emclient/emclient/MailClient.Abstractions.Test/Security.Integration/SMimeTest.cs:line 65
   at NUnit.Framework.Internal.TaskAwaitAdapter.GenericAdapter`1.GetResult()
   at NUnit.Framework.Internal.AsyncToSyncAdapter.Await(Func`1 invoke)
   at NUnit.Framework.Internal.Commands.TestMethodCommand.RunTestMethod(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.TestMethodCommand.Execute(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.BeforeTestCommand.Execute(TestExecutionContext context)
   at NUnit.Framework.Internal.Commands.TimeoutCommand.<>c__DisplayClass5_0.<RunTestOnSeparateThread>b__0()
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__272_0(Object obj)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location ---
   at NUnit.Framework.Internal.Commands.TimeoutCommand.Execute(TestExecutionContext context)

The tests run on two different machines - Intel and ARM Mac Mini - and it only fails on the ARM64 (ie. osx-arm64). There may be other differences between the machines and the keychains on those machines.

Author: filipnavara
Assignees: -
Labels:

area-System.Security
Milestone: -

@filipnavara
Copy link
Member Author

The relevant part of the test code:

				var certificateCollection = new X509Certificate2Collection();
				certificateCollection.Import(data, password, Certificate.CERTIFICATE_FLAGS);

				if (windowsStore.HasValue)
				{
					windowsCertificatesToRemove = new List<X509Certificate2>();
					using (var store = new X509Store(windowsStore.Value, StoreLocation.CurrentUser))
					{
						store.Open(OpenFlags.ReadWrite | OpenFlags.OpenExistingOnly);
						foreach (var certificateToImport in certificateCollection)
						{
							var existing = store.Certificates.Find(X509FindType.FindByThumbprint, certificateToImport.Thumbprint, true).OfType<X509Certificate2>().FirstOrDefault();
							if (existing != null)
							{
								TestContext.WriteLine(
									$"WARNING: The certificate {existing.Subject} {existing.FriendlyName} si already in {windowsStore.Value} store. Reusing it for this test case but will be deleted once test case is finished.");
								WindowsStoreX509Certificate = existing;
							}
							else
							{
								store.Add(certificateToImport);
								windowsCertificatesToRemove.Add(certificateToImport);
								WindowsStoreX509Certificate = certificateToImport;
							}
						}
					}
				}

@bartonjs
Copy link
Member

bartonjs commented Jan 4, 2023

Does macOS (at our minimum target) support non-persisted keychains yet? Or ways of working with SecIdentifyRef that aren't backed by keychains?

@filipnavara
Copy link
Member Author

Does macOS (at our minimum target) support non-persisted keychains yet?

Generally speaking yes. There may be some issues with interoperability with the old-style keychain keys though.

@vcsjones
Copy link
Member

vcsjones commented Jan 5, 2023

Generally speaking yes

Are you referring to the data protection keychain, or something else?

@filipnavara
Copy link
Member Author

Generally speaking yes

Are you referring to the data protection keychain, or something else?

Yep, the data protection keychain (the terminology is little fuzzy on that but let's say the iOS-style APIs).

@filipnavara
Copy link
Member Author

filipnavara commented Feb 15, 2023 •
edited
Loading

Turns out the fix in #41787 was incomplete. It fixed the intialization of new X509Certificate(...) from PKCS#12 data by holding a reference to the keychain for the lifetime of the certificate object.

The same general problem happens for this code:

var c = new X509Certificate2Collection();
c.Import(data, password, flags);

In this case the temporary keychain gets disposed here while the certificates get created here without the reference to the keychain.

@filipnavara
Copy link
Member Author

This fix passes our unit tests. I currently don't have time to polish it though.

@filipnavara filipnavara mentioned this issue Feb 15, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 15, 2023
@filipnavara
Copy link
Member Author

Here's a more-or-less reliable standalone repro:

using System.Security.Cryptography.X509Certificates;

var pfxData = Convert.FromHexString(
    "308206980201033082065406092A864886F70D010701A0820645048206413082" +
    "063D308203B606092A864886F70D010701A08203A7048203A33082039F308203" +
    "9B060B2A864886F70D010C0A0102A08202B6308202B2301C060A2A864886F70D" +
    "010C0103300E04083A55E19A8875CF7E020207D004820290C39707B6E15BC075" +
    "62B296E37D44A4C3654F398528F51988932D4E78C96AC7E783A21DD306AEB4BB" +
    "7490551F5CCB21DBF883B3D01D3CCFEB0148CC79C8C15AD7266BC0C0D23563FF" +
    "A879DDB05ACC5B1F9A9BA5F8E32ECE05458A55DC91882750B0573315D843524D" +
    "72E400C14E24B530A228B477DF60F01A1CBA22085350C72B8E871AEF219D1457" +
    "3A660D71064D4226814B8D360A84FDF9B24578E61D62119C893DF1CB97C733BC" +
    "D9CCA8C3FBA40B3E35F305AFCF1E24E720379FC94FB7103A27F817D98AF70D12" +
    "0A357EB5840720F48AE6ED59990E80541CCA08B2B3F9FAE0F7554F491616E72B" +
    "E2383478709947F5BD5AC8D2F74550A2C4260DAA789C66559DBC25D0D5657DEF" +
    "FF491287E9E982C870732E6C1494F25FED513D2DF246EDABA1DEF8FE05A1C283" +
    "676A0722961FBAD4B47E8D27D08E4129FACE86CAAB657A1899C7F9286DD534AC" +
    "3AE6B1A100C90207B9A39857C1D9B7A061A5E8496F0E099F1323FA56DE85BF34" +
    "96DBBE6FFEAAC0321F65A40BAE63503BB704553F1027700D15D12A20EC3A8FB9" +
    "8D62D902FBDBD1BA9984242A6413CF243FE1C5A5A42083EA6E821D302CC4BE4D" +
    "23C0D92247C027A6D5AE3D645F378916ACBD068F52A7772571F51F4AF46652E6" +
    "D9821A24267F6A8CF9D69B3EF5FF33702B71CCDA87A3704DB815685F83E22C64" +
    "B60D9B659EEA517BCACDD638DB2EF171692AA17176891901F2BB97F574908CCA" +
    "943743C1E8D2520FE7B5E04C10E748A666EAAE81EF5DB3577F669E049BE57CCF" +
    "7FFCF7583A9ABE0BC44F0D33EBE7F3BFC2F1F4012F588D0FC8CB0FFDF3BF1A43" +
    "A962B0F9059FAAC0BD68B23275584180EDDE1F1703BFE977456079FF0890BFE5" +
    "ED9CA93ABECF335C02DEBB7DB1A1E60B5B7746F6F152299A1DB983ACE6D9D5B9" +
    "DBAF68A3DEDCCC013181D1301306092A864886F70D0109153106040401000000" +
    "305B06092A864886F70D010914314E1E4C007B00430035004600430030003800" +
    "350032002D0038004200410033002D0034003700460041002D00410045003400" +
    "33002D004200440039003300420044003200350041004300440042007D305D06" +
    "092B060104018237110131501E4E004D006900630072006F0073006F00660074" +
    "0020005300740072006F006E0067002000430072007900700074006F00670072" +
    "00610070006800690063002000500072006F007600690064006500723082027F" +
    "06092A864886F70D010706A08202703082026C0201003082026506092A864886" +
    "F70D010701301C060A2A864886F70D010C0103300E0408812EAA0D26B0151B02" +
    "0207D080820238A471B31EBE90A6B540889297C09BFC73185C2C2676665DBAD2" +
    "BC5759464079976B4C82DCB425289BCFABF9CD11AABF18B3CFA1879A958919E5" +
    "05465E5B2DBCA5039990650F7D7490D322F2CCCD444ACE4625BE3C361FA5C096" +
    "615ACDA3123A478340332961655C973D74EE64F32015194558F4B8148DED4671" +
    "7EE0381E8A53B582F176DB914475FB57C2648DCC709D6153825F33F082B91EAB" +
    "BB2236343CA919065B9D9112D301E6700CE8104035469EDF7874246373351E13" +
    "DC1DAB74DF1DE1E076FE03869F494C0427DE55CA7C17536DEF40D0E3F1B7E5FF" +
    "5A2050175624A1F2D9EF4069186424CC956B84997768E589E38D3D836592CF27" +
    "881DB2EE51AB682B4F4C58ED14585C51B72CCF156BE3CD2EEC538AD15A598E84" +
    "0CFCDF6CD4A151B7EA4BE5388EA903298B4D1A86E7371300A6242DD7A654E430" +
    "2B4F849650230F467042CD328A4FD4B33803213C807C37F98024A718B40AD0BD" +
    "3AC6279E47FBD27E50D743BC8F595B78F8805D8143EBB3119B919DFFBFAF606F" +
    "A858C77C5E0526646B6AACC536A3B2FEA1E0A67CBAE166A8F85913F3600C55F7" +
    "850EF48619B9E93FB15F21994BC9ABA8FF7156B68B1DAD472ED2FA3917E1F84D" +
    "AD4EB34992493B7084F6490FB326697A0A52BA08BF7F537B3BDC09B421A0D47F" +
    "DCB8BD0271963EC4CE4F9C21A45AC382E8AD7ED8D69824BD5C00BCEAF56D88B3" +
    "F2D0D5A81CF1C14884040A8E522428154A9C752C0E64204FA849689739E5138F" +
    "0F96C471C5FE026EA7EA68AA42DF0A693213ECF06555804A191637A8F7858A30" +
    "3B301F300706052B0E03021A04146B4EDA4227EAFC85EDA331AC88761DD06D54" +
    "60FD0414C8A2B10C67EAECB3D48BEC69616133185721613D020207D0");
const string pfxDataPassword = "12345";

using var store = new X509Store(StoreName.My, StoreLocation.CurrentUser);
store.Open(OpenFlags.ReadWrite);

var certificateCollection = new X509Certificate2Collection();
certificateCollection.Import(pfxData, pfxDataPassword, X509KeyStorageFlags.Exportable);

GC.Collect();
GC.WaitForPendingFinalizers();

foreach (var certificateToImport in certificateCollection)
{
    store.Add(certificateToImport);
}

store.Close();

I don't see how to easily turn it into unit test since it requires precise GC and it modifies a user's keychain which is undesirable.

@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Feb 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-System.Security
Projects
None yet
Milestone
No milestone
4 participants
@vcsjones @filipnavara @bartonjs and others