Skip to content

Commit 4eecd14

Browse files
nahsranahsra
authored
Add ability to remediate other XSS code shapes (#481)
Took logic specific to Semgrep and generalized.
1 parent 42914df commit 4eecd14

File tree

7 files changed

+212
-40
lines changed

7 files changed

+212
-40
lines changed

‎core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepJavaDeserializationCodemod.java

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22

33
import com.contrastsecurity.sarif.Result;
44
import com.github.javaparser.ast.CompilationUnit;
5-
import com.github.javaparser.ast.expr.MethodCallExpr;
6-
import com.github.javaparser.ast.expr.VariableDeclarationExpr;
75
import io.codemodder.Codemod;
86
import io.codemodder.CodemodExecutionPriority;
97
import io.codemodder.CodemodFileScanningResult;
@@ -14,11 +12,9 @@
1412
import io.codemodder.SarifFindingKeyUtil;
1513
import io.codemodder.codetf.DetectorRule;
1614
import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
17-
import io.codemodder.remediation.FixCandidateSearcher;
1815
import io.codemodder.remediation.GenericRemediationMetadata;
1916
import io.codemodder.remediation.Remediator;
20-
import io.codemodder.remediation.SearcherStrategyRemediator;
21-
import io.codemodder.remediation.javadeserialization.JavaDeserializationFixStrategy;
17+
import io.codemodder.remediation.javadeserialization.JavaDeserializationRemediator;
2218
import java.util.Optional;
2319
import javax.inject.Inject;
2420

@@ -41,32 +37,7 @@ public SemgrepJavaDeserializationCodemod(
4137
ruleId = "java.lang.security.audit.object-deserialization.object-deserialization")
4238
final RuleSarif sarif) {
4339
super(GenericRemediationMetadata.DESERIALIZATION.reporter(), sarif);
44-
this.remediator =
45-
new SearcherStrategyRemediator.Builder<Result>()
46-
.withSearcherStrategyPair(
47-
// matches declarations
48-
new FixCandidateSearcher.Builder<Result>()
49-
.withMatcher(
50-
n ->
51-
Optional.empty()
52-
.or(
53-
() ->
54-
Optional.of(n)
55-
.map(
56-
m ->
57-
m instanceof VariableDeclarationExpr vde
58-
? vde
59-
: null)
60-
.filter(JavaDeserializationFixStrategy::match))
61-
.or(
62-
() ->
63-
Optional.of(n)
64-
.map(m -> m instanceof MethodCallExpr mce ? mce : null)
65-
.filter(JavaDeserializationFixStrategy::match))
66-
.isPresent())
67-
.build(),
68-
new JavaDeserializationFixStrategy())
69-
.build();
40+
this.remediator = new JavaDeserializationRemediator<>();
7041
}
7142

7243
@Override

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package io.codemodder.remediation.javadeserialization;
22

33
import com.github.javaparser.ast.CompilationUnit;
4+
import com.github.javaparser.ast.expr.MethodCallExpr;
5+
import com.github.javaparser.ast.expr.VariableDeclarationExpr;
46
import io.codemodder.CodemodFileScanningResult;
57
import io.codemodder.codetf.DetectorRule;
68
import io.codemodder.remediation.*;
@@ -21,8 +23,26 @@ public JavaDeserializationRemediator() {
2123
this.searchStrategyRemediator =
2224
new SearcherStrategyRemediator.Builder<T>()
2325
.withSearcherStrategyPair(
26+
// matches declarations
2427
new FixCandidateSearcher.Builder<T>()
25-
.withMatcher(JavaDeserializationFixStrategy::match)
28+
.withMatcher(
29+
n ->
30+
Optional.empty()
31+
.or(
32+
() ->
33+
Optional.of(n)
34+
.map(
35+
m ->
36+
m instanceof VariableDeclarationExpr vde
37+
? vde
38+
: null)
39+
.filter(JavaDeserializationFixStrategy::match))
40+
.or(
41+
() ->
42+
Optional.of(n)
43+
.map(m -> m instanceof MethodCallExpr mce ? mce : null)
44+
.filter(JavaDeserializationFixStrategy::match))
45+
.isPresent())
2646
.build(),
2747
new JavaDeserializationFixStrategy())
2848
.build();

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/ResponseEntityFixStrategy.java renamed to ‎framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/ResponseEntityConstructorFixStrategy.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,10 @@
1010
import java.util.Optional;
1111

1212
/**
13-
* Fix strategy for XSS vulnerabilities where a variable/expr is sent to a Spring ResponseEntity.
13+
* Fix strategy for XSS vulnerabilities where a variable/expr is sent to a Spring ResponseEntity
14+
* constructor.
1415
*/
15-
final class ResponseEntityFixStrategy implements RemediationStrategy {
16+
final class ResponseEntityConstructorFixStrategy implements RemediationStrategy {
1617

1718
@Override
1819
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
package io.codemodder.remediation.xss;
2+
3+
import com.github.javaparser.ast.CompilationUnit;
4+
import com.github.javaparser.ast.Node;
5+
import com.github.javaparser.ast.expr.Expression;
6+
import com.github.javaparser.ast.expr.MethodCallExpr;
7+
import com.github.javaparser.resolution.types.ResolvedType;
8+
import io.codemodder.remediation.RemediationStrategy;
9+
import io.codemodder.remediation.SuccessOrReason;
10+
import java.util.Optional;
11+
12+
/**
13+
* Fix strategy for XSS vulnerabilities where a variable/expr is sent to a Spring ResponseEntity
14+
* write method like ok().
15+
*/
16+
final class ResponseEntityWriteFixStrategy implements RemediationStrategy {
17+
18+
@Override
19+
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
20+
var maybeCall =
21+
Optional.of(node).map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null);
22+
if (maybeCall.isEmpty()) {
23+
return SuccessOrReason.reason("Not a method call.");
24+
}
25+
26+
MethodCallExpr call = maybeCall.get();
27+
return EncoderWrapping.fix(call, 0);
28+
}
29+
30+
static boolean match(final Node node) {
31+
return Optional.of(node)
32+
.map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null)
33+
.filter(m -> "ok".equals(m.getNameAsString()))
34+
.filter(m -> !m.getArguments().isEmpty())
35+
.filter(
36+
c -> {
37+
Expression firstArg = c.getArguments().getFirst().get();
38+
try {
39+
ResolvedType resolvedType = firstArg.calculateResolvedType();
40+
return "java.lang.String".equals(resolvedType.describe());
41+
} catch (Exception e) {
42+
// this is expected often, and indicates its a non-String type anyway
43+
return false;
44+
}
45+
})
46+
.isPresent();
47+
}
48+
}

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,14 @@ public XSSRemediator() {
3030
new PrintingMethodFixStrategy())
3131
.withSearcherStrategyPair(
3232
new FixCandidateSearcher.Builder<T>()
33-
.withMatcher(ResponseEntityFixStrategy::match)
33+
.withMatcher(ResponseEntityConstructorFixStrategy::match)
3434
.build(),
35-
new ResponseEntityFixStrategy())
35+
new ResponseEntityConstructorFixStrategy())
36+
.withSearcherStrategyPair(
37+
new FixCandidateSearcher.Builder<T>()
38+
.withMatcher(ResponseEntityWriteFixStrategy::match)
39+
.build(),
40+
new ResponseEntityWriteFixStrategy())
3641
.build();
3742
}
3843

‎framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/ResponseEntityFixStrategyTest.java renamed to ‎framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/ResponseEntityConstructorFixStrategyTest.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,15 @@
1919
import org.junit.jupiter.params.provider.Arguments;
2020
import org.junit.jupiter.params.provider.MethodSource;
2121

22-
final class ResponseEntityFixStrategyTest {
22+
final class ResponseEntityConstructorFixStrategyTest {
2323

24-
private ResponseEntityFixStrategy fixer;
24+
private ResponseEntityConstructorFixStrategy fixer;
2525
private DetectorRule rule;
2626
private JavaParser parser;
2727

2828
@BeforeEach
2929
void setup() throws IOException {
30-
this.fixer = new ResponseEntityFixStrategy();
30+
this.fixer = new ResponseEntityConstructorFixStrategy();
3131
this.parser = JavaParserFactory.newFactory().create(List.of());
3232
this.rule = new DetectorRule("xss", "XSS", null);
3333
}
@@ -86,7 +86,7 @@ private CodemodFileScanningResult scanAndFix(final CompilationUnit cu, final int
8686
new SearcherStrategyRemediator.Builder<XSSFinding>()
8787
.withSearcherStrategyPair(
8888
new FixCandidateSearcher.Builder<XSSFinding>()
89-
.withMatcher(ResponseEntityFixStrategy::match)
89+
.withMatcher(ResponseEntityConstructorFixStrategy::match)
9090
.build(),
9191
fixer)
9292
.build();

‎framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/ResponseEntityWriteFixStrategyTest.java

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
package io.codemodder.remediation.xss;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
5+
import com.github.javaparser.JavaParser;
6+
import com.github.javaparser.ast.CompilationUnit;
7+
import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter;
8+
import io.codemodder.CodemodFileScanningResult;
9+
import io.codemodder.codetf.DetectorRule;
10+
import io.codemodder.javaparser.JavaParserFactory;
11+
import io.codemodder.remediation.FixCandidateSearcher;
12+
import io.codemodder.remediation.SearcherStrategyRemediator;
13+
import java.io.IOException;
14+
import java.util.List;
15+
import java.util.Optional;
16+
import java.util.stream.Stream;
17+
import org.junit.jupiter.api.BeforeEach;
18+
import org.junit.jupiter.params.ParameterizedTest;
19+
import org.junit.jupiter.params.provider.Arguments;
20+
import org.junit.jupiter.params.provider.MethodSource;
21+
22+
final class ResponseEntityWriteFixStrategyTest {
23+
24+
private ResponseEntityWriteFixStrategy fixer;
25+
private DetectorRule rule;
26+
private JavaParser parser;
27+
28+
@BeforeEach
29+
void setup() throws IOException {
30+
this.fixer = new ResponseEntityWriteFixStrategy();
31+
this.parser = JavaParserFactory.newFactory().create(List.of());
32+
this.rule = new DetectorRule("xss", "XSS", null);
33+
}
34+
35+
private static Stream<Arguments> fixableSamples() {
36+
return Stream.of(
37+
Arguments.of(
38+
"""
39+
class Samples {
40+
String should_be_fixed(String s) {
41+
return ResponseEntity.ok("Value: " + s);
42+
}
43+
}
44+
""",
45+
"""
46+
import org.owasp.encoder.Encode;
47+
class Samples {
48+
String should_be_fixed(String s) {
49+
return ResponseEntity.ok("Value: " + Encode.forHtml(s));
50+
}
51+
}
52+
"""),
53+
Arguments.of(
54+
"""
55+
class Samples {
56+
String should_be_fixed(Object s) {
57+
return ResponseEntity.ok("Value: " + s.toString());
58+
}
59+
}
60+
""",
61+
"""
62+
import org.owasp.encoder.Encode;
63+
class Samples {
64+
String should_be_fixed(Object s) {
65+
return ResponseEntity.ok("Value: " + Encode.forHtml(s.toString()));
66+
}
67+
}
68+
"""));
69+
}
70+
71+
@ParameterizedTest
72+
@MethodSource("fixableSamples")
73+
void it_fixes_obvious_response_write_methods(final String beforeCode, final String afterCode) {
74+
CompilationUnit cu = parser.parse(beforeCode).getResult().orElseThrow();
75+
LexicalPreservingPrinter.setup(cu);
76+
77+
var result = scanAndFix(cu, 3);
78+
assertThat(result.changes()).isNotEmpty();
79+
String actualCode = LexicalPreservingPrinter.print(cu);
80+
assertThat(actualCode).isEqualToIgnoringWhitespace(afterCode);
81+
}
82+
83+
private CodemodFileScanningResult scanAndFix(final CompilationUnit cu, final int line) {
84+
XSSFinding finding = new XSSFinding("should_be_fixed", line, null);
85+
var remediator =
86+
new SearcherStrategyRemediator.Builder<XSSFinding>()
87+
.withSearcherStrategyPair(
88+
new FixCandidateSearcher.Builder<XSSFinding>()
89+
.withMatcher(ResponseEntityWriteFixStrategy::match)
90+
.build(),
91+
fixer)
92+
.build();
93+
return remediator.remediateAll(
94+
cu,
95+
"path",
96+
rule,
97+
List.of(finding),
98+
XSSFinding::key,
99+
XSSFinding::line,
100+
x -> Optional.empty(),
101+
x -> Optional.ofNullable(x.column()));
102+
}
103+
104+
@ParameterizedTest
105+
@MethodSource("unfixableSamples")
106+
void it_does_not_fix_unfixable_samples(final String beforeCode, final int line) {
107+
CompilationUnit cu = parser.parse(beforeCode).getResult().orElseThrow();
108+
LexicalPreservingPrinter.setup(cu);
109+
var result = scanAndFix(cu, line);
110+
assertThat(result.changes()).isEmpty();
111+
}
112+
113+
private static Stream<Arguments> unfixableSamples() {
114+
return Stream.of(
115+
// this is not a ResponseEntity, shouldn't touch it
116+
Arguments.of(
117+
// this is not a ResponseEntity, shouldn't touch it
118+
"""
119+
class Samples {
120+
String should_be_fixed(String s) {
121+
return ResponseEntity.something_besides_ok("Value: " + s);
122+
}
123+
}
124+
""",
125+
3));
126+
}
127+
}

0 commit comments

Comments
 (0)