Skip to content

Commit 35ba4a8

Browse files
nahsranahsra
authored
Added XXE remediation at intermediate events (#433)
Also added tests, a. common reporter, etc.
1 parent 848ff93 commit 35ba4a8

File tree

10 files changed

+298
-3
lines changed

10 files changed

+298
-3
lines changed

‎framework/codemodder-base/src/main/java/io/codemodder/CodemodFileScanningResult.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,12 @@ static CodemodFileScanningResult from(
1313
return new CodemodFileScanningResult() {
1414
@Override
1515
public List<CodemodChange> changes() {
16-
return changes;
16+
return List.copyOf(changes);
1717
}
1818

1919
@Override
2020
public List<UnfixedFinding> unfixedFindings() {
21-
return unfixedFindings;
21+
return List.copyOf(unfixedFindings);
2222
}
2323
};
2424
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ public enum GenericRemediationMetadata {
1010
JNDI("jndi-injection"),
1111
HEADER_INJECTION("header-injection"),
1212
REFLECTION_INJECTION("reflection-injection"),
13+
DESERIALIZATION("java-deserialization"),
1314
SQL_INJECTION("sql-injection");
1415

1516
private final CodemodReporterStrategy reporter;

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediator.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,8 @@ public <T> CodemodFileScanningResult remediateAll(
123123
.forEach(unfixedFindings::add);
124124
}
125125
}
126+
127+
unfixedFindings.addAll(results.unfixableFindings());
126128
return CodemodFileScanningResult.from(changes, unfixedFindings);
127129
}
128130

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

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
package io.codemodder.remediation.xxe;
2+
3+
import com.github.javaparser.ast.CompilationUnit;
4+
import com.github.javaparser.ast.expr.Expression;
5+
import com.github.javaparser.ast.expr.MethodCallExpr;
6+
import com.github.javaparser.ast.expr.NameExpr;
7+
import com.github.javaparser.ast.stmt.Statement;
8+
import io.codemodder.CodemodChange;
9+
import io.codemodder.CodemodFileScanningResult;
10+
import io.codemodder.codetf.DetectorRule;
11+
import io.codemodder.codetf.FixedFinding;
12+
import io.codemodder.codetf.UnfixedFinding;
13+
import io.codemodder.remediation.FixCandidate;
14+
import io.codemodder.remediation.FixCandidateSearchResults;
15+
import io.codemodder.remediation.FixCandidateSearcher;
16+
import java.util.ArrayList;
17+
import java.util.List;
18+
import java.util.Optional;
19+
import java.util.function.Function;
20+
21+
final class DefaultXXEIntermediateXMLStreamReaderRemediator
22+
implements XXEIntermediateXMLStreamReaderRemediator {
23+
24+
@Override
25+
public <T> CodemodFileScanningResult remediateAll(
26+
final CompilationUnit cu,
27+
final String path,
28+
final DetectorRule detectorRule,
29+
final List<T> issuesForFile,
30+
final Function<T, String> getKey,
31+
final Function<T, Integer> getLine,
32+
final Function<T, Integer> getColumn) {
33+
FixCandidateSearcher<T> searcher =
34+
new FixCandidateSearcher.Builder<T>()
35+
.withMethodName("createXMLStreamReader")
36+
.withMatcher(mce -> mce.getScope().isPresent())
37+
.withMatcher(mce -> mce.getArguments().isNonEmpty())
38+
.build();
39+
40+
FixCandidateSearchResults<T> results =
41+
searcher.search(cu, path, detectorRule, issuesForFile, getKey, getLine, getColumn);
42+
43+
List<CodemodChange> changes = new ArrayList<>();
44+
List<UnfixedFinding> unfixedFindings = new ArrayList<>();
45+
46+
for (FixCandidate<T> fixCandidate : results.fixCandidates()) {
47+
List<T> issues = fixCandidate.issues();
48+
49+
// get the xmlstreamreader scope variable
50+
MethodCallExpr createXMLStreamReaderCall = fixCandidate.methodCall();
51+
Expression xmlStreamReaderScope = createXMLStreamReaderCall.getScope().get();
52+
// make sure its a variable
53+
if (!xmlStreamReaderScope.isNameExpr()) {
54+
issues.stream()
55+
.map(
56+
issue ->
57+
new UnfixedFinding(
58+
getKey.apply(issue),
59+
detectorRule,
60+
path,
61+
getLine.apply(issue),
62+
"Could not find the XMLStreamReader variable"))
63+
.forEach(unfixedFindings::add);
64+
continue;
65+
}
66+
// get the variable
67+
NameExpr xmlStreamReaderVariable = xmlStreamReaderScope.asNameExpr();
68+
// get the JavaParser statement that contains the create call
69+
Optional<Statement> ancestorStatement =
70+
createXMLStreamReaderCall.findAncestor(Statement.class);
71+
if (ancestorStatement.isEmpty()) {
72+
issues.stream()
73+
.map(
74+
issue ->
75+
new UnfixedFinding(
76+
getKey.apply(issue),
77+
detectorRule,
78+
path,
79+
getLine.apply(issue),
80+
"Could not find the statement containing the XMLStreamReader creation"))
81+
.forEach(unfixedFindings::add);
82+
continue;
83+
}
84+
Statement stmt = ancestorStatement.get();
85+
XMLFeatures.addXMLInputFactoryDisablingStatement(xmlStreamReaderVariable, stmt, true);
86+
issues.stream()
87+
.map(
88+
issue ->
89+
CodemodChange.from(
90+
getLine.apply(issue), new FixedFinding(getKey.apply(issue), detectorRule)))
91+
.forEach(changes::add);
92+
}
93+
94+
unfixedFindings.addAll(results.unfixableFindings());
95+
return CodemodFileScanningResult.from(changes, unfixedFindings);
96+
}
97+
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,37 @@ static MethodCallExpr createGeneralEntityDisablingCall(final NameExpr nameExpr)
3838
new BooleanLiteralExpr(false)));
3939
}
4040

41+
/**
42+
* Creates a call for {@link javax.xml.stream.XMLInputFactory#setProperty(java.lang.String,
43+
* java.lang.Object)} for disabling.
44+
*/
45+
static XXEFixAttempt addXMLInputFactoryDisablingStatement(
46+
final NameExpr variable, final Statement statementToInjectAround, final boolean before) {
47+
MethodCallExpr call =
48+
new MethodCallExpr(
49+
variable,
50+
"setProperty",
51+
NodeList.nodeList(
52+
new StringLiteralExpr("javax.xml.stream.isSupportingExternalEntities"),
53+
new BooleanLiteralExpr(false)));
54+
55+
Optional<BlockStmt> block = ASTs.findBlockStatementFrom(statementToInjectAround);
56+
if (block.isEmpty()) {
57+
return new XXEFixAttempt(true, false, "No block statement found for newFactory() call");
58+
}
59+
60+
BlockStmt blockStmt = block.get();
61+
NodeList<Statement> existingStatements = blockStmt.getStatements();
62+
int index = existingStatements.indexOf(statementToInjectAround);
63+
if (!before) {
64+
index++;
65+
}
66+
67+
Statement fixStatement = new ExpressionStmt(call);
68+
existingStatements.add(index, fixStatement);
69+
return new XXEFixAttempt(true, true, null);
70+
}
71+
4172
static XXEFixAttempt addFeatureDisablingStatements(
4273
final NameExpr variable, final Statement statementToInjectAround, final boolean before) {
4374
Optional<BlockStmt> block = ASTs.findBlockStatementFrom(statementToInjectAround);

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
package io.codemodder.remediation.xxe;
2+
3+
import com.github.javaparser.ast.CompilationUnit;
4+
import io.codemodder.CodemodFileScanningResult;
5+
import io.codemodder.codetf.DetectorRule;
6+
import java.util.List;
7+
import java.util.function.Function;
8+
9+
/**
10+
* Strategy for remediating XXE vulnerabilities at an intermediate step in {@link
11+
* javax.xml.stream.XMLInputFactory#createXMLStreamReader}.
12+
*/
13+
public interface XXEIntermediateXMLStreamReaderRemediator {
14+
15+
/** A default implementation for callers. */
16+
XXEIntermediateXMLStreamReaderRemediator DEFAULT =
17+
new DefaultXXEIntermediateXMLStreamReaderRemediator();
18+
19+
/** Remediate all XXE vulnerabilities in the given compilation unit. */
20+
<T> CodemodFileScanningResult remediateAll(
21+
CompilationUnit cu,
22+
String path,
23+
DetectorRule detectorRule,
24+
List<T> issuesForFile,
25+
Function<T, String> getKey,
26+
Function<T, Integer> getLine,
27+
Function<T, Integer> getColumn);
28+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import java.util.List;
77
import java.util.function.Function;
88

9-
/** Strategy for remediating XXE vulnerabilities using Java's DOM parser. */
9+
/** Strategy for remediating XXE vulnerabilities at the sink for multiple parser APIs. */
1010
public interface XXERemediator {
1111

1212
/** A default implementation for callers. */

‎framework/codemodder-base/src/main/resources/generic-remediation-reports/java-deserialization/description.md

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
This change fixes Java deserialization vulnerabilities. Even a simple operation like an object deserialization is an opportunity to yield control of your system to an attacker. In fact, without specific, non-default protections, any object deserialization call can lead to arbitrary code execution. The JavaDoc [now even says](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/ObjectInputFilter.html):
2+
3+
> Deserialization of untrusted data is inherently dangerous and should be avoided.
4+
5+
Let's discuss the attack. In Java, types can customize how they should be deserialized by specifying a `readObject()` method like this real example from an [old version of Spring](https://github.com/spring-projects/spring-framework/blob/4.0.x/spring-core/src/main/java/org/springframework/core/SerializableTypeWrapper.java#L404):
6+
7+
```java
8+
static class MethodInvokeTypeProvider implements TypeProvider {
9+
private final TypeProvider provider;
10+
private final String methodName;
11+
12+
private void readObject(ObjectInputStream inputStream) {
13+
inputStream.defaultReadObject();
14+
Method method = ReflectionUtils.findMethod(
15+
this.provider.getType().getClass(),
16+
this.methodName
17+
);
18+
this.result = ReflectionUtils.invokeMethod(method,this.provider.getType());
19+
}
20+
}
21+
```
22+
23+
Reflecting on this code reveals a terrifying conclusion. If an attacker presents this object to be deserialized by your app, the runtime will take a class and a method name from the attacker and then call them. Note that an attacker can provide any serliazed type -- it doesn't have to be the one you're expecting, and it will still deserialize.
24+
25+
Attackers can repurpose the logic of selected types within the Java classpath (called "gadgets") and chain them together to achieve arbitrary remote code execution. There are a limited number of publicly known gadgets that can be used for attack, and our change simply inserts an [ObjectInputFilter](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/ObjectInputStream.html#setObjectInputFilter(java.io.ObjectInputFilter)) into the `ObjectInputStream` to prevent them from being used.
26+
27+
```diff
28+
+ import io.github.pixee.security.ObjectInputFilters.createSafeObjectInputStream;
29+
- ObjectInputStream ois = new ObjectInputStream(is);
30+
+ ObjectInputStream ois = createSafeObjectInputStream(is);
31+
AcmeObject acme = (AcmeObject)ois.readObject();
32+
```
33+
34+
This is a tough vulnerability class to understand, but it is deadly serious. It offers the highest impact possible (remote code execution), it's a common vulnerability (it's in the OWASP Top 10), and exploitation is easy enough that automated exploitation is possible. It's best to remove deserialization entirely, but our protections is effective against all known exploitation strategies.

‎framework/codemodder-base/src/main/resources/generic-remediation-reports/java-deserialization/report.json

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
"summary" : "Introduced protections against deserialization attacks",
3+
"control" : "https://github.com/pixee/java-security-toolkit/blob/main/src/main/java/io/github/pixee/security/ObjectInputFilters.java",
4+
"change" : "Hardened the deserialization call by introducing a filter that prevents known malicious gadgets from executing arbitrary code",
5+
"references" : ["https://cheatsheetseries.owasp.org/cheatsheets/Deserialization_Cheat_Sheet.html", "https://portswigger.net/web-security/deserialization/exploiting"],
6+
"faqs" : [
7+
{
8+
"question" : "Why does this codemod require a Pixee dependency?",
9+
"answer" : "We always prefer to use existing controls built into Java, or a control from a well-known and trusted community dependency. However, older versions of Java don't support fine-grained deserialization filter controls, and there are no community-trusted controls."
10+
}
11+
]
12+
}

‎framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
package io.codemodder.remediation.xxe;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
5+
import com.github.javaparser.StaticJavaParser;
6+
import com.github.javaparser.ast.CompilationUnit;
7+
import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter;
8+
import io.codemodder.CodemodChange;
9+
import io.codemodder.CodemodFileScanningResult;
10+
import io.codemodder.codetf.DetectorRule;
11+
import io.codemodder.codetf.FixedFinding;
12+
import java.util.List;
13+
import org.junit.jupiter.api.BeforeEach;
14+
import org.junit.jupiter.api.Test;
15+
16+
final class DefaultXXEIntermediateXMLStreamReaderRemediatorTest {
17+
18+
private DefaultXXEIntermediateXMLStreamReaderRemediator remediator;
19+
private DetectorRule rule;
20+
21+
@BeforeEach
22+
void setup() {
23+
remediator = new DefaultXXEIntermediateXMLStreamReaderRemediator();
24+
rule = new DetectorRule("xxe", "XXE Fixed At XMLStreamReader", null);
25+
}
26+
27+
@Test
28+
void it_remediates_finding() {
29+
String fixableCode =
30+
"""
31+
import javax.xml.stream.XMLInputFactory;
32+
import javax.xml.stream.XMLStreamReader;
33+
import java.io.StringReader;
34+
35+
class MessageReader {
36+
37+
Message read(String xml) {
38+
XMLInputFactory factory = XMLInputFactory.newInstance();
39+
XMLStreamReader reader = factory.createXMLStreamReader(new StringReader(xml));
40+
// turn into a Message
41+
Message message = convertMessage(reader);
42+
return message;
43+
}
44+
}
45+
""";
46+
CompilationUnit cu = StaticJavaParser.parse(fixableCode);
47+
LexicalPreservingPrinter.setup(cu);
48+
49+
CodemodFileScanningResult result =
50+
remediator.remediateAll(
51+
cu, "foo", rule, List.of(new Object()), f -> "my-id-1", f -> 9, f -> null);
52+
53+
// confirm code is what's expected
54+
String actualCode = LexicalPreservingPrinter.print(cu);
55+
assertThat(actualCode)
56+
.isEqualToIgnoringWhitespace(
57+
"""
58+
import javax.xml.stream.XMLInputFactory;
59+
import javax.xml.stream.XMLStreamReader;
60+
import java.io.StringReader;
61+
62+
class MessageReader {
63+
Message read(String xml) {
64+
XMLInputFactory factory = XMLInputFactory.newInstance();
65+
factory.setProperty("javax.xml.stream.isSupportingExternalEntities", false);
66+
XMLStreamReader reader = factory.createXMLStreamReader(new StringReader(xml));
67+
// turn into a Message
68+
Message message = convertMessage(reader);
69+
return message;
70+
}
71+
}
72+
""");
73+
74+
// confirm reporting metadata is all correct
75+
assertThat(result.unfixedFindings()).isEmpty();
76+
77+
List<CodemodChange> changes = result.changes();
78+
assertThat(changes).hasSize(1);
79+
80+
CodemodChange change = changes.get(0);
81+
assertThat(change.lineNumber()).isEqualTo(9);
82+
assertThat(change.getDependenciesNeeded()).isEmpty();
83+
;
84+
List<FixedFinding> fixedFindings = change.getFixedFindings();
85+
assertThat(fixedFindings).hasSize(1);
86+
FixedFinding fixedFinding = fixedFindings.get(0);
87+
assertThat(fixedFinding.getId()).isEqualTo("my-id-1");
88+
assertThat(fixedFinding.getRule()).isEqualTo(rule);
89+
}
90+
}

0 commit comments

Comments
 (0)