Skip to content

Commit

Permalink
Improve auto accept in cert validation handler, add tests (#1386)
Browse files Browse the repository at this point in the history 
- if a cert validator is enabled, use it and ignore auto accept flag.
- trace auto accept message, if used
- make all config flags public, otherwise they can only be configured using a SecurityConfiguration. Once a config flag is set using a property, it is protected from modification by a SecurityConfiguration. This way apps can consistently override.
- add tests for the SHA1 and AutoAccept configuration flags
- add a method to reset the validated list of certificates. The list of certs should be revalidated from time to time to catch expired or revoked certificates. Applications can call `ResetValidatedCertificates` to clear the list with a daily timer.
  • Loading branch information
@mregen
mregen committed Apr 30, 2021
1 parent ea1e2b3 commit 7e736d2
Show file tree
Hide file tree
Showing 5 changed files with 273 additions and 70 deletions.
2 changes: 2 additions & 0 deletions .editorconfig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,5 @@ dotnet_naming_style.pascal_case_style.capitalization =
# CA3075: Insecure DTD processing in XML
dotnet_diagnostic.CA3075.severity = warning

# RCS1090: Add call to 'ConfigureAwait' (or vice versa)
dotnet_diagnostic.RCS1090.severity = warning
200 changes: 164 additions & 36 deletions Stack/Opc.Ua.Core/Security/Certificates/CertificateValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class CertificateValidator : ICertificateValidator
public CertificateValidator()
{
m_validatedCertificates = new Dictionary<string, X509Certificate2>();
m_protectFlags = 0;
m_autoAcceptUntrustedCertificates = false;
m_rejectSHA1SignedCertificates = CertificateFactory.DefaultHashSize >= 256;
m_rejectUnknownRevocationStatus = false;
Expand Down Expand Up @@ -109,7 +110,7 @@ public virtual async Task Update(ApplicationConfiguration configuration)
{
lock (m_lock)
{
m_validatedCertificates.Clear();
ResetValidatedCertificates();

m_trustedCertificateStore = null;
m_trustedCertificateList = null;
Expand All @@ -129,7 +130,6 @@ public virtual async Task Update(ApplicationConfiguration configuration)
}
}


m_issuerCertificateStore = null;
m_issuerCertificateList = null;

Expand Down Expand Up @@ -173,10 +173,23 @@ public virtual async Task Update(SecurityConfiguration configuration)
configuration.TrustedIssuerCertificates,
configuration.TrustedPeerCertificates,
configuration.RejectedCertificateStore);
m_autoAcceptUntrustedCertificates = configuration.AutoAcceptUntrustedCertificates;
m_rejectSHA1SignedCertificates = configuration.RejectSHA1SignedCertificates;
m_rejectUnknownRevocationStatus = configuration.RejectUnknownRevocationStatus;
m_minimumCertificateKeySize = configuration.MinimumCertificateKeySize;
// protect the flags if application called to set property
if ((m_protectFlags & ProtectFlags.AutoAcceptUntrustedCertificates) == 0)
{
m_autoAcceptUntrustedCertificates = configuration.AutoAcceptUntrustedCertificates;
}
if ((m_protectFlags & ProtectFlags.RejectSHA1SignedCertificates) == 0)
{
m_rejectSHA1SignedCertificates = configuration.RejectSHA1SignedCertificates;
}
if ((m_protectFlags & ProtectFlags.RejectUnknownRevocationStatus) == 0)
{
m_rejectUnknownRevocationStatus = configuration.RejectUnknownRevocationStatus;
}
if ((m_protectFlags & ProtectFlags.MinimumCertificateKeySize) == 0)
{
m_minimumCertificateKeySize = configuration.MinimumCertificateKeySize;
}
}

if (configuration.ApplicationCertificate != null)
Expand Down Expand Up @@ -209,6 +222,101 @@ public virtual async Task UpdateCertificate(SecurityConfiguration securityConfig
}
}

/// <summary>
/// Reset the list of validated certificates.
/// </summary>
public void ResetValidatedCertificates()
{
lock (m_lock)
{
// dispose outdated list
foreach (var cert in m_validatedCertificates.Values)
{
Utils.SilentDispose(cert);
}
m_validatedCertificates.Clear();
}
}

/// <summary>
/// If untrusted certificates should be accepted.
/// </summary>
public bool AutoAcceptUntrustedCertificates
{
get { return m_autoAcceptUntrustedCertificates; }
set
{
lock (m_lock)
{
m_protectFlags |= ProtectFlags.AutoAcceptUntrustedCertificates;
if (m_autoAcceptUntrustedCertificates != value)
{
m_autoAcceptUntrustedCertificates = value;
ResetValidatedCertificates();
}
}
}
}

/// <summary>
/// If certificates using a SHA1 signature should be trusted.
/// </summary>
public bool RejectSHA1SignedCertificates
{
get { return m_rejectSHA1SignedCertificates; }
set
{
lock (m_lock)
{
m_protectFlags |= ProtectFlags.RejectSHA1SignedCertificates;
if (m_rejectSHA1SignedCertificates != value)
{
m_rejectSHA1SignedCertificates = value;
ResetValidatedCertificates();
}
}
}
}

/// <summary>
/// if certificates with unknown revocation status should be rejected.
/// </summary>
public bool RejectUnknownRevocationStatus
{
get { return m_rejectUnknownRevocationStatus; }
set
{
lock (m_lock)
{
m_protectFlags |= ProtectFlags.RejectUnknownRevocationStatus;
if (m_rejectUnknownRevocationStatus != value)
{
m_rejectUnknownRevocationStatus = value;
ResetValidatedCertificates();
}
}
}
}

/// <summary>
/// The minimum size of a certificate key to be trusted.
/// </summary>
public ushort MinimumCertificateKeySize
{
get { return m_minimumCertificateKeySize; }
set
{
lock (m_lock)
{
m_protectFlags |= ProtectFlags.MinimumCertificateKeySize;
if (m_minimumCertificateKeySize != value)
{
m_minimumCertificateKeySize = value;
ResetValidatedCertificates();
}
}
}
}

/// <summary>
/// Validates the specified certificate against the trust list.
Expand Down Expand Up @@ -284,9 +392,9 @@ public virtual void Validate(X509Certificate2Collection chain, ConfiguredEndpoin
ServiceResult serviceResult = se.Result;
lock (m_callbackLock)
{
if (m_CertificateValidation != null)
do
{
do
if (m_CertificateValidation != null)
{
CertificateValidationEventArgs args = new CertificateValidationEventArgs(serviceResult, certificate);
m_CertificateValidation(this, args);
Expand All @@ -297,25 +405,32 @@ public virtual void Validate(X509Certificate2Collection chain, ConfiguredEndpoin
break;
}
accept = args.Accept;
if (accept)
{
serviceResult = serviceResult.InnerResult;
}
else
{
// report the rejected service result
se = new ServiceResultException(serviceResult);
}
} while (accept && serviceResult != null);
}
}
else if (m_autoAcceptUntrustedCertificates &&
serviceResult.StatusCode == StatusCodes.BadCertificateUntrusted)
{
accept = true;
Utils.Trace(Utils.TraceMasks.Security, "Automatically accepted certificate: {0}", certificate.Subject);
}

if (accept)
{
serviceResult = serviceResult.InnerResult;
}
else
{
// report the rejected service result
se = new ServiceResultException(serviceResult);
}
} while (accept && serviceResult != null);
}

// throw if rejected.
if (!accept)
{
// write the invalid certificate to rejected store if specified.
Utils.Trace(Utils.TraceMasks.Error, "Certificate '{0}' rejected. Reason={1}",
certificate.Subject, serviceResult != null ? serviceResult.ToString() : "Unknown Error" );
certificate.Subject, serviceResult != null ? serviceResult.ToString() : "Unknown Error");
SaveCertificate(certificate);

throw new ServiceResultException(se, StatusCodes.BadCertificateInvalid);
Expand Down Expand Up @@ -783,25 +898,22 @@ protected virtual async Task InternalValidate(X509Certificate2Collection certifi
}
}

if (!m_autoAcceptUntrustedCertificates)
// check if certificate issuer is trusted.
if (issuedByCA && !isIssuerTrusted && trustedCertificate == null)
{
// check if certificate issuer is trusted.
if (issuedByCA && !isIssuerTrusted && trustedCertificate == null)
{
var message = CertificateMessage("Certificate Issuer is not trusted.", certificate);
sresult = new ServiceResult(StatusCodes.BadCertificateUntrusted,
null, null, message, null, sresult);
}
var message = CertificateMessage("Certificate Issuer is not trusted.", certificate);
sresult = new ServiceResult(StatusCodes.BadCertificateUntrusted,
null, null, message, null, sresult);
}

// check if certificate is trusted.
if (trustedCertificate == null && !isIssuerTrusted)
// check if certificate is trusted.
if (trustedCertificate == null && !isIssuerTrusted)
{
if (m_applicationCertificate == null || !Utils.IsEqual(m_applicationCertificate.RawData, certificate.RawData))
{
if (m_applicationCertificate == null || !Utils.IsEqual(m_applicationCertificate.RawData, certificate.RawData))
{
var message = CertificateMessage("Certificate is not trusted.", certificate);
sresult = new ServiceResult(StatusCodes.BadCertificateUntrusted,
null, null, message, null, sresult);
}
var message = CertificateMessage("Certificate is not trusted.", certificate);
sresult = new ServiceResult(StatusCodes.BadCertificateUntrusted,
null, null, message, null, sresult);
}
}

Expand Down Expand Up @@ -1139,6 +1251,21 @@ private bool FindDomain(X509Certificate2 serverCertificate, ConfiguredEndpoint e
}
#endregion

#region Private Enum
/// <summary>
/// Flag to protect setting by application
/// from a modification by a SecurityConfiguration.
/// </summary>
[Flags]
private enum ProtectFlags
{
AutoAcceptUntrustedCertificates = 1,
RejectSHA1SignedCertificates = 2,
RejectUnknownRevocationStatus = 4,
MinimumCertificateKeySize = 8
};
#endregion

#region Private Fields
private object m_lock = new object();
private object m_callbackLock = new object();
Expand All @@ -1151,6 +1278,7 @@ private bool FindDomain(X509Certificate2 serverCertificate, ConfiguredEndpoint e
private event CertificateValidationEventHandler m_CertificateValidation;
private event CertificateUpdateEventHandler m_CertificateUpdate;
private X509Certificate2 m_applicationCertificate;
private ProtectFlags m_protectFlags;
private bool m_autoAcceptUntrustedCertificates;
private bool m_rejectSHA1SignedCertificates;
private bool m_rejectUnknownRevocationStatus;
Expand Down
36 changes: 18 additions & 18 deletions Stack/Opc.Ua.Core/Types/Utils/Utils.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,6 @@ private static void TraceWriteLine(string message, params object[] args)

writer.WriteLine(output);
writer.Flush();
writer.Dispose();
}
}
catch (Exception e)
Expand Down Expand Up @@ -924,10 +923,6 @@ public static string GetFilePathDisplayName(string filePath, int maxLength)
#endregion

#region String, Object and Data Convienence Functions
private const int MAX_MESSAGE_LENGTH = 1024;
private const uint FORMAT_MESSAGE_IGNORE_INSERTS = 0x00000200;
private const uint FORMAT_MESSAGE_FROM_SYSTEM = 0x00001000;

/// <summary>
/// Supresses any exceptions while disposing the object.
/// </summary>
Expand All @@ -937,24 +932,29 @@ public static string GetFilePathDisplayName(string filePath, int maxLength)
public static void SilentDispose(object objectToDispose)
{
IDisposable disposable = objectToDispose as IDisposable;
SilentDispose(disposable);
}

if (disposable != null)
/// <summary>
/// Supresses any exceptions while disposing the object.
/// </summary>
/// <remarks>
/// Writes errors to trace output in DEBUG builds.
/// </remarks>
public static void SilentDispose(IDisposable disposable)
{
try
{
try
{
disposable.Dispose();
}
disposable?.Dispose();
}
#if DEBUG
catch (Exception e)
{
Utils.Trace(e, "Error disposing object: {0}", disposable.GetType().Name);
}
catch (Exception e)
{
Utils.Trace(e, "Error disposing object: {0}", disposable.GetType().Name);
}
#else
catch (Exception)
{
}
catch (Exception) {;}
#endif
}
}

/// <summary>
Expand Down

0 comments on commit 7e736d2

Please sign in to comment.