From 7f3b233087594aeabe17a44a568a7ede3b44fa2a Mon Sep 17 00:00:00 2001 From: RoseOfficial Date: Mon, 19 Jan 2026 17:30:02 -0500 Subject: [PATCH] feat(network): Add safe packet handling and pointer validation - Add NetworkPointerValidator for validating packet pointers with bounds checking and user-mode address space limits - Add SafePacket wrapper providing lifetime safety, bounds checking, and sensitive data clearing on dispose - Fix integer overflow vulnerabilities in bounds checking (use safe subtraction pattern instead of addition) - Fix pointer validation timing in GameNetwork to validate after adjustment - Add deprecation documentation to IGameNetwork interface - Add runtime check for scoped service access outside plugin context --- Dalamud/Game/Network/GameNetwork.cs | 79 ++++++-- .../Game/Network/NetworkPointerValidator.cs | 101 ++++++++++ Dalamud/Game/Network/SafePacket.cs | 189 ++++++++++++++++++ Dalamud/Plugin/Services/IGameNetwork.cs | 39 +++- Dalamud/Service/Service{T}.cs | 13 +- 5 files changed, 400 insertions(+), 21 deletions(-) create mode 100644 Dalamud/Game/Network/NetworkPointerValidator.cs create mode 100644 Dalamud/Game/Network/SafePacket.cs diff --git a/Dalamud/Game/Network/GameNetwork.cs b/Dalamud/Game/Network/GameNetwork.cs index b8c91b235..9610bae67 100644 --- a/Dalamud/Game/Network/GameNetwork.cs +++ b/Dalamud/Game/Network/GameNetwork.cs @@ -16,6 +16,26 @@ namespace Dalamud.Game.Network; [ServiceManager.EarlyLoadedService] internal sealed unsafe class GameNetwork : IInternalDisposableService { + /// + /// Offset from the data pointer to the start of the packet header. + /// + private const int PacketHeaderOffset = 0x10; + + /// + /// Offset from the packet header to where the opcode is located. + /// + private const int OpCodeOffset = 0x12; + + /// + /// Offset from the packet header to where the packet data begins. + /// + private const int PacketDataOffset = 0x20; + + /// + /// Size of the packet header for validation and logging purposes. + /// + private const int PacketHeaderSize = 0x20; + private readonly GameNetworkAddressResolver address; private readonly Hook processZonePacketDownHook; private readonly Hook processZonePacketUpHook; @@ -77,16 +97,33 @@ internal sealed unsafe class GameNetwork : IInternalDisposableService { this.hitchDetectorDown.Start(); - // Go back 0x10 to get back to the start of the packet header - dataPtr -= 0x10; + // Go back to the start of the packet header + dataPtr -= PacketHeaderOffset; + + // Validate the adjusted pointer (after moving to packet header start) + // We need PacketHeaderSize bytes for the full header + data offset + if (!NetworkPointerValidator.IsValidPacketPointer(dataPtr, PacketHeaderSize)) + { + Log.Warning("ProcessZonePacketDown received invalid pointer: {Ptr:X}", dataPtr + PacketHeaderOffset); + this.processZonePacketDownHook.Original(dispatcher, targetId, dataPtr + PacketHeaderOffset); + this.hitchDetectorDown.Stop(); + return; + } foreach (var d in Delegate.EnumerateInvocationList(this.NetworkMessage)) { try { + // Extract opcode with bounds awareness + if (!NetworkPointerValidator.TrySafeRead(dataPtr, OpCodeOffset, PacketHeaderSize, out var opCode)) + { + Log.Warning("Failed to read packet opcode at offset {Offset}", OpCodeOffset); + continue; + } + d.Invoke( - dataPtr + 0x20, - (ushort)Marshal.ReadInt16(dataPtr, 0x12), + dataPtr + PacketDataOffset, + opCode, 0, targetId, NetworkMessageDirection.ZoneDown); @@ -96,8 +133,8 @@ internal sealed unsafe class GameNetwork : IInternalDisposableService string header; try { - var data = new byte[32]; - Marshal.Copy(dataPtr, data, 0, 32); + var data = new byte[PacketHeaderSize]; + Marshal.Copy(dataPtr, data, 0, PacketHeaderSize); header = BitConverter.ToString(data); } catch (Exception) @@ -109,7 +146,7 @@ internal sealed unsafe class GameNetwork : IInternalDisposableService } } - this.processZonePacketDownHook.Original(dispatcher, targetId, dataPtr + 0x10); + this.processZonePacketDownHook.Original(dispatcher, targetId, dataPtr + PacketHeaderOffset); this.hitchDetectorDown.Stop(); } @@ -117,19 +154,37 @@ internal sealed unsafe class GameNetwork : IInternalDisposableService { this.hitchDetectorUp.Start(); + // Validate the incoming pointer before use + if (!NetworkPointerValidator.IsValidPacketPointer(dataPtr, PacketHeaderSize)) + { + Log.Warning("ProcessZonePacketUp received invalid pointer: {Ptr:X}", dataPtr); + this.hitchDetectorUp.Stop(); + return this.processZonePacketUpHook.Original(a1, dataPtr, a3, a4); + } + try { - // Call events - // TODO: Implement actor IDs - this.NetworkMessage?.Invoke(dataPtr + 0x20, (ushort)Marshal.ReadInt16(dataPtr), 0x0, 0x0, NetworkMessageDirection.ZoneUp); + // Extract opcode with bounds awareness + // Note: Upstream packets have a different structure - opcode is at offset 0, not OpCodeOffset + // This is intentional as upstream packet format differs from downstream + if (NetworkPointerValidator.TrySafeRead(dataPtr, 0, PacketHeaderSize, out var opCode)) + { + // Call events + // TODO: Implement actor IDs + this.NetworkMessage?.Invoke(dataPtr + PacketDataOffset, opCode, 0x0, 0x0, NetworkMessageDirection.ZoneUp); + } + else + { + Log.Warning("Failed to read packet opcode for upstream packet"); + } } catch (Exception ex) { string header; try { - var data = new byte[32]; - Marshal.Copy(dataPtr, data, 0, 32); + var data = new byte[PacketHeaderSize]; + Marshal.Copy(dataPtr, data, 0, PacketHeaderSize); header = BitConverter.ToString(data); } catch (Exception) diff --git a/Dalamud/Game/Network/NetworkPointerValidator.cs b/Dalamud/Game/Network/NetworkPointerValidator.cs new file mode 100644 index 000000000..3c9fa7b2f --- /dev/null +++ b/Dalamud/Game/Network/NetworkPointerValidator.cs @@ -0,0 +1,101 @@ +using System.Runtime.InteropServices; + +namespace Dalamud.Game.Network; + +/// +/// Provides validation utilities for network packet pointers. +/// +internal static class NetworkPointerValidator +{ + /// + /// Minimum address threshold below which pointers are considered invalid. + /// Addresses below this are typically reserved by the OS. + /// + private const long MinValidAddress = 0x10000; + + /// + /// Maximum valid user-mode address for 64-bit Windows. + /// Addresses above this are kernel-mode and inaccessible from user-mode. + /// + private const long MaxValidAddress = 0x7FFFFFFFFFFF; + + /// + /// Validates a network packet pointer before use. + /// + /// The pointer to validate. + /// The minimum expected size of the data. + /// True if the pointer appears valid; false otherwise. + public static bool IsValidPacketPointer(nint ptr, int minSize) + { + if (ptr == nint.Zero) + return false; + + // Ensure pointer is within reasonable memory range + if (ptr < MinValidAddress) + return false; + + // Ensure pointer is within user-mode address space + if (ptr > MaxValidAddress) + return false; + + // Minimum size must be positive + if (minSize <= 0) + return false; + + return true; + } + + /// + /// Safely reads a value from a packet pointer with bounds checking. + /// + /// The unmanaged type to read. + /// The base pointer to read from. + /// The byte offset from the base pointer. + /// The total size of the packet for bounds checking. + /// The value read from memory. + /// + /// Thrown when the read would exceed packet boundaries. + /// + /// + /// Thrown when the pointer is invalid. + /// + public static unsafe T SafeRead(nint ptr, int offset, int packetSize) where T : unmanaged + { + if (!IsValidPacketPointer(ptr, packetSize)) + throw new ArgumentException("Invalid packet pointer.", nameof(ptr)); + + var size = sizeof(T); + if (offset < 0 || offset > packetSize || size > packetSize - offset) + { + throw new ArgumentOutOfRangeException( + nameof(offset), + $"Cannot read {size} bytes at offset {offset} from packet of size {packetSize}."); + } + + return *(T*)(ptr + offset); + } + + /// + /// Attempts to safely read a value from a packet pointer with bounds checking. + /// + /// The unmanaged type to read. + /// The base pointer to read from. + /// The byte offset from the base pointer. + /// The total size of the packet for bounds checking. + /// The value read from memory, or default if the read failed. + /// True if the read succeeded; false otherwise. + public static unsafe bool TrySafeRead(nint ptr, int offset, int packetSize, out T value) where T : unmanaged + { + value = default; + + if (!IsValidPacketPointer(ptr, packetSize)) + return false; + + var size = sizeof(T); + if (offset < 0 || offset > packetSize || size > packetSize - offset) + return false; + + value = *(T*)(ptr + offset); + return true; + } +} diff --git a/Dalamud/Game/Network/SafePacket.cs b/Dalamud/Game/Network/SafePacket.cs new file mode 100644 index 000000000..edb5f7aac --- /dev/null +++ b/Dalamud/Game/Network/SafePacket.cs @@ -0,0 +1,189 @@ +using System.Runtime.InteropServices; + +namespace Dalamud.Game.Network; + +/// +/// A safe wrapper around network packet data with lifetime and bounds guarantees. +/// +/// +/// +/// This class copies packet data to managed memory, ensuring: +/// +/// +/// Lifetime safety - data persists as long as this object +/// Bounds checking - all reads are validated against packet size +/// Thread safety - the copied data cannot be modified externally +/// +/// +/// This class is intended to replace raw pointer access in future API versions. +/// +/// +internal sealed class SafePacket : IDisposable +{ + private readonly byte[] data; + private bool disposed; + + /// + /// Initializes a new instance of the class by copying data from an unmanaged pointer. + /// + /// The source pointer to copy from. + /// The number of bytes to copy. + /// Thrown when the pointer is invalid. + /// Thrown when size is not positive. + internal SafePacket(nint ptr, int size) + { + if (!NetworkPointerValidator.IsValidPacketPointer(ptr, size)) + throw new ArgumentException("Invalid packet pointer.", nameof(ptr)); + + if (size <= 0) + throw new ArgumentOutOfRangeException(nameof(size), "Size must be positive."); + + this.data = new byte[size]; + Marshal.Copy(ptr, this.data, 0, size); + this.Size = size; + } + + /// + /// Initializes a new instance of the class from existing data. + /// + /// The source data to copy. + /// Thrown when data is null. + /// Thrown when data is empty. + internal SafePacket(byte[] data) + { + ArgumentNullException.ThrowIfNull(data); + + if (data.Length == 0) + throw new ArgumentException("Data cannot be empty.", nameof(data)); + + this.data = new byte[data.Length]; + data.CopyTo(this.data, 0); + this.Size = data.Length; + } + + /// + /// Gets the total size of the packet data in bytes. + /// + public int Size { get; } + + /// + /// Gets the packet opcode (first two bytes interpreted as ushort). + /// + /// Thrown when the packet has been disposed. + /// Thrown when packet is too small to contain an opcode. + public ushort OpCode + { + get + { + ObjectDisposedException.ThrowIf(this.disposed, this); + + if (this.Size < sizeof(ushort)) + throw new InvalidOperationException("Packet too small to contain an opcode."); + + return BitConverter.ToUInt16(this.data, 0); + } + } + + /// + /// Safely reads a value of type T at the specified byte offset. + /// + /// The unmanaged type to read. + /// The byte offset from the start of the packet. + /// The value read from the packet data. + /// Thrown when the packet has been disposed. + /// Thrown when the read would exceed packet boundaries. + public unsafe T Read(int offset) where T : unmanaged + { + ObjectDisposedException.ThrowIf(this.disposed, this); + + var size = sizeof(T); + if (offset < 0 || offset > this.Size || size > this.Size - offset) + { + throw new ArgumentOutOfRangeException( + nameof(offset), + $"Cannot read {size} bytes at offset {offset} from packet of size {this.Size}."); + } + + return MemoryMarshal.Read(this.data.AsSpan(offset)); + } + + /// + /// Attempts to safely read a value of type T at the specified byte offset. + /// + /// The unmanaged type to read. + /// The byte offset from the start of the packet. + /// When this method returns, contains the value read, or default if the read failed. + /// True if the read succeeded; false otherwise. + public unsafe bool TryRead(int offset, out T value) where T : unmanaged + { + value = default; + + if (this.disposed) + return false; + + var size = sizeof(T); + if (offset < 0 || offset > this.Size || size > this.Size - offset) + return false; + + value = MemoryMarshal.Read(this.data.AsSpan(offset)); + return true; + } + + /// + /// Gets a read-only span of the entire packet data. + /// + /// A read-only span covering all packet data. + /// Thrown when the packet has been disposed. + public ReadOnlySpan AsSpan() + { + ObjectDisposedException.ThrowIf(this.disposed, this); + return this.data; + } + + /// + /// Gets a read-only span of a portion of the packet data. + /// + /// The starting offset. + /// The number of bytes to include. + /// A read-only span covering the specified portion of packet data. + /// Thrown when the packet has been disposed. + /// Thrown when the range exceeds packet boundaries. + public ReadOnlySpan AsSpan(int offset, int length) + { + ObjectDisposedException.ThrowIf(this.disposed, this); + + if (offset < 0 || length < 0 || offset > this.Size || length > this.Size - offset) + { + throw new ArgumentOutOfRangeException( + nameof(offset), + $"Range [{offset}, {offset + length}) exceeds packet size {this.Size}."); + } + + return this.data.AsSpan(offset, length); + } + + /// + /// Creates a copy of the packet data as a new byte array. + /// + /// A new byte array containing a copy of the packet data. + /// Thrown when the packet has been disposed. + public byte[] ToArray() + { + ObjectDisposedException.ThrowIf(this.disposed, this); + + var copy = new byte[this.Size]; + this.data.CopyTo(copy, 0); + return copy; + } + + /// + public void Dispose() + { + if (!this.disposed) + { + // Clear the data array to prevent sensitive network data from persisting in memory + Array.Clear(this.data); + this.disposed = true; + } + } +} diff --git a/Dalamud/Plugin/Services/IGameNetwork.cs b/Dalamud/Plugin/Services/IGameNetwork.cs index 4abf20834..cfcf7bc19 100644 --- a/Dalamud/Plugin/Services/IGameNetwork.cs +++ b/Dalamud/Plugin/Services/IGameNetwork.cs @@ -5,23 +5,50 @@ namespace Dalamud.Plugin.Services; /// /// This class handles interacting with game network events. /// -[Obsolete("Will be removed in a future release. Use packet handler hooks instead.", true)] +/// +/// +/// DEPRECATED: This interface passes raw unmanaged pointers which are unsafe for the following reasons: +/// +/// +/// No bounds checking on pointer arithmetic - can cause access violations +/// No lifetime management - pointer may be freed while plugin holds it +/// No size information - callers must guess packet boundaries +/// Async usage can cause use-after-free vulnerabilities +/// +/// +/// Migration Guide: +/// +/// +/// For market board data: Use the MarketBoard observable services +/// For duty finder: Use DutyFinder observable services +/// For custom packets: Create typed hooks using Hook<T> with proper packet structures +/// +/// +/// See the Dalamud developer documentation for detailed migration instructions. +/// +/// +[Obsolete("Will be removed in a future release. Use packet handler hooks instead. See XML documentation for migration guide.", true)] public interface IGameNetwork : IDalamudService { - // TODO(v9): we shouldn't be passing pointers to the actual data here - /// /// The delegate type of a network message event. /// - /// The pointer to the raw data. + /// + /// The pointer to the raw data. WARNING: This pointer has no lifetime guarantees + /// and must not be stored or used asynchronously. + /// /// The operation ID code. /// The source actor ID. - /// The taret actor ID. - /// The direction of the packed. + /// The target actor ID. + /// The direction of the packet. public delegate void OnNetworkMessageDelegate(nint dataPtr, ushort opCode, uint sourceActorId, uint targetActorId, NetworkMessageDirection direction); /// /// Event that is called when a network message is sent/received. /// + /// + /// WARNING: The dataPtr passed to handlers is only valid during the synchronous + /// execution of the handler. Do not store the pointer or use it in async contexts. + /// public event OnNetworkMessageDelegate NetworkMessage; } diff --git a/Dalamud/Service/Service{T}.cs b/Dalamud/Service/Service{T}.cs index c965eeb3d..66d2e754e 100644 --- a/Dalamud/Service/Service{T}.cs +++ b/Dalamud/Service/Service{T}.cs @@ -23,9 +23,6 @@ namespace Dalamud; [SuppressMessage("ReSharper", "StaticMemberInGenericType", Justification = "Service container static type")] internal static class Service where T : IServiceType { - // TODO: Service should only work with singleton services. Trying to call Service.Get() on a scoped service should - // be a compile-time error. - private static readonly ServiceManager.ServiceAttribute ServiceAttribute; private static TaskCompletionSource instanceTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); private static List? dependencyServices; @@ -39,6 +36,16 @@ internal static class Service where T : IServiceType ?? throw new InvalidOperationException( $"{nameof(T)} is missing {nameof(ServiceManager.ServiceAttribute)} annotations."); + // Prevent Service from being used with scoped services. + // Scoped services must be resolved through constructor injection or IServiceScope. + if (ServiceAttribute.Kind.HasFlag(ServiceManager.ServiceKind.ScopedService)) + { + throw new InvalidOperationException( + $"Service<{type.Name}> cannot be used with scoped services. " + + $"Scoped services must be resolved through constructor injection or IServiceScope. " + + $"See: https://dalamud.dev/api/services#scoped-services"); + } + var exposeToPlugins = type.GetCustomAttribute() != null; if (exposeToPlugins) ServiceManager.Log.Debug("Service<{0}>: Static ctor called; will be exposed to plugins", type.Name);