Skip to content

Commit f6ef082

Browse files
committed
CheckTrustedIssuer: Fixes for invalid chains
This issue was brought to my attention last night (thanks reporter!): 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.
1 parent 441f89a commit f6ef082

File tree

8 files changed

+192
-6
lines changed

8 files changed

+192
-6
lines changed

src/StackExchange.Redis/ConfigurationOptions.cs

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -296,15 +296,25 @@ private static RemoteCertificateValidationCallback TrustIssuerCallback(X509Certi
296296
{
297297
if (issuer == null) throw new ArgumentNullException(nameof(issuer));
298298

299-
return (object _, X509Certificate? certificate, X509Chain? __, SslPolicyErrors sslPolicyError)
300-
=> sslPolicyError == SslPolicyErrors.RemoteCertificateChainErrors
301-
&& certificate is X509Certificate2 v2
302-
&& CheckTrustedIssuer(v2, issuer);
299+
return (object _, X509Certificate? certificate, X509Chain? __, SslPolicyErrors sslPolicyError) =>
300+
{
301+
// If we're already valid, there's nothing further to check
302+
if (sslPolicyError == SslPolicyErrors.None)
303+
{
304+
return true;
305+
}
306+
// If we're not valid due to chain errors - check against the trusted issuer
307+
return sslPolicyError == SslPolicyErrors.RemoteCertificateChainErrors
308+
&& certificate is X509Certificate2 v2
309+
&& CheckTrustedIssuer(v2, issuer);
310+
};
303311
}
304312

305313
private static bool CheckTrustedIssuer(X509Certificate2 certificateToValidate, X509Certificate2 authority)
306314
{
307-
// reference: https://stackoverflow.com/questions/6497040/how-do-i-validate-that-a-certificate-was-created-by-a-particular-certification-a
315+
// Reference:
316+
// https://stackoverflow.com/questions/6497040/how-do-i-validate-that-a-certificate-was-created-by-a-particular-certification-a
317+
// https://github.com/stewartadam/dotnet-x509-certificate-verification
308318
X509Chain chain = new X509Chain();
309319
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
310320
chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot;
@@ -313,7 +323,23 @@ private static bool CheckTrustedIssuer(X509Certificate2 certificateToValidate, X
313323
chain.ChainPolicy.UrlRetrievalTimeout = new TimeSpan(0, 0, 0);
314324

315325
chain.ChainPolicy.ExtraStore.Add(authority);
316-
return chain.Build(certificateToValidate);
326+
// This only verifies that the chain is valid, but with AllowUnknownCertificateAuthority could trust
327+
// self-signed or partial chained vertificates
328+
var chainIsVerfiied = chain.Build(certificateToValidate);
329+
if (chainIsVerfiied)
330+
{
331+
// Our method is "TrustIssuer", which means any intermediate cert we're being told to trust
332+
// is a valid thing to trust, up until it's a root CA
333+
foreach (var chainElement in chain.ChainElements)
334+
{
335+
if (chainElement.Certificate.RawData.SequenceEqual(authority.RawData))
336+
{
337+
return true;
338+
}
339+
}
340+
}
341+
// If we didn't find the trusted issuer in the chain at all - we do not trust the result.
342+
return false;
317343
}
318344

319345
/// <summary>
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;
5+
using System.Security.Cryptography.X509Certificates;
6+
using System.Text;
7+
using System.Text.RegularExpressions;
8+
using Xunit;
9+
using Xunit.Abstractions;
10+
11+
namespace StackExchange.Redis.Tests;
12+
13+
public class CertValidationTests : TestBase
14+
{
15+
public CertValidationTests(ITestOutputHelper output) : base(output) { }
16+
17+
[Fact]
18+
public void CheckIssuerValidity()
19+
{
20+
// The endpoint cert is the same here
21+
var endpointCert = LoadCert(Path.Combine("Certificates", "device01.foo.com.pem"));
22+
23+
// Trusting CA explicitly
24+
var callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "ca.foo.com.pem"));
25+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
26+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
27+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
28+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
29+
30+
// Trusting the remote endpoint cert directly
31+
callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "device01.foo.com.pem"));
32+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
33+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
34+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
35+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
36+
37+
// Attempting to trust another CA (mismatch)
38+
callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "ca2.foo.com.pem"));
39+
Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
40+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
41+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
42+
Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
43+
}
44+
45+
private static X509Certificate2 LoadCert(string certificatePath) =>
46+
new X509Certificate2(StripPEM(File.ReadAllBytes(certificatePath)));
47+
private static byte[] StripPEM(byte[] pemData) =>
48+
Convert.FromBase64String(Regex.Replace(Encoding.UTF8.GetString(pemData), "(-+BEGIN CERTIFICATE-+)|(-+END CERTIFICATE-+)", "").Trim());
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+
}
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.
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-----
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-----
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+
# Adopted 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"
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)