Merge pull request #1102 from EdwardCooke/ec-security

EdwardCooke · web-flow · commit ba6dd7d1ff13 · 2026-04-28T03:10:50.000-06:00
Security improvements

+semver:feature
diff --git a/SECURITY.md b/SECURITY.md
@@ -0,0 +1,13 @@
+# Reporting
+
+Please submit reports of security vulnerabilities and, if possible, code to reproduce the vulnerability.
+
+That code will be the basis for the fix.
+
+Please send reports to <edward@frakkingsweet.com> and <antoine@aaubry.net> instead of directly opening an issue detailing the finding.
+
+We will reach out and let you know when it's appropriate to open that issue and post that information.
+
+Though, likely the reproduction will be in the unit test and details included in the PR around the vulnerability.
+
+Reports will promptly be investigated and responded to.
diff --git a/YamlDotNet.Core7AoTCompileTest/YamlDotNet.Core7AoTCompileTest.csproj b/YamlDotNet.Core7AoTCompileTest/YamlDotNet.Core7AoTCompileTest.csproj
@@ -2,7 +2,7 @@
 
   <PropertyGroup>
     <OutputType>Exe</OutputType>
-    <TargetFramework>net8.0</TargetFramework>
+    <TargetFramework>net10.0</TargetFramework>
     <PublishAot>true</PublishAot>
     <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles>
     <Nullable>enable</Nullable>
diff --git a/YamlDotNet.Fsharp.Test/YamlDotNet.Fsharp.Test.fsproj b/YamlDotNet.Fsharp.Test/YamlDotNet.Fsharp.Test.fsproj
@@ -1,6 +1,7 @@
 ﻿<Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
-    <TargetFrameworks>net8.0;net47</TargetFrameworks>
+    <TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('Windows'))">net10.0;net8.0;net4.7</TargetFrameworks>
+    <TargetFrameworks Condition="!$([MSBuild]::IsOSPlatform('Windows'))">net10.0;net8.0</TargetFrameworks>
     <IsPackable>false</IsPackable>
     <AssemblyOriginatorKeyFile>..\YamlDotNet.snk</AssemblyOriginatorKeyFile>
     <SignAssembly>true</SignAssembly>
diff --git a/YamlDotNet.Samples.Fsharp/YamlDotNet.Samples.Fsharp.fsproj b/YamlDotNet.Samples.Fsharp/YamlDotNet.Samples.Fsharp.fsproj
@@ -2,7 +2,7 @@
 
   <PropertyGroup>
     <OutputType>Exe</OutputType>
-    <TargetFramework>net8.0</TargetFramework>
+    <TargetFrameworks>net10.0</TargetFrameworks>
     <IsPackable>false</IsPackable>
     <LangVersion></LangVersion>
     <WarningLevel>5</WarningLevel>
diff --git a/YamlDotNet.Samples/YamlDotNet.Samples.csproj b/YamlDotNet.Samples/YamlDotNet.Samples.csproj
@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFramework>net8.0</TargetFramework>
+    <TargetFramework>net10.0</TargetFramework>
     <IsPackable>false</IsPackable>
   </PropertyGroup>
 
diff --git a/YamlDotNet.Test/Core/StringInterningTests.cs b/YamlDotNet.Test/Core/StringInterningTests.cs
@@ -0,0 +1,82 @@
+// This file is part of YamlDotNet - A .NET library for YAML.
+// Copyright (c) Antoine Aubry and contributors
+//
+// 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 Xunit;
+using YamlDotNet.Core;
+using YamlDotNet.Core.Events;
+
+namespace YamlDotNet.Test.Core
+{
+    public class StringInterningTests
+    {
+        [Fact]
+        public void AnchorNameDoesNotInternUniqueValues()
+        {
+            var value = UniqueValue("anchor");
+            Assert.Null(string.IsInterned(value));
+
+            var anchor = new AnchorName(value);
+
+            Assert.Equal(value, anchor.Value);
+            Assert.Null(string.IsInterned(value));
+        }
+
+        [Fact]
+        public void TagNameDoesNotInternUniqueValues()
+        {
+            var value = "!" + UniqueValue("tag");
+            Assert.Null(string.IsInterned(value));
+
+            var tag = new TagName(value);
+
+            Assert.Equal(value, tag.Value);
+            Assert.Null(string.IsInterned(value));
+        }
+
+        [Fact]
+        public void ScalarKeyDoesNotInternUniqueValues()
+        {
+            var value = UniqueValue("key");
+            Assert.Null(string.IsInterned(value));
+
+            var scalar = new Scalar(
+                AnchorName.Empty,
+                TagName.Empty,
+                value,
+                ScalarStyle.Plain,
+                isPlainImplicit: true,
+                isQuotedImplicit: false,
+                Mark.Empty,
+                Mark.Empty,
+                isKey: true);
+
+            Assert.True(scalar.IsKey);
+            Assert.Equal(value, scalar.Value);
+            Assert.Null(string.IsInterned(value));
+        }
+
+        private static string UniqueValue(string prefix)
+        {
+            return string.Concat(prefix, "_", Guid.NewGuid().ToString("N"));
+        }
+    }
+}
diff --git a/YamlDotNet.Test/Serialization/MergingParserTests.cs b/YamlDotNet.Test/Serialization/MergingParserTests.cs
@@ -19,8 +19,12 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 // SOFTWARE.
 
+using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
 using FluentAssertions;
 using Xunit;
 using YamlDotNet.Core;
@@ -151,5 +155,134 @@ public void MergingParserWithNestedSequence_ShouldNotThrowException()
 
             new SerializerBuilder().Build().Serialize(yamlObject!).NormalizeNewLines().Should().Be(etalonMergedYaml);
         }
+
+        [Fact]
+        public async Task MergingParserWithMergeKeyBomb_ShouldThrowExceptionWhenTooManyEvents()
+        {
+            // Timebox this test to avoid infinite loops in case of bugs.
+            // 30 seconds should be more than enough for this test to run even on a slow machine, and if it takes longer than that,
+            // it's likely that the merging parser is not correctly counting events and enforcing the limit.
+            var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
+            cancellationTokenSource.Token.Register(() =>
+            {
+                throw new TimeoutException("The test took too long, likely due to an infinite loop in the merging parser.");
+            });
+
+            await Task.Run(() =>
+            {
+                var sb = new StringBuilder();
+
+                // Base anchor
+                sb.AppendLine("a0: &a0");
+                sb.AppendLine("  x: 1");
+                sb.AppendLine();
+
+                // Each level merges the previous anchor TWICE (fanout=2), doubling event count
+                for (int i = 1; i <= 25; i++)
+                {
+                    sb.AppendLine($"a{i}: &a{i}");
+                    sb.AppendLine($"  <<: *a{i - 1}");  // first merge
+                    sb.AppendLine($"  <<: *a{i - 1}");  // second merge
+                    sb.AppendLine();
+                }
+
+                sb.AppendLine("final:");
+                sb.AppendLine("  <<: *a25");
+
+                var yaml = sb.ToString();
+                var parser = new Parser(new StringReader(yaml));
+                var mergingParser = new MergingParser(parser, 1000);
+                try
+                {
+                    while (mergingParser.MoveNext())
+                    {
+                        //move through everything, we're in a timebox so if this takes too long, the cancellation token will trigger and fail the test
+                    }
+                }
+                catch (YamlException ex) when (ex.Message.Contains("Too many parsing events"))
+                {
+                    // Expected exception, test passes
+                    return;
+                }
+                catch (Exception ex)
+                {
+                    throw new Exception($"Unexpected exception", ex);
+                }
+            }, cancellationTokenSource.Token);
+        }
+
+        [Fact]
+        public async Task MergingParserWithManySmallMerges_ShouldThrowExceptionWhenCumulativeEventsExceedLimit()
+        {
+            // Timebox this test to avoid infinite loops in case of bugs.
+            // 30 seconds should be more than enough for this test to run even on a slow machine, and if it takes longer than that,
+            // it's likely that the merging parser is not correctly counting events and enforcing the limit.
+            var cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(30));
+            cancellationTokenSource.Token.Register(() =>
+            {
+                throw new TimeoutException("The test took too long, likely due to an infinite loop in the merging parser.");
+            });
+
+            await Task.Run(() =>
+            {
+                var sb = new StringBuilder();
+                sb.AppendLine("base: &base");
+                for (var i = 0; i < 25; i++)
+                {
+                    sb.AppendLine($"  k{i}: v{i}");
+                }
+
+                sb.AppendLine();
+                for (var i = 0; i < 35; i++)
+                {
+                    sb.AppendLine($"entry{i}:");
+                    sb.AppendLine("  <<: *base");
+                    sb.AppendLine();
+                }
+
+                var parser = new Parser(new StringReader(sb.ToString()));
+                var mergingParser = new MergingParser(parser, 1000);
+
+                Action parse = () =>
+                {
+                    while (mergingParser.MoveNext())
+                    {
+                    }
+                };
+
+                parse.Should().Throw<YamlException>()
+                    .Where(ex => ex.Message.Contains("Too many parsing events"));
+            }, cancellationTokenSource.Token);
+        }
+
+        [Fact]
+        public void MergingParserWithDeepSingleChain_ShouldParseWithinLimit()
+        {
+            const int depth = 200;
+            var sb = new StringBuilder();
+
+            sb.AppendLine("a0: &a0");
+            sb.AppendLine("  root: value");
+            sb.AppendLine();
+
+            for (var i = 1; i <= depth; i++)
+            {
+                sb.AppendLine($"a{i}: &a{i}");
+                sb.AppendLine($"  <<: *a{i - 1}");
+                sb.AppendLine($"  level{i}: {i}");
+                sb.AppendLine();
+            }
+
+            sb.AppendLine("final:");
+            sb.AppendLine($"  <<: *a{depth}");
+
+            var parser = new Parser(new StringReader(sb.ToString()));
+            var mergingParser = new MergingParser(parser, 50000);
+            var deserializer = new DeserializerBuilder().Build();
+
+            var yamlObject = deserializer.Deserialize<Dictionary<string, object>>(mergingParser);
+
+            yamlObject.Should().ContainKey("final");
+        }
     }
 }
diff --git a/YamlDotNet.Test/YamlDotNet.Test.csproj b/YamlDotNet.Test/YamlDotNet.Test.csproj
@@ -1,6 +1,7 @@
 ﻿<Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
-    <TargetFrameworks>net10.0;net8.0;net47</TargetFrameworks>
+    <TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('Windows'))">net10.0;net8.0;net4.7</TargetFrameworks>
+    <TargetFrameworks Condition="!$([MSBuild]::IsOSPlatform('Windows'))">net10.0;net8.0</TargetFrameworks>
     <IsPackable>false</IsPackable>
     <AssemblyOriginatorKeyFile>..\YamlDotNet.snk</AssemblyOriginatorKeyFile>
     <SignAssembly>true</SignAssembly>
diff --git a/YamlDotNet/Core/AnchorName.cs b/YamlDotNet/Core/AnchorName.cs
@@ -50,7 +50,7 @@ public AnchorName(string value)
                                             $"disallowed characters: []{{}},\nThe value was '{value}'.", nameof(value));
             }
 
-            this.value = string.Intern(value);
+            this.value = string.IsInterned(value) ?? value;
         }
 
         public override string ToString() => value ?? "[empty]";
diff --git a/YamlDotNet/Core/Events/Scalar.cs b/YamlDotNet/Core/Events/Scalar.cs
@@ -79,11 +79,8 @@ public sealed class Scalar : NodeEvent
         public Scalar(AnchorName anchor, TagName tag, string value, ScalarStyle style, bool isPlainImplicit, bool isQuotedImplicit, Mark start, Mark end, bool isKey = false)
             : base(anchor, tag, start, end)
         {
-            // In a real client program, keys are likely to be
-            // interned already. Thus, IsInterned might work
-            // much better, but you can't really benchmark that.
             this.Value = isKey ?
-                    string.Intern(value) :
+                    string.IsInterned(value) ?? value :
                     value;
             this.Style = style;
             this.IsPlainImplicit = isPlainImplicit;
diff --git a/YamlDotNet/Core/MergingParser.cs b/YamlDotNet/Core/MergingParser.cs
@@ -36,13 +36,25 @@ public sealed class MergingParser : IParser
         private readonly IParser innerParser;
         private IEnumerator<LinkedListNode<ParsingEvent>> iterator;
         private bool merged;
+        private readonly int maxParsingEvents;
 
         public MergingParser(IParser innerParser)
+          : this(innerParser, 100_000)
         {
+        }
+
+        public MergingParser(IParser innerParser, int maxParsingEvents = 100_000)
+        {
+            if (maxParsingEvents <= 0)
+            {
+                throw new ArgumentOutOfRangeException(nameof(maxParsingEvents), "Max parsing events must be a positive integer.");
+            }
+
             events = new ParsingEventCollection();
             merged = false;
             iterator = events.GetEnumerator();
             this.innerParser = innerParser;
+            this.maxParsingEvents = maxParsingEvents;
         }
 
         public ParsingEvent? Current => iterator.Current?.Value;
@@ -64,7 +76,9 @@ private void Merge()
         {
             while (innerParser.MoveNext())
             {
-                events.Add(innerParser.Current!);
+                var parsingEvent = innerParser.Current!;
+                events.Add(parsingEvent);
+                EnsureMaxParsingEventsNotExceeded(parsingEvent);
             }
 
             foreach (var node in events)
@@ -126,7 +140,7 @@ private bool HandleAnchorAlias(LinkedListNode<ParsingEvent> node, LinkedListNode
         {
             var mergedEvents = GetMappingEvents(anchorAlias.Value);
 
-            events.AddAfter(node, mergedEvents);
+            events.AddAfter(node, mergedEvents, EnsureMaxParsingEventsNotExceeded);
             events.MarkDeleted(anchorNode);
 
             return true;
@@ -165,6 +179,14 @@ private IEnumerable<ParsingEvent> GetMappingEvents(AnchorName anchor)
                 .Select(cloner.Clone);
         }
 
+        private void EnsureMaxParsingEventsNotExceeded(ParsingEvent parsingEvent)
+        {
+            if (events.Count > maxParsingEvents)
+            {
+                throw new YamlException(parsingEvent.Start, parsingEvent.End, $"Too many parsing events. The configured limit of {maxParsingEvents} was exceeded to prevent excessive memory usage.");
+            }
+        }
+
         private sealed class ParsingEventCollection : IEnumerable<LinkedListNode<ParsingEvent>>
         {
             private readonly LinkedList<ParsingEvent> events;
@@ -178,11 +200,14 @@ public ParsingEventCollection()
                 references = [];
             }
 
-            public void AddAfter(LinkedListNode<ParsingEvent> node, IEnumerable<ParsingEvent> items)
+            public int Count => events.Count;
+
+            public void AddAfter(LinkedListNode<ParsingEvent> node, IEnumerable<ParsingEvent> items, Action<ParsingEvent> onItemAdded)
             {
                 foreach (var item in items)
                 {
                     node = events.AddAfter(node, item);
+                    onItemAdded(item);
                 }
             }
 
diff --git a/YamlDotNet/Core/TagName.cs b/YamlDotNet/Core/TagName.cs
@@ -49,7 +49,7 @@ public TagName(string value)
                 throw new ArgumentException("Tag value must not be empty.", nameof(value));
             }
 
-            this.value = string.Intern(value);
+            this.value = string.IsInterned(value) ?? value;
 
             if (IsGlobal && !Uri.IsWellFormedUriString(value, UriKind.RelativeOrAbsolute))
             {

Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ public AnchorName(string value)`
`50`	`50`	`$"disallowed characters: []{{}},\nThe value was '{value}'.", nameof(value));`
`51`	`51`	`}`
`52`	`52`
`53`		`- this.value = string.Intern(value);`
	`53`	`+ this.value = string.IsInterned(value) ?? value;`
`54`	`54`	`}`
`55`	`55`
`56`	`56`	`public override string ToString() => value ?? "[empty]";`