Improve crl handling in certificate stores (#2829)

* improve crl handling in certificate stores by not loading CRL with invalid or unsupported content. Hence the revocation check for such certificates may fail.
OPCFoundation · Nov 2, 2024 · ff3674f · ff3674f
1 parent 787844c
commit ff3674f
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 16 deletions.
diff --git a/Libraries/Opc.Ua.Security.Certificates/X509Crl/X509Crl.cs b/Libraries/Opc.Ua.Security.Certificates/X509Crl/X509Crl.cs
@@ -50,6 +50,7 @@ public class X509CRL : IX509CRL
         public X509CRL(string filePath) : this()
         {
             RawData = File.ReadAllBytes(filePath);
+            EnsureDecoded();
         }
 
         /// <summary>
@@ -58,6 +59,7 @@ public X509CRL(string filePath) : this()
         public X509CRL(byte[] crl) : this()
         {
             RawData = crl;
+            EnsureDecoded();
         }
 
         /// <summary>
@@ -78,6 +80,7 @@ public X509CRL(IX509CRL crl)
                 m_crlExtensions.Add(extension);
             }
             RawData = crl.RawData;
+            EnsureDecoded();
         }
 
         /// <summary>
@@ -99,7 +102,6 @@ public X500DistinguishedName IssuerName
         {
             get
             {
-                EnsureDecoded();
                 return m_issuerName;
             }
         }
@@ -112,7 +114,6 @@ public DateTime ThisUpdate
         {
             get
             {
-                EnsureDecoded();
                 return m_thisUpdate;
             }
         }
@@ -122,7 +123,6 @@ public DateTime NextUpdate
         {
             get
             {
-                EnsureDecoded();
                 return m_nextUpdate;
             }
         }
@@ -132,7 +132,6 @@ public HashAlgorithmName HashAlgorithmName
         {
             get
             {
-                EnsureDecoded();
                 return m_hashAlgorithmName;
             }
         }
@@ -142,7 +141,6 @@ public IList<RevokedCertificate> RevokedCertificates
         {
             get
             {
-                EnsureDecoded();
                 return m_revokedCertificates.AsReadOnly();
             }
         }
@@ -152,7 +150,6 @@ public X509ExtensionCollection CrlExtensions
         {
             get
             {
-                EnsureDecoded();
                 return m_crlExtensions;
             }
         }

diff --git a/Libraries/Opc.Ua.Server/Configuration/TrustList.cs b/Libraries/Opc.Ua.Server/Configuration/TrustList.cs
@@ -471,7 +471,7 @@ private ServiceResult AddCertificate(
                         result = StatusCodes.BadCertificateInvalid;
                     }
 
-                    var storeIdentifier = isTrustedCertificate? m_trustedStore : m_issuerStore;
+                    var storeIdentifier = isTrustedCertificate ? m_trustedStore : m_issuerStore;
                     ICertificateStore store = storeIdentifier.OpenStore();
                     try
                     {
@@ -539,7 +539,7 @@ private ServiceResult RemoveCertificate(
                                 foreach (var cert in certCollection)
                                 {
                                     if (X509Utils.CompareDistinguishedName(cert.SubjectName, crl.IssuerName) &&
-                                        crl.VerifySignature(cert, false))
+                                   crl.VerifySignature(cert, false))
                                     {
                                         crlsToDelete.Add(crl);
                                         break;

diff --git a/Stack/Opc.Ua.Core/Security/Certificates/DirectoryCertificateStore.cs b/Stack/Opc.Ua.Core/Security/Certificates/DirectoryCertificateStore.cs
@@ -20,7 +20,6 @@ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 using System.Threading.Tasks;
 using Opc.Ua.Security.Certificates;
 using Opc.Ua.Redaction;
-using System.Threading;
 
 namespace Opc.Ua
 {
@@ -620,7 +619,7 @@ public Task<StatusCode> IsRevoked(X509Certificate2 issuer, X509Certificate2 cert
                     }
                     catch (Exception e)
                     {
-                        Utils.LogError(e, "Could not parse CRL file.");
+                        Utils.LogError(e, "Failed to parse CRL {0} in store {1}.", file.FullName, StorePath);
                         continue;
                     }
 
@@ -670,8 +669,16 @@ public Task<X509CRLCollection> EnumerateCRLs()
             {
                 foreach (FileInfo file in m_crlSubdir.GetFiles("*" + kCrlExtension))
                 {
-                    var crl = new X509CRL(file.FullName);
-                    crls.Add(crl);
+                    try
+                    {
+                        var crl = new X509CRL(file.FullName);
+                        crls.Add(crl);
+                    }
+                    catch (Exception e)
+                    {
+                        Utils.LogError(e, "Failed to parse CRL {0} in store {1}.", file.FullName, StorePath);
+                    }
+
                 }
             }
 

diff --git a/Stack/Opc.Ua.Core/Security/Certificates/X509CertificateStore/X509CertificateStore.cs b/Stack/Opc.Ua.Core/Security/Certificates/X509CertificateStore/X509CertificateStore.cs
@@ -15,7 +15,6 @@ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 */
 
 using System;
-using System.IO;
 using System.Security.Cryptography.X509Certificates;
 using System.Text;
 using System.Threading.Tasks;
@@ -251,7 +250,6 @@ public async Task<StatusCode> IsRevoked(X509Certificate2 issuer, X509Certificate
 
             foreach (X509CRL crl in crls)
             {
-
                 if (!X509Utils.CompareDistinguishedName(crl.IssuerName, issuer.SubjectName))
                 {
                     continue;
@@ -299,8 +297,15 @@ public Task<X509CRLCollection> EnumerateCRLs()
                 byte[][] rawCrls = store.EnumerateCrls();
                 foreach (byte[] rawCrl in rawCrls)
                 {
-                    var crl = new X509CRL(rawCrl);
-                    crls.Add(crl);
+                    try
+                    {
+                        var crl = new X509CRL(rawCrl);
+                        crls.Add(crl);
+                    }
+                    catch (Exception e)
+                    {
+                        Utils.LogError(e, "Failed to parse CRL in store {0}.", store.Name);
+                    }
                 }
             }
             return Task.FromResult(crls);