-
Notifications
You must be signed in to change notification settings - Fork 471
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
XXE Mitigation #870
XXE Mitigation #870
Conversation
…XXE attacks. The existing functionality resolving svg11.dtd references to the embedded version was preserved.
…to URIs that look like DTD requests. Only checking for 'svg' could match a directory named 'svg' containing a different resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution, this looks good! Can you please add an entry in the release notes?
private const string SvgTextId = "secretText"; | ||
private const string Secret = "This is a secret!"; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done
Added "change" to release notes, as requested. |
Thank you! |
…s Nuget README.md Samples Source Tests doc docfx.json index.md license.txt No longer resolves external resources in DTDs by default, to prevent XXE attacks. The existing functionality resolving svg11.dtd references to the embedded version was preserved. BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Added tests to show the vulnerability and the mitigation. BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Narrowed the check around whether to serve up the embedded svg11.dtd to URIs that look like DTD requests. Only checking for 'svg' could match a directory named 'svg' containing a different resource.
What perfect timing! We just noticed this problem too. Do you know anything about when the next release will be? Thanks a lot, @gmwalker! ♥ |
…nerators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt No longer resolves external resources in DTDs by default, to prevent XXE attacks. The existing functionality resolving svg11.dtd references to the embedded version was preserved. BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Added tests to show the vulnerability and the mitigation. BuildProcessTemplates CONTRIBUTING.md Generators Nuget README.md Samples Source Tests doc docfx.json index.md license.txt Narrowed the check around whether to serve up the embedded svg11.dtd to URIs that look like DTD requests. Only checking for 'svg' could match a directory named 'svg' containing a different resource.
Reference Issue
Fixes #869
What does this implement/fix? Explain your changes.
This address the issue of XXE Vulnerability by changing
SvgDtdResolver
so it only resolves external resources ifSvgDocument.ResolveExternalResources
istrue
.SvgDocument
will no longer resolve external resources by default, to be on the safe side as suggested in the issue.Any other comments?
In
SvgDtdResolver
I narrowed the check around whether to serve up the embedded svg11.dtd to URIs that look like DTD requests. Only checking for 'svg' could match a directory named 'svg' containing a different resource altogether - which was a problem I encountered making the unit tests for this pull request.This is my first contribution to this project and I'm looking forward to collaborating with you all.