Additional XXE mitigation

Source/Document Structure/SvgImage.cs

@@ -262,6 +262,12 @@ public object GetImage(string uriString)
                 if (!uri.IsAbsoluteUri)
                     uri = new Uri(OwnerDocument.BaseUri, uri);
 
+                if (!SvgDocument.ResolveExternalImages.AllowsResolving(uri))
+                {
+                    Trace.TraceWarning("Trying to resolve image from '{0}', but resolving external resources of that type is disabled.", uri);
+                    return null;
+                }
+
                 // should work with http: and file: protocol urls
                 var httpRequest = WebRequest.Create(uri);
 

Source/ExternalType.cs

@@ -0,0 +1,22 @@
+using System;
+
+namespace Svg
+{
+    [Flags]
+    public enum ExternalType
+    {
+        None = 0x0,
+        Local = 0x1,
+        Remote = 0x2,
+    }
+
+    public static class ExternalTypeExtensions
+    {
+        public static bool AllowsResolving(this ExternalType externalType, Uri uri)
+        {
+            var isLocalUri = !uri.IsAbsoluteUri || uri.IsFile;
+            return externalType.HasFlag(ExternalType.Local) && isLocalUri ||
+                   externalType.HasFlag(ExternalType.Remote) && !isLocalUri;
+        }
+    }
+}

Source/SvgDocument.cs

@@ -37,10 +37,20 @@ public partial class SvgDocument : SvgFragment, ITypeDescriptorContext
         public static bool DisableDtdProcessing { get; set; }
 
         /// <summary>
-        /// Defaults to `false` to prevent XXE.  Set to `true` to resolve external resources.
+        /// Which types of XML external entities are allowed to be resolved. Defaults to <see cref="ExternalType.None"/> to prevent XXE.
         /// </summary>
         /// <see ref="https://owasp.org/www-community/vulnerabilities/XML_External_Entity_(XXE)_Processing"/>
-        public static bool ResolveExternalResources { get; set; }
+        public static ExternalType ResolveExternalXmlEntites { get; set; } = ExternalType.None;
+
+        /// <summary>
+        /// Which types of external images are allowed to be resolved. Defaults to <see cref="ExternalType.Local"/> and <see cref="ExternalType.Remote"/>.
+        /// </summary>
+        public static ExternalType ResolveExternalImages { get; set; } = ExternalType.Local | ExternalType.Remote;
+
+        /// <summary>
+        /// Which types of external elements, for example text definitions, are allowed to be resolved. Defaults to <see cref="ExternalType.Local"/> and <see cref="ExternalType.Remote"/>.
+        /// </summary>
+        public static ExternalType ResolveExternalElements { get; set; } = ExternalType.Local | ExternalType.Remote;
 
         private static int? pointsPerInch;
 
@@ -364,7 +374,7 @@ public static SvgDocument Open(string path)
                 {
                     XmlResolver = new SvgDtdResolver
                     {
-                        ResolveExternalResources = ResolveExternalResources
+                        ResolveExternalXmlEntities = ResolveExternalXmlEntites
                     },
                     WhitespaceHandling = WhitespaceHandling.Significant,
                     DtdProcessing = SvgDocument.DisableDtdProcessing ? DtdProcessing.Ignore : DtdProcessing.Parse,
@@ -391,7 +401,7 @@ public static SvgDocument Open(string path)
             {
                 XmlResolver = new SvgDtdResolver
                 {
-                    ResolveExternalResources = ResolveExternalResources
+                    ResolveExternalXmlEntities = ResolveExternalXmlEntites
                 },
                 WhitespaceHandling = WhitespaceHandling.Significant,
                 DtdProcessing = SvgDocument.DisableDtdProcessing ? DtdProcessing.Ignore : DtdProcessing.Parse,

Source/SvgDtdResolver.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Diagnostics;
 using System.IO;
 using System.Reflection;
 using System.Text.RegularExpressions;
@@ -9,10 +10,10 @@ namespace Svg
     internal class SvgDtdResolver : XmlUrlResolver
     {
         /// <summary>
-        /// Defaults to `false` to prevent XXE.  Set to `true` to resolve external resources.
+        /// Defaults to <see cref="ExternalType.None"/> to prevent XXE.  Set to <see cref="ExternalType.Local"/> and/or <see cref="ExternalType"/> to resolve external resources.
         /// </summary>
         /// <see ref="https://owasp.org/www-community/vulnerabilities/XML_External_Entity_(XXE)_Processing"/>
-        public bool ResolveExternalResources { get; set; }
+        public ExternalType ResolveExternalXmlEntities { get; set; }
 
         /// <summary>
         /// Maps a URI to an object containing the actual resource.
@@ -36,11 +37,12 @@ public override object GetEntity(Uri absoluteUri, string role, Type ofObjectToRe
                 return Assembly.GetExecutingAssembly().GetManifestResourceStream("Svg.Resources.svg11.dtd");
             }
 
-            if (ResolveExternalResources)
+            if (ResolveExternalXmlEntities.AllowsResolving(absoluteUri))
             {
                 return base.GetEntity(absoluteUri, role, ofObjectToReturn);
             }
 
+            Trace.TraceWarning("Trying to resolve entity from '{0}', but resolving external entities of that type is disabled.", absoluteUri);
             return new MemoryStream();
         }
 

Source/SvgElementIdManager.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Net;
 using System.Text;
 using System.Text.RegularExpressions;
@@ -47,6 +48,13 @@ public virtual SvgElement GetElementById(Uri uri)
                 if (!uri.IsAbsoluteUri && _document.BaseUri != null)
                     uri = new Uri(_document.BaseUri, uri);
 
+
+                if (!SvgDocument.ResolveExternalElements.AllowsResolving(uri))
+                {
+                    Trace.TraceWarning("Trying to resolve element by ID from '{0}', but resolving external resources of that type is disabled.", uri);
+                    return null;
+                }
+
                 if (uri.IsAbsoluteUri)
                 {
                     if (uri.IsFile)
@@ -94,7 +102,7 @@ public virtual void Add(SvgElement element)
         }
 
         /// <summary>
-        /// Adds the specified <see cref="SvgElement"/> for ID management. 
+        /// Adds the specified <see cref="SvgElement"/> for ID management.
         /// And can auto fix the ID if it already exists or it starts with a number.
         /// </summary>
         /// <param name="element">The <see cref="SvgElement"/> to be managed.</param>

Tests/Svg.UnitTests/ImageComparisonTest.cs

@@ -28,6 +28,11 @@ public class ImageComparisonTest
         [TestCaseSource(typeof(ImageTestDataSource), nameof(ImageTestDataSource.PassingTests))]
         public void CompareSvgImageWithReference(ImageTestDataSource.TestData testData)
         {
+            // W3C Test Suites use external references to local fonts
+            SvgDocument.ResolveExternalXmlEntites = ExternalType.Local;
+            SvgDocument.ResolveExternalElements = ExternalType.Local;
+            SvgDocument.ResolveExternalImages = ExternalType.Local;
+
             string basePath = testData.BasePath;
             string baseName = testData.BaseName;
             bool testSaveLoad = !baseName.StartsWith("#");
@@ -44,6 +49,11 @@ public void CompareSvgImageWithReference(ImageTestDataSource.TestData testData)
         [TestCaseSource(typeof(ImageTestDataSource), "PassingTests")]
         public void CompareSvgImageWithReference(ImageTestDataSource.TestData testData)
         {
+            // W3C Test Suites use external references to local fonts
+            SvgDocument.ResolveExternalXmlEntites = ExternalType.Local;
+            SvgDocument.ResolveExternalElements = ExternalType.Local;
+            SvgDocument.ResolveExternalImages = ExternalType.Local;
+
             var basePath = testData.BasePath;
             while (!basePath.ToLower().EndsWith("svg"))
             {
@@ -62,8 +72,8 @@ public void CompareSvgImageWithReference(ImageTestDataSource.TestData testData)
             CompareSvgImageWithReferenceImpl(baseName, svgPath, pngPath, testSaveLoad);
         }
 #endif
-
-        private void CompareSvgImageWithReferenceImpl(string baseName,
+        
+        internal static void CompareSvgImageWithReferenceImpl(string baseName,
             string svgPath, string pngPath, bool testSaveLoad)
         {
             var svgDoc = LoadSvgDocument(svgPath);
@@ -91,7 +101,6 @@ private void CompareSvgImageWithReferenceImpl(string baseName,
                     svgDoc.Write(memStream);
                     memStream.Position = 0;
                     var baseUri = svgDoc.BaseUri;
-                    SvgDocument.ResolveExternalResources = true;
                     svgDoc = SvgDocument.Open<SvgDocument>(memStream);
                     svgDoc.BaseUri = baseUri;
                     using (var svgImage = LoadSvgImage(svgDoc, useFixedSize))

Tests/Svg.UnitTests/XxeVulnerabilityTests.cs

@@ -9,11 +9,13 @@ namespace Svg.UnitTests
     [TestFixture]
     public class XxeVulnerabilityTests
     {
-        [TestCase(true)]
-        [TestCase(false)]
-        public void XxeResolveExternalResources(bool resolveExternalResources)
+        [TestCase(ExternalType.None)]
+        [TestCase(ExternalType.Local)]
+        [TestCase(ExternalType.Remote)]
+        [TestCase(ExternalType.Local | ExternalType.Remote)]
+        public void XxeResolveExternalResources(ExternalType resolveExternalXmlEntites)
         {
-            SvgDocument.ResolveExternalResources = resolveExternalResources;
+            SvgDocument.ResolveExternalXmlEntites = resolveExternalXmlEntites;
 
             var secretFilePath = MakeSecretFilePath();
 
@@ -28,7 +30,7 @@ public void XxeResolveExternalResources(bool resolveExternalResources)
                 var svgTextElementContent = svgTextElement?.Content;
 
                 var expectedValue = 
-                    resolveExternalResources
+                    resolveExternalXmlEntites.HasFlag(ExternalType.Local)
                         ? Secret
                         : null;
 
@@ -39,6 +41,35 @@ public void XxeResolveExternalResources(bool resolveExternalResources)
                 DeleteSecretFile(secretFilePath);
             }
         }
+
+        [TestCase(ExternalType.None, "__pull_request-873-01", "__pull_request-873-01-ExternalReferencesDisabled")]
+        [TestCase(ExternalType.Remote, "__pull_request-873-01", "__pull_request-873-01-ExternalReferencesDisabled")]
+        [TestCase(ExternalType.Local, "__pull_request-873-01", "__pull_request-873-01-ExternalReferencesEnabled")]
+        [TestCase(ExternalType.Local | ExternalType.Remote, "__pull_request-873-01", "__pull_request-873-01-ExternalReferencesEnabled")]
+        public void XxeResolveExternalImage(ExternalType resolveExternalImages, string svgName, string pngName)
+        {
+            SvgDocument.ResolveExternalImages = resolveExternalImages;
+
+            var svgPath = Path.Combine(ImageTestDataSource.SuiteTestsFolder, "W3CTestSuite", "svg", svgName + ".svg");
+            var pngPath = Path.Combine(ImageTestDataSource.SuiteTestsFolder, "W3CTestSuite", "png", pngName + ".png");
+
+            ImageComparisonTest.CompareSvgImageWithReferenceImpl(svgName, svgPath, pngPath, false);
+        }
+
+
+        [TestCase(ExternalType.None, "__pull_request-873-02", "__pull_request-873-02-ExternalReferencesDisabled")]
+        [TestCase(ExternalType.Remote, "__pull_request-873-02", "__pull_request-873-02-ExternalReferencesDisabled")]
+        [TestCase(ExternalType.Local, "__pull_request-873-02", "__pull_request-873-02-ExternalReferencesEnabled")]
+        [TestCase(ExternalType.Local | ExternalType.Remote, "__pull_request-873-02", "__pull_request-873-02-ExternalReferencesEnabled")]
+        public void XxeResolveExternalElement(ExternalType resolveExternalElements, string svgName, string pngName)
+        {
+            SvgDocument.ResolveExternalElements = resolveExternalElements;
+
+            var svgPath = Path.Combine(ImageTestDataSource.SuiteTestsFolder, "W3CTestSuite", "svg", svgName + ".svg");
+            var pngPath = Path.Combine(ImageTestDataSource.SuiteTestsFolder, "W3CTestSuite", "png", pngName + ".png");
+
+            ImageComparisonTest.CompareSvgImageWithReferenceImpl(svgName, svgPath, pngPath, false);
+        }
 
         private static void MakeSecretFile(string secretFilePath)
         {

doc/ReleaseNotes.md

@@ -5,7 +5,8 @@ The release versions are NuGet releases.
 
 ### Changes
 * change namespace of SvgSymbol from Svg.Document_Structure to Svg. (see [PR #556](https://github.com/svg-net/SVG/pull/556))
-* changed default behaviour of DTD resolution so external references are not resolved by default; to mitigate XXE vulnerability. (see [PR #870](https://github.com/svg-net/SVG/pull/870))
+* changed default behavior of DTD resolution so external references are not resolved by default; to mitigate XXE vulnerability. (see [PR #870](https://github.com/svg-net/SVG/pull/870))
+* changed default behavior so external references to images, text definitions, and other resources are not resolved by default; to improve safety of rendering untrusted files. (see [PR #873](https://github.com/svg-net/SVG/pull/873))
 
 ### Enhancements
 * minimize XmlTextReader customization (see [PR #836](https://github.com/svg-net/SVG/pull/836))