diff --git a/ICSharpCode.BamlDecompiler/Baml/BamlReader.cs b/ICSharpCode.BamlDecompiler/Baml/BamlReader.cs index 51b2b67fa..4fa262d62 100644 --- a/ICSharpCode.BamlDecompiler/Baml/BamlReader.cs +++ b/ICSharpCode.BamlDecompiler/Baml/BamlReader.cs @@ -64,6 +64,11 @@ namespace ICSharpCode.BamlDecompiler.Baml { var rdr = new BinaryReader(str, Encoding.Unicode); uint len = rdr.ReadUInt32(); + // len is read straight from the file, before the MSBAML_SIG check below. The only + // accepted signature is the fixed-length "MSBAML", so reject any other length here + // rather than allocating an attacker-sized string from a crafted value. + if (len >> 1 != (uint)MSBAML_SIG.Length) + throw new InvalidDataException("Invalid BAML signature length."); var sig = new string(rdr.ReadChars((int)(len >> 1))); rdr.ReadBytes((int)(((len + 3) & ~3) - len)); return sig; @@ -263,7 +268,9 @@ namespace ICSharpCode.BamlDecompiler.Baml for (int i = 0; i < ret.Count; i++) { if (ret[i] is IBamlDeferRecord defer) - defer.ReadDefer(ret, i, _ => recs[_]); + defer.ReadDefer(ret, i, offset => recs.TryGetValue(offset, out var rec) + ? rec + : throw new InvalidDataException("BAML defer record points at an offset that is not a record boundary.")); } return ret; diff --git a/ICSharpCode.BamlDecompiler/Baml/BamlRecords.cs b/ICSharpCode.BamlDecompiler/Baml/BamlRecords.cs index 68cc5a272..2ce74a048 100644 --- a/ICSharpCode.BamlDecompiler/Baml/BamlRecords.cs +++ b/ICSharpCode.BamlDecompiler/Baml/BamlRecords.cs @@ -149,6 +149,79 @@ namespace ICSharpCode.BamlDecompiler.Baml void WriteDefer(BamlDocument doc, int index, BinaryWriter wtr); } + /// + /// Walks the key records that precede a defer target. The record offsets are part of + /// the BAML payload and therefore attacker-controlled, so every list access is bounded + /// and the nesting walk is depth-capped: a crafted resource fails with a catchable + /// instead of reading past the end of the record list + /// or recursing until the process dies with an uncatchable StackOverflowException. + /// + internal static class BamlDeferReader + { + // Legitimate BAML defer blocks nest only a handful of levels; this cap sits far + // below the CLR stack limit, so it never rejects real input but stops a crafted + // chain of nested start records before it overflows the stack. + const int MaxNestingDepth = 1000; + + static InvalidDataException Malformed(string detail) + => new InvalidDataException("Malformed BAML defer block: " + detail + "."); + + /// + /// Advances past the leading key records of a defer block and returns the index of + /// the record that terminates it. + /// + public static int SkipKeys(BamlDocument doc, int index) + { + bool keys = true; + do + { + if (index < 0 || index >= doc.Count) + throw Malformed("ran off the end of the record list while scanning keys"); + switch (doc[index].Type) + { + case BamlRecordType.DefAttributeKeyString: + case BamlRecordType.DefAttributeKeyType: + case BamlRecordType.OptimizedStaticResource: + keys = true; + break; + case BamlRecordType.StaticResourceStart: + NavigateTree(doc, BamlRecordType.StaticResourceStart, BamlRecordType.StaticResourceEnd, ref index, 0); + keys = true; + break; + case BamlRecordType.KeyElementStart: + NavigateTree(doc, BamlRecordType.KeyElementStart, BamlRecordType.KeyElementEnd, ref index, 0); + keys = true; + break; + default: + keys = false; + index--; + break; + } + index++; + } while (keys); + if (index < 0 || index >= doc.Count) + throw Malformed("ran off the end of the record list before the defer target"); + return index; + } + + static void NavigateTree(BamlDocument doc, BamlRecordType start, BamlRecordType end, ref int index, int depth) + { + if (depth >= MaxNestingDepth) + throw Malformed("nested start records exceed the maximum supported depth"); + index++; + while (true) + { + if (index >= doc.Count) + throw Malformed("a start record has no matching end record"); + if (doc[index].Type == start) + NavigateTree(doc, start, end, ref index, depth + 1); + else if (doc[index].Type == end) + return; + index++; + } + } + } + internal class XmlnsPropertyRecord : SizedBamlRecord { public override BamlRecordType Type => BamlRecordType.XmlnsProperty; @@ -336,61 +409,13 @@ namespace ICSharpCode.BamlDecompiler.Baml public void ReadDefer(BamlDocument doc, int index, Func resolve) { - bool keys = true; - do - { - switch (doc[index].Type) - { - case BamlRecordType.DefAttributeKeyString: - case BamlRecordType.DefAttributeKeyType: - case BamlRecordType.OptimizedStaticResource: - keys = true; - break; - case BamlRecordType.StaticResourceStart: - NavigateTree(doc, BamlRecordType.StaticResourceStart, BamlRecordType.StaticResourceEnd, ref index); - keys = true; - break; - case BamlRecordType.KeyElementStart: - NavigateTree(doc, BamlRecordType.KeyElementStart, BamlRecordType.KeyElementEnd, ref index); - keys = true; - break; - default: - keys = false; - index--; - break; - } - index++; - } while (keys); + index = BamlDeferReader.SkipKeys(doc, index); Record = resolve(doc[index].Position + pos); } public void WriteDefer(BamlDocument doc, int index, BinaryWriter wtr) { - bool keys = true; - do - { - switch (doc[index].Type) - { - case BamlRecordType.DefAttributeKeyString: - case BamlRecordType.DefAttributeKeyType: - case BamlRecordType.OptimizedStaticResource: - keys = true; - break; - case BamlRecordType.StaticResourceStart: - NavigateTree(doc, BamlRecordType.StaticResourceStart, BamlRecordType.StaticResourceEnd, ref index); - keys = true; - break; - case BamlRecordType.KeyElementStart: - NavigateTree(doc, BamlRecordType.KeyElementStart, BamlRecordType.KeyElementEnd, ref index); - keys = true; - break; - default: - keys = false; - index--; - break; - } - index++; - } while (keys); + index = BamlDeferReader.SkipKeys(doc, index); wtr.BaseStream.Seek(pos, SeekOrigin.Begin); wtr.Write((uint)(Record.Position - doc[index].Position)); } @@ -411,19 +436,6 @@ namespace ICSharpCode.BamlDecompiler.Baml writer.Write(Shared); writer.Write(SharedSet); } - - static void NavigateTree(BamlDocument doc, BamlRecordType start, BamlRecordType end, ref int index) - { - index++; - while (true) //Assume there alway is a end - { - if (doc[index].Type == start) - NavigateTree(doc, start, end, ref index); - else if (doc[index].Type == end) - return; - index++; - } - } } internal class TypeInfoRecord : SizedBamlRecord @@ -801,61 +813,13 @@ namespace ICSharpCode.BamlDecompiler.Baml public void ReadDefer(BamlDocument doc, int index, Func resolve) { - bool keys = true; - do - { - switch (doc[index].Type) - { - case BamlRecordType.DefAttributeKeyString: - case BamlRecordType.DefAttributeKeyType: - case BamlRecordType.OptimizedStaticResource: - keys = true; - break; - case BamlRecordType.StaticResourceStart: - NavigateTree(doc, BamlRecordType.StaticResourceStart, BamlRecordType.StaticResourceEnd, ref index); - keys = true; - break; - case BamlRecordType.KeyElementStart: - NavigateTree(doc, BamlRecordType.KeyElementStart, BamlRecordType.KeyElementEnd, ref index); - keys = true; - break; - default: - keys = false; - index--; - break; - } - index++; - } while (keys); + index = BamlDeferReader.SkipKeys(doc, index); Record = resolve(doc[index].Position + pos); } public void WriteDefer(BamlDocument doc, int index, BinaryWriter wtr) { - bool keys = true; - do - { - switch (doc[index].Type) - { - case BamlRecordType.DefAttributeKeyString: - case BamlRecordType.DefAttributeKeyType: - case BamlRecordType.OptimizedStaticResource: - keys = true; - break; - case BamlRecordType.StaticResourceStart: - NavigateTree(doc, BamlRecordType.StaticResourceStart, BamlRecordType.StaticResourceEnd, ref index); - keys = true; - break; - case BamlRecordType.KeyElementStart: - NavigateTree(doc, BamlRecordType.KeyElementStart, BamlRecordType.KeyElementEnd, ref index); - keys = true; - break; - default: - keys = false; - index--; - break; - } - index++; - } while (keys); + index = BamlDeferReader.SkipKeys(doc, index); wtr.BaseStream.Seek(pos, SeekOrigin.Begin); wtr.Write((uint)(Record.Position - doc[index].Position)); } @@ -876,19 +840,6 @@ namespace ICSharpCode.BamlDecompiler.Baml writer.Write(Shared); writer.Write(SharedSet); } - - static void NavigateTree(BamlDocument doc, BamlRecordType start, BamlRecordType end, ref int index) - { - index++; - while (true) - { - if (doc[index].Type == start) - NavigateTree(doc, start, end, ref index); - else if (doc[index].Type == end) - return; - index++; - } - } } internal class PropertyListStartRecord : PropertyComplexStartRecord diff --git a/ILSpy.BamlDecompiler.Tests/BamlSecurityTests.cs b/ILSpy.BamlDecompiler.Tests/BamlSecurityTests.cs new file mode 100644 index 000000000..cefdba6a1 --- /dev/null +++ b/ILSpy.BamlDecompiler.Tests/BamlSecurityTests.cs @@ -0,0 +1,140 @@ +// Copyright (c) 2026 AlphaSierraPapa for the SharpDevelop Team +// +// 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.IO; +using System.Text; + +using ICSharpCode.BamlDecompiler; +using ICSharpCode.Decompiler.Metadata; + +using NUnit.Framework; + +namespace ILSpy.BamlDecompiler.Tests +{ + /// + /// Hardening tests for the BAML reader: a crafted or corrupt resource must surface a + /// single catchable exception rather than crashing the process (uncontrolled recursion + /// terminating with an uncatchable StackOverflowException) or allocating gigabytes. + /// These drive the public entry point, + /// the same one resource browsing uses. + /// + [TestFixture] + public class BamlSecurityTests + { + // BAML record type ids (see BamlRecordType in the decompiler). + const byte DefAttributeKeyString = 0x26; + const byte StaticResourceStart = 0x30; + + static readonly string AssemblyPath = typeof(BamlSecurityTests).Assembly.Location; + + static XamlDecompiler CreateDecompiler(PEFile module) + { + var resolver = new UniversalAssemblyResolver(AssemblyPath, false, module.Metadata.DetectTargetFrameworkId()); + var typeSystem = new BamlDecompilerTypeSystem(module, resolver); + return new XamlDecompiler(typeSystem, new BamlDecompilerSettings()); + } + + /// + /// Writes the smallest header the reader accepts: the "MSBAML" signature block and + /// the three (reader/updater/writer) 0.0x60 version structs. + /// + static void WriteValidHeader(BinaryWriter w) + { + byte[] sig = Encoding.Unicode.GetBytes("MSBAML"); + w.Write((uint)sig.Length); // byte count; the reader takes len >> 1 characters + w.Write(sig); // 12 bytes, already 4-byte aligned so no padding follows + for (int i = 0; i < 3; i++) + { + w.Write((ushort)0x0000); // Major + w.Write((ushort)0x0060); // Minor + } + } + + static void WriteDefAttributeKeyString(BinaryWriter w) + { + w.Write(DefAttributeKeyString); + w.Write((byte)9); // SizedBamlRecord size prefix (1 byte size + 8 bytes data) + w.Write((ushort)0); // ValueId + w.Write((uint)0); // pos + w.Write(false); // Shared + w.Write(false); // SharedSet + } + + static void WriteStaticResourceStart(BinaryWriter w) + { + w.Write(StaticResourceStart); + w.Write((ushort)0); // TypeId + w.Write((byte)0); // Flags + } + + [Test] + public void DeeplyNestedDeferBlock_DoesNotStackOverflow() + { + // A DefAttributeKeyString defer block followed by tens of thousands of nested + // StaticResourceStart records makes the unfixed NavigateTree recurse without a + // depth cap, terminating the process with an uncatchable StackOverflowException. + var ms = new MemoryStream(); + using (var w = new BinaryWriter(ms, Encoding.Unicode, leaveOpen: true)) + { + WriteValidHeader(w); + WriteDefAttributeKeyString(w); + for (int i = 0; i < 20000; i++) + WriteStaticResourceStart(w); + } + ms.Position = 0; + + using var module = new PEFile(AssemblyPath); + Assert.Throws(() => CreateDecompiler(module).Decompile(ms)); + } + + [Test] + public void UnterminatedDeferBlock_ThrowsInvalidData() + { + // A single StaticResourceStart with no matching StaticResourceEnd walks the + // record index off the end of the list (ArgumentOutOfRangeException before the + // fix); the reader must report it as malformed data instead. + var ms = new MemoryStream(); + using (var w = new BinaryWriter(ms, Encoding.Unicode, leaveOpen: true)) + { + WriteValidHeader(w); + WriteDefAttributeKeyString(w); + WriteStaticResourceStart(w); + } + ms.Position = 0; + + using var module = new PEFile(AssemblyPath); + Assert.Throws(() => CreateDecompiler(module).Decompile(ms)); + } + + [Test] + public void OversizedSignatureLength_ThrowsInvalidData() + { + // The signature length is attacker-controlled and read before the MSBAML check; + // an enormous value must be rejected before it drives a multi-gigabyte allocation. + var ms = new MemoryStream(); + using (var w = new BinaryWriter(ms, Encoding.Unicode, leaveOpen: true)) + { + w.Write((uint)0x7FFFFFFF); + } + ms.Position = 0; + + using var module = new PEFile(AssemblyPath); + Assert.Throws(() => CreateDecompiler(module).Decompile(ms)); + } + } +} diff --git a/ILSpy.BamlDecompiler.Tests/ILSpy.BamlDecompiler.Tests.csproj b/ILSpy.BamlDecompiler.Tests/ILSpy.BamlDecompiler.Tests.csproj index 7b15c728a..16bdd1167 100644 --- a/ILSpy.BamlDecompiler.Tests/ILSpy.BamlDecompiler.Tests.csproj +++ b/ILSpy.BamlDecompiler.Tests/ILSpy.BamlDecompiler.Tests.csproj @@ -49,6 +49,7 @@ + AttachedEvent.xaml