Prevent XXE vulnerabilities #258
Conversation
There was a problem hiding this comment.
Thanks @clausnagel. Very important fix. I have only one remark from my review.
| factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); | ||
| factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); | ||
| factory.setFeature("http://xml.org/sax/features/external-general-entities", false); | ||
| factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); |
There was a problem hiding this comment.
Is the following code also needed? see here
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);There was a problem hiding this comment.
Not sure whether it's actually needed. I've found different examples on the web, and some even just use factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);.
I am not against adding it. It's just that not all properties are supported by all XML implementations in every use case. So, could you please check that we do not get property-is-not-supported exceptions when adding it (e.g. when creating a validating or a non-validating reader)?
There was a problem hiding this comment.
I haven't got exceptions after adding it.
This PR adds necessary code to prevent XXE vulnerabilities when parsing XML files.
XML files are not just CityGML input files. But also the config file of the Importer/Exporter, XML query expressions, XSD schema files, and XSLT stylesheets are XML-based. So, various classes are affected by this PR.
ConfigUtilhad to be moved from impexp-config to impexp-util to be able to use the newSecureXMLProcessorsclass.