Skip to content

Commit 3981995

Browse files
authored
Refactored XXE remediation (#391)
This change introduces more features into XXE protection. * Refactored to make it easier to extend and test * Added more protection cases * Added more tests * Added a generic reporter for cases where you could fix from multiple APIs
1 parent 92a218c commit 3981995

17 files changed

+535
-206
lines changed

core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import io.codemodder.providers.sonar.ProvidedSonarScan;
77
import io.codemodder.providers.sonar.RuleIssue;
88
import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
9+
import io.codemodder.remediation.GenericVulnerabilityReporterStrategies;
910
import io.codemodder.remediation.xxe.XXEJavaRemediatorStrategy;
1011
import io.codemodder.sonar.model.Issue;
1112
import io.codemodder.sonar.model.SonarFinding;
@@ -25,9 +26,7 @@ public final class SonarXXECodemod extends SonarRemediatingJavaParserChanger {
2526

2627
@Inject
2728
public SonarXXECodemod(@ProvidedSonarScan(ruleId = "java:S2755") final RuleIssue issues) {
28-
super(
29-
CodemodReporterStrategy.fromClasspathDirectory(SonarXXECodemod.class, "xxe-generic"),
30-
issues);
29+
super(GenericVulnerabilityReporterStrategies.xxeReporterStrategy, issues);
3130
this.issues = Objects.requireNonNull(issues);
3231
this.remediationStrategy = XXEJavaRemediatorStrategy.DEFAULT;
3332
}

core-codemods/src/test/java/io/codemodder/remediation/xxe/DefaultXXEJavaRemediatorStrategyTest.java

Lines changed: 0 additions & 194 deletions
This file was deleted.
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package io.codemodder.remediation;
2+
3+
import io.codemodder.CodeChanger;
4+
import io.codemodder.CodemodReporterStrategy;
5+
6+
/** Reporter strategies that are useful without mentioning specific APIs. */
7+
public final class GenericVulnerabilityReporterStrategies {
8+
9+
private GenericVulnerabilityReporterStrategies() {}
10+
11+
public static final CodemodReporterStrategy xxeReporterStrategy =
12+
CodemodReporterStrategy.fromClasspathDirectory(CodeChanger.class, "/reports/xxe-generic");
13+
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ final class DefaultXXEJavaRemediatorStrategy implements XXEJavaRemediatorStrateg
1818
this.fixers =
1919
List.of(
2020
new DocumentBuilderFactoryAndSAXParserAtCreationFixer(),
21+
new DocumentBuilderFactoryAtParseFixer(),
2122
new TransformerFactoryAtCreationFixer(),
2223
new XMLReaderAtParseFixer());
2324
}
@@ -41,7 +42,7 @@ public <T> CodemodFileScanningResult remediateAll(
4142
int line = getLine.apply(issue);
4243
Integer column = getColumn.apply(issue);
4344
for (XXEFixer fixer : fixers) {
44-
XXEFixAttempt fixAttempt = fixer.tryFix(issue, line, column, cu);
45+
XXEFixAttempt fixAttempt = fixer.tryFix(line, column, cu);
4546
if (!fixAttempt.isResponsibleFixer()) {
4647
continue;
4748
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@
2020
final class DocumentBuilderFactoryAndSAXParserAtCreationFixer implements XXEFixer {
2121

2222
@Override
23-
public <T> XXEFixAttempt tryFix(
24-
final T issue, final int line, final Integer column, CompilationUnit cu) {
23+
public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) {
2524
List<MethodCallExpr> candidateMethods =
2625
ASTs.findMethodCallsWhichAreAssignedToType(
2726
cu, line, column, "newInstance", List.of("DocumentBuilderFactory", "SAXParserFactory"));
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
package io.codemodder.remediation.xxe;
2+
3+
import static io.codemodder.remediation.RemediationMessages.multipleCallsFound;
4+
import static io.codemodder.remediation.RemediationMessages.noCallsAtThatLocation;
5+
6+
import com.github.javaparser.Position;
7+
import com.github.javaparser.ast.CompilationUnit;
8+
import com.github.javaparser.ast.Node;
9+
import com.github.javaparser.ast.body.MethodDeclaration;
10+
import com.github.javaparser.ast.body.Parameter;
11+
import com.github.javaparser.ast.body.VariableDeclarator;
12+
import com.github.javaparser.ast.expr.Expression;
13+
import com.github.javaparser.ast.expr.MethodCallExpr;
14+
import com.github.javaparser.ast.expr.NameExpr;
15+
import com.github.javaparser.ast.nodeTypes.NodeWithType;
16+
import com.github.javaparser.ast.stmt.Statement;
17+
import io.codemodder.ast.ASTs;
18+
import java.util.List;
19+
import java.util.Optional;
20+
import java.util.Set;
21+
22+
/** Fix XXEs that are reported at the DocumentBuilder.parse() call locations. */
23+
final class DocumentBuilderFactoryAtParseFixer implements XXEFixer {
24+
25+
@Override
26+
public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) {
27+
List<MethodCallExpr> candidateMethods =
28+
cu.findAll(MethodCallExpr.class).stream()
29+
.filter(m -> "parse".equals(m.getNameAsString()))
30+
.filter(m -> m.getScope().isPresent())
31+
.filter(m -> m.getScope().get().isNameExpr())
32+
.filter(m -> m.getRange().isPresent() && m.getRange().get().begin.line == line)
33+
.toList();
34+
35+
if (candidateMethods.size() > 1 && column != null) {
36+
candidateMethods =
37+
candidateMethods.stream()
38+
.filter(m -> m.getRange().get().contains(new Position(line, column)))
39+
.toList();
40+
}
41+
42+
if (candidateMethods.isEmpty()) {
43+
return new XXEFixAttempt(false, false, noCallsAtThatLocation);
44+
} else if (candidateMethods.size() > 1) {
45+
return new XXEFixAttempt(false, false, multipleCallsFound);
46+
}
47+
48+
// find the variable that we're calling parse() on
49+
MethodCallExpr parseCall = candidateMethods.get(0);
50+
51+
// we know from the filter that it's scope is a name expression
52+
NameExpr documentBuilder = parseCall.getScope().get().asNameExpr();
53+
54+
// so, we need to see if we can see the DocumentBuilderFactory from whence this came
55+
Optional<MethodDeclaration> methodBody = ASTs.findMethodBodyFrom(documentBuilder);
56+
if (methodBody.isEmpty()) {
57+
return new XXEFixAttempt(false, false, "No method body found for the call");
58+
}
59+
60+
Optional<Node> documentBuilderAssignment =
61+
ASTs.findNonCallableSimpleNameSource(documentBuilder.getName());
62+
if (documentBuilderAssignment.isEmpty()) {
63+
return new XXEFixAttempt(false, false, "No assignment found for the DocumentBuilder");
64+
}
65+
Node parserAssignmentNode = documentBuilderAssignment.get();
66+
if (!(parserAssignmentNode instanceof NodeWithType<?, ?>)) {
67+
return new XXEFixAttempt(false, false, "Unknown DocumentBuilder assignment");
68+
}
69+
String parserType = ((NodeWithType<?, ?>) parserAssignmentNode).getTypeAsString();
70+
if (!Set.of("DocumentBuilder", "javax.xml.parsers.DocumentBuilder").contains(parserType)) {
71+
return new XXEFixAttempt(false, false, "Parsing method is not a DocumentBuilder");
72+
}
73+
74+
if (parserAssignmentNode instanceof VariableDeclarator dbVar) {
75+
Optional<Expression> initializer = dbVar.getInitializer();
76+
if (initializer.isEmpty()) {
77+
return new XXEFixAttempt(
78+
false, false, "DocumentBuilder was not initialized in an expected way");
79+
} else if (!(initializer.get() instanceof MethodCallExpr)) {
80+
return new XXEFixAttempt(
81+
false, false, "DocumentBuilder was not initialized with a factory call");
82+
}
83+
MethodCallExpr potentialFactoryCall = (MethodCallExpr) initializer.get();
84+
if (!"newDocumentBuilder".equals(potentialFactoryCall.getNameAsString())) {
85+
return new XXEFixAttempt(
86+
false, false, "DocumentBuilder was initialized with newDocumentBuilder");
87+
}
88+
if (potentialFactoryCall.getScope().isEmpty()) {
89+
return new XXEFixAttempt(
90+
true, false, "DocumentBuilder was initialized with a factory call without a scope");
91+
}
92+
if (!(potentialFactoryCall.getScope().get() instanceof NameExpr)) {
93+
return new XXEFixAttempt(
94+
false,
95+
false,
96+
"DocumentBuilder was initialized with a factory call with a non-name scope");
97+
}
98+
NameExpr factoryNameExpr = (NameExpr) potentialFactoryCall.getScope().get();
99+
Optional<Statement> newDocumentBuilderStatement = ASTs.findParentStatementFrom(dbVar);
100+
if (newDocumentBuilderStatement.isEmpty()) {
101+
return new XXEFixAttempt(
102+
false,
103+
false,
104+
"DocumentBuilder was initialized with a factory call without a statement");
105+
}
106+
return XMLFeatures.addFeatureDisablingStatements(
107+
cu, factoryNameExpr, newDocumentBuilderStatement.get(), true);
108+
} else if (parserAssignmentNode instanceof Parameter) {
109+
return new XXEFixAttempt(true, false, "DocumentBuilder came from outside the method scope");
110+
}
111+
112+
return new XXEFixAttempt(true, false, "DocumentBuilder was not initialized in an expected way");
113+
}
114+
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,7 @@
2323
final class TransformerFactoryAtCreationFixer implements XXEFixer {
2424

2525
@Override
26-
public <T> XXEFixAttempt tryFix(
27-
final T issue, final int line, final Integer column, CompilationUnit cu) {
26+
public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) {
2827
List<MethodCallExpr> candidateMethods =
2928
ASTs.findMethodCallsWhichAreAssignedToType(
3029
cu, line, column, "newInstance", List.of("TransformerFactory"));

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@
2020
final class XMLReaderAtParseFixer implements XXEFixer {
2121

2222
@Override
23-
public <T> XXEFixAttempt tryFix(
24-
final T issue, final int line, final Integer column, final CompilationUnit cu) {
23+
public XXEFixAttempt tryFix(final int line, final Integer column, final CompilationUnit cu) {
2524
List<MethodCallExpr> candidateMethods =
2625
cu.findAll(MethodCallExpr.class).stream()
2726
.filter(m -> "parse".equals(m.getNameAsString()))

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,5 +6,5 @@
66
interface XXEFixer {
77

88
/** A provider (for a given XML API) attempts to fix the given issue. */
9-
<T> XXEFixAttempt tryFix(T issue, int line, Integer column, CompilationUnit cu);
9+
XXEFixAttempt tryFix(int line, Integer column, CompilationUnit cu);
1010
}

0 commit comments

Comments
 (0)