Bound .rsrc resource-tree parsing against crafted input

ReadWin32Resources walked the PE resource directory tree with raw native pointer
arithmetic over attacker-controlled offsets, counts and sizes, with no bounds
checks, no recursion-depth limit and no cycle detection. The root section pointer
came from GetSectionData, whose length was read and then discarded, leaving every
dereference unbounded.

A crafted assembly could therefore turn merely opening it (the Save as project
feature reads these resources unconditionally) into an uncatchable process kill or
an out-of-bounds native read: a subdirectory entry pointing back at itself recursed
until the stack overflowed; an inflated entry count walked off the section end; and
a data entry whose Size was up to 4 GB made Buffer.MemoryCopy read far past the
section, faulting on an unmapped page or copying adjacent process memory into the
byte[] later written to app.ico/app.manifest on disk. None of this is containable,
since a StackOverflowException cannot be caught and the repo has no corrupted-state
exception handling. This is the sibling of the bundle signature fix in a154a7bbb.

Carry the section length alongside the root pointer and bounds-check every offset,
entry count, name-string length and data Size against it, cap recursion depth and
track visited directory offsets to break cycles. A hostile or truncated file now
yields a bounded, partial tree instead of a crash; well-formed resources parse
exactly as before. The parser no longer needs the whole PEReader, only a delegate
that resolves a data RVA to a bounded pointer, which is the seam the new tests drive
over a pinned buffer.

Assisted-by: Claude:claude-opus-4-8:Claude Code

ICSharpCode.Decompiler.Tests/Util/Win32ResourcesTests.cs

@@ -0,0 +1,256 @@
+// Copyright (c) 2026 Siegfried Pammer
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy of this
+// software and associated documentation files (the "Software"), to deal in the Software
+// without restriction, including without limitation the rights to use, copy, modify, merge,
+// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
+// to whom the Software is furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all copies or
+// substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
+// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
+// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
+// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+// DEALINGS IN THE SOFTWARE.
+
+using System;
+using System.Runtime.InteropServices;
+
+using ICSharpCode.Decompiler.Util;
+
+using NUnit.Framework;
+
+namespace ICSharpCode.Decompiler.Tests.Util
+{
+	// Exercises the bounds, recursion-depth and cycle guards in Win32Resources against crafted
+	// .rsrc section bytes. Each test hands a hand-built directory tree to the parser through a
+	// resolver that maps an RVA to an offset inside the same pinned buffer (a single-section PE).
+	[TestFixture]
+	public unsafe class Win32ResourcesTests
+	{
+		const int DirectorySize = 16;   // IMAGE_RESOURCE_DIRECTORY
+		const int EntrySize = 8;        // IMAGE_RESOURCE_DIRECTORY_ENTRY
+		const int DataEntrySize = 16;   // IMAGE_RESOURCE_DATA_ENTRY
+		const uint SubdirectoryFlag = 0x80000000;
+
+		// Pins the buffer, parses it as a resource section, and runs the assertions while the data
+		// pointers captured during parsing still point into the pinned buffer.
+		static void Parse(byte[] buffer, Action<Win32ResourceDirectory> assert)
+		{
+			var handle = GCHandle.Alloc(buffer, GCHandleType.Pinned);
+			try
+			{
+				byte* pRoot = (byte*)handle.AddrOfPinnedObject();
+				var resolver = new BufferResolver(pRoot, buffer.Length);
+				var root = Win32ResourceDirectory.ReadDirectoryTree(pRoot, buffer.Length, resolver.Resolve);
+				assert(root);
+			}
+			finally
+			{
+				handle.Free();
+			}
+		}
+
+		// Resolves a data RVA to a pointer inside the buffer, returning the bytes that remain from
+		// that offset to the end - the same "length to end of section" contract PEReader.GetSectionData
+		// provides, so a crafted Size larger than the data can be bounded.
+		sealed class BufferResolver
+		{
+			readonly byte* pRoot;
+			readonly int length;
+
+			public BufferResolver(byte* pRoot, int length)
+			{
+				this.pRoot = pRoot;
+				this.length = length;
+			}
+
+			public byte* Resolve(int rva, out int dataLength)
+			{
+				if (rva < 0 || rva > length)
+				{
+					dataLength = 0;
+					return null;
+				}
+				dataLength = length - rva;
+				return pRoot + rva;
+			}
+		}
+
+		static void WriteDirectory(byte[] buffer, int offset, ushort namedEntries, ushort idEntries)
+		{
+			BitConverter.GetBytes(namedEntries).CopyTo(buffer, offset + 12);
+			BitConverter.GetBytes(idEntries).CopyTo(buffer, offset + 14);
+		}
+
+		static void WriteEntry(byte[] buffer, int offset, uint name, uint offsetToData)
+		{
+			BitConverter.GetBytes(name).CopyTo(buffer, offset);
+			BitConverter.GetBytes(offsetToData).CopyTo(buffer, offset + 4);
+		}
+
+		static void WriteDataEntry(byte[] buffer, int offset, uint rva, uint size)
+		{
+			BitConverter.GetBytes(rva).CopyTo(buffer, offset);
+			BitConverter.GetBytes(size).CopyTo(buffer, offset + 4);
+		}
+
+		[Test]
+		public void SelfReferentialSubdirectory_DoesNotRecurseInfinitely()
+		{
+			// One directory with a single subdirectory entry that points back at itself (offset 0).
+			// The unfixed parser follows it forever, yielding an uncatchable StackOverflowException.
+			byte[] buffer = new byte[DirectorySize + EntrySize];
+			WriteDirectory(buffer, 0, namedEntries: 0, idEntries: 1);
+			WriteEntry(buffer, DirectorySize, name: 1, offsetToData: SubdirectoryFlag /* offset 0 */);
+
+			Parse(buffer, root => {
+				Assert.That(root.Directories.Count, Is.EqualTo(1));
+				var child = root.Directories[0];
+				Assert.That(child.Directories.Count, Is.EqualTo(0), "the cycle back to the root must be cut");
+				Assert.That(child.Datas.Count, Is.EqualTo(0));
+			});
+		}
+
+		[Test]
+		public void DeeplyNestedDirectories_AreBoundedByDepthLimit()
+		{
+			// A long chain of distinct nested directories. Even without a cycle this would recurse
+			// as deep as the chain; the depth cap must stop it well before that.
+			const int chainLength = 40;
+			byte[] buffer = new byte[chainLength * (DirectorySize + EntrySize)];
+			for (int k = 0; k < chainLength; k++)
+			{
+				int dirOffset = k * (DirectorySize + EntrySize);
+				bool hasChild = k < chainLength - 1;
+				WriteDirectory(buffer, dirOffset, namedEntries: 0, idEntries: (ushort)(hasChild ? 1 : 0));
+				if (hasChild)
+				{
+					uint childOffset = (uint)((k + 1) * (DirectorySize + EntrySize));
+					WriteEntry(buffer, dirOffset + DirectorySize, name: (uint)(k + 1), offsetToData: SubdirectoryFlag | childOffset);
+				}
+			}
+
+			Parse(buffer, root => {
+				int depth = 0;
+				var current = root;
+				while (current != null && current.Directories.Count > 0)
+				{
+					current = current.Directories[0];
+					depth++;
+				}
+				// The parser caps nesting at a small constant (well above any real resource tree),
+				// so the measured depth must be far below the crafted chain length.
+				Assert.That(depth, Is.LessThanOrEqualTo(17));
+			});
+		}
+
+		[Test]
+		public void EntryCountBeyondSection_IsClamped()
+		{
+			// A directory header that claims far more entries than the section can hold. The unfixed
+			// parser walks the declared count straight off the end of the section.
+			byte[] buffer = new byte[DirectorySize];
+			WriteDirectory(buffer, 0, namedEntries: 0, idEntries: 0xFFFF);
+
+			Parse(buffer, root => {
+				Assert.That(root.Directories.Count, Is.EqualTo(0));
+				Assert.That(root.Datas.Count, Is.EqualTo(0));
+			});
+		}
+
+		[Test]
+		public void DataSizeBeyondSection_IsClampedToAvailable()
+		{
+			// A data leaf whose declared Size dwarfs the bytes actually present. The unfixed Data
+			// getter copies the full Size, reading gigabytes past the section base.
+			const int dataBytes = 8;
+			int dataEntryOffset = DirectorySize + EntrySize;
+			int dataOffset = dataEntryOffset + DataEntrySize;
+			byte[] buffer = new byte[dataOffset + dataBytes];
+			WriteDirectory(buffer, 0, namedEntries: 0, idEntries: 1);
+			WriteEntry(buffer, DirectorySize, name: 1, offsetToData: (uint)dataEntryOffset /* data leaf */);
+			WriteDataEntry(buffer, dataEntryOffset, rva: (uint)dataOffset, size: 0xFFFFFFF0);
+
+			Parse(buffer, root => {
+				Assert.That(root.Datas.Count, Is.EqualTo(1));
+				var data = root.Datas[0];
+				Assert.That(data.Size, Is.EqualTo(0xFFFFFFF0));
+				Assert.That(data.Data.Length, Is.EqualTo(dataBytes), "the copy must be bounded to the bytes that exist");
+			});
+		}
+
+		[Test]
+		public void NegativeDataRva_YieldsEmptyDataWithoutThrowing()
+		{
+			// The data entry's RVA is a file uint; with the high bit set it casts to a negative int.
+			// The resolver must reject it (PEReader.GetSectionData throws on a negative RVA) so the
+			// leaf yields empty data rather than aborting the parse.
+			int dataEntryOffset = DirectorySize + EntrySize;
+			byte[] buffer = new byte[dataEntryOffset + DataEntrySize];
+			WriteDirectory(buffer, 0, namedEntries: 0, idEntries: 1);
+			WriteEntry(buffer, DirectorySize, name: 1, offsetToData: (uint)dataEntryOffset /* data leaf */);
+			WriteDataEntry(buffer, dataEntryOffset, rva: 0xFFFFFFFF /* negative as int */, size: 0x100);
+
+			Parse(buffer, root => {
+				Assert.That(root.Datas.Count, Is.EqualTo(1));
+				Assert.That(root.Datas[0].Data, Is.Empty);
+			});
+		}
+
+		[Test]
+		public void OutOfRangeStringName_DoesNotReadOutOfBounds()
+		{
+			// A named entry whose name-string offset lies past the section end. The unfixed parser
+			// dereferences it directly, reading the length prefix and characters out of bounds.
+			int dataEntryOffset = DirectorySize + EntrySize;
+			byte[] buffer = new byte[dataEntryOffset + DataEntrySize];
+			WriteDirectory(buffer, 0, namedEntries: 1, idEntries: 0);
+			WriteEntry(buffer, DirectorySize, name: SubdirectoryFlag | 0x100 /* string offset past the buffer */, offsetToData: (uint)dataEntryOffset);
+			WriteDataEntry(buffer, dataEntryOffset, rva: 0, size: 0);
+
+			Parse(buffer, root => {
+				Assert.That(root.Datas.Count, Is.EqualTo(1));
+				var name = root.Datas[0].Name;
+				Assert.That(name.HasName, Is.True);
+				Assert.That(name.Name, Is.Empty, "an out-of-range string name must resolve to empty, not an OOB read");
+			});
+		}
+
+		[Test]
+		public void ValidResourceTree_ParsesAndReadsData()
+		{
+			// A well-formed Type -> Name -> (language data leaf) tree, mirroring how a manifest is
+			// laid out, to prove the bounds checks do not break normal parsing.
+			const int RT_MANIFEST = 24;
+			int typeDir = 0;
+			int rootEntry = typeDir + DirectorySize;             // 16
+			int nameDir = rootEntry + EntrySize;                 // 24
+			int typeEntry = nameDir + DirectorySize;             // 40
+			int leafDir = typeEntry + EntrySize;                 // 48
+			int nameEntry = leafDir + DirectorySize;             // 64
+			int dataEntry = nameEntry + EntrySize;               // 72
+			int dataOffset = dataEntry + DataEntrySize;          // 88
+			byte[] payload = { 0xDE, 0xAD, 0xBE, 0xEF };
+			byte[] buffer = new byte[dataOffset + payload.Length];
+
+			WriteDirectory(buffer, typeDir, namedEntries: 0, idEntries: 1);
+			WriteEntry(buffer, rootEntry, name: RT_MANIFEST, offsetToData: SubdirectoryFlag | (uint)nameDir);
+			WriteDirectory(buffer, nameDir, namedEntries: 0, idEntries: 1);
+			WriteEntry(buffer, typeEntry, name: 1, offsetToData: SubdirectoryFlag | (uint)leafDir);
+			WriteDirectory(buffer, leafDir, namedEntries: 0, idEntries: 1);
+			WriteEntry(buffer, nameEntry, name: 1033, offsetToData: (uint)dataEntry /* data leaf */);
+			WriteDataEntry(buffer, dataEntry, rva: (uint)dataOffset, size: (uint)payload.Length);
+			payload.CopyTo(buffer, dataOffset);
+
+			Parse(buffer, root => {
+				var manifest = root.Find(new Win32ResourceName(RT_MANIFEST))?.FirstDirectory()?.FirstData()?.Data;
+				Assert.That(manifest, Is.Not.Null);
+				Assert.That(manifest, Is.EqualTo(payload));
+			});
+		}
+	}
+}

ICSharpCode.Decompiler/Util/Win32Resources.cs

@@ -6,6 +6,13 @@ using System.Reflection.PortableExecutable;
 
 namespace ICSharpCode.Decompiler.Util
 {
+	/// <summary>
+	/// Resolves a resource data RVA to a pointer into the PE image and reports how many bytes remain
+	/// from that point to the end of the containing section, so a crafted Size can be bounded against
+	/// the data that actually exists.
+	/// </summary>
+	internal unsafe delegate byte* ResolveResourceData(int rva, out int length);
+
 	/// <summary>
 	/// Represents win32 resources
 	/// </summary>
@@ -24,10 +31,39 @@ namespace ICSharpCode.Decompiler.Util
 			}
 
 			int rva = pe.PEHeaders.PEHeader?.ResourceTableDirectory.RelativeVirtualAddress ?? 0;
-			if (rva == 0)
+			// A crafted header can set the directory RVA negative; GetSectionData throws on a negative
+			// RVA, so reject it here rather than let it abort parsing.
+			if (rva <= 0)
+				return null;
+			var block = pe.GetSectionData(rva);
+			if (block.Pointer == null || block.Length <= 0)
 				return null;
-			byte* pRoot = pe.GetSectionData(rva).Pointer;
-			return new Win32ResourceDirectory(pe, pRoot, 0, new Win32ResourceName("Root"));
+
+			byte* Resolve(int dataRva, out int length)
+			{
+				// OffsetToData is a file uint cast to int, so it can be negative; GetSectionData throws
+				// on a negative RVA, and a positive RVA outside any section yields an empty block.
+				if (dataRva < 0)
+				{
+					length = 0;
+					return null;
+				}
+				var dataBlock = pe.GetSectionData(dataRva);
+				length = dataBlock.Length;
+				return dataBlock.Pointer;
+			}
+
+			return Win32ResourceDirectory.ReadDirectoryTree(block.Pointer, block.Length, Resolve);
+		}
+
+		/// <summary>
+		/// Returns true when reading <paramref name="size"/> bytes at <paramref name="offset"/> stays
+		/// within a section of <paramref name="length"/> bytes. Subtraction avoids the overflow an
+		/// <c>offset + size</c> bound would have on attacker-controlled values.
+		/// </summary>
+		internal static bool InBounds(int offset, int size, int length)
+		{
+			return offset >= 0 && size >= 0 && offset <= length && size <= length - offset;
 		}
 
 		public static Win32ResourceDirectory? Find(this Win32ResourceDirectory root, Win32ResourceName type)
@@ -91,8 +127,33 @@ namespace ICSharpCode.Decompiler.Util
 
 		public IList<Win32ResourceData> Datas { get; }
 
-		internal unsafe Win32ResourceDirectory(PEReader pe, byte* pRoot, int offset, Win32ResourceName name)
+		// Windows resource trees are conventionally Type -> Name -> Language (three directory levels).
+		// This cap sits well above that, so it never rejects a real tree, while keeping a crafted deep
+		// chain from exhausting the stack - which the cycle check alone cannot, since a non-repeating
+		// chain of distinct offsets is acyclic yet still arbitrarily deep.
+		const int MaxDepth = 16;
+
+		internal static unsafe Win32ResourceDirectory ReadDirectoryTree(byte* pRoot, int length, ResolveResourceData resolveData)
 		{
+			return new Win32ResourceDirectory(resolveData, pRoot, length, 0, new Win32ResourceName("Root"), 0, new HashSet<int>());
+		}
+
+		unsafe Win32ResourceDirectory(ResolveResourceData resolveData, byte* pRoot, int length, int offset, Win32ResourceName name, int depth, HashSet<int> visited)
+		{
+			Name = name;
+			Directories = new List<Win32ResourceDirectory>();
+			Datas = new List<Win32ResourceData>();
+
+			// A crafted .rsrc tree can be cyclic or arbitrarily deep. The depth cap bounds nesting; the
+			// visited set, shared across the whole walk, parses each directory offset at most once,
+			// which breaks cycles and caps total work at the distinct offsets in the section. Real
+			// resource trees never reference one directory from two branches, so parsing a repeated
+			// offset only once is lossless for valid input.
+			if (depth > MaxDepth || !visited.Add(offset))
+				return;
+			if (!Win32Resources.InBounds(offset, sizeof(IMAGE_RESOURCE_DIRECTORY), length))
+				return;
+
 			var p = (IMAGE_RESOURCE_DIRECTORY*)(pRoot + offset);
 			Characteristics = p->Characteristics;
 			TimeDateStamp = p->TimeDateStamp;
@@ -101,28 +162,28 @@ namespace ICSharpCode.Decompiler.Util
 			NumberOfNamedEntries = p->NumberOfNamedEntries;
 			NumberOfIdEntries = p->NumberOfIdEntries;
 
-			Name = name;
-			Directories = new List<Win32ResourceDirectory>();
-			Datas = new List<Win32ResourceData>();
-			var pEntries = (IMAGE_RESOURCE_DIRECTORY_ENTRY*)(p + 1);
+			int entriesOffset = offset + sizeof(IMAGE_RESOURCE_DIRECTORY);
 			int total = NumberOfNamedEntries + NumberOfIdEntries;
+			// Clamp the file-declared entry count to the entries that actually fit before the section
+			// end, so the walk below cannot read past it.
+			int available = (length - entriesOffset) / sizeof(IMAGE_RESOURCE_DIRECTORY_ENTRY);
+			if (available < 0)
+				available = 0;
+			if (total > available)
+				total = available;
+
+			var pEntries = (IMAGE_RESOURCE_DIRECTORY_ENTRY*)(pRoot + entriesOffset);
 			for (int i = 0; i < total; i++)
 			{
 				var pEntry = pEntries + i;
-				name = new Win32ResourceName(pRoot, pEntry);
+				name = new Win32ResourceName(pRoot, length, pEntry);
 				if ((pEntry->OffsetToData & 0x80000000) == 0)
-					Datas.Add(new Win32ResourceData(pe, pRoot, (int)pEntry->OffsetToData, name));
+					Datas.Add(new Win32ResourceData(resolveData, pRoot, length, (int)pEntry->OffsetToData, name));
 				else
-					Directories.Add(new Win32ResourceDirectory(pe, pRoot, (int)(pEntry->OffsetToData & 0x7FFFFFFF), name));
+					Directories.Add(new Win32ResourceDirectory(resolveData, pRoot, length, (int)(pEntry->OffsetToData & 0x7FFFFFFF), name, depth + 1, visited));
 			}
 		}
 
-		static unsafe string ReadString(byte* pRoot, int offset)
-		{
-			var pString = (IMAGE_RESOURCE_DIRECTORY_STRING*)(pRoot + offset);
-			return new string(pString->NameString, 0, pString->Length);
-		}
-
 		public Win32ResourceDirectory? FindDirectory(Win32ResourceName name)
 		{
 			foreach (var directory in Directories)
@@ -164,29 +225,41 @@ namespace ICSharpCode.Decompiler.Util
 		public uint Reserved { get; }
 		#endregion
 
-		private readonly void* _pointer;
+		private readonly byte* _pointer;
+		private readonly int _dataLength;
 
 		public Win32ResourceName Name { get; }
 
 		public byte[] Data {
 			get {
-				byte[] data = new byte[Size];
+				// Size is file-controlled (up to 4 GB); bound both the allocation and the copy to the
+				// bytes the resolver reports between the data pointer and the end of its section. That
+				// length is never negative, so the clamped count fits in an int.
+				if (_pointer == null || _dataLength <= 0)
+					return Array.Empty<byte>();
+				int count = (int)Math.Min((uint)Size, (uint)_dataLength);
+				byte[] data = new byte[count];
 				fixed (void* pData = data)
-					Buffer.MemoryCopy(_pointer, pData, Size, Size);
+					Buffer.MemoryCopy(_pointer, pData, count, count);
 				return data;
 			}
 		}
 
-		internal Win32ResourceData(PEReader pe, byte* pRoot, int offset, Win32ResourceName name)
+		internal Win32ResourceData(ResolveResourceData resolveData, byte* pRoot, int length, int offset, Win32ResourceName name)
 		{
+			Name = name;
+			if (!Win32Resources.InBounds(offset, sizeof(IMAGE_RESOURCE_DATA_ENTRY), length))
+				return;
+
 			var p = (IMAGE_RESOURCE_DATA_ENTRY*)(pRoot + offset);
 			OffsetToData = p->OffsetToData;
 			Size = p->Size;
 			CodePage = p->CodePage;
 			Reserved = p->Reserved;
 
-			_pointer = pe.GetSectionData((int)OffsetToData).Pointer;
-			Name = name;
+			// OffsetToData is an RVA that can exceed int range; wrap deliberately (the library builds
+			// checked) so an out-of-range value reaches the resolver as a negative RVA it rejects.
+			_pointer = resolveData(unchecked((int)OffsetToData), out _dataLength);
 		}
 	}
 
@@ -216,14 +289,22 @@ namespace ICSharpCode.Decompiler.Util
 			_name = id;
 		}
 
-		internal unsafe Win32ResourceName(byte* pRoot, IMAGE_RESOURCE_DIRECTORY_ENTRY* pEntry)
+		internal unsafe Win32ResourceName(byte* pRoot, int length, IMAGE_RESOURCE_DIRECTORY_ENTRY* pEntry)
 		{
-			_name = (pEntry->Name & 0x80000000) == 0 ? (object)(ushort)pEntry->Name : ReadString(pRoot, (int)(pEntry->Name & 0x7FFFFFFF));
+			_name = (pEntry->Name & 0x80000000) == 0 ? (object)(ushort)pEntry->Name : ReadString(pRoot, length, (int)(pEntry->Name & 0x7FFFFFFF));
 
-			static string ReadString(byte* pRoot, int offset)
+			static string ReadString(byte* pRoot, int length, int offset)
 			{
+				// A ushort character count followed by that many UTF-16 chars. Reject a prefix that
+				// runs past the section and clamp the character count to the bytes that remain.
+				if (!Win32Resources.InBounds(offset, sizeof(ushort), length))
+					return string.Empty;
 				var pString = (IMAGE_RESOURCE_DIRECTORY_STRING*)(pRoot + offset);
-				return new string(pString->NameString, 0, pString->Length);
+				int charCount = pString->Length;
+				int available = (length - (offset + sizeof(ushort))) / sizeof(char);
+				if (charCount > available)
+					charCount = available;
+				return new string(pString->NameString, 0, charCount);
 			}
 		}