Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security: vulnerable to XXE attacks #869

Closed
hazcod opened this issue May 3, 2021 · 4 comments · Fixed by #870
Closed

Security: vulnerable to XXE attacks #869

hazcod opened this issue May 3, 2021 · 4 comments · Fixed by #870

Comments

@hazcod
Copy link

hazcod commented May 3, 2021

Description

The library currently does not protect against XXE attacks.
e.g. SvgDocument.DisableDtdProcessing defaults to false, which leads to DTDs being accepted in general.
But even when setting this to true, it is still possible to use xrefs to reach external resources.

Please:

  1. Set default behaviour to be the most secure
  2. Provide documentation on the risks and toggles.

Example data

<?xml version="1.0" standalone="yes"?>
<!DOCTYPE foo [ <!ENTITY xxe SYSTEM "file://etc/hosts" > ]>
<svg>....</svg>
<image x="90" y="-65" width="128" height="146"
     xlink:href="https://malicious.net"/>

Used Versions

latest.

@gmwalker
Copy link
Contributor

gmwalker commented May 4, 2021

This could be a problem for anyone who hosts a service where SVG's could be supplied by a third party. A malicious SVG doc could be used to retrieve file content from the host.

Taking a peek at the code...
Setting XmlResolver to null instead of new SvgDtdResolver(...) (when creating an SvgTextReader in SvgDocument.cs) would prevent any external resources from being resolved...

Would that be too severe a solution?

What was the original intention of the SvgDtdResolver? Serving up an embedded version of svg11.dtd instead of resolving it externally for systems not on the internet? If that is still useful/necessary, would it be possible to only resolve svg11.dtd and not any other external resource (via the call to base.GetEntity(...))?

@gmwalker gmwalker mentioned this issue May 6, 2021
@mrbean-bremen
Copy link
Member

What was the original intention of the SvgDtdResolver?

The SvgDtdResolver has been originally written by Microsoft, so we don't really know the intention. I would just guess that at that time security concerns didn't have a high priority.

Would that be too severe a solution?

I don't think so. As @tebjan (the maintainer of the library) has already given his thumbs up, I see no problem with merging your PR.

@hazcod
Copy link
Author

hazcod commented May 10, 2021

@mrbean-bremen thanks, can a new release be created?

@mrbean-bremen
Copy link
Member

@tebjan - what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants