Skip to content

Commit baf2b1e

Browse files
NickCraverNickCraver
authored
CheckTrustedIssuer: Fixes for invalid chains (#2665)
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.
1 parent 91520e0 commit baf2b1e

9 files changed

Lines changed: 216 additions & 12 deletions

File tree

‎docs/ReleaseNotes.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ Current package versions:
99
## Unreleased
1010

1111
- Add new `LoggingTunnel` API; see https://stackexchange.github.io/StackExchange.Redis/Logging ([#2660 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2660))
12+
- **Potentiallty Breaking**: Fix `CheckTrustedIssuer` certificate validation for broken chain scenarios ([#2665 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2665))
13+
- Users inadvertently trusting a remote cert with a broken chain could not be failing custom validation before this change. This is only in play if you are using `ConfigurationOptions.TrustIssuer` at all.
1214

1315
## 2.7.27
1416

‎src/StackExchange.Redis/ConfigurationOptions.cs‎

Lines changed: 54 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Net.Security;
77
using System.Net.Sockets;
88
using System.Security.Authentication;
9+
using System.Security.Cryptography;
910
using System.Security.Cryptography.X509Certificates;
1011
using System.Text;
1112
using System.Threading;
@@ -279,13 +280,13 @@ public bool CheckCertificateRevocation
279280
}
280281

281282
/// <summary>
282-
/// Create a certificate validation check that checks against the supplied issuer even if not known by the machine.
283+
/// Create a certificate validation check that checks against the supplied issuer even when not known by the machine.
283284
/// </summary>
284285
/// <param name="issuerCertificatePath">The file system path to find the certificate at.</param>
285286
public void TrustIssuer(string issuerCertificatePath) => CertificateValidationCallback = TrustIssuerCallback(issuerCertificatePath);
286287

287288
/// <summary>
288-
/// Create a certificate validation check that checks against the supplied issuer even if not known by the machine.
289+
/// Create a certificate validation check that checks against the supplied issuer even when not known by the machine.
289290
/// </summary>
290291
/// <param name="issuer">The issuer to trust.</param>
291292
public void TrustIssuer(X509Certificate2 issuer) => CertificateValidationCallback = TrustIssuerCallback(issuer);
@@ -296,24 +297,65 @@ private static RemoteCertificateValidationCallback TrustIssuerCallback(X509Certi
296297
{
297298
if (issuer == null) throw new ArgumentNullException(nameof(issuer));
298299

299-
return (object _, X509Certificate? certificate, X509Chain? __, SslPolicyErrors sslPolicyError)
300-
=> sslPolicyError == SslPolicyErrors.RemoteCertificateChainErrors
301-
&& certificate is X509Certificate2 v2
302-
&& CheckTrustedIssuer(v2, issuer);
300+
return (object _, X509Certificate? certificate, X509Chain? certificateChain, SslPolicyErrors sslPolicyError) =>
301+
{
302+
// If we're already valid, there's nothing further to check
303+
if (sslPolicyError == SslPolicyErrors.None)
304+
{
305+
return true;
306+
}
307+
// If we're not valid due to chain errors - check against the trusted issuer
308+
// Note that we're only proceeding at all here if the *only* issue is chain errors (not more flags in SslPolicyErrors)
309+
return sslPolicyError == SslPolicyErrors.RemoteCertificateChainErrors
310+
&& certificate is X509Certificate2 v2
311+
&& CheckTrustedIssuer(v2, certificateChain, issuer);
312+
};
303313
}
304314

305-
private static bool CheckTrustedIssuer(X509Certificate2 certificateToValidate, X509Certificate2 authority)
315+
private static bool CheckTrustedIssuer(X509Certificate2 certificateToValidate, X509Chain? chainToValidate, X509Certificate2 authority)
306316
{
307-
// reference: https://stackoverflow.com/questions/6497040/how-do-i-validate-that-a-certificate-was-created-by-a-particular-certification-a
308-
X509Chain chain = new X509Chain();
317+
// Reference:
318+
// https://stackoverflow.com/questions/6497040/how-do-i-validate-that-a-certificate-was-created-by-a-particular-certification-a
319+
// https://github.com/stewartadam/dotnet-x509-certificate-verification
320+
using X509Chain chain = new X509Chain();
309321
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
310-
chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot;
311322
chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllowUnknownCertificateAuthority;
312-
chain.ChainPolicy.VerificationTime = DateTime.Now;
323+
chain.ChainPolicy.VerificationTime = chainToValidate?.ChainPolicy?.VerificationTime ?? DateTime.Now;
313324
chain.ChainPolicy.UrlRetrievalTimeout = new TimeSpan(0, 0, 0);
314325

315326
chain.ChainPolicy.ExtraStore.Add(authority);
316-
return chain.Build(certificateToValidate);
327+
try
328+
{
329+
// This only verifies that the chain is valid, but with AllowUnknownCertificateAuthority could trust
330+
// self-signed or partial chained vertificates
331+
var chainIsVerified = chain.Build(certificateToValidate);
332+
if (chainIsVerified)
333+
{
334+
// Our method is "TrustIssuer", which means any intermediate cert we're being told to trust
335+
// is a valid thing to trust, up until it's a root CA
336+
bool found = false;
337+
byte[] authorityData = authority.RawData;
338+
foreach (var chainElement in chain.ChainElements)
339+
{
340+
#if NET8_0_OR_GREATER
341+
#error TODO: use RawDataMemory (needs testing)
342+
#endif
343+
using var chainCert = chainElement.Certificate;
344+
if (!found && chainCert.RawData.SequenceEqual(authorityData))
345+
{
346+
found = true;
347+
}
348+
}
349+
return found;
350+
}
351+
}
352+
catch (CryptographicException)
353+
{
354+
// We specifically don't want to throw during validation here and would rather exit out gracefully
355+
}
356+
357+
// If we didn't find the trusted issuer in the chain at all - we do not trust the result.
358+
return false;
317359
}
318360

319361
/// <summary>

‎tests/StackExchange.Redis.Tests/Certificates/CertValidationTests.cs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
using System;
2+
using System.IO;
3+
using System.Net.Security;
4+
using System.Security.Cryptography.X509Certificates;
5+
using Xunit;
6+
using Xunit.Abstractions;
7+
8+
namespace StackExchange.Redis.Tests;
9+
10+
public class CertValidationTests : TestBase
11+
{
12+
public CertValidationTests(ITestOutputHelper output) : base(output) { }
13+
14+
[Fact]
15+
public void CheckIssuerValidity()
16+
{
17+
// The endpoint cert is the same here
18+
var endpointCert = LoadCert(Path.Combine("Certificates", "device01.foo.com.pem"));
19+
20+
// Trusting CA explicitly
21+
var callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "ca.foo.com.pem"));
22+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
23+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
24+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
25+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
26+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNameMismatch));
27+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNotAvailable));
28+
29+
// Trusting the remote endpoint cert directly
30+
callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "device01.foo.com.pem"));
31+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
32+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
33+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
34+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
35+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNameMismatch));
36+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNotAvailable));
37+
38+
// Attempting to trust another CA (mismatch)
39+
callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "ca2.foo.com.pem"));
40+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
41+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
42+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
43+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
44+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNameMismatch));
45+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNotAvailable));
46+
}
47+
48+
private static X509Certificate2 LoadCert(string certificatePath) => new X509Certificate2(File.ReadAllBytes(certificatePath));
49+
50+
[Fact]
51+
public void CheckIssuerArgs()
52+
{
53+
Assert.ThrowsAny<Exception>(() => ConfigurationOptions.TrustIssuerCallback(""));
54+
55+
var opt = new ConfigurationOptions();
56+
Assert.Throws<ArgumentNullException>(() => opt.TrustIssuer((X509Certificate2)null!));
57+
}
58+
}

‎tests/StackExchange.Redis.Tests/Certificates/README.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
The certificates here are randomly generated for testing only.
2+
They are not valid and only used for test validation.
3+
4+
Please do not file security issue noise - these have no impact being public at all.

‎tests/StackExchange.Redis.Tests/Certificates/ca.foo.com.pem‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIDTTCCAjWgAwIBAgIUU9SR3QMGVO8yN/mr8SQJ1p/OgIAwDQYJKoZIhvcNAQEL
3+
BQAwNjETMBEGA1UEAwwKY2EuZm9vLmNvbTESMBAGA1UECgwJTXlEZXZpY2VzMQsw
4+
CQYDVQQGEwJVUzAeFw0yNDAzMDcxNDAxMzJaFw00ODEwMjcxNDAxMzJaMDYxEzAR
5+
BgNVBAMMCmNhLmZvby5jb20xEjAQBgNVBAoMCU15RGV2aWNlczELMAkGA1UEBhMC
6+
VVMwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCqk8FT5dHU335oSEuY
7+
RGeHOHmtxtr5Eoxe4pRHWcBKARzRYi+fPjP/aSWh75yYcmyQ5o5e2JQTZQRwSaLh
8+
q8lrsT7AIeZboATVxECyT3kZdIJkLgWbfyzwJQtrW+ccDx3gDRt0FKRt8Hd3foIf
9+
ULICgkiz3C5sihT589QWmcP4XhcRf3A1bt3rrFWJBO1jmKz0P7pijT14lkdW4sVL
10+
AdFhoNg/a042a7wq2i8PxrkhWpwmHkW9ErnbWG9pRjMme+GDeNfGdHslL5grzbzC
11+
4B4w3QP4opLUp29O9oO1DjaAuZ86JVdy3+glugMvj4f8NVCVlHxRM5Kn/3WgWIIM
12+
XBK7AgMBAAGjUzBRMB0GA1UdDgQWBBRmgj4urVgvTcPgJtyqyUHaFX0svjAfBgNV
13+
HSMEGDAWgBRmgj4urVgvTcPgJtyqyUHaFX0svjAPBgNVHRMBAf8EBTADAQH/MA0G
14+
CSqGSIb3DQEBCwUAA4IBAQB2DIGlKpCdluVHURfgA5zfwoOnhtOZm7lwC/zbNd5a
15+
wNmb6Vy29feN/+6/dv7MFTXXB5f0TkDGrGbAsKVLD5mNSfQhHC8sxwotheMYq6LS
16+
T1Pjv3Vxku1O7q6FQrslDWfSBxzwep2q8fDXwD4C7VgVRM2EGg/vVee2juuTCmMU
17+
Z1LwJrOkBczW6b3ZvUThFGOvZkuI138EuR2gqPHMQIiQcPyX1syT7yhJAyDQRYOG
18+
cHSRojNciYVotSTgyYguUJdU7vfRJ+MLfoZClzJgvNav8yUC+sSrb5RD5vQlvxzG
19+
KrJ8Hh+hpIFsmQKj5dcochKvLLd1Z748b2+FB0jtxraU
20+
-----END CERTIFICATE-----

‎tests/StackExchange.Redis.Tests/Certificates/ca2.foo.com.pem‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIDTzCCAjegAwIBAgIUYXv168vvB1NPg3PfoRzcNFEMaC8wDQYJKoZIhvcNAQEL
3+
BQAwNzEUMBIGA1UEAwwLY2EyLmZvby5jb20xEjAQBgNVBAoMCU15RGV2aWNlczEL
4+
MAkGA1UEBhMCVVMwHhcNMjQwMzA3MTQwMTMyWhcNNDgxMDI3MTQwMTMyWjA3MRQw
5+
EgYDVQQDDAtjYTIuZm9vLmNvbTESMBAGA1UECgwJTXlEZXZpY2VzMQswCQYDVQQG
6+
EwJVUzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAOFDZ2sf8Ik/I3jD
7+
Mp4NGoo+kMY1BiRWjSKnaphfcosR2MAT/ROIVhnkzSeiQQByf34cqN/7xNkHaufr
8+
oVcMuuiyWERPoZjBqnfzLZ+93uxnyIU6DVDdNIcKcBQxyhHMfOigFhKTia6eWhrf
9+
zSaBhbkndaUsXdINBAJgSq3HDuk8bIw8MTZH0giorhIyyyqT/gjWEbzKx6Ww99qV
10+
MMcjFIvXEmD9AEaNilHD4TtwqZrZKZpnVBaQvWrCK3tCGBDyiFlUhAibchbt/JzV
11+
sK002TFfUbn41ygHmcrBVL7lSEsuT2W/PNuDOdWa6NQ2RVzYivs5jYbWV1cAvAJP
12+
HMWJkZ8CAwEAAaNTMFEwHQYDVR0OBBYEFA6ZeCMPgDEu+eIUoCxU/Q06ViyoMB8G
13+
A1UdIwQYMBaAFA6ZeCMPgDEu+eIUoCxU/Q06ViyoMA8GA1UdEwEB/wQFMAMBAf8w
14+
DQYJKoZIhvcNAQELBQADggEBAGOa/AD0JNPwvyDi9wbVU+Yktx3vfuVyOMbnUQSn
15+
nOyWd6d95rZwbeYyN908PjERQT3EMo8/O0eOpoG9I79vjbcD6LAIbxS9XdI8kK4+
16+
D4e/DX/R85KoWSprB+VRXGqsChY0Y+4x5x2q/IoCi6+tywhzjqIlaGDYrlc688HO
17+
/+4iR9L945gpg4NT1hLnCwDYcdZ5vjv4NfgXDbGPUcEheYnfz3cHE2mYxEG9KXta
18+
f8hSj/MNNv31BzNcj4XKcDqp4Ke3ow4lAZsPPlixOxxRaLnpsKZmEYYQcLI8KVNk
19+
gdAUOSPZgzRqAag0rvVfrpyvfvlu0D9xeiBLdhaJeZCq1/s=
20+
-----END CERTIFICATE-----

‎tests/StackExchange.Redis.Tests/Certificates/create_certificates.sh‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
#!/bin/bash
2+
set -eu
3+
# Adapted from https://github.com/stewartadam/dotnet-x509-certificate-verification/blob/main/create_certificates.sh
4+
5+
base_dir="certificates"
6+
7+
create_ca() {
8+
local CA_CN="$1"
9+
local certificate_output="${base_dir}/${CA_CN}.pem"
10+
11+
openssl genrsa -out "${base_dir}/${CA_CN}.key.pem" 2048 # Generate private key
12+
openssl req -x509 -new -nodes -key "${base_dir}/${CA_CN}.key.pem" -sha256 -days 9000 -out "${certificate_output}" -subj "/CN=${CA_CN}/O=MyDevices/C=US" # Generate root certificate
13+
14+
echo -e "\nCertificate for CA ${CA_CN} saved to ${certificate_output}\n\n"
15+
}
16+
17+
create_leaf_cert_req() {
18+
local DEVICE_CN="$1"
19+
20+
openssl genrsa -out "${base_dir}/${DEVICE_CN}.key.pem" 2048 # new private key
21+
openssl req -new -key "${base_dir}/${DEVICE_CN}.key.pem" -out "${base_dir}/${DEVICE_CN}.csr.pem" -subj "/CN=${DEVICE_CN}/O=MyDevices/C=US" # generate signing request for the CA
22+
}
23+
24+
sign_leaf_cert() {
25+
local DEVICE_CN="$1"
26+
local CA_CN="$2"
27+
local certificate_output="${base_dir}/${DEVICE_CN}.pem"
28+
29+
openssl x509 -req -in "${base_dir}/${DEVICE_CN}.csr.pem" -CA ""${base_dir}/${CA_CN}.pem"" -CAkey "${base_dir}/${CA_CN}.key.pem" -set_serial 01 -out "${certificate_output}" -days 8999 -sha256 # sign the CSR
30+
31+
echo -e "\nCertificate for ${DEVICE_CN} saved to ${certificate_output}\n\n"
32+
}
33+
34+
mkdir -p "${base_dir}"
35+
36+
# Create one self-issued CA + signed cert
37+
create_ca "ca.foo.com"
38+
create_leaf_cert_req "device01.foo.com"
39+
sign_leaf_cert "device01.foo.com" "ca.foo.com"

‎tests/StackExchange.Redis.Tests/Certificates/device01.foo.com.pem‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIC5jCCAc4CAQEwDQYJKoZIhvcNAQELBQAwNjETMBEGA1UEAwwKY2EuZm9vLmNv
3+
bTESMBAGA1UECgwJTXlEZXZpY2VzMQswCQYDVQQGEwJVUzAeFw0yNDAzMDcxNDAx
4+
MzJaFw00ODEwMjYxNDAxMzJaMDwxGTAXBgNVBAMMEGRldmljZTAxLmZvby5jb20x
5+
EjAQBgNVBAoMCU15RGV2aWNlczELMAkGA1UEBhMCVVMwggEiMA0GCSqGSIb3DQEB
6+
AQUAA4IBDwAwggEKAoIBAQDBb4Mv87+MFVGLIWArc0wV1GH4h7Ha+49K+JAi8rtk
7+
fpQACcu3OGq5TjUuxecOz5eXDwJj/vR1rvjP0DaCuIlx4SNXXqVKooWpCLb2g4Mr
8+
IIiFcBsiaJNmhFvd92bqHOyuXsUTjkJKaLmH6nUqVIXEA/Py+jpuSFRp9N475IGZ
9+
yUUdaQUx9Ur953FagLbPVeE5Vh+NEA8vnw+ZBCQRBHlRgvSJtCAR/oznXXPdHGGZ
10+
rMWeNjl+v1iP8fZMq4vvooW0/zTVgH8lE/HJXtpaWEVeGpnOqBsgvl12WTGL5dMU
11+
n81JiI3AdUyW0ieh/5yr+OFNa/HNqGLK1NvnCDPbBFpnAgMBAAEwDQYJKoZIhvcN
12+
AQELBQADggEBAEpIIJJ7q4/EbCJog29Os9l5unn7QJon4R5TGJQIxdqDGrhXG8QA
13+
HiBGl/lFhAp9tfKvQIj4aODzMgHmDpzZmH/yhjDlquJgB4JYDDjf9UhtwUUbRDp0
14+
rEc5VikLuTJ21hcusKALH5fgBjzplRNPH8P660FxWnv9gSWCMNaiFCCxTU91g4L3
15+
/qZPTl5nr1j6M/+zXbndS5qlF7GkU5Kv9xEmasZ0Z65Wao6Ufhw+nczLbiLErrxD
16+
zLtTfr6WYFqzXeiPGnjTueG+1cYDjngmj2fbtjPn4W67q8Z0M/ZMj9ikr881d3zP
17+
3dzUEaGexGqvA2MCapIQ2vCCMDF33ismQts=
18+
-----END CERTIFICATE-----

‎tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
<None Update="*.json" CopyToOutputDirectory="Always" />
1414
<EmbeddedResource Include="*Config.json" />
1515
<None Update="redislabs_ca.pem" CopyToOutputDirectory="PreserveNewest" />
16+
<None Update="Certificates\*.pem" CopyToOutputDirectory="PreserveNewest" />
1617
</ItemGroup>
1718

1819
<ItemGroup>

0 commit comments

Comments
 (0)