Skip to content

Commit 9485da2

Browse files
nahsranahsra
committed
improve xss fixer and create codeql mapping
1 parent 0214068 commit 9485da2

File tree

6 files changed

+161
-10
lines changed

6 files changed

+161
-10
lines changed

‎core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ public static List<Class<? extends CodeChanger>> asList() {
4141
CodeQLSSRFCodemod.class,
4242
CodeQLStackTraceExposureCodemod.class,
4343
CodeQLUnverifiedJwtCodemod.class,
44+
CodeQLXSSCodemod.class,
4445
CodeQLXXECodemod.class,
4546
DeclareVariableOnSeparateLineCodemod.class,
4647
DefectDojoSqlInjectionCodemod.class,

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package io.codemodder.codemods.codeql;
2+
3+
import com.contrastsecurity.sarif.Result;
4+
import com.github.javaparser.ast.CompilationUnit;
5+
import io.codemodder.*;
6+
import io.codemodder.codetf.DetectorRule;
7+
import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
8+
import io.codemodder.remediation.GenericRemediationMetadata;
9+
import io.codemodder.remediation.Remediator;
10+
import io.codemodder.remediation.xss.XSSRemediator;
11+
import java.util.Optional;
12+
import javax.inject.Inject;
13+
14+
/** A codemod for automatically fixing XSS from CodeQL. */
15+
@Codemod(
16+
id = "codeql:java/xss",
17+
reviewGuidance = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW,
18+
importance = Importance.HIGH,
19+
executionPriority = CodemodExecutionPriority.HIGH)
20+
public final class CodeQLXSSCodemod extends CodeQLRemediationCodemod {
21+
22+
private final Remediator<Result> remediator;
23+
24+
@Inject
25+
public CodeQLXSSCodemod(@ProvidedCodeQLScan(ruleId = "java/xss") final RuleSarif sarif) {
26+
super(GenericRemediationMetadata.XSS.reporter(), sarif);
27+
this.remediator = new XSSRemediator<>();
28+
}
29+
30+
@Override
31+
public DetectorRule detectorRule() {
32+
return new DetectorRule(
33+
"xss",
34+
"Cross-site scripting",
35+
"https://codeql.github.com/codeql-query-help/java/java-xss/");
36+
}
37+
38+
@Override
39+
public CodemodFileScanningResult visit(
40+
final CodemodInvocationContext context, final CompilationUnit cu) {
41+
return remediator.remediateAll(
42+
cu,
43+
context.path().toString(),
44+
detectorRule(),
45+
ruleSarif.getResultsByLocationPath(context.path()),
46+
SarifFindingKeyUtil::buildFindingId,
47+
r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
48+
r ->
49+
Optional.ofNullable(
50+
r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
51+
r ->
52+
Optional.ofNullable(
53+
r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
54+
}
55+
}

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,13 @@
1010
import io.codemodder.remediation.SuccessOrReason;
1111
import java.util.List;
1212
import java.util.Optional;
13-
import org.jetbrains.annotations.VisibleForTesting;
1413

14+
/**
15+
* Fix strategy for XSS vulnerabilities where a variable is returned directly and that is what's
16+
* vulnerable.
17+
*/
1518
final class NakedVariableReturnFixStrategy implements RemediationStrategy {
19+
1620
@Override
1721
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
1822
var maybeReturn = Optional.of(node).map(n -> n instanceof ReturnStmt ? (ReturnStmt) n : null);
@@ -25,8 +29,7 @@ public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
2529
return SuccessOrReason.success(List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
2630
}
2731

28-
@VisibleForTesting
29-
public static boolean match(final Node node) {
32+
static boolean match(final Node node) {
3033
return Optional.of(node)
3134
.map(n -> n instanceof ReturnStmt ? (ReturnStmt) n : null)
3235
.filter(rs -> rs.getExpression().isPresent())

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

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,17 @@
44

55
import com.github.javaparser.ast.CompilationUnit;
66
import com.github.javaparser.ast.Node;
7+
import com.github.javaparser.ast.expr.BinaryExpr;
8+
import com.github.javaparser.ast.expr.Expression;
79
import com.github.javaparser.ast.expr.MethodCallExpr;
810
import io.codemodder.DependencyGAV;
911
import io.codemodder.remediation.RemediationStrategy;
1012
import io.codemodder.remediation.SuccessOrReason;
1113
import java.util.List;
1214
import java.util.Optional;
1315
import java.util.Set;
14-
import org.jetbrains.annotations.VisibleForTesting;
1516

17+
/** Fix strategy for XSS vulnerabilities where a variable is sent to a simple print/write call. */
1618
final class PrintingMethodFixStrategy implements RemediationStrategy {
1719

1820
@Override
@@ -23,14 +25,55 @@ public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
2325
return SuccessOrReason.reason("Not a method call.");
2426
}
2527
MethodCallExpr call = maybeCall.get();
26-
wrap(call.getArgument(0)).withStaticMethod("org.owasp.encoder.Encode", "forHtml", false);
28+
29+
Expression methodArgument = call.getArgument(0);
30+
31+
Optional<Expression> thingToWrap = findExpressionToWrap(methodArgument);
32+
if (thingToWrap.isEmpty()) {
33+
return SuccessOrReason.reason("Could not find recognize code shape to fix.");
34+
}
35+
Expression expressionToWrap = thingToWrap.get();
36+
wrap(expressionToWrap).withStaticMethod("org.owasp.encoder.Encode", "forHtml", false);
2737
return SuccessOrReason.success(List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
2838
}
2939

40+
/**
41+
* We handle 4 expression code shapes. <code>
42+
* print(user.getName());
43+
* print("Hello, " + user.getName());
44+
* print(user.getName() + ", hello!");
45+
* print("Hello, " + user.getName() + ", hello!");
46+
* </code>
47+
*
48+
* <p>Note that we should only handle, for the tougher cases, string literals in combination with
49+
* the given expression. Note any other combination of expressions.
50+
*/
51+
private Optional<Expression> findExpressionToWrap(final Expression expression) {
52+
if (expression.isNameExpr()) {
53+
return Optional.of(expression);
54+
} else if (expression.isBinaryExpr()) {
55+
BinaryExpr binaryExpr = expression.asBinaryExpr();
56+
if (binaryExpr.getLeft().isBinaryExpr() && binaryExpr.getRight().isStringLiteralExpr()) {
57+
BinaryExpr leftBinaryExpr = binaryExpr.getLeft().asBinaryExpr();
58+
if (leftBinaryExpr.getLeft().isStringLiteralExpr()
59+
&& !leftBinaryExpr.getRight().isStringLiteralExpr()) {
60+
return Optional.of(leftBinaryExpr.getRight());
61+
}
62+
} else if (binaryExpr.getLeft().isStringLiteralExpr()
63+
&& binaryExpr.getRight().isStringLiteralExpr()) {
64+
return Optional.empty();
65+
} else if (binaryExpr.getLeft().isStringLiteralExpr()) {
66+
return Optional.of(binaryExpr.getRight());
67+
} else if (binaryExpr.getRight().isStringLiteralExpr()) {
68+
return Optional.of(binaryExpr.getLeft());
69+
}
70+
}
71+
return Optional.empty();
72+
}
73+
3074
private static final Set<String> writingMethodNames = Set.of("print", "println", "write");
3175

32-
@VisibleForTesting
33-
public static boolean match(final Node node) {
76+
static boolean match(final Node node) {
3477
return Optional.of(node)
3578
.map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null)
3679
.filter(mce -> writingMethodNames.contains(mce.getNameAsString()))

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
import java.util.Optional;
1111
import java.util.function.Function;
1212

13-
public class XSSRemediator<T> implements Remediator<T> {
13+
/** Remediator for XSS vulnerabilities. */
14+
public final class XSSRemediator<T> implements Remediator<T> {
1415

1516
private final SearcherStrategyRemediator<T> searchStrategyRemediator;
1617

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

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,55 @@ void should_be_fixed(String s) {
7676
getWriter().write(Encode.forHtml(s));
7777
}
7878
}
79-
"""));
79+
"""),
80+
Arguments.of(
81+
"""
82+
class Samples {
83+
void should_be_fixed(String s) {
84+
getWriter().write("<div>" + s);
85+
}
86+
}
87+
""",
88+
"""
89+
import org.owasp.encoder.Encode;
90+
class Samples {
91+
void should_be_fixed(String s) {
92+
getWriter().write("<div>" + Encode.forHtml(s));
93+
}
94+
}
95+
"""),
96+
Arguments.of(
97+
"""
98+
class Samples {
99+
void should_be_fixed(String s) {
100+
getWriter().write("<div>" + s + "</div>");
101+
}
102+
}
103+
""",
104+
"""
105+
import org.owasp.encoder.Encode;
106+
class Samples {
107+
void should_be_fixed(String s) {
108+
getWriter().write("<div>" + Encode.forHtml(s) + "</div>");
109+
}
110+
}
111+
"""),
112+
Arguments.of(
113+
"""
114+
class Samples {
115+
void should_be_fixed(String s) {
116+
getWriter().write(s + "</div>");
117+
}
118+
}
119+
""",
120+
"""
121+
import org.owasp.encoder.Encode;
122+
class Samples {
123+
void should_be_fixed(String s) {
124+
getWriter().write(Encode.forHtml(s) + "</div>");
125+
}
126+
}
127+
"""));
80128
}
81129

82130
@ParameterizedTest
@@ -104,7 +152,7 @@ void it_fixes_obvious_response_write_methods(final String beforeCode, final Stri
104152
XSSFinding::line,
105153
x -> Optional.empty(),
106154
x -> Optional.ofNullable(x.column()));
107-
assertThat(result.changes().isEmpty()).isFalse();
155+
assertThat(result.changes()).isNotEmpty();
108156
String actualCode = LexicalPreservingPrinter.print(cu);
109157
assertThat(actualCode).isEqualToIgnoringWhitespace(afterCode);
110158
}

0 commit comments

Comments
 (0)