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

Sonar violation: Disable XML external entity (XXE) processing #7468

Closed
romani opened this issue Jan 13, 2020 · 7 comments
Closed

Sonar violation: Disable XML external entity (XXE) processing #7468

romani opened this issue Jan 13, 2020 · 7 comments
Milestone

Comments

@romani
Copy link
Member

@romani romani commented Jan 13, 2020

https://sonarcloud.io/project/issues?id=org.checkstyle%3Acheckstyle&issues=AW9t2w41YD2QG1pPXIVJ&open=AW9t2w41YD2QG1pPXIVJ

Vulnerability at src/.../tools/checkstyle/XmlLoader.java
Disable XML external entity (XXE) processing.

All details of such rule - https://rules.sonarsource.com/java/RSPEC-2755

Reply from Security expert:

Hi Roman,
The next line setFeaturesBySystemProperty does disable the loading of external entities when the system property is not configured by the user.

public static void setFeaturesBySystemProperty(SAXParserFactory factory)
throws SAXException, ParserConfigurationException {
final boolean enableExternalDtdLoad = Boolean.parseBoolean(
System.getProperty(ENABLE_EXTERNAL_DTD_LOAD, "false"));
factory.setFeature(LOAD_EXTERNAL_DTD, enableExternalDtdLoad);
factory.setFeature(EXTERNAL_GENERAL_ENTITIES, enableExternalDtdLoad);
}

I would validate that you are using all of the prevention methods suggested in this document here:
https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
The cases you need to be concerned with are any involving the use of the SAXParserFactory.
If you find any cases you are missing, then you may be vulnerable. In which case, please let me know.
Cheers,
Jonathan Leitschuh

TODO:
We need to investigate this to make sure if we are vulnerable

@romani
Copy link
Member Author

@romani romani commented Jan 19, 2020

from links of Sonar description (same link provide us Jonathan), https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#jaxb-unmarshaller

here is clear example that we do correct disablement BUT there 3 features and we use only 2.
spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); is missed on our side.

@pbludov
Copy link
Member

@pbludov pbludov commented Jan 20, 2020

Fix is merged.

@romani can this issue be a breaking change?

@pbludov pbludov closed this Jan 20, 2020
@romani
Copy link
Member Author

@romani romani commented Jan 20, 2020

It should not, but it might, let's keep it misc

@JLLeitschuh
Copy link

@JLLeitschuh JLLeitschuh commented Jan 21, 2020

Does this need a CVE assigned to it?

@romani
Copy link
Member Author

@romani romani commented Jan 21, 2020

I do not know how to reproduce it, so I am not sure that CVE need to be created.
If you can reproduce security, lets create CVE .

@JLLeitschuh
Copy link

@JLLeitschuh JLLeitschuh commented Jan 22, 2020

Hey @romani,

I've got confirmation from someone that there is indeed a vuln here.
Can you open an advisory here: https://github.com/checkstyle/checkstyle/security/advisories

Please add myself and these two security researchers from Snyk.

@romani
Copy link
Member Author

@romani romani commented Jan 22, 2020

Can you open an advisory here

Done.
GHSA-763g-fqq7-48wg

@romani romani added bug and removed miscellaneous labels Jan 25, 2020
huntertran added a commit to huntertran/soen6441-riskgame that referenced this issue Feb 1, 2020
razo7 added a commit to razo7/Nap-Hadoop-2.9.1 that referenced this issue Sep 20, 2021
com.puppycrawl.tools:checkstyle
Upgrade com.puppycrawl.tools:checkstyle to version 8.29 or later. 
https://cwe.mitre.org/data/definitions/611.html
checkstyle/checkstyle#7468
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants