Repository navigation
ASN1 corrupted data while calling X509Store.Certificates on Mac #54336
Description
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):
- 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).
Activity
bartonjs commentedon Jun 17, 2021
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 commentedon Jun 17, 2021
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 commentedon Jun 17, 2021
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 commentedon Jun 17, 2021
I see. Makes sense to me.
odhanson commentedon Jun 17, 2021
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 commentedon Jun 17, 2021
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.
vcsjones commentedon Jun 17, 2021
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.
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 commentedon Jun 18, 2021
@bartonjs
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:
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
I'm a bit unsure how this would work.
Findreturns 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 commentedon Jun 20, 2021
@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 commentedon Jun 20, 2021
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 excludeCertificateAuthority. I would be deeply concerned if there were malformed root certificates. Maybe I'm not making any sense - just some ideas.I have two thoughts here, the first, is
SecItemCopyMatchingrich enough to expose all of the query options thatFinddoes? For example, I don't see a way to express aX509FindType.FindByExtension,FindByIssuerName,FindByCertificatePolicyetc.The second is, even if
SecItemCopyMatchingdoes have the query / attribute filters that we want, will this differ in behavior from the managed implementation? TheManagedCertificateFinderis 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 commentedon Jun 20, 2021
@vcsjones I agree that implementing Find using
SecItemCopyMatchingmight be hard.Another option I thought of, is at least provide some more detailed information in the error. So I suggest the following:
_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