ChannelToken: Dispose HMAC and improve lifetime calculations. (#2846)

- Channels are not disposed (client and server)
- During long running tests an HMAC object leak was observed.
- The HMAC and SymmetricSign objects were not disposed after use, leading to a small incremental memory leak per key renewal. 
- The channel token lifetime is calculated by the system clock, which can change. Instead use the continous HiResClock.TickCount to calculate the lifetimes.
- Token renewal and connection requests are not removed from the request dictionary

Improvements:
- Make ChannelToken disposable. Dispose channels.
- Dispose HMAC and SymmetricSign objects after use.
- Use TickCount to calculate key renewal and key expiry.
- Introduce a 5% jitter to the token renewal timer, to avoid token renewal storms.
- Reduce the allocation per use of the HMAC objects by using ReadOnlySpan and avoid MemoryStream.

Author: mregen

.editorconfig

@@ -342,6 +342,9 @@ dotnet_diagnostic.CA1819.severity                                             =
 # CA1721: The property name is confusing given the existence of another method with the same name.
 dotnet_diagnostic.CA1721.severity                                             = silent
 
+# CA2014: Do not use stackalloc in loops
+dotnet_diagnostic.CA2014.severity                                             = error
+
 # exclude generated code
 [**/Generated/*.cs]
 generated_code = true

Libraries/Opc.Ua.Security.Certificates/Common/AsnUtils.cs

@@ -50,24 +50,35 @@ internal static string ToHexString(this byte[] buffer, bool invertEndian = false
                 return String.Empty;
             }
 
-            var builder = new StringBuilder(buffer.Length * 2);
-
-            if (invertEndian)
+#if NET6_0_OR_GREATER
+            if (!invertEndian)
             {
-                for (int ii = buffer.Length - 1; ii >= 0; ii--)
-                {
-                    builder.AppendFormat(CultureInfo.InvariantCulture, "{0:X2}", buffer[ii]);
-                }
+                return Convert.ToHexString(buffer);
             }
             else
+#endif
             {
-                for (int ii = 0; ii < buffer.Length; ii++)
+                StringBuilder builder = new StringBuilder(buffer.Length * 2);
+
+#if !NET6_0_OR_GREATER
+                if (!invertEndian)
+                {
+                    for (int ii = 0; ii < buffer.Length; ii++)
+                    {
+                        builder.AppendFormat(CultureInfo.InvariantCulture, "{0:X2}", buffer[ii]);
+                    }
+                }
+                else
+#endif
                 {
-                    builder.AppendFormat(CultureInfo.InvariantCulture, "{0:X2}", buffer[ii]);
+                    for (int ii = buffer.Length - 1; ii >= 0; ii--)
+                    {
+                        builder.AppendFormat(CultureInfo.InvariantCulture, "{0:X2}", buffer[ii]);
+                    }
                 }
-            }
 
-            return builder.ToString();
+                return builder.ToString();
+            }
         }
 
         /// <summary>
@@ -85,6 +96,9 @@ internal static byte[] FromHexString(this string buffer)
                 return Array.Empty<byte>();
             }
 
+#if NET6_0_OR_GREATER
+            return Convert.FromHexString(buffer);
+#else
             const string digits = "0123456789ABCDEF";
 
             byte[] bytes = new byte[(buffer.Length / 2) + (buffer.Length % 2)];
@@ -120,6 +134,7 @@ internal static byte[] FromHexString(this string buffer)
             }
 
             return bytes;
+#endif
         }
 
         /// <summary>

Stack/Opc.Ua.Core/Stack/Tcp/ChannelToken.cs

@@ -11,14 +11,15 @@ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 */
 
 using System;
+using System.Diagnostics;
 using System.Security.Cryptography;
 
 namespace Opc.Ua.Bindings
 {
     /// <summary>
     /// Represents a security token associate with a channel.
     /// </summary>
-    public class ChannelToken
+    public sealed class ChannelToken : IDisposable
     {
         #region Constructors
         /// <summary>
@@ -29,6 +30,49 @@ public ChannelToken()
         }
         #endregion
 
+        #region IDisposable
+        /// <summary>
+        /// The private version of the Dispose.
+        /// </summary>
+        private void Dispose(bool disposing)
+        {
+            if (!m_disposed)
+            {
+                if (disposing)
+                {
+                    Utils.SilentDispose(m_clientHmac);
+                    Utils.SilentDispose(m_serverHmac);
+                    Utils.SilentDispose(m_clientEncryptor);
+                    Utils.SilentDispose(m_serverEncryptor);
+                }
+                m_clientHmac = null;
+                m_serverHmac = null;
+                m_clientEncryptor = null;
+                m_serverEncryptor = null;
+                m_disposed = true;
+            }
+        }
+
+#if DEBUG
+        /// <summary>
+        /// The finalizer is used to catch issues with the dispose.
+        /// </summary>
+        ~ChannelToken()
+        {
+            Dispose(disposing: false);
+        }
+#endif
+
+        /// <summary>
+        /// Disposes the channel tokens.
+        /// </summary>
+        public void Dispose()
+        {
+            Dispose(disposing: true);
+            GC.SuppressFinalize(this);
+        }
+        #endregion
+
         #region Public Properties
         /// <summary>
         /// The id assigned to the channel that the token belongs to.
@@ -57,6 +101,16 @@ public DateTime CreatedAt
             set { m_createdAt = value; }
         }
 
+        /// <summary>
+        /// When the token was created (refers to the local tick count).
+        /// Used for calculation of renewals. Uses <see cref="Opc.Ua.HiResClock.TickCount"/>.
+        /// </summary>
+        public int CreatedAtTickCount
+        {
+            get { return m_createdAtTickCount; }
+            set { m_createdAtTickCount = value; }
+        }
+
         /// <summary>
         /// The lifetime of the token in milliseconds.
         /// </summary>
@@ -73,12 +127,7 @@ public bool Expired
         {
             get
             {
-                if (DateTime.UtcNow > m_createdAt.AddMilliseconds(m_lifetime))
-                {
-                    return true;
-                }
-
-                return false;
+                return (HiResClock.TickCount - m_createdAtTickCount) > m_lifetime;
             }
         }
 
@@ -89,12 +138,7 @@ public bool ActivationRequired
         {
             get
             {
-                if (DateTime.UtcNow > m_createdAt.AddMilliseconds(m_lifetime * TcpMessageLimits.TokenActivationPeriod))
-                {
-                    return true;
-                }
-
-                return false;
+                return (HiResClock.TickCount - m_createdAtTickCount) > (int)Math.Round(m_lifetime * TcpMessageLimits.TokenActivationPeriod);
             }
         }
 
@@ -213,12 +257,13 @@ public HMAC ServerHmac
             get { return m_serverHmac; }
             set { m_serverHmac = value; }
         }
-        #endregion       
+        #endregion
 
         #region Private Fields
         private uint m_channelId;
         private uint m_tokenId;
         private DateTime m_createdAt;
+        private int m_createdAtTickCount;
         private int m_lifetime;
         private byte[] m_clientNonce;
         private byte[] m_serverNonce;
@@ -232,6 +277,7 @@ public HMAC ServerHmac
         private HMAC m_serverHmac;
         private SymmetricAlgorithm m_clientEncryptor;
         private SymmetricAlgorithm m_serverEncryptor;
+        private bool m_disposed;
         #endregion
     }
 }

Stack/Opc.Ua.Core/Stack/Tcp/TcpMessageType.cs

@@ -298,6 +298,11 @@ public static class TcpMessageLimits
         /// </summary>
         public const double TokenRenewalPeriod = 0.75;
 
+        /// <summary>
+        /// The fraction of the lifetime to jitter renewing a token.
+        /// </summary>
+        public const double TokenRenewalJitterPeriod = 0.05;
+
         /// <summary>
         /// The fraction of the lifetime to wait before forcing the activation of the renewed token.
         /// </summary>

Stack/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs

@@ -67,7 +67,10 @@ public TcpServerChannel(
         /// </summary>
         protected override void Dispose(bool disposing)
         {
-            base.Dispose(disposing);
+            lock (DataLock)
+            {
+                base.Dispose(disposing);
+            }
         }
         #endregion
 
@@ -549,6 +552,7 @@ private bool ProcessOpenSecureChannelRequest(uint messageType, ArraySegment<byte
 
             BufferCollection chunksToProcess = null;
             OpenSecureChannelRequest request = null;
+            ChannelToken token = null;
             try
             {
                 bool firstCall = ClientCertificate == null;
@@ -589,13 +593,12 @@ private bool ProcessOpenSecureChannelRequest(uint messageType, ArraySegment<byte
                 }
 
                 // create a new token.
-                ChannelToken token = CreateToken();
-
+                token = CreateToken();
                 token.TokenId = GetNewTokenId();
                 token.ServerNonce = CreateNonce();
+
                 // check the client nonce.
                 token.ClientNonce = request.ClientNonce;
-
                 if (!ValidateNonce(token.ClientNonce))
                 {
                     throw ServiceResultException.Create(StatusCodes.BadNonceInvalid, "Client nonce is not the correct length or not random enough.");
@@ -637,6 +640,8 @@ private bool ProcessOpenSecureChannelRequest(uint messageType, ArraySegment<byte
                             token,
                             request);
 
+                        token = null;
+
                         Utils.LogInfo(
                             "{0} ReconnectToExistingChannel Socket={1:X8}, ChannelId={2}, TokenId={3}",
                             ChannelName,
@@ -688,10 +693,13 @@ private bool ProcessOpenSecureChannelRequest(uint messageType, ArraySegment<byte
                     ActivateToken(token);
                 }
 
+                // ensure the token is not disposed
+                token = null;
+
                 State = TcpChannelState.Open;
 
                 // send the response.
-                SendOpenSecureChannelResponse(requestId, token, request);
+                SendOpenSecureChannelResponse(requestId, CurrentToken, request);
 
                 // notify reverse 
                 CompleteReverseHello(null);
@@ -718,6 +726,7 @@ private bool ProcessOpenSecureChannelRequest(uint messageType, ArraySegment<byte
             }
             finally
             {
+                Utils.SilentDispose(token);
                 if (chunksToProcess != null)
                 {
                     chunksToProcess.Release(BufferManager, "ProcessOpenSecureChannelRequest");
@@ -1118,7 +1127,7 @@ protected override void DoMessageLimitsExceeded()
         private bool ValidateDiscoveryServiceCall(ChannelToken token, uint requestId, ArraySegment<byte> messageBody, out BufferCollection chunksToProcess)
         {
             chunksToProcess = null;
-            using (var decoder = new BinaryDecoder(messageBody.AsMemory().ToArray(), Quotas.MessageContext))
+            using (var decoder = new BinaryDecoder(messageBody, Quotas.MessageContext))
             {
                 // read the type of the message before more chunks are processed.
                 NodeId typeId = decoder.ReadNodeId(null);
@@ -1134,7 +1143,6 @@ private bool ValidateDiscoveryServiceCall(ChannelToken token, uint requestId, Ar
                 return true;
             }
         }
-
         #endregion
 
         #region Private Fields

Stack/Opc.Ua.Core/Stack/Tcp/TcpTransportListener.cs

@@ -241,8 +241,10 @@ public bool ReconnectToExistingChannel(
         /// </summary>
         public void ChannelClosed(uint channelId)
         {
-            if (m_channels?.TryRemove(channelId, out _) == true)
+            TcpListenerChannel channel = null;
+            if (m_channels?.TryRemove(channelId, out channel) == true)
             {
+                Utils.SilentDispose(channel);
                 Utils.LogInfo("ChannelId {0}: closed", channelId);
             }
             else
@@ -307,11 +309,17 @@ private void OnReverseHelloComplete(IAsyncResult result)
                     channel.SetReportCloseSecureChannelAuditCallback(new ReportAuditCloseSecureChannelEventHandler(OnReportAuditCloseSecureChannelEvent));
                     channel.SetReportCertificateAuditCallback(new ReportAuditCertificateEventHandler(OnReportAuditCertificateEvent));
                 }
+
+                channel = null;
             }
             catch (Exception e)
             {
                 ConnectionStatusChanged?.Invoke(this, new ConnectionStatusEventArgs(channel.ReverseConnectionUrl, new ServiceResult(e), true));
             }
+            finally
+            {
+                Utils.SilentDispose(channel);
+            }
         }
         #endregion
 
@@ -533,6 +541,7 @@ private void OnAccept(object sender, SocketAsyncEventArgs e)
                         // check if the accept socket has been created.
                         if (serveChannel && e.AcceptSocket != null && e.SocketError == SocketError.Success)
                         {
+                            channel = null;
                             try
                             {
                                 if (m_reverseConnectListener)
@@ -578,11 +587,17 @@ private void OnAccept(object sender, SocketAsyncEventArgs e)
 
                                 // start accepting messages on the channel.
                                 channel.Attach(channelId, e.AcceptSocket);
+
+                                channel = null;
                             }
                             catch (Exception ex)
                             {
                                 Utils.LogError(ex, "Unexpected error accepting a new connection.");
                             }
+                            finally
+                            {
+                                Utils.SilentDispose(channel);
+                            }
                         }
                     }