fix: Detection and graceful handling of corrupt packets (#2419)

* fix: Detection and graceful handling of corrupt packets

Adds multiple ways to detect and handle corrupt packets that could otherwise cause issues and put the game into a bad state, or potentially cause crashes:

- A "magic" value at the start of each packet helps when looking at packet dumps to see where potential start-of-packet is if the data gets offset (as was seen in the recent BatchSendQueue corruption bug)
- A "size in bytes" value helps us detect if the packet has been truncated or if garbage has been added to the end
- A "hash" value helps to detect when the other values are correct but one or more bytes within the packet have been corrupted

Additionally, code has been added to refuse to process ConnectionRequestMessage or ConnectionAcceptedMessage on an alredy-established connection, or either of those being received by the wrong side, as those could otherwise be used as a vector for attack by a malicious actor and render the framework completely broken.

In all cases, if these issues occur, the log requests the user to file an issue with us with the full hex dump of the packet that was received.

Author: ShadauxCat

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