diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 9db55ecea6..1870fd33b4 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -155,6 +155,8 @@ All existing rules have been updated to reflect these changes. If you have custo ### Fixed Issues +* all + * [#532](https://github.com/pmd/pmd/issues/532): \[core] security concerns on URL-based rulesets * apex * [#488](https://github.com/pmd/pmd/pull/488): \[apex] Use Apex lexer for CPD * [#489](https://github.com/pmd/pmd/pull/489): \[apex] Update Apex compiler diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java index abf2007223..70fac15de7 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java @@ -334,7 +334,7 @@ public class RuleSetFactory { throw new IllegalArgumentException( "Cannot parse a RuleSet from a non-external reference: <" + ruleSetReferenceId + ">."); } - DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder(); + DocumentBuilder builder = createDocumentBuilder(); InputSource inputSource; if (compatibilityFilter != null) { inputSource = new InputSource(compatibilityFilter.filterRuleSetFile(inputStream)); @@ -395,6 +395,42 @@ public class RuleSetFactory { } } + private DocumentBuilder createDocumentBuilder() throws ParserConfigurationException { + final DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + + try { + /* + * parser hardening + * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#JAXP_DocumentBuilderFactory.2C_SAXParserFactory_and_DOM4J + */ + // This is the PRIMARY defense. If DTDs (doctypes) are disallowed, almost all XML entity attacks are prevented + // Xerces 2 only - http://xerces.apache.org/xerces2-j/features.html#disallow-doctype-decl + dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + + // If you can't completely disable DTDs, then at least do the following: + // Xerces 1 - http://xerces.apache.org/xerces-j/features.html#external-general-entities + // Xerces 2 - http://xerces.apache.org/xerces2-j/features.html#external-general-entities + // JDK7+ - http://xml.org/sax/features/external-general-entities + dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); + + // Xerces 1 - http://xerces.apache.org/xerces-j/features.html#external-parameter-entities + // Xerces 2 - http://xerces.apache.org/xerces2-j/features.html#external-parameter-entities + // JDK7+ - http://xml.org/sax/features/external-parameter-entities + dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + + // Disable external DTDs as well + dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + + // and these as well, per Timothy Morgan's 2014 paper: "XML Schema, DTD, and Entity Attacks" + dbf.setXIncludeAware(false); + dbf.setExpandEntityReferences(false); + } catch (final ParserConfigurationException e) { + // an unsupported feature... too bad, but won't fail execution due to this + } + + return dbf.newDocumentBuilder(); + } + private static RuleSet classNotFoundProblem(Exception ex) { ex.printStackTrace(); throw new RuntimeException("Couldn't find the class " + ex.getMessage()); @@ -739,7 +775,7 @@ public class RuleSetFactory { private boolean containsRule(RuleSetReferenceId ruleSetReferenceId, String ruleName) { boolean found = false; try (InputStream ruleSet = ruleSetReferenceId.getInputStream(classLoader)) { - DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder(); + DocumentBuilder builder = createDocumentBuilder(); Document document = builder.parse(ruleSet); Element ruleSetElement = document.getDocumentElement(); @@ -862,7 +898,7 @@ public class RuleSetFactory { } // casting is not pretty but prevents the interface from having this method - PropertyDescriptor desc = ((AbstractPropertyDescriptorFactory) pdFactory).createExternalWith(values); + PropertyDescriptor desc = ((AbstractPropertyDescriptorFactory) pdFactory).createExternalWith(values); rule.definePropertyDescriptor(desc); setValue(rule, desc, strValue);