Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include Intermediate Certificate Resolution for S/MIME and Ensure Trust Validation during BuildCertificateChain() #1124

Merged
merged 3 commits into from
Jan 1, 2025
Merged

Include Intermediate Certificate Resolution for S/MIME and Ensure Trust Validation during BuildCertificateChain() #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f703a8
Adding CRL checks during BuildCertificateChain
JoeShook Dec 24, 2024
0feb662
ValidateCertificateChain
JoeShook Dec 27, 2024
e1599c1
Fixup CRL tests
JoeShook Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
202 changes: 193 additions & 9 deletions MimeKit/Cryptography/BouncyCastleSecureMimeContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
using IssuerAndSerialNumber = Org.BouncyCastle.Asn1.Cms.IssuerAndSerialNumber;

using MimeKit.IO;
using System.Linq;
using Org.BouncyCastle.Tls;

namespace MimeKit.Cryptography {
/// <summary>
Expand Down Expand Up @@ -169,6 +171,9 @@ protected virtual HttpClient HttpClient {
/// generally issued by a certificate authority (CA).</para>
/// <para>This method is used to build a certificate chain while verifying
/// signed content.</para>
/// <para>It is critical to always load the designated trust anchors
/// and not the anchor in the end certificate when building a certificate chain
/// to validated trust.</para>
/// </remarks>
/// <returns>The trusted anchors.</returns>
protected abstract ISet<TrustAnchor> GetTrustedAnchors ();
Expand Down Expand Up @@ -325,6 +330,9 @@ CmsSignedDataStreamGenerator CreateSignedDataGenerator (CmsSigner signer)

Stream Sign (CmsSigner signer, Stream content, bool encapsulate, CancellationToken cancellationToken)
{
if (CheckCertificateRevocation)
ValidateCertificateChain (signer.CertificateChain, DateTime.UtcNow, cancellationToken);

var signedData = CreateSignedDataGenerator (signer);
var memory = new MemoryBlockStream ();

Expand All @@ -339,6 +347,9 @@ Stream Sign (CmsSigner signer, Stream content, bool encapsulate, CancellationTok

async Task<Stream> SignAsync (CmsSigner signer, Stream content, bool encapsulate, CancellationToken cancellationToken)
{
if (CheckCertificateRevocation)
await ValidateCertificateChainAsync (signer.CertificateChain, DateTime.UtcNow, cancellationToken);

var signedData = CreateSignedDataGenerator (signer);
var memory = new MemoryBlockStream ();

Expand Down Expand Up @@ -694,20 +705,31 @@ X509Certificate GetCertificate (IStore<X509Certificate> store, SignerID signer)
/// <returns>The certificate chain, including the specified certificate.</returns>
protected IList<X509Certificate> BuildCertificateChain (X509Certificate certificate)
{
var selector = new X509CertStoreSelector {
Certificate = certificate
};
var selector = new X509CertStoreSelector ();

var intermediates = new X509CertificateStore ();
intermediates.Add (certificate);
var userCertificateStore = new X509CertificateStore ();
userCertificateStore.Add (certificate);

var parameters = new PkixBuilderParameters (GetTrustedAnchors (), selector) {
var issuerStore = GetTrustedAnchors ();
var anchorStore = new X509CertificateStore ();

foreach (var anchor in issuerStore) {
anchorStore.Add (anchor.TrustedCert);
}

var parameters = new PkixBuilderParameters (issuerStore, selector) {
ValidityModel = PkixParameters.PkixValidityModel,
IsRevocationEnabled = false,
Date = DateTime.UtcNow
};
parameters.AddStoreCert (intermediates);
parameters.AddStoreCert (GetIntermediateCertificates ());
parameters.AddStoreCert (userCertificateStore);

var intermediateStore = GetIntermediateCertificates ();

foreach (var intermediate in intermediateStore.EnumerateMatches (new X509CertStoreSelector ()))
anchorStore.Add (intermediate);

parameters.AddStoreCert (anchorStore);

var builder = new PkixCertPathBuilder ();
var result = builder.Build (parameters);
Expand All @@ -720,6 +742,158 @@ protected IList<X509Certificate> BuildCertificateChain (X509Certificate certific
return chain;
}

/// <summary>
/// Validate an S/MIME certificate chain.
/// </summary>
/// <remarks>
/// <para>Validates an S/MIME certificate chain.</para>
/// <para>Downloads the CRLs for each certificate in the chain and then validates that the chain
/// is both valid and that none of the certificates in the chain have been revoked or compromised
/// in any way.</para>
/// </remarks>
/// <returns><c>true</c> if the certificate chain is valid; otherwise, <c>false</c>.</returns>
/// <param name="chain">The S/MIME certificate chain.</param>
/// <param name="dateTime">The date and time to use for validation.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <exception cref="ArgumentNullException">
/// <paramref name="chain"/> is <see langword="null"/>.
/// </exception>
/// <exception cref="ArgumentException">
/// <paramref name="chain"/> is empty or contains a <see langword="null"/> certificate.
/// </exception>
public bool ValidateCertificateChain (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default)
{
if (chain == null)
throw new ArgumentNullException (nameof (chain));

if (chain.Count == 0)
throw new ArgumentException ("The certificate chain must contain at least one certificate.", nameof (chain));

if (chain.Any (certificate => certificate == null))
throw new ArgumentException ("The certificate chain contains at least one null certificate.", nameof (chain));

var selector = new X509CertStoreSelector ();

var userCertificateStore = new X509CertificateStore ();
userCertificateStore.AddRange (chain);

var issuerStore = GetTrustedAnchors ();
var anchorStore = new X509CertificateStore ();

foreach (var anchor in issuerStore) {
anchorStore.Add (anchor.TrustedCert);
}

var parameters = new PkixBuilderParameters (issuerStore, selector) {
ValidityModel = PkixParameters.PkixValidityModel,
IsRevocationEnabled = false,
Date = DateTime.UtcNow
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of BuildCertificateChain wasn't to validate the chain, it was only to build it.

IMHO, if you really think we need to validate the chain when signing a message, then we should probably do it in the Sign/EncapsulatedSign methods, this way we can sync-vs-async download the CRLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should validate during the signing process. This option is essential for my workflows. One of the key advantages of SMIME is the ability to revoke a bad actor at scale. While this might not be as evident in scenarios where certificates are exchanged manually, it makes a lot of sense in my environment where the public certificate is discoverable via DNS.

I fully expected you to push back on the missing async paths.

I just got to a place where you could see the theme of the PR and tests to support it. I am used to writing the chain validation with System.Security.Cryptography. And there is just a lot going on in here to get familiar with. The thing I like about the BouncyCastle technique is I can control the caching. where with the MS stack on Windows it puts discovered intermediates in the machine store and crls in the file system and caches in memory. You can control the machine store and file system but you have to restart the service to clear the in memory cache. Maybe there is a PInvoke method that could punt the cache but I am not sure.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this might not be as evident in scenarios where certificates are exchanged manually, it makes a lot of sense in my environment where the public certificate is discoverable via DNS.

Are you signing with a certificate & private key discovered via DNS?

I guess that's unexpected for me.

When I think of signing a message, I think: Okay, the user is about to sign with his/her own private key that they 100% control and so they would know if it is invalid (except, I suppose, if it was invalid due to expiration perhaps). But I imagined they would know if they had revoked it, at least.

When encrypting to recipient certificates, it makes more sense to me to validate the certificate chains because those recipients could have revoked certificates. And obviously it also makes sense to me to validate a chain when verifying a signed message that you received.

};
parameters.AddStoreCert (userCertificateStore);

if (CheckCertificateRevocation) {
foreach (var certificate in chain)
DownloadCrls (certificate, cancellationToken);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we do not want in this method. If the user is calling an XyzAsync method, we want to make sure that all I/O goes through async channels.


var intermediateStore = GetIntermediateCertificates ();

foreach (var intermediate in intermediateStore.EnumerateMatches (new X509CertStoreSelector ())) {
anchorStore.Add (intermediate);
if (CheckCertificateRevocation)
DownloadCrls (intermediate, cancellationToken);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.


parameters.AddStoreCert (anchorStore);

if (CheckCertificateRevocation)
parameters.AddStoreCrl (GetCertificateRevocationLists ());

try {
var builder = new PkixCertPathBuilder ();
builder.Build (parameters);
return true;
} catch {
return false;
}
}

/// <summary>
/// Validate an S/MIME certificate chain.
/// </summary>
/// <remarks>
/// <para>Asynchronously validates an S/MIME certificate chain.</para>
/// <para>Downloads the CRLs for each certificate in the chain and then validates that the chain
/// is both valid and that none of the certificates in the chain have been revoked or compromised
/// in any way.</para>
/// </remarks>
/// <returns><c>true</c> if the certificate chain is valid; otherwise, <c>false</c>.</returns>
/// <param name="chain">The S/MIME certificate chain.</param>
/// <param name="dateTime">The date and time to use for validation.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <exception cref="ArgumentNullException">
/// <paramref name="chain"/> is <see langword="null"/>.
/// </exception>
/// <exception cref="ArgumentException">
/// <paramref name="chain"/> is empty or contains a <see langword="null"/> certificate.
/// </exception>
public async Task<bool> ValidateCertificateChainAsync (X509CertificateChain chain, DateTime dateTime, CancellationToken cancellationToken = default)
{
if (chain == null)
throw new ArgumentNullException (nameof (chain));

if (chain.Count == 0)
throw new ArgumentException ("The certificate chain must contain at least one certificate.", nameof (chain));

if (chain.Any (certificate => certificate == null))
throw new ArgumentException ("The certificate chain contains at least one null certificate.", nameof (chain));

var selector = new X509CertStoreSelector ();

var userCertificateStore = new X509CertificateStore ();
userCertificateStore.AddRange (chain);

var issuerStore = GetTrustedAnchors ();
var anchorStore = new X509CertificateStore ();

foreach (var anchor in issuerStore) {
anchorStore.Add (anchor.TrustedCert);
}

var parameters = new PkixBuilderParameters (issuerStore, selector) {
ValidityModel = PkixParameters.PkixValidityModel,
IsRevocationEnabled = false,
Date = DateTime.UtcNow
};
parameters.AddStoreCert (userCertificateStore);

if (CheckCertificateRevocation) {
foreach (var certificate in chain)
await DownloadCrlsAsync (certificate, cancellationToken).ConfigureAwait (false);
}

var intermediateStore = GetIntermediateCertificates ();

foreach (var intermediate in intermediateStore.EnumerateMatches (new X509CertStoreSelector ())) {
anchorStore.Add (intermediate);
if (CheckCertificateRevocation)
await DownloadCrlsAsync (intermediate, cancellationToken).ConfigureAwait (false);
}

parameters.AddStoreCert (anchorStore);

if (CheckCertificateRevocation)
parameters.AddStoreCrl (GetCertificateRevocationLists ());

try {
var builder = new PkixCertPathBuilder ();
builder.Build (parameters);
return true;
} catch {
return false;
}
}

PkixCertPath BuildCertPath (ISet<TrustAnchor> anchors, IStore<X509Certificate> certificates, IStore<X509Crl> crls, X509Certificate certificate, DateTime signingTime)
{
var selector = new X509CertStoreSelector {
Expand Down Expand Up @@ -995,7 +1169,7 @@ static IEnumerable<string> EnumerateCrlDistributionPointUrls (X509Certificate ce
}
}

void DownloadCrls (X509Certificate certificate, CancellationToken cancellationToken)
void DownloadCrls (X509Certificate certificate, CancellationToken cancellationToken = default)
{
var nextUpdate = GetNextCertificateRevocationListUpdate (certificate.IssuerDN);
var now = DateTime.UtcNow;
Expand Down Expand Up @@ -1125,10 +1299,15 @@ DigitalSignatureCollection GetDigitalSignatures (CmsSignedDataParser parser, Can
}

var anchors = GetTrustedAnchors ();
var intermediates = GetIntermediateCertificates ();

if (CheckCertificateRevocation) {
foreach (var anchor in anchors)
DownloadCrls (anchor.TrustedCert, cancellationToken);

foreach (X509Certificate intermediate in intermediates.EnumerateMatches(new X509CertStoreSelector())) {
DownloadCrls (intermediate, cancellationToken);
}
jstedfast marked this conversation as resolved.
Show resolved Hide resolved
}

try {
Expand Down Expand Up @@ -1179,10 +1358,15 @@ async Task<DigitalSignatureCollection> GetDigitalSignaturesAsync (CmsSignedDataP
}

var anchors = GetTrustedAnchors ();
var intermediates = GetIntermediateCertificates ();

if (CheckCertificateRevocation) {
foreach (var anchor in anchors)
await DownloadCrlsAsync (anchor.TrustedCert, cancellationToken).ConfigureAwait (false);

foreach (X509Certificate intermediate in intermediates.EnumerateMatches (new X509CertStoreSelector ())) {
await DownloadCrlsAsync (intermediate, cancellationToken);
}
}

try {
Expand Down
21 changes: 10 additions & 11 deletions MimeKit/Cryptography/DefaultSecureMimeContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,20 +432,19 @@ protected override ISet<TrustAnchor> GetTrustedAnchors ()
/// <returns>The intermediate certificates.</returns>
protected override IStore<X509Certificate> GetIntermediateCertificates ()
{
//var intermediates = new X509CertificateStore ();
//var selector = new X509CertStoreSelector ();
//var keyUsage = new bool[9];
var intermediates = new X509CertificateStore ();
var selector = new X509CertStoreSelector ();
var keyUsage = new bool[9];

//keyUsage[(int) X509KeyUsageBits.KeyCertSign] = true;
//selector.KeyUsage = keyUsage;
keyUsage[(int) X509KeyUsageBits.KeyCertSign] = true;
selector.KeyUsage = keyUsage;

//foreach (var record in dbase.Find (selector, false, X509CertificateRecordFields.Certificate)) {
// if (!record.Certificate.IsSelfSigned ())
// intermediates.Add (record.Certificate);
//}
foreach (var record in dbase.Find (selector, false, X509CertificateRecordFields.Certificate)) {
if (!record.Certificate.IsSelfSigned ())
intermediates.Add (record.Certificate);
}

//return intermediates;
return dbase;
return intermediates;
}
jstedfast marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
Expand Down
13 changes: 13 additions & 0 deletions UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,19 @@
using MimeKit.Cryptography;

using BCX509Certificate = Org.BouncyCastle.X509.X509Certificate;
using Org.BouncyCastle.Cms;
using Org.BouncyCastle.Crypto;
using Org.BouncyCastle.OpenSsl;

using Org.BouncyCastle.Crypto;

Check warning on line 42 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Debug) 

The using directive for 'Org.BouncyCastle.Crypto' appeared previously in this namespace

Check warning on line 42 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Release) 

The using directive for 'Org.BouncyCastle.Crypto' appeared previously in this namespace

Check warning on line 42 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Release) 

The using directive for 'Org.BouncyCastle.Crypto' appeared previously in this namespace

Check warning on line 42 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The using directive for 'Org.BouncyCastle.Crypto' appeared previously in this namespace

Check warning on line 42 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Debug) 

The using directive for 'Org.BouncyCastle.Crypto' appeared previously in this namespace
using Org.BouncyCastle.OpenSsl;

Check warning on line 43 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Debug) 

The using directive for 'Org.BouncyCastle.OpenSsl' appeared previously in this namespace

Check warning on line 43 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Release) 

The using directive for 'Org.BouncyCastle.OpenSsl' appeared previously in this namespace

Check warning on line 43 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Release) 

The using directive for 'Org.BouncyCastle.OpenSsl' appeared previously in this namespace

Check warning on line 43 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The using directive for 'Org.BouncyCastle.OpenSsl' appeared previously in this namespace

Check warning on line 43 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Debug) 

The using directive for 'Org.BouncyCastle.OpenSsl' appeared previously in this namespace
using Org.BouncyCastle.Security;

Check warning on line 44 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Debug) 

The using directive for 'Org.BouncyCastle.Security' appeared previously in this namespace

Check warning on line 44 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Release) 

The using directive for 'Org.BouncyCastle.Security' appeared previously in this namespace

Check warning on line 44 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Release) 

The using directive for 'Org.BouncyCastle.Security' appeared previously in this namespace

Check warning on line 44 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The using directive for 'Org.BouncyCastle.Security' appeared previously in this namespace

Check warning on line 44 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Debug) 

The using directive for 'Org.BouncyCastle.Security' appeared previously in this namespace
using Org.BouncyCastle.X509;

Check warning on line 45 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Debug) 

The using directive for 'Org.BouncyCastle.X509' appeared previously in this namespace

Check warning on line 45 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Release) 

The using directive for 'Org.BouncyCastle.X509' appeared previously in this namespace

Check warning on line 45 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Release) 

The using directive for 'Org.BouncyCastle.X509' appeared previously in this namespace

Check warning on line 45 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The using directive for 'Org.BouncyCastle.X509' appeared previously in this namespace

Check warning on line 45 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Debug) 

The using directive for 'Org.BouncyCastle.X509' appeared previously in this namespace
using Org.BouncyCastle.Pkcs;
using Org.BouncyCastle.Cms;

Check warning on line 47 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Debug) 

The using directive for 'Org.BouncyCastle.Cms' appeared previously in this namespace

Check warning on line 47 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (ubuntu-latest, Release) 

The using directive for 'Org.BouncyCastle.Cms' appeared previously in this namespace

Check warning on line 47 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Release) 

The using directive for 'Org.BouncyCastle.Cms' appeared previously in this namespace

Check warning on line 47 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp) 

The using directive for 'Org.BouncyCastle.Cms' appeared previously in this namespace

Check warning on line 47 in UnitTests/Cryptography/ApplicationPkcs7MimeTests.cs

View workflow job for this annotation

GitHub Actions / ci (windows-latest, Debug) 

The using directive for 'Org.BouncyCastle.Cms' appeared previously in this namespace
using Org.BouncyCastle.Crypto.Encodings;
using Org.BouncyCastle.Crypto.Engines;
using Org.BouncyCastle.Crypto.Parameters;

namespace UnitTests.Cryptography {
[TestFixture]
Expand Down
Loading
Loading