fix: prevent XXE vulnerability in XML parser (CVE-2017-9096)#233
fix: prevent XXE vulnerability in XML parser (CVE-2017-9096)#233VahidN merged 2 commits intoVahidN:masterfrom
Conversation
Summary of ChangesHello @michaelcheers, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a critical security fix to prevent XML External Entity (XXE) vulnerabilities in the XML parsing logic of the iTextSharp.LGPLv2.Core library. By configuring Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Summary
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the XXE vulnerability (CVE-2017-9096) by configuring the XmlReader with secure settings (DtdProcessing.Prohibit and XmlResolver = null) in ParserBase.cs. This is a critical security fix. My review includes a suggestion to refactor the new code to reduce duplication, improving maintainability. Additionally, for consistency and to promote secure practices throughout the codebase, you might consider applying similar secure settings to the XmlReader creation in XmlITextTests.cs in a future change, even though it's in test code.
| var reader = XmlReader.Create(stringReader, new XmlReaderSettings | ||
| { | ||
| DtdProcessing = DtdProcessing.Prohibit, | ||
| XmlResolver = null | ||
| }); |
There was a problem hiding this comment.
To improve maintainability and avoid code duplication, consider extracting these XmlReaderSettings into a private static readonly field. The same settings are configured again in the Parse(string url) method. Centralizing this configuration would make it easier to manage and ensure consistency.
| var reader = XmlReader.Create(stringReader, new XmlReaderSettings | ||
| { | ||
| DtdProcessing = DtdProcessing.Prohibit, | ||
| XmlResolver = null | ||
| }); |
There was a problem hiding this comment.
This is the second instance of the XmlReaderSettings configuration. As mentioned in the other comment, creating a shared private static readonly field for these settings would eliminate this code duplication and improve the overall maintainability of the class.
|
@michaelcheers |
- Extract XmlReaderSettings as public static readonly field in ParserBase - Apply secure settings to XmpReader and XfaForm XXE vulnerability fixes - Addresses maintainer feedback to centralize configuration
|
@VahidN I've updated the PR to:
|
2 participants
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The XML parser in
ParserBase.cscreatesXmlReaderinstances without configuring secure settings, leaving it vulnerable to XML External Entity (XXE) attacks (CVE-2017-9096). An attacker could exploit this by providing a malicious PDF containing crafted XML to:This affects any application using iTextSharp.LGPLv2.Core to process untrusted PDFs on the server side.
What is the new behavior?
XmlReader.Create()now uses secureXmlReaderSettingsthat:DtdProcessing = DtdProcessing.Prohibitto disable DTD processingXmlResolver = nullto prevent resolution of external entitiesThis prevents XXE attacks while maintaining all existing functionality for legitimate XML processing within PDFs.
Does this PR introduce a breaking change?
The change only affects the internal XML parsing behavior and does not modify any public APIs or expected functionality. All legitimate use cases continue to work unchanged.
Other information
This fix addresses CVE-2017-9096, which affected the original iText library (versions before 5.5.12). Since iTextSharp.LGPLv2.Core is based on iTextSharp 4.1.6 (equivalent to iText 2.1.7), it inherited this vulnerability.