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.

Sign up for GitHub

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

Increase limits for XML parsing #11126

Merged
merged 2 commits into from
Jan 7, 2025
Merged

Increase limits for XML parsing #11126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81e63c5
Increase limits for XML parsing
cushon Dec 19, 2024
44d9a49
Also disable external DTD loading
cushon Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions languagetool-core/src/main/java/org/languagetool/rules/patterns/PatternRuleLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ public final List<AbstractPatternRule> getRules(InputStream is, String filename,
Tools.setPasswordAuthenticator();
}
saxParser.getXMLReader().setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0);
saxParser.parse(is, handler);
return handler.getRules();
} catch (Exception e) {
Expand Down
3 changes: 3 additions & 0 deletions ...src/main/java/org/languagetool/tagging/disambiguation/rules/DisambiguationRuleLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public final List<DisambiguationPatternRule> getRules(InputStream stream, Langua
DisambiguationRuleHandler handler = new DisambiguationRuleHandler(language, xmlPath);
SAXParserFactory factory = SAXParserFactory.newInstance();
SAXParser saxParser = factory.newSAXParser();
saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0);
Comment on lines +48 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add protection against external DTD loading

While the entity expansion limits are good security measures, consider also disabling external DTD loading for consistent security with PatternRuleLoader. Additionally, consider adding error handling for the property settings.

Apply this diff to enhance security and robustness:

+    saxParser.getXMLReader().setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
     saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0);
     saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0);
     saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0);
saxParser.getXMLReader().setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
saxParser.getXMLReader().setProperty("jdk.xml.maxGeneralEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.totalEntitySizeLimit", 0);
saxParser.getXMLReader().setProperty("jdk.xml.entityExpansionLimit", 0);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cushon Thanks! Does coderabbit's suggestion make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the entity expansion limits are good security measures, consider also disabling external DTD loading

@cushon Thanks! Does coderabbit's suggestion make sense?

I'm not sure--I have context about the JDK change, but less about the requirements of this project.

I can try adding the change to disable external DTD loading while we're here, but it's somewhat unrelated. I think coderabbit's is a bit confused--this isn't setting entity expansion limits, it's actually disabling the limits (setting 0 means 'unlimited').

Are the XML files parsed by this code are trusted, either because they're the ones included in this project, or provided by a user of the library, rather than being arbitrary user input?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the XML files parsed by this code are trusted

Generally, yes. While there could be external entities, one (= a user who edits the XML files) would need to add them explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I went ahead and added the change to disable external DTD loading.


if (JLanguageTool.isCustomPasswordAuthenticatorUsed()) {
Tools.setPasswordAuthenticator();
Expand Down