ASN1 corrupted data while calling X509Store.Certificates on Mac

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.

https://docs.microsoft.com/dotnet/core/

MIT License

15.18k stars 4.72k forks source link

ASN1 corrupted data while calling X509Store.Certificates on Mac #54336

Closed odhanson closed 3 years ago

odhanson commented 3 years ago

We are receiving the following exception from one of our customer (no contact information).

System.Security.Cryptography.CryptographicException: ASN1 corrupted data.
 ---> System.Formats.Asn1.AsnContentException: The ASN.1 value is invalid.
   at System.Formats.Asn1.AsnDecoder.GetIntegerContents(ReadOnlySpan`1 source, AsnEncodingRules ruleSet, Asn1Tag expectedTag, UniversalTagNumber tagNumber, Int32& bytesConsumed)
   at System.Formats.Asn1.AsnDecoder.ReadIntegerBytes(ReadOnlySpan`1 source, AsnEncodingRules ruleSet, Int32& bytesConsumed, Nullable`1 expectedTag)
   at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded)
   --- End of inner exception stack trace ---
   at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, CertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.Decode(Asn1Tag expectedTag, ReadOnlyMemory`1 encoded, AsnEncodingRules ruleSet)
   at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.Decode(ReadOnlyMemory`1 encoded, AsnEncodingRules ruleSet)
   at Internal.Cryptography.Pal.CertificateData..ctor(Byte[] rawData)
   at Internal.Cryptography.Pal.AppleCertificatePal.EnsureCertData()
   at Internal.Cryptography.Pal.AppleCertificatePal.get_Thumbprint()
   at System.Security.Cryptography.X509Certificates.X509Certificate.GetRawCertHash()
   at System.Security.Cryptography.X509Certificates.X509Certificate.GetHashCode()
   at System.Collections.Generic.HashSet`1.AddIfNotPresent(T value, Int32& location)
   at Internal.Cryptography.Pal.StorePal.ReadCollection(SafeCFArrayHandle matches, HashSet`1 collection)
   at Internal.Cryptography.Pal.StorePal.AppleKeychainStore.CloneTo(X509Certificate2Collection collection)
   at System.Security.Cryptography.X509Certificates.X509Store.get_Certificates()
   at Microsoft.Mashup.Security.CertificateStore.get_Certificates()

I don't know which certificate in the users' key chain is causing the exception, and if its really corrupted or not. However, it seems like even if one certificate in the store is corrupted, this will cause the entire System.Security.Cryptography.X509Certificates.X509Store.get_Certificates() to fail. And there is no method for a "Find" certificate. So even if the certificate I am looking for is fine, we can't reach it because we are enumerating all certificates.

I can suggest two alternatives (which would require introducing new API):

Add a TryGetCertificates that will return a list of certificates, and perhaps a list names that could not be read ?
Add a X509Store.Find that will return only a single certificate (and will throw if that certificate cannot get populated).

ghost commented 3 years ago

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchForks See info in area-owners.md if you want to be subscribed.

Issue Details

Hi We are receiving the following exception from one of our customer (no contact information). ``` System.Security.Cryptography.CryptographicException: ASN1 corrupted data. ---> System.Formats.Asn1.AsnContentException: The ASN.1 value is invalid. at System.Formats.Asn1.AsnDecoder.GetIntegerContents(ReadOnlySpan`1 source, AsnEncodingRules ruleSet, Asn1Tag expectedTag, UniversalTagNumber tagNumber, Int32& bytesConsumed) at System.Formats.Asn1.AsnDecoder.ReadIntegerBytes(ReadOnlySpan`1 source, AsnEncodingRules ruleSet, Int32& bytesConsumed, Nullable`1 expectedTag) at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded) at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded) --- End of inner exception stack trace --- at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded) at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded) at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, CertificateAsn& decoded) at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.Decode(Asn1Tag expectedTag, ReadOnlyMemory`1 encoded, AsnEncodingRules ruleSet) at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.Decode(ReadOnlyMemory`1 encoded, AsnEncodingRules ruleSet) at Internal.Cryptography.Pal.CertificateData..ctor(Byte[] rawData) at Internal.Cryptography.Pal.AppleCertificatePal.EnsureCertData() at Internal.Cryptography.Pal.AppleCertificatePal.get_Thumbprint() at System.Security.Cryptography.X509Certificates.X509Certificate.GetRawCertHash() at System.Security.Cryptography.X509Certificates.X509Certificate.GetHashCode() at System.Collections.Generic.HashSet`1.AddIfNotPresent(T value, Int32& location) at Internal.Cryptography.Pal.StorePal.ReadCollection(SafeCFArrayHandle matches, HashSet`1 collection) at Internal.Cryptography.Pal.StorePal.AppleKeychainStore.CloneTo(X509Certificate2Collection collection) at System.Security.Cryptography.X509Certificates.X509Store.get_Certificates() at Microsoft.Mashup.Security.CertificateStore.get_Certificates() ``` I don't know which certificate in the users' key chain is causing the exception, and if its really corrupted or not. However, it seems like even if one certificate in the store is corrupted, this will cause the entire System.Security.Cryptography.X509Certificates.X509Store.get_Certificates() to fail. And there is no method for a "Find" certificate. So even if the certificate I am looking for is fine, we can't reach it because we are enumerating all certificates. I can suggest two alternatives (which would require introducing new API): 1. Add a TryGetCertificates that will return a list of certificates, and perhaps a list names that could not be read ? 2. Add a X509Store.Find that will return only a single certificate (and will throw if that certificate cannot get populated).

Author:	odhanson
Assignees:	-
Labels:	`area-System.Security`, `untriaged`
Milestone:	-

bartonjs commented 3 years ago

The easier answer might just be to teach this layer that things can fail and have it skip bad entries.

Testing it will have to be on faith, unless someone can come up with a way to repro this.

odhanson commented 3 years ago

I am fine with that. But that might cause the caller to think that the certificate doesn't exist (and perhaps try add it) when in actuality it does, but is corrupted (in the case that the corrupted certificate is actually the one we are looking for).

bartonjs commented 3 years ago

But that might cause the caller to think that the certificate doesn't exist

That seems to also apply to the suggested alternatives, though. e.g. here we failed in determining a certificate's thumbprint, so find by thumbprint wouldn't be able to find it. Find by name would have a similar "how are we determining the value?" question, particularly if there are any sorts of normalization differences between a macOS native version (assuming it exists) and the managed implementation we'd have elsewhere.

odhanson commented 3 years ago

I see. Makes sense to me.

odhanson commented 3 years ago

Alternatively, perhaps the Certificates collection should enumerate the entire list (and throw, when accessing a corrupted certificate so we don't affect existing functionality) lazily ? That is only when the collection is actually enumerated ? Then we can implement the Find method to only query for the specific certificate we are looking for ? Basically we do a: certStore.Certificates.Find(X509FindType.FindByIssuerDistinguishedName, distinguishedName, false); which enumerates the entire collection just to search for a single cert, but similar to the EnumerateKeychain inside the pal_keychain, we could update the query to only search for that specific cert. This would both improve performance (not needing to populate all the certs just to find a single one), will continue to throw if the cert we are looking for is the corrupted one.

vcsjones commented 3 years ago

Hm. I can get similar results with the same underlying root cause.

The public X.509 contents in a keychain file are mutable and keychain doesn't really do anything to stop the contents from changing and you can just flip bits in the file. In the example below I started flipping bits in the signature OID. It's a little tricky to get right because the wrong changes will just result in the keychain APIs ignoring the certificate all together and acting like it isn't there.

Perhaps there is a legitimate way to import such a damaged file, but one could presume it is a flipped bit on a disk or similar.

While I can make the issue occur, unit testing it seems difficult.

Unhandled exception. System.Security.Cryptography.CryptographicException: ASN1 corrupted data.
 ---> System.Formats.Asn1.AsnContentException: The ASN.1 value is invalid.
   at System.Formats.Asn1.AsnDecoder.ReadSubIdentifier(ReadOnlySpan`1 source, Int32& bytesRead, Nullable`1& smallValue, Nullable`1& largeValue)
   at System.Formats.Asn1.AsnDecoder.ReadObjectIdentifier(ReadOnlySpan`1 source, AsnEncodingRules ruleSet, Nullable`1 expectedTag, Int32& totalBytesRead)
   at System.Formats.Asn1.AsnDecoder.ReadObjectIdentifier(ReadOnlySpan`1 source, AsnEncodingRules ruleSet, Int32& bytesConsumed, Nullable`1 expectedTag)
   at System.Security.Cryptography.Asn1.AlgorithmIdentifierAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, AlgorithmIdentifierAsn& decoded)
   at System.Security.Cryptography.Asn1.AlgorithmIdentifierAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, AlgorithmIdentifierAsn& decoded)
   --- End of inner exception stack trace ---
   at System.Security.Cryptography.Asn1.AlgorithmIdentifierAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, AlgorithmIdentifierAsn& decoded)
   at System.Security.Cryptography.Asn1.AlgorithmIdentifierAsn.Decode(AsnValueReader& reader, ReadOnlyMemory`1 rebind, AlgorithmIdentifierAsn& decoded)
   at System.Security.Cryptography.Asn1.SubjectPublicKeyInfoAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, SubjectPublicKeyInfoAsn& decoded)
   at System.Security.Cryptography.Asn1.SubjectPublicKeyInfoAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, SubjectPublicKeyInfoAsn& decoded)
   at System.Security.Cryptography.Asn1.SubjectPublicKeyInfoAsn.Decode(AsnValueReader& reader, ReadOnlyMemory`1 rebind, SubjectPublicKeyInfoAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.TbsCertificateAsn.Decode(AsnValueReader& reader, ReadOnlyMemory`1 rebind, TbsCertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.DecodeCore(AsnValueReader& reader, Asn1Tag expectedTag, ReadOnlyMemory`1 rebind, CertificateAsn& decoded)
   at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.Decode(Asn1Tag expectedTag, ReadOnlyMemory`1 encoded, AsnEncodingRules ruleSet)
   at System.Security.Cryptography.X509Certificates.Asn1.CertificateAsn.Decode(ReadOnlyMemory`1 encoded, AsnEncodingRules ruleSet)
   at Internal.Cryptography.Pal.CertificateData..ctor(Byte[] rawData)
   at Internal.Cryptography.Pal.AppleCertificatePal.EnsureCertData()
   at Internal.Cryptography.Pal.AppleCertificatePal.get_Thumbprint()
   at System.Security.Cryptography.X509Certificates.X509Certificate.GetRawCertHash()
   at System.Security.Cryptography.X509Certificates.X509Certificate.GetHashCode()
   at System.Collections.Generic.HashSet`1.AddIfNotPresent(T value, Int32& location)
   at System.Collections.Generic.HashSet`1.Add(T item)
   at Internal.Cryptography.Pal.StorePal.ReadCollection(SafeCFArrayHandle matches, HashSet`1 collection)
   at Internal.Cryptography.Pal.StorePal.AppleKeychainStore.CloneTo(X509Certificate2Collection collection)
   at System.Security.Cryptography.X509Certificates.X509Store.get_Certificates()
   at <Program>$.<Main>$(String[] args) in /Users/vcsjones/Projects/csharp-scratch/Program.cs:line 6

vcsjones commented 3 years ago

Okay, here is how to reproduce the above.

Make a new keychain file. You don't want to do this in your login keychain. Many things get angry.

Import this certificate.

You need to do this through the keychain GUI. Doing it through API or CLI blocks the import. Keychain.app will complain the certificate is malformed, but will import it anyway (at least on macOS Catalina. Big Sur got stricter X.509 validation so it would not surprise me if the GUI outright blocked it).

Try to enumerate the certificates in the keychain file.

using System.Security.Cryptography.X509Certificates;

X509Store store = new("damaged"); //Whatever you called your keychain.
store.Open(OpenFlags.ReadOnly);
store.Certificates.Find(X509FindType.FindByIssuerDistinguishedName, "3BEDAC7330E9FD4286DDF60A96380A0467D34E53", false);

So, conceivably the person running in to this either A) was able to import a corrupt X.509 certificate since the keychain app seems to permit it, or used to, or B) they have just the right bit-flip on their disk.

vcsjones commented 3 years ago

@bartonjs

teach this layer that things can fail and have it skip bad entries

Hm. That makes some sense, but then I wonder if that would be problematic if the absence of a certificate would lead to some potential issues.

Consider:

X509Store store = new X509Store(StoreName.Disallowed);
store.Open(OpenFlags.ReadOnly);

If someone is using the disallowed store on macOS (really just a separate keychain) to store known bad certificates, and somehow an incorrectly encoded certificate was added to the certificate store (or we assume a disk is flipping bits), I (personally) would prefer throwing behavior rather than potentially be in a situation where I am trusting something I shouldn't be (put another way, I am now in a situation where I cannot safely continue).

@odhanson

Basically we do a: certStore.Certificates.Find(X509FindType.FindByIssuerDistinguishedName, distinguishedName, false); which enumerates the entire collection just to search for a single cert, but similar to the EnumerateKeychain inside the pal_keychain, we could update the query to only search for that specific cert.

I'm a bit unsure how this would work. Find returns a collection, not a single certificate. We wouldn't know if we found all of them unless we were able to parse all of them. Let's say there are two certificates in the keychain that have a matching Issuer DN, but one is corrupted. We can return one that matches, but since we could not parse the second, we wouldn't know if it is a match, or not. So we'd end up throwing, anyway.

odhanson commented 3 years ago

@vcsjones what would you tell the user of application when such an exception is thrown ? Please look at your keychain and find which item is offending ? We can't even tell you its name? Although its not even relevant to the key our application needs, you can't use our application, goodbye ? :)

SecItemCopyMatching (the underlying MacOS API used) will return a list of matching items in the keychain. If one of those would be the offending item, then Find will still throw, but if not, then I don't see why we should break our entire app just because an item we don't even care about is corrupted.

Note, that we are using SecItemCopyMatching today, to create the entire collection. And then we decode the entire list of items returned from this API. This is a costly operation. And then, we perform Find on top of that decoded array returned from before - on our own. But that is too late, because we have already thrown while trying to decode an item, we most probably don't even care about (or yes, we don't know at this point).

My suggestion is to call SecItemCopyMatching each time we call Find, and refine the query arguments we pass into it. This should return a much smaller set of relevant certs, and only decode those - it will be faster, will still throw if one of the certs is corrupted, but will only return the relevant data we need.

509Store.get_Certificates() will still return all the certificates like today, and will throw if one of them is corrupted, so we don't break the current experience. However, we lazily compute the list only once we actually enumerate it. This will allow to call the Find method with needing to enumerate the entire list.

vcsjones commented 3 years ago

Although its not even relevant to the key our application needs, you can't use our application, goodbye ? :)

Not exactly a proposal, just some idle thoughts. If I had any real "ideas" out of that thought then, perhaps we limit any new / additional behavior to well-known keychains/trust store like My, etc. Maybe we would exclude CertificateAuthority. I would be deeply concerned if there were malformed root certificates. Maybe I'm not making any sense - just some ideas.

My suggestion is to call SecItemCopyMatching each time we call Find, and refine the query arguments we pass into it.

I have two thoughts here, the first, is SecItemCopyMatching rich enough to expose all of the query options that Find does? For example, I don't see a way to express a X509FindType.FindByExtension, FindByIssuerName, FindByCertificatePolicy etc.

The second is, even if SecItemCopyMatching does have the query / attribute filters that we want, will this differ in behavior from the managed implementation? The ManagedCertificateFinder is used by Linux, Android, and macOS. If macOS now uses native APIs, I suspect there will be differing behaviors between the managed and Windows implementation (and it would not surprise me if the managed implementation was modeled off of how Windows does it). I like the general direction of moving things towards managed implementations, not away from it.

odhanson commented 3 years ago

@vcsjones I agree that implementing Find using SecItemCopyMatching might be hard. Another option I thought of, is at least provide some more detailed information in the error. So I suggest the following:

Add AppleCryptoNative_X509GetSubject to the native library. This will use the SecCertificateCopySubjectSummary to get a human readable representation of the cert.
We add a wrapper in Interop.X509.ca X509GetSubject which will be similar to X509GetRawData and will throw if there is any error. According to my checks, even with your corrupted example, this method will succeed. And if it throws, it should be easy for the user to locate the corrupted cert in the keychain using the Apple UI.
In AppleCertPal.EnsureCertData, we make a call to X509GetSubject first, so we have a string representation of the cert we are working on.
Then we can wrap the call to _certData = new CertificateData(Interop.AppleCrypto.X509GetRawData(_certHandle)); with a try/catch and append the name of the cert stored before.

This will at least give our users something to work with. Our app will break, but he will know which certificate to delete or fix in the keychain.

If everyone is on board this proposal, I will create a PR