Skip to content

Commit

Permalink
Modify rule S2755: Simplify how to fix it section (#4215)
Browse files Browse the repository at this point in the history
  • Loading branch information
hendrik-buchwald-sonarsource authored Sep 3, 2024
1 parent 6baf583 commit e5ae27a
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 49 deletions.
1 change: 1 addition & 0 deletions docs/header_names/allowed_framework_names.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
* Java Cryptography Extension
* Apache HttpClient
* Couchbase
* SAX
* Servlet
* Spring
* Spring Data MongoDB
Expand Down
4 changes: 2 additions & 2 deletions rules/S2755/java/how-to-fix-it/dom4j.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ include::../../common/fix/code-rationale.adoc[]

==== Noncompliant code example

[source,java,diff-id=1,diff-type=noncompliant]
[source,java,diff-id=21,diff-type=noncompliant]
----
import org.dom4j.io.SAXReader;
Expand All @@ -17,7 +17,7 @@ public void decode() {

==== Compliant solution

[source,java,diff-id=1,diff-type=compliant]
[source,java,diff-id=21,diff-type=compliant]
----
import org.dom4j.io.SAXReader;
Expand Down
83 changes: 37 additions & 46 deletions rules/S2755/java/how-to-fix-it/java-se.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -6,73 +6,64 @@ include::../../common/fix/code-rationale.adoc[]

==== Noncompliant code example

[source,java]
[source,java,diff-id=1,diff-type=noncompliant]
----
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); // Noncompliant
----

==== Compliant solution

Protection from XXE can be done in several different ways. Choose one depending
on how the affected parser object is used in your code.
import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilderFactory;
**1.** The first way is to completely disable `DOCTYPE` declarations:
public void decode() {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); // Noncompliant
}
----

[source, java]
[source,java,diff-id=2,diff-type=noncompliant]
----
// Applicable to:
// - DocumentBuilderFactory
// - SAXParserFactory
// - SchemaFactory
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
// For XMLInputFactory:
factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
import javax.xml.stream.XMLInputFactory;
public void decode() {
XMLInputFactory factory = XMLInputFactory.newInstance(); // Noncompliant
}
----

**2.** Disable external entity declarations completely:
==== Compliant solution

[source, java]
For `DocumentBuilderFactory`, `SAXParserFactory`, `TransformerFactory`, and
`SchemaFactory` set `XMLConstants.FEATURE_SECURE_PROCESSING` to `true`.

[source,java,diff-id=1,diff-type=compliant]
----
// Applicable to:
// - DocumentBuilderFactory
// - SAXParserFactory
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
// For XMLInputFactory:
factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE);
import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilderFactory;
public void decode() {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
}
----

**3.** Prohibit the use of all protocols by external entities:
For `XMLInputFactory` set `SUPPORT_DTD` to `false`.

[source, java]
[source,java,diff-id=2,diff-type=compliant]
----
// `setAttribute` variant, applicable to:
// - DocumentBuilderFactory
// - TransformerFactory
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
// `setProperty` variant, applicable to:
// - XMLInputFactory
// - SchemaFactory
factory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
factory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
// For SAXParserFactory, the prohibition is done on child objects:
SAXParser parser = factory.newSAXParser();
parser.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
parser.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
import javax.xml.stream.XMLInputFactory;
public void decode() {
XMLInputFactory factory = XMLInputFactory.newInstance();
factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
}
----

Other combinations of settings are secure, but in general, it is recommendable
to use the approaches shown here, as they are the most clear.

=== How does this work?

include::../../common/fix/xxe.adoc[]

=== Going the extra mile

==== Disable entity expansion

Specifically for `DocumentBuilderFactory`, it is possible to disable the entity
expansion. Note, however, that this does not prevent the retrieval of external
entities.
Expand Down
1 change: 0 additions & 1 deletion rules/S2755/java/how-to-fix-it/jdom2.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import org.jdom2.input.SAXBuilder;
public void decode() {
SAXBuilder builder = new SAXBuilder();
builder.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
builder.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
}
----

Expand Down
36 changes: 36 additions & 0 deletions rules/S2755/java/how-to-fix-it/sax.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
== How to fix it in SAX

=== Code examples

include::../../common/fix/code-rationale.adoc[]

==== Noncompliant code example

[source,java,diff-id=31,diff-type=noncompliant]
----
import org.xml.sax.XMLReader;
import org.xml.sax.helpers.XMLReaderFactory;
public void decode() {
XMLReader reader = XMLReaderFactory.createXMLReader(); // Noncompliant
}
----

==== Compliant solution

Set `disallow-doctype-decl` to `true`.

[source,java,diff-id=31,diff-type=compliant]
----
import org.xml.sax.XMLReader;
import org.xml.sax.helpers.XMLReaderFactory;
public void decode() {
XMLReader reader = XMLReaderFactory.createXMLReader();
reader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
}
----

=== How does this work?

include::../../common/fix/xxe.adoc[]
2 changes: 2 additions & 0 deletions rules/S2755/java/rule.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ include::how-to-fix-it/dom4j.adoc[]

include::how-to-fix-it/jdom2.adoc[]

include::how-to-fix-it/sax.adoc[]

== Resources

include::../common/resources/standards.adoc[]
Expand Down

0 comments on commit e5ae27a

Please sign in to comment.