fix: Detection and graceful handling of corrupt packets

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -10,6 +10,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Added
 
+- Added detection and graceful handling of corrupt packets for additional safety. (#2419)
+
 ### Changed
 
 - The UTP component UI has been updated to be more user-friendly for new users by adding a simple toggle to switch between local-only (127.0.0.1) and remote (0.0.0.0) binding modes, using the toggle "Allow Remote Connections" (#2408)

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -143,15 +143,55 @@ public bool OnVerifyCanSend(ulong destinationId, Type messageType, NetworkDelive
 
             public bool OnVerifyCanReceive(ulong senderId, Type messageType, FastBufferReader messageContent, ref NetworkContext context)
             {
-                if (m_NetworkManager.PendingClients.TryGetValue(senderId, out PendingClient client) &&
-                    (client.ConnectionState == PendingClient.State.PendingApproval || (client.ConnectionState == PendingClient.State.PendingConnection && messageType != typeof(ConnectionRequestMessage))))
+                if (m_NetworkManager.IsServer)
                 {
-                    if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                    if (messageType == typeof(ConnectionApprovedMessage))
                     {
-                        NetworkLog.LogWarning($"Message received from {nameof(senderId)}={senderId} before it has been accepted");
+                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                        {
+                            NetworkLog.LogError($"A {nameof(ConnectionApprovedMessage)} was received from a client on the server side. This should not happen. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Message Size: {messageContent.Length}. Message Content: {MessagingSystem.ByteArrayToString(messageContent.ToArray(), 0, messageContent.Length)}");
+                        }
+                        return false;
                     }
+                    if (m_NetworkManager.PendingClients.TryGetValue(senderId, out PendingClient client) &&
+                                     (client.ConnectionState == PendingClient.State.PendingApproval || (client.ConnectionState == PendingClient.State.PendingConnection && messageType != typeof(ConnectionRequestMessage))))
+                    {
+                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                        {
+                            NetworkLog.LogWarning($"Message received from {nameof(senderId)}={senderId} before it has been accepted");
+                        }
 
-                    return false;
+                        return false;
+                    }
+
+                    if (m_NetworkManager.ConnectedClients.TryGetValue(senderId, out NetworkClient connectedClient) && messageType == typeof(ConnectionRequestMessage))
+                    {
+                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                        {
+                            NetworkLog.LogError($"A {nameof(ConnectionRequestMessage)} was received from a client when the connection has already been established. This should not happen. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Message Size: {messageContent.Length}. Message Content: {MessagingSystem.ByteArrayToString(messageContent.ToArray(), 0, messageContent.Length)}");
+                        }
+
+                        return false;
+                    }
+                }
+                else
+                {
+                    if (messageType == typeof(ConnectionRequestMessage))
+                    {
+                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                        {
+                            NetworkLog.LogError($"A {nameof(ConnectionRequestMessage)} was received from the server on the client side. This should not happen. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Message Size: {messageContent.Length}. Message Content: {MessagingSystem.ByteArrayToString(messageContent.ToArray(), 0, messageContent.Length)}");
+                        }
+                        return false;
+                    }
+                    if (m_NetworkManager.IsConnectedClient && messageType == typeof(ConnectionApprovedMessage))
+                    {
+                        if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                        {
+                            NetworkLog.LogError($"A {nameof(ConnectionApprovedMessage)} was received from the server when the connection has already been established. This should not happen. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Message Size: {messageContent.Length}. Message Content: {MessagingSystem.ByteArrayToString(messageContent.ToArray(), 0, messageContent.Length)}");
+                        }
+                        return false;
+                    }
                 }
 
                 return !m_NetworkManager.m_StopProcessingMessages;

com.unity.netcode.gameobjects/Runtime/Messaging/BatchHeader.cs

@@ -5,9 +5,26 @@ namespace Unity.Netcode
     /// </summary>
     internal struct BatchHeader : INetworkSerializeByMemcpy
     {
+        internal const ushort MagicValue = 0x1160;
+        /// <summary>
+        /// A magic number to detect corrupt messages.
+        /// Always set to k_MagicValue
+        /// </summary>
+        public ushort Magic;
+
+        /// <summary>
+        /// Total number of bytes in the batch.
+        /// </summary>
+        public int BatchSize;
+
+        /// <summary>
+        /// Hash of the message to detect corrupt messages.
+        /// </summary>
+        public ulong BatchHash;
+
         /// <summary>
         /// Total number of messages in the batch.
         /// </summary>
-        public ushort BatchSize;
+        public ushort BatchCount;
     }
 }

com.unity.netcode.gameobjects/Runtime/Messaging/MessagingSystem.cs

@@ -2,6 +2,7 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Runtime.CompilerServices;
+using System.Text;
 using Unity.Collections;
 using Unity.Collections.LowLevel.Unsafe;
 using UnityEngine;
@@ -41,7 +42,7 @@ public SendQueueItem(NetworkDelivery delivery, int writerSize, Allocator writerA
             {
                 Writer = new FastBufferWriter(writerSize, writerAllocator, maxWriterSize);
                 NetworkDelivery = delivery;
-                BatchHeader = default;
+                BatchHeader = new BatchHeader { Magic = BatchHeader.MagicValue };
             }
         }
 
@@ -204,6 +205,17 @@ public int GetLocalVersion(Type messageType)
             return m_LocalVersions[messageType];
         }
 
+        internal static string ByteArrayToString(byte[] ba, int offset, int count)
+        {
+            var hex = new StringBuilder(ba.Length * 2);
+            for (int i = offset; i < offset + count; ++i)
+            {
+                hex.AppendFormat("{0:x2} ", ba[i]);
+            }
+
+            return hex.ToString();
+        }
+
         internal void HandleIncomingData(ulong clientId, ArraySegment<byte> data, float receiveTime)
         {
             unsafe
@@ -214,18 +226,38 @@ internal void HandleIncomingData(ulong clientId, ArraySegment<byte> data, float
                         new FastBufferReader(nativeData + data.Offset, Allocator.None, data.Count);
                     if (!batchReader.TryBeginRead(sizeof(BatchHeader)))
                     {
-                        NetworkLog.LogWarning("Received a packet too small to contain a BatchHeader. Ignoring it.");
+                        NetworkLog.LogError("Received a packet too small to contain a BatchHeader. Ignoring it.");
                         return;
                     }
 
                     batchReader.ReadValue(out BatchHeader batchHeader);
 
+                    if (batchHeader.Magic != BatchHeader.MagicValue)
+                    {
+                        NetworkLog.LogError($"Received a packet with an invalid Magic Value. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Offset: {data.Offset}, Size: {data.Count}, Full receive array: {ByteArrayToString(data.Array, 0, data.Array.Length)}");
+                        return;
+                    }
+
+                    if (batchHeader.BatchSize != data.Count)
+                    {
+                        NetworkLog.LogError($"Received a packet with an invalid Batch Size Value. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Offset: {data.Offset}, Size: {data.Count}, Expected Size: {batchHeader.BatchSize}, Full receive array: {ByteArrayToString(data.Array, 0, data.Array.Length)}");
+                        return;
+                    }
+
+                    var hash = XXHash.Hash64(batchReader.GetUnsafePtrAtCurrentPosition(), batchReader.Length - batchReader.Position);
+
+                    if (hash != batchHeader.BatchHash)
+                    {
+                        NetworkLog.LogError($"Received a packet with an invalid Hash Value. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Received Hash: {batchHeader.BatchHash}, Calculated Hash: {hash}, Offset: {data.Offset}, Size: {data.Count}, Full receive array: {ByteArrayToString(data.Array, 0, data.Array.Length)}");
+                        return;
+                    }
+
                     for (var hookIdx = 0; hookIdx < m_Hooks.Count; ++hookIdx)
                     {
-                        m_Hooks[hookIdx].OnBeforeReceiveBatch(clientId, batchHeader.BatchSize, batchReader.Length);
+                        m_Hooks[hookIdx].OnBeforeReceiveBatch(clientId, batchHeader.BatchCount, batchReader.Length);
                     }
 
-                    for (var messageIdx = 0; messageIdx < batchHeader.BatchSize; ++messageIdx)
+                    for (var messageIdx = 0; messageIdx < batchHeader.BatchCount; ++messageIdx)
                     {
 
                         var messageHeader = new MessageHeader();
@@ -237,15 +269,15 @@ internal void HandleIncomingData(ulong clientId, ArraySegment<byte> data, float
                         }
                         catch (OverflowException)
                         {
-                            NetworkLog.LogWarning("Received a batch that didn't have enough data for all of its batches, ending early!");
+                            NetworkLog.LogError("Received a batch that didn't have enough data for all of its batches, ending early!");
                             throw;
                         }
 
                         var receivedHeaderSize = batchReader.Position - position;
 
                         if (!batchReader.TryBeginRead((int)messageHeader.MessageSize))
                         {
-                            NetworkLog.LogWarning("Received a message that claimed a size larger than the packet, ending early!");
+                            NetworkLog.LogError("Received a message that claimed a size larger than the packet, ending early!");
                             return;
                         }
                         m_IncomingMessageQueue.Add(new ReceiveQueueItem
@@ -263,7 +295,7 @@ internal void HandleIncomingData(ulong clientId, ArraySegment<byte> data, float
                     }
                     for (var hookIdx = 0; hookIdx < m_Hooks.Count; ++hookIdx)
                     {
-                        m_Hooks[hookIdx].OnAfterReceiveBatch(clientId, batchHeader.BatchSize, batchReader.Length);
+                        m_Hooks[hookIdx].OnAfterReceiveBatch(clientId, batchHeader.BatchCount, batchReader.Length);
                     }
                 }
             }
@@ -650,7 +682,7 @@ internal unsafe int SendPreSerializedMessage<TMessageType>(in FastBufferWriter t
 
                 writeQueueItem.Writer.WriteBytes(headerSerializer.GetUnsafePtr(), headerSerializer.Length);
                 writeQueueItem.Writer.WriteBytes(tmpSerializer.GetUnsafePtr(), tmpSerializer.Length);
-                writeQueueItem.BatchHeader.BatchSize++;
+                writeQueueItem.BatchHeader.BatchCount++;
                 for (var hookIdx = 0; hookIdx < m_Hooks.Count; ++hookIdx)
                 {
                     m_Hooks[hookIdx].OnAfterSendMessage(clientId, ref message, delivery, tmpSerializer.Length + headerSerializer.Length);
@@ -745,31 +777,36 @@ internal unsafe void ProcessSendQueues()
                 for (var i = 0; i < sendQueueItem.Length; ++i)
                 {
                     ref var queueItem = ref sendQueueItem.ElementAt(i);
-                    if (queueItem.BatchHeader.BatchSize == 0)
+                    if (queueItem.BatchHeader.BatchCount == 0)
                     {
                         queueItem.Writer.Dispose();
                         continue;
                     }
 
                     for (var hookIdx = 0; hookIdx < m_Hooks.Count; ++hookIdx)
                     {
-                        m_Hooks[hookIdx].OnBeforeSendBatch(clientId, queueItem.BatchHeader.BatchSize, queueItem.Writer.Length, queueItem.NetworkDelivery);
+                        m_Hooks[hookIdx].OnBeforeSendBatch(clientId, queueItem.BatchHeader.BatchCount, queueItem.Writer.Length, queueItem.NetworkDelivery);
                     }
 
                     queueItem.Writer.Seek(0);
 #if UNITY_EDITOR || DEVELOPMENT_BUILD
                     // Skipping the Verify and sneaking the write mark in because we know it's fine.
-                    queueItem.Writer.Handle->AllowedWriteMark = 2;
+                    queueItem.Writer.Handle->AllowedWriteMark = sizeof(BatchHeader);
 #endif
+                    queueItem.BatchHeader.BatchHash = XXHash.Hash64(queueItem.Writer.GetUnsafePtr() + sizeof(BatchHeader), queueItem.Writer.Length - sizeof(BatchHeader));
+
+                    queueItem.BatchHeader.BatchSize = queueItem.Writer.Length;
+
                     queueItem.Writer.WriteValue(queueItem.BatchHeader);
 
+
                     try
                     {
                         m_MessageSender.Send(clientId, queueItem.NetworkDelivery, queueItem.Writer);
 
                         for (var hookIdx = 0; hookIdx < m_Hooks.Count; ++hookIdx)
                         {
-                            m_Hooks[hookIdx].OnAfterSendBatch(clientId, queueItem.BatchHeader.BatchSize, queueItem.Writer.Length, queueItem.NetworkDelivery);
+                            m_Hooks[hookIdx].OnAfterSendBatch(clientId, queueItem.BatchHeader.BatchCount, queueItem.Writer.Length, queueItem.NetworkDelivery);
                         }
                     }
                     finally