Additional XXE mitigation #873

kimsey0 · 2021-05-11T15:23:11Z

Reference Issue

#872

What does this implement/fix? Explain your changes.

This prevents loading external images and elements when SvgDocument.ResolveExternalResources is false.

Any other comments?

This is just a draft pull request to get the ball rolling.

Should loading external images and text content like this be disabled by default, or should this behavior be behind a separate flag from ResolveExternalResources? The property name seems to indicate that it controls the loading of all kinds of external resources, but there might be users who expect to be able to include external images?
Should SvgImage and SvgElementIdManager access the static SvgDocument.ResolveExternalResources like this or should it somehow be passed down to them, like for SvgDtdResolver?
It's missing unit tests. I'll add some, but I'm just trying to figure out what external URI's I can use that will always be available and that will allow distinguishing behaviors from each other. Is it and should it be possible to run the tests without an internet connection?

…abled

kimsey0 · 2021-05-11T15:27:26Z

@gmwalker: Since this builds on your work, perhaps you have some comments, specifically about the use of the static property?

@H1Gdev, @mrbean-bremen: You were both part of reviewing #870 a few days ago. (Sorry if tagging you like this is unnecessary.)

mrbean-bremen · 2021-05-11T18:34:02Z

@tebjan , @wieslawsoltes : I'm not really comfortable with reviewing this, having no experience with handling of vulnerabilities in C# - if one of you has more experience, it would be nice to get a review. Same goes for @gmwalker or anyone else with more knowledge in this field.

mrbean-bremen · 2021-05-11T18:36:47Z

@kimsey0 - thanks for the pull request, we appreciate the effort to make the library more secure!
Looks like the image comparison test crashes with that change - can you please have a look?

kimsey0 · 2021-05-12T08:29:14Z

It looks like quite a few (190) of the W3C test suite SVG's use external references, specifically for fonts, for example:

SVG/Tests/W3CTestSuite/svg/color-prop-01-b.svg

Line 40 in 062dc7c

<font-face-uri xlink:href="../resources/SVGFreeSans.svg#ascii"/>

I have very little information about how SVG.NET is used elsewhere. It might be common and expected to load and render SVG's with external font references. However, in our use-case, we render user-uploaded SVG's, and would like to prevent them from both loading local files and making web requests. That also seems to be the concern of @gmwalker in #869 (comment).

It could be that these W3C suite tests should be run with SvgDocument.ResolveExternalResources = true, or it could be that the behavior of loading external images and fonts should be controlled by another flag that's enabled by default.

kimsey0 · 2021-05-12T08:38:17Z

For now, I enabled SvgDocument.ResolveExternalResources in the rendering tests.

Tests/Svg.UnitTests/ImageComparisonTest.cs

mrbean-bremen

FWIW, the changes look good to me, but as I wrote -- I have no experience here and probably wouldn't find any missing checks, so I'll wait a bit for someone else to have a look.
I also think that having ResolveExternalSources default to false is the correct way, we just have to make sure that this is made clear in the release notes. As this is a breaking change, we'll probably need a new major release version, if we want to follow semantic versioning -- @tebjan , what do you think?

kimsey0 · 2021-05-14T09:50:27Z

I've added image comparison tests that render an SVG with an external image reference and one with a text reference, and check that the image and text is only shown if ResolveExternalSources is true. I put the tests in XxeVulnerabilityTests, but made the CompareSvgImageWithReferenceImpl from ImageComparisonTest public and static and used it. Let me know if you'd rather have the tests moved to ImageComparisonTest, have the rendering and comparison logic duplicated in XxeVulnerabilityTests, or think this could be tested better with a different kind of test.

I also think that having ResolveExternalSources default to false is the correct way, we just have to make sure that this is made clear in the release notes. As this is a breaking change, we'll probably need a new major release version, if we want to follow semantic versioning

I agree. If loading external images and other references is disabled by default, it's a breaking change and should be a new major version.

H1Gdev · 2021-05-14T10:37:00Z

Tests/Svg.UnitTests/ImageComparisonTest.cs

@@ -28,6 +28,9 @@ 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.ResolveExternalResources = true;


Is it possible to move this line to head of CompareSvgImageWithReferenceImpl method ?

I did so in an earlier commit, but moved it back here because I now also use CompareSvgImageWithReferenceImpl from XxeVulnerabilityTests, which need XxeVulnerabilityTests to not be overwritten. I've added a few suggestions for alternative organization of the test here #873 (comment).

I can also make the value for ResolveExternalResources a parameter of CompareSvgImageWithReferenceImpl and pass it in from CompareSvgImageWithReference and XxeResolveExternalImage? However you'd prefer.

In XxeVulnerabilityTests, please evaluate DOM value etc. instead of comparing reference images.

Now in ImageComparisonTest, I think that I do is only in case of SvgDocument.ResolveExternalResources = true.

For both <image xlink:href="..." /> and <tref xlink:href="..."/>, resolving the external reference only seems to happen during rendering, and even after calling Draw, I can't see anything that has changed in the DOM which a test could look for. Am I overlooking something?

Please add property or method to evaluate.

H1Gdev

I think setting this feature with Boolean value will significantly reduce usability.
In most cases, we want to disable external file reference, but we want to display images.

What do you want to disable in this PR ?

H1Gdev · 2021-05-15T01:04:17Z

Tests/Svg.UnitTests/ImageComparisonTest.cs

@@ -28,6 +28,9 @@ 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.ResolveExternalResources = true;


Please add property or method to evaluate.

kimsey0 · 2021-05-17T08:51:06Z

@H1Gdev: I'm not sure I quite understand your questions.

I think setting this feature with Boolean value will significantly reduce usability.

Do you mean to suggest having multiple different Boolean properties controlling the different types of external resources that can be loaded:

public bool ResolveExternalXmlEntites { get; set } = false;
public bool ResolveExternalImages { get; set; } = true;
public bool ResolveExternalElements { get; set } = true;

Or something similar with an enum or flags enum?

In most cases, we want to disable external file reference, but we want to display images.

The current implementation in this pull request allows displaying images via data: URI's, but neither via local file system references nor web references:

<!-- Displayed when SvgDocument.ResolveExternalResources == false -->
<image href="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" />

<!-- Not displayed when SvgDocument.ResolveExternalResources == false -->
<image href="../../MySecretImages/Secret.png" />
<image href="https://example.org/External.png" />

Are you saying that, in most cases, we want to allow loading external images from the local file system and the web?

What do you want to disable in this PR ?

Our use-case is to allow users to upload SVG files and to generate previews of them. When rendering these previews, we want to:

Disable loading files from the local file system, since that could allow including confidential information from the local system in the previews.
Disable loading files from the web, since that exposes us to Server Side Request Forgery, where a user has our server make malicious web requests.

Please add property or method to evaluate.

I'm not sure where I should add a property or method to evaluate what?

tebjan · 2021-05-17T23:13:54Z

@mrbean-bremen sorry for the late reply. Yes, I agree with what you said about the versioning, since it's a breaking change.

Regarding code, it's out of my comfort zone a bit. But from what I've seen, it's definitely an improvement, even if it isn't mitigating all possible vulnerabilities.

mrbean-bremen · 2021-05-18T19:36:53Z

@tebjan - thanks! I also think that the changes are good, and we may just wait a bit for any other comments and ideas before merging them in.

@H1Gdev - maybe you can show in a code example what you mean? I also didn't quite get your review comments.

H1Gdev · 2021-05-19T05:32:49Z

@kimsey0

Or something similar with an enum or flags enum?

I think flags enum is easy to use.

However, we haven't been able to identify use cases, so I'm wondering what kind of interfaces should be defined.

design 1

[Flags]
enum ExternalType
{
  None = 0x0,
  Local = 0x1,
  Remote = 0x2,
}
public ExternalType ResolveExternalXmlEntites { get; set } = ExternalType.None;
public ExternalType ResolveExternalImages { get; set; } = ExternalType.Local;
public ExternalType ResolveExternalElements { get; set } = ExternalType.Local | ExternalType.Remote;

design 2

[Flags]
enum ExternalType
{
  Local_XML = 0x1,
  Remote_XML = 0x2,
  Local_Image = 0x4,
  Remote_Image = 0x8,
  Local_Element = 0x10,
  Remote_Element = 0x20,
}
public ExternalType ResolveExternalResources { get; set } = ExternalType.Local_Image | ExternalType.Remote_Image;

Are you saying that, in most cases, we want to allow loading external images from the local file system and the web?

This depends on intended use of users.
(I mainly use it for SVG image output.)

Our use-case is to allow users to upload SVG files and to generate previews of them. When rendering these previews, we want to:

It seems that default values are different on server side or client side...
It may be kind to define default values in each case.

I'm not sure where I should add a property or method to evaluate what ?

Please add it to corresponding element.
(For example SvgImage)

public ExternalType ResolveExternalResources { get; set }

                if (ResolveExternalResources.HasFlag(ExternalType.Local))
                {
                }
                if (ResolveExternalResources.HasFlag(ExternalType.Remote))
                {
                }

Split into ResolveExternalXmlEntites, ResolveExternalImages, and ResolveExternalElements, each of which can allow local and/or remote resources

kimsey0 · 2021-05-25T12:20:39Z

@H1Gdev

I think flags enum is easy to use.

OK. I went with your design 1, since I think it makes both setup and usage of the properties more clear and makes it harder to accidentally overwrite flags for other resource types. Have a look!

It seems that default values are different on server side or client side...
It may be kind to define default values in each case.

I'm not exactly sure how this should be accomplished. We can't reliably determine which environment SVG.NET is being used in, so it would have to be configurable. We could add additional properties or configuration methods like (shorthand):

public static void ConfigureServerSecurity() => ResolveExternalXmlEntites = ResolveExternalImages = ResolveExternalElements = ExternalType.None;

But I'm not sure that would be any more discoverable or understandable than just setting the properties manually.

I've set ResolveExternalXmlEntites to ExternalType.None and ResolveExternalImages and ResolveExternalElements to ExternalType.Local | ExternalType.Remote for now. That keeps the current behavior on master, but the defaults changes from #870 may still be considered breaking and necessitate a new major version.

Please add it to corresponding element.
(For example SvgImage)
I can add this property to SvgImage, but I'm not sure how to get it initialized. SvgImage seems to be instantiated by SvgElementFactory, which is source generated by AvailableElementsGenerator. I can't find any functionality to initialize this kind of property which isn't bound to anything in the SVG file. Can you point me in the right direction?

H1Gdev · 2021-05-27T01:10:44Z

Tests/Svg.UnitTests/XxeVulnerabilityTests.cs

@@ -39,6 +41,35 @@ public void XxeResolveExternalResources(bool resolveExternalResources)
                DeleteSecretFile(secretFilePath);
            }
        }
+
+        [TestCase(ExternalType.None, "__pull_request-873-01", "__pull_request-873-01-ExternalReferencesDisabled")]


Include this test in ImageComparisonTest.

#873 (comment)

There is no need for this, as the image comparison tests are already executed here.

I commented that DOM test should be done here because images comparison test is uncertain...
I think tests like PR #870 is ideal.

Ok, I obviously didn't get this, sorry. If you want to change this, you can add a new PR, of course. There are still 2 open issues that can go into the next release, so we may wait with a release a couple of days anyway.

If it looks like it will be in time for this release, I will create a PR.

Go ahead 👍

H1Gdev · 2021-05-27T01:11:20Z

Tests/Svg.UnitTests/ImageComparisonTest.cs


-        private void CompareSvgImageWithReferenceImpl(string baseName,
+        public static void CompareSvgImageWithReferenceImpl(string baseName,


Please revert this line.

This is used in the other test, so it cannot be private. Can be made internal instead.

H1Gdev · 2021-05-27T01:30:02Z

@kimsey0

Please add it to corresponding element.
(For example SvgImage)

I can add this property to SvgImage, but I'm not sure how to get it initialized. SvgImage seems to be instantiated by SvgElementFactory, which is source generated by AvailableElementsGenerator. I can't find any functionality to initialize this kind of property which isn't bound to anything in the SVG file. Can you point me in the right direction?

At this time, there is no need to initialize.

A place that has access to direct SvgDocument from SvgImage, ensure that you access through the property or method of SvgImage.

It may be better to have it in SvgElement.

H1Gdev · 2021-05-28T00:12:33Z

But I'm not sure that would be any more discoverable or understandable than just setting the properties manually.

If we want to make it stronger, you can force users to set it.
If not set, an exception will be thrown and it will not work.

@mrbean-bremen

How do you think we should decide (default value ? client ? server ? require ?) ? 😟

mrbean-bremen · 2021-08-10T18:25:49Z

Sorry for the long delay - I missed the question, and I didn't follow up here, as I was busy with other stuff.
I just had another look, and I actually think this is sufficient as is. It does the main task to make the library usage safer by default, and allows to configure it easily.
I will wait a couple of days for further comments and merge if nothing substantial comes up. It also probably makes sense to make a new release soon.

H1Gdev · 2021-08-11T01:27:45Z

It looks like some code hasn't been fixed yet.

mrbean-bremen · 2021-08-11T05:10:42Z

It looks like some code hasn't been fixed yet.

You refer to the private method made public, and the reference to the test? Yes, these could be changed, though these are minor things. Anything else?

H1Gdev · 2021-08-11T05:24:40Z

Maybe that's it.

…d Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Don't load external elements when resolving external resources is disabled BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Enable resolving external resources for rendering tests BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Add image comparison tests for loading external images and text BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Add release notes BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Split ResolveExternalResources into three properties (ResolveExternalXmlEntites, ResolveExternalImages, and ResolveExternalElements)

MarvinPogoda · 2023-10-10T09:55:54Z

Hey @mrbean-bremen ,
is it possible to mark the nuget-versions before this fix as vulnerable, so using the older version will know that there might be security issues?

mrbean-bremen · 2023-10-10T17:23:30Z

I'm not aware of a possibility to do that. Generally I think this is what the release notes are for - the user has to check if a newer version fixes some known vulneribilities.
Vulnerabilities will be found in most versions with time, and the general policy is nowadays to go with the newest version (or at least the newest patch version, if a version is still supported).

kimsey0 added 2 commits May 11, 2021 17:15

Don't load external images when resolving external resources is disabled

09178b8

Don't load external elements when resolving external resources is dis…

a50dbbd

…abled

Enable resolving external resources for rendering tests

958f2be

H1Gdev reviewed May 12, 2021

View reviewed changes

Tests/Svg.UnitTests/ImageComparisonTest.cs Outdated Show resolved Hide resolved

Merge test assignments to ResolveExternalResources property

2c38102

mrbean-bremen reviewed May 12, 2021

View reviewed changes

kimsey0 added 2 commits May 14, 2021 11:37

Add image comparison tests for loading external images and text

3b8f108

Add release notes

8e4c646

kimsey0 marked this pull request as ready for review May 14, 2021 09:52

H1Gdev reviewed May 14, 2021

View reviewed changes

H1Gdev reviewed May 15, 2021

View reviewed changes

Split ResolveExternalResources into three properties

71f8511

Split into ResolveExternalXmlEntites, ResolveExternalImages, and ResolveExternalElements, each of which can allow local and/or remote resources

H1Gdev reviewed May 27, 2021

View reviewed changes

Minor review finding (internal instead of public method)

19c71a1

mrbean-bremen approved these changes Aug 12, 2021

View reviewed changes

mrbean-bremen merged commit 2dfa0de into svg-net:master Aug 12, 2021

H1Gdev mentioned this pull request Aug 17, 2021

Refactoring for XXE Mitigation. #885

Merged

mrbean-bremen mentioned this pull request Sep 4, 2021

Additional XXE vulnerabilities #872

Closed

kimsey0 deleted the feature/872-prevent-more-xxe branch June 16, 2022 11:43

kimsey0 mentioned this pull request Jan 14, 2024

Adding support for <link> css inclusion #721

Open


		private void CompareSvgImageWithReferenceImpl(string baseName,
		public static void CompareSvgImageWithReferenceImpl(string baseName,

Additional XXE mitigation #873

Additional XXE mitigation #873

Conversation

kimsey0 commented May 11, 2021 • edited Loading

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

kimsey0 commented May 11, 2021

mrbean-bremen commented May 11, 2021

mrbean-bremen commented May 11, 2021

kimsey0 commented May 12, 2021

kimsey0 commented May 12, 2021

mrbean-bremen left a comment

Choose a reason for hiding this comment

kimsey0 commented May 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

H1Gdev May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

H1Gdev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimsey0 commented May 17, 2021 • edited Loading

tebjan commented May 17, 2021

mrbean-bremen commented May 18, 2021

H1Gdev commented May 19, 2021

kimsey0 commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

H1Gdev Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

H1Gdev commented May 27, 2021

H1Gdev commented May 28, 2021 • edited Loading

mrbean-bremen commented Aug 10, 2021

H1Gdev commented Aug 11, 2021

mrbean-bremen commented Aug 11, 2021

H1Gdev commented Aug 11, 2021

MarvinPogoda commented Oct 10, 2023

mrbean-bremen commented Oct 10, 2023

kimsey0 commented May 11, 2021 •

edited

Loading

H1Gdev May 14, 2021 •

edited

Loading

H1Gdev left a comment •

edited

Loading

kimsey0 commented May 17, 2021 •

edited

Loading

H1Gdev Aug 12, 2021 •

edited

Loading

H1Gdev commented May 28, 2021 •

edited

Loading