Skip to content

Commit 174ffae

Browse files
nahsranahsra
authored
Fix more XXE cases (#488)
Fixes cases of XXE where the tool points to the `XMLInputFactory#newInstance()` call.
1 parent bb3eced commit 174ffae

8 files changed

Lines changed: 215 additions & 71 deletions

File tree

‎core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
88
import io.codemodder.remediation.GenericRemediationMetadata;
99
import io.codemodder.remediation.Remediator;
10-
import io.codemodder.remediation.xxe.XXEIntermediateXMLStreamReaderRemediator;
10+
import io.codemodder.remediation.xxe.XMLStreamReaderIntermediateRemediator;
1111
import java.util.Optional;
1212
import javax.inject.Inject;
1313

@@ -24,7 +24,7 @@ public final class CodeQLXXECodemod extends CodeQLRemediationCodemod {
2424
@Inject
2525
public CodeQLXXECodemod(@ProvidedCodeQLScan(ruleId = "java/xxe") final RuleSarif sarif) {
2626
super(GenericRemediationMetadata.XXE.reporter(), sarif);
27-
this.remediator = new XXEIntermediateXMLStreamReaderRemediator<>();
27+
this.remediator = new XMLStreamReaderIntermediateRemediator<>();
2828
}
2929

3030
@Override

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
import java.util.Optional;
2222

2323
/**
24-
* Fix strategy for XXE vulnerabilities anchored to the TransformerParser newInstance() calls. Finds
24+
* Fix strategy for XXE vulnerabilities anchored to the TransformerParser.newInstance() calls. Finds
2525
* the parser's declaration and add statements disabling external entities and features.
2626
*/
2727
final class TransformerFactoryAtCreationFixStrategy extends MatchAndFixStrategy {
@@ -70,7 +70,7 @@ public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
7070
}
7171

7272
/**
73-
* Matches against TransformerFactory.newInstance() calls
73+
* Matches against XMLInputFactory.newInstance() calls.
7474
*
7575
* @param node
7676
* @return

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLInputFactoryAtNewInstanceFixStrategy.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
package io.codemodder.remediation.xxe;
2+
3+
import com.github.javaparser.ast.CompilationUnit;
4+
import com.github.javaparser.ast.Node;
5+
import com.github.javaparser.ast.body.VariableDeclarator;
6+
import com.github.javaparser.ast.expr.Expression;
7+
import com.github.javaparser.ast.expr.MethodCallExpr;
8+
import com.github.javaparser.ast.stmt.Statement;
9+
import io.codemodder.ast.ASTs;
10+
import io.codemodder.remediation.SuccessOrReason;
11+
import java.util.List;
12+
import java.util.Optional;
13+
14+
final class XMLInputFactoryAtNewInstanceFixStrategy
15+
extends io.codemodder.remediation.MatchAndFixStrategy {
16+
17+
/**
18+
* Matches (XmlInputFactory | var xif) y = XMLInputFactory.newInstance(), where the node is the
19+
* newDocumentBuilder call.
20+
*
21+
* @param node the node to match
22+
* @return true if this is an assignment to an XMLInputFactory
23+
*/
24+
@Override
25+
public boolean match(final Node node) {
26+
Optional<MethodCallExpr> method =
27+
ASTs.isInitializedToType(node, "newInstance", List.of("var", "XMLInputFactory"));
28+
if (method.isEmpty()) {
29+
return false;
30+
}
31+
MethodCallExpr newInstance = method.get();
32+
Optional<Expression> scope = newInstance.getScope();
33+
if (scope.isEmpty()) {
34+
return false;
35+
}
36+
return scope.get().toString().equals("XMLInputFactory");
37+
}
38+
39+
@Override
40+
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
41+
MethodCallExpr newInstance = (MethodCallExpr) node;
42+
Optional<VariableDeclarator> initExpr = ASTs.isInitExpr(newInstance);
43+
if (initExpr.isEmpty()) {
44+
return SuccessOrReason.reason("Not an initialization expression");
45+
}
46+
VariableDeclarator xmlInputFactoryVariable = initExpr.get();
47+
Optional<Statement> variableDeclarationStmtRef =
48+
xmlInputFactoryVariable.findAncestor(Statement.class);
49+
50+
if (variableDeclarationStmtRef.isEmpty()) {
51+
return SuccessOrReason.reason("Not assigned as part of statement");
52+
}
53+
54+
Statement stmt = variableDeclarationStmtRef.get();
55+
return XMLFixBuilder.addXMLInputFactoryDisablingStatement(
56+
xmlInputFactoryVariable.getNameAsExpression(), stmt, false);
57+
}
58+
}

framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java renamed to framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateFixStrategy.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
* Fix strategy for XXE vulnerabilities anchored to arguments of a XMLStreamReader calls. Finds the
1616
* parser's declaration and add statements disabling external entities and features.
1717
*/
18-
final class XXEIntermediateXMLStreamReaderFixStrategy implements RemediationStrategy {
18+
final class XMLStreamReaderIntermediateFixStrategy implements RemediationStrategy {
1919

2020
@Override
2121
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
@@ -27,7 +27,7 @@ public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
2727
if (maybeCall.isEmpty()) {
2828
return SuccessOrReason.reason("Not a method call");
2929
}
30-
// get the xmlstreamreader scope variable
30+
// get the XMLStreamReader scope variable
3131
MethodCallExpr createXMLStreamReaderCall = maybeCall.get();
3232
Expression xmlStreamReaderScope = createXMLStreamReaderCall.getScope().get();
3333

framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java renamed to framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLStreamReaderIntermediateRemediator.java

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,12 @@
1313
import java.util.Optional;
1414
import java.util.function.Function;
1515

16-
public class XXEIntermediateXMLStreamReaderRemediator<T> implements Remediator<T> {
16+
/** Remediator for XXE vulnerabilities anchored to the XMLStreamReader calls. */
17+
public final class XMLStreamReaderIntermediateRemediator<T> implements Remediator<T> {
1718

1819
private final SearcherStrategyRemediator<T> searchStrategyRemediator;
1920

20-
public XXEIntermediateXMLStreamReaderRemediator() {
21+
public XMLStreamReaderIntermediateRemediator() {
2122
this.searchStrategyRemediator =
2223
new SearcherStrategyRemediator.Builder<T>()
2324
.withSearcherStrategyPair(
@@ -30,20 +31,20 @@ public XXEIntermediateXMLStreamReaderRemediator() {
3031
.filter(Node::hasScope)
3132
.isPresent())
3233
.build(),
33-
new XXEIntermediateXMLStreamReaderFixStrategy())
34+
new XMLStreamReaderIntermediateFixStrategy())
3435
.build();
3536
}
3637

3738
@Override
3839
public CodemodFileScanningResult remediateAll(
39-
CompilationUnit cu,
40-
String path,
41-
DetectorRule detectorRule,
42-
Collection<T> findingsForPath,
43-
Function<T, String> findingIdExtractor,
44-
Function<T, Integer> findingStartLineExtractor,
45-
Function<T, Optional<Integer>> findingEndLineExtractor,
46-
Function<T, Optional<Integer>> findingColumnExtractor) {
40+
final CompilationUnit cu,
41+
final String path,
42+
final DetectorRule detectorRule,
43+
final Collection<T> findingsForPath,
44+
final Function<T, String> findingIdExtractor,
45+
final Function<T, Integer> findingStartLineExtractor,
46+
final Function<T, Optional<Integer>> findingEndLineExtractor,
47+
final Function<T, Optional<Integer>> findingColumnExtractor) {
4748
return searchStrategyRemediator.remediateAll(
4849
cu,
4950
path,

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ public XXERemediator(final NodePositionMatcher matcher) {
2929
.withMatchAndFixStrategyAndNodeMatcher(
3030
new TransformerFactoryAtCreationFixStrategy(), matcher)
3131
.withMatchAndFixStrategyAndNodeMatcher(new XMLReaderAtParseFixStrategy(), matcher)
32+
.withMatchAndFixStrategyAndNodeMatcher(
33+
new XMLInputFactoryAtNewInstanceFixStrategy(), matcher)
3234
.build();
3335
}
3436

framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java renamed to framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXMLStreamReaderIntermediateRemediatorTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,14 @@
1414
import org.junit.jupiter.api.BeforeEach;
1515
import org.junit.jupiter.api.Test;
1616

17-
final class DefaultXXEIntermediateXMLStreamReaderRemediatorTest {
17+
final class DefaultXMLStreamReaderIntermediateRemediatorTest {
1818

19-
private XXEIntermediateXMLStreamReaderRemediator<Object> remediator;
19+
private XMLStreamReaderIntermediateRemediator<Object> remediator;
2020
private DetectorRule rule;
2121

2222
@BeforeEach
2323
void setup() {
24-
remediator = new XXEIntermediateXMLStreamReaderRemediator<>();
24+
remediator = new XMLStreamReaderIntermediateRemediator<>();
2525
rule = new DetectorRule("xxe", "XXE Fixed At XMLStreamReader", null);
2626
}
2727

0 commit comments

Comments
 (0)