CheckTrustedIssuer: Fixes for invalid chains

[NickCraver]

This issue was brought to my attention last night (thanks to Badrish Chandramouli): dotnet/dotnet-api-docs#6660

This changeset ensures that we do not honor self-signed certs or partial/broken chains as a result of X509VerificationFlags.AllowUnknownCertificateAuthority downstream and adds a few tests and utilities to generate test certificates (currently valid for ~9000 days). Instead we are checking that the certificate we're being told to trust is explicitly in the chain, given that the result of .Build() cannot be trusted for this case.

This also resolves an issue where TrustIssuer could be called but we'd error when no errors were detected (due to requiring chain errors in our validator), this means users couldn't temporarily trust a cert while getting it installed on the machine for instance and migrating between the 2 setups was difficult.

This needs careful eyes, please scrutinize heavily. It's possible this breaks an existing user, but...it should be broken if so unless there's a case I'm not seeing.

[mgravell]

Conceptually I'm 👍 here, however: I do not consider myself a certificates expert. Locally, have we tried connecting to (say) azure redis after this change?

[NickCraver]

Same here - reaching out for more test scenarios to be careful here.

[vcsjones]

The "right" way to do this in .NET 5+ is to use CustomTrustStore. But unfortunately it does not work for your needs since it is based on a trusted root, not a trusted issuer.

The reason I point this out is that AllowUnknownCertificateAuthority can be unreliable. Android, for example, does not work with AllowUnknownCertificateAuthority (reference: https://github.com/dotnet/runtime/blob/80b1e143c686c9548ee5930f2466805a76f095bd/src/libraries/System.Security.Cryptography/tests/X509Certificates/DynamicChainTests.cs#L198). The Android chain builder just doesn't give back partial or untrusted chains. This is "fine" since it doesn't cause you to fail open (that is, not a security issue) but someone could run in to a situation where TrustIssuer doesn't work.

Who is calling Redis from Android, Kevin?

Well, you never know, plus there are other platform quirks about returning partial chains.

So, should I use CustomTrustStore?

Initially I was going to recommend it for .NET 5+, but seeing how your implementation is based on trusting an arbitrary anchor, not a root, it makes it difficult to recommend, or would otherwise have different behavior.

You still aren't looking at the pull request Kevin

Oh, right. Let's take a look. Well, it builds a chain and permits exactly one error, AUCA, which is fine. If it's valid, then see if any of the certificates in the chain match the "trusted" certificate by thumbprint. If nothing matches, then "false".

That all seems fine.

Suggestions

1. It is possible for X509Chain.Build to throw CryptographicException in very rare scenarios. You may want to catch it and return false instead of letting the exception bubble up.
2. X509Chain is disposable. You may want to dispose of it instead of leaving it up to the GC.

[bartonjs]

It is possible for X509Chain.Build to throw CryptographicException in very rare scenarios.

FWIW, I believe all of those scenarios are deterministic, based on the EE certificate. And since this is from an SslStream callback, X509Chain has already run once (and not kerploded), so it's probably fine.

But adding the try/catch doesn't hurt.

[bartonjs]

Instead of checking in certificates that will eventually expire; you could use System.Security.Cryptography.X509Certificates.CertificateRequest to just create them on the fly. (net472+/net5+)

[bartonjs]

OK. Last comment (I think 😉).

Aside from Internet connectivity flakiness, the only difference between this chain build and the one you were given in the callback is that you've definitely added the specified pin/anchor to ExtraStore. That means the chain will realistically only be different if the incoming chain terminated in PartialChain and the specified pin/anchor is the next hop.

That means that you could run this same logic on the existing chain, with two minor changes:

1. You'd need to check each chainElement.ChainElementStatus for the presence of an error other than PartialChain or UnknownRootAuthority.
2. If it did have PartialChain, and you didn't find your anchor, then you'd run it again with this logic.

Both the flow I suggested here and what the code is trying to accomplish, you'll potentially have issues regarding cross-certification. Since you can't ask .NET to ask the OS to consider this intermediate as a trust anchor, the OS might prefer a variant of the certificate. For example, the Let's Encrypt R3 intermediate was signed by both "ISRG Root X1" and "DST Root CA X3" (https://crt.sh/?caid=183267). If the anchor in your flow was the R3 signed by DST-X3; but the underlying OS trusts ISRG-X1 and not DST-X3, then the OS will completely ignore the ExtraStore input as irrelevant because it always prefers to build trusted chains over untrusted chains. If it trusts both, or neither, then "stuff happens" (I don't know if the ExtraStore nudge will act as a tiebreaker, or if it comes down to load-order, phase of the moon, etc). Really this is just a concern for the inputs: the certificates should always be from nice single-path segments of a chain. Once you've made that constraint/assumption, then you don't also have a reason to rerun the chain build just because you don't see the intended anchor.

And, finally, regarding disposing things... for maximum reduction of finalization, you should dispose all of the certificates from chain.ChainElements in the chain the callback gave you. .NET Framework didn't dispose them (so they went off to the finalizer). And I thought the .NET Core line only did it when you didn't have a callback; but looking at the code just now it seems to do it in a finally and has no suppression for if the chain was passed to a callback. Huh.

[bartonjs]

OK, I lied. One more comment: If you get a trusted chain as input here, you're returning that it's trusted even if the intended trust anchor was missing. So this early success is bad if you're trying to enforce pinning.

If the anchor cert is REQUIRED, then you want this instead to be

if ((sslPolicyError & ~SslPolicyErrors.RemoteCertificateChainErrors) != SslPolicyErrors.None)
{
    return false;
}

That will cause an immediate false return for:

• There is no remote cert
• Hostname mismatch
• Any other error we ever add in the future. (Right now there are only those two plus the one you're expecting)

[vcsjones]

Hm. Based on the documentation I did not think the intention was pinning

StackExchange.Redis/src/StackExchange.Redis/ConfigurationOptions.cs

Line 282 in 441f89a

/// Create a certificate validation check that checks against the supplied issuer even if not known by the machine.

But a good question to ask.

[bartonjs]

Since @vcsjones and I have had a discussion offline regarding what that sentence means... if it is pinning, the code's wrong. If it isn't pinning, the docs would probably be more clear by replacing "even if" with "when".

• I think "even if" means "when normal, and when also", which would make it pinning.
• @vcsjones agrees, but thinks "even if not" is somehow special. Which I disagree with.

/me and @vcsjones now watch to see what Nick says the right answer should be.

[NickCraver]

Playing favorites only gets me in trouble, tried going with making everyone angry instead - thoughts?

[bartonjs]

So is the intention that anything that's trusted by default is acceptable, and if it's not trusted then (and only then) this anchor is checked as a fallback?

If that's the intention, my concern in the docs is with the word "even";

Create a certificate validation check that checks against the supplied issuer even when not known by the machine.

Sounds like pinning to me.

Create a certificate validation check that checks against the supplied issuer when not known by the machine.

Sounds like a fallback.

[vcsjones]

The RawData property has the behavior of allocating by returning defensive copies. That can't really be avoided for each intermediate, but you could stash the RawData from the authority so it doesn't allocate in each loop.

.NET 8 has RawDataMemory which does not allocate if and when you ever start to target .NET 8, you could consider conditionally using RawDataMemory.

Suggested change

	bool found = false;
	foreach (var chainElement in chain.ChainElements)
	{
	using var chainCert = chainElement.Certificate;
	if (!found && chainCert.RawData.SequenceEqual(authority.RawData))
	{
	found = true;
	}
	}
	return found;
	bool found = false;
	byte[] authorityBytes = authority.RawData;
	foreach (var chainElement in chain.ChainElements)
	{
	using var chainCert = chainElement.Certificate;
	if (!found && chainCert.RawData.SequenceEqual(authorityBytes))
	{
	found = true;
	}
	}
	return found;

[NickCraver]

ACK - no target at the moment but will keep in mind!

[mgravell]

@NickCraver maybe we should add a pre-emptive:

#if NET8_0_OR_GREATER
#error TODO: use RawDataMemory (needs testing)
#endif

??

[NickCraver]

@bartonjs @vcsjones Latest comments appreciated - if you have time for final eyes would appreciate it, want to do a release with this and other fixes today if we're good.