[macOS] Listing system certificates bails out on invalid certificates in keychain

[filipnavara]

When there are invalid certificates are present in the user's keychain the enumeration of the whole X509Store fails with an exception:

System.Security.Cryptography.X509Certificates.System.Security.Cryptography.CryptographicException: Cryptography_X509_CertificateCorrupted, mgate.redsoft.at
    at Internal.Cryptography.Pal.AppleCertificatePal.EnsureCertData
    at Internal.Cryptography.Pal.AppleCertificatePal.get_Thumbprint
    at System.Security.Cryptography.X509Certificates.X509Certificate.GetHashCode
    at System.Collections.Generic.HashSet`1.AddIfNotPresent
    at System.Collections.Generic.HashSet`1.Add
    at Internal.Cryptography.Pal.StorePal.ReadCollection
    at Internal.Cryptography.Pal.StorePal+AppleKeychainStore.CloneTo
    at System.Security.Cryptography.X509Certificates.X509Store.get_Certificates

One such invalid certificate is here (from another user): 1satelit.cz.cer.zip

Unfortunately I cannot seem to be able to import it into keychain anymore with macOS 12.3.1 but have multiple users who already have preexisting corrupted certificates in the login keychain.

Perhaps we should handle the corrupted certificates differently so it doesn't block enumeration of the whole store?

[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

---

When there are invalid certificates are present in the user's keychain the enumeration of the whole `X509Store` fails with an exception: ``` System.Security.Cryptography.X509Certificates.System.Security.Cryptography.CryptographicException: Cryptography_X509_CertificateCorrupted, mgate.redsoft.at at Internal.Cryptography.Pal.AppleCertificatePal.EnsureCertData at Internal.Cryptography.Pal.AppleCertificatePal.get_Thumbprint at System.Security.Cryptography.X509Certificates.X509Certificate.GetHashCode at System.Collections.Generic.HashSet`1.AddIfNotPresent at System.Collections.Generic.HashSet`1.Add at Internal.Cryptography.Pal.StorePal.ReadCollection at Internal.Cryptography.Pal.StorePal+AppleKeychainStore.CloneTo at System.Security.Cryptography.X509Certificates.X509Store.get_Certificates ``` One such invalid certificate is here (from another user): [1satelit.cz.cer.zip](https://github.com/dotnet/runtime/files/8777079/1satelit.cz.cer.zip) Unfortunately I cannot seem to be able to import it into keychain anymore with macOS 12.3.1 but have multiple users who already have preexisting corrupted certificates in the login keychain. Perhaps we should handle the corrupted certificates differently so it doesn't block enumeration of the whole store?

Author:	filipnavara
Assignees:	-
Labels:	`area-System.Security`, `os-mac-os-x`
Milestone:	-

[filipnavara]

Actually, the certificate did import, just doesn't show up in the Keychain Access for me, so the error is reproducible.

[vcsjones]

This is another instance of https://github.com/dotnet/runtime/issues/54336. I don't think we came to a hard "no" on skipping over such entries, but there is some prior discussion there. I personally had some reservations about doing so.

It also looks like the SR.Format isn't producing the intended exception message correctly, which I will fix separately.

[vcsjones]

It also looks like the SR.Format isn't producing the intended exception message correctly, which I will fix separately.

Looks like it's working, I think you are just getting an unlocalized message?

[vcsjones]

Just because I was curious, the certificate is a v1 certificate.

/Users/vcsjones/Downloads/1satelit.cz.cer
└── Sequence (Length: 941; Offset: 0)
    ├── Sequence (Length: 661; Offset: 4)
    │   ├── Integer: 16639827517035674083 (0x0E6EC8AF714AE79E3) (Length: 11; Offset: 8)
    │   ├── Sequence (Length: 15; Offset: 19)
    │   │   ├── ObjectIdentifier: 1.2.840.113549.1.1.5 sha1WithRSAEncryption (Length: 11; Offset: 21)
    │   │   └── Null (Length: 2; Offset: 32)
    │   ├── Sequence (Length: 127; Offset: 34)
    │   │   ├── SetOf (Length: 13; Offset: 36)

But later down, it contains a subjectAltName extension, which requires version to be 3 (2 if zero-based). So we blow up in ValidateVersion.

    │   └── [3] (Length: 51; Offset: 614)
    │       └── Sequence (Length: 49; Offset: 616)
    │           └── Sequence (Length: 47; Offset: 618)
    │               ├── ObjectIdentifier: 2.5.29.17 subjectAltName (Length: 5; Offset: 620)
    │               └── OctetString: 3024820B31736174656C69742E637A820F7777772E31736174656C69742E637A870453A7EA1F (Length: 40; Offset: 625)
    │                   └── Sequence (Length: 38; Offset: 627)
    │                       ├── [2]: 1satelit.cz (Length: 13; Offset: 629)
    │                       ├── [2]: www.1satelit.cz (Length: 17; Offset: 642)
    │                       └── [7]: 53A7EA1F (Length: 6; Offset: 659)

I find it interesting that security is willing to tolerate this:

security verify-cert -d 2013-01-01-00:00:00 -c 1satelit.cz.cer -r 1satelit.cz.cer -t

...certificate verification successful.

[filipnavara]

It also looks like the SR.Format isn't producing the intended exception message correctly, which I will fix separately.

That's just a build flag for mobile builds to reduce size I think. It may be turned on because it's a net6.0-macos build with some defaults set by the Xamarin workload.

Ref: UseSystemResourceKeys

[filipnavara]

This is another instance of #54336.

Thanks for finding that. I was almost sure it was reported before but I didn't find it. We had 3 customers hit this error so far which suggests it's slightly more common than I would expect.

I don't think we came to a hard "no" on skipping over such entries, but there is some prior discussion there. I personally had some reservations about doing so.

There are basically three options: 1) Skip the invalid certificates 2) Don't decode the certificates during the enumeration (needs some poking at the data through Apple API but seems doable) and surface the exception only once the enumerated certificate is accessed 3) Obtain the corrupted certificates and relax the validation

I find skipping least offensive. On my machine that seems to be what Keychain Access app does despite the security command line tool being able to manipulate the keychain and the invalid certificate. However, on customer machines the certificates were visible in the Keychain app which suggests the behavior may not be universal.

[vcsjones]

Don't decode the certificates during the enumeration

That was discussed somewhat in the previous issue, that gets tricky with Find because Apple's APIs are not rich enough to express all of the search operations we need, so we have no choice but to enumerate the whole store and filter out in managed code.

Obtain the corrupted certificates and relax the validation

I don't think this one is feasible either. In this particular example the certificate is well-formed... just not spec conforming. In the other issue though, I gave an example of a invalid DER certificate that can get added to a keychain.

In my mind then, there are three options:

1. Skip the invalid certs. Maybe we only do this for certain keychains, etc.
2. Do nothing.
3. Do an incomplete fix. Allow certificates with bad versions to be loaded and move the version check elsewhere, or remove the version check.

@filipnavara do you know offhand know what Windows does with this certificate?

I get that option 2 is not ideal. Since the cert doesn't even appear in the keychain, customers cannot easily remediate. They have to go through CLI. .NET can't delete the certificate either on behalf of the user.

@bartonjs what say you?

[filipnavara]

That was discussed somewhat in the previous issue, that gets tricky with Find because Apple's APIs are not rich enough to express all of the search operations we need, so we have no choice but to enumerate the whole store and filter out in managed code.

Ah, right. I was thinking about the case of no filtering and whether the certificates should be returned and have at least RawData work. In case of filtering I would argue that unparsable certificate doesn't match any criteria.

In my mind then, there are three options:

1. Skip the invalid certs. Maybe we only do this for certain keychains, etc.
2. Do nothing.

The problem with doing nothing is that we get an exception and we cannot get access to any valid certificates in the keychain. Obviously that's even worse than skipping from consumer perspective. They usually don't even know about the invalid certificate in the first place.

3. Do an incomplete fix. Allow certificates with bad versions to be loaded and move the version check elsewhere, or remove the version check.

I am leaning towards skipping them, to be honest.

Since the cert doesn't even appear in the keychain, customers cannot easily remediate.

It's relatively easy to fix on command line with security tool once the problem is identified and the certificate subject is known.

@filipnavara do you know offhand know what Windows does with this certificate?

I will check when I get home. I remember browsers rejecting the certificate (since it was used on HTTPS endpoint) with error.

[filipnavara]

@filipnavara do you know offhand know what Windows does with this certificate?

Windows treats the certificate as valid:

Chromium rejects it with ERR_SSL_SERVER_CERT_BAD_FORMAT.

[bartonjs]

I... can probably be convinced that for some cases we should skip invalid certificates. Whether "some" means "everything except Disallowed" or "only for CurrentUser\My, LocalMachine\Root, and CurrentUser\Root" I'm open to debate on (starting with allowing the three positive trust cases and expanding it to an except model is probably the safer option). That, of course, depends partially on my memory being right and Disallowed is a read-only store on macOS is an interpretation of the system trust rules.

As for removing the checks for "unless it says v3 Extensions MUST be NULL"... if Windows doesn't care and Linux/OpenSSL doesn't care, then I'm fine relaxing our interpretation. If either of them fails loading it, then it'll take more work to convince me to not be spec compliant.