Skip to content

Commit 0d88f08

Browse files
nahsranahsra
authored
Add fixer for Sonar SQLi issues rather than just hotspots (#491)
Adds support for SQLi issues, which are reported differently and separately (at least at times) from Sonar.
1 parent 37de8c3 commit 0d88f08

File tree

7 files changed

+4610
-9
lines changed

7 files changed

+4610
-9
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ public static List<Class<? extends CodeChanger>> asList() {
9494
SonarJNDIInjectionCodemod.class,
9595
SonarObjectDeserializationCodemod.class,
9696
SonarRemoveUnthrowableExceptionCodemod.class,
97-
SonarSQLInjectionCodemod.class,
97+
SonarSQLInjectionHotspotCodemod.class,
98+
SonarSQLInjectionIssueCodemod.class,
9899
SonarSSRFCodemod.class,
99100
SonarUnsafeReflectionRemediationCodemod.class,
100101
SonarWeakHashingAlgorithmCodemod.class,

core-codemods/src/main/java/io/codemodder/codemods/sonar/SonarSQLInjectionCodemod.java renamed to core-codemods/src/main/java/io/codemodder/codemods/sonar/SonarSQLInjectionHotspotCodemod.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,13 @@
2626
reviewGuidance = ReviewGuidance.MERGE_AFTER_REVIEW,
2727
importance = Importance.HIGH,
2828
executionPriority = CodemodExecutionPriority.HIGH)
29-
public final class SonarSQLInjectionCodemod extends SonarRemediatingJavaParserChanger {
29+
public final class SonarSQLInjectionHotspotCodemod extends SonarRemediatingJavaParserChanger {
3030

3131
private final Remediator<Hotspot> remediationStrategy;
3232
private final RuleHotspot hotspots;
3333

3434
@Inject
35-
public SonarSQLInjectionCodemod(
35+
public SonarSQLInjectionHotspotCodemod(
3636
@ProvidedSonarScan(ruleId = "java:S2077") final RuleHotspot hotspots) {
3737
super(GenericRemediationMetadata.SQL_INJECTION.reporter(), hotspots);
3838
this.hotspots = Objects.requireNonNull(hotspots);

‎core-codemods/src/main/java/io/codemodder/codemods/sonar/SonarSQLInjectionIssueCodemod.java‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
package io.codemodder.codemods.sonar;
2+
3+
import com.github.javaparser.ast.CompilationUnit;
4+
import com.github.javaparser.ast.expr.Expression;
5+
import io.codemodder.*;
6+
import io.codemodder.ast.ASTs;
7+
import io.codemodder.codetf.DetectorRule;
8+
import io.codemodder.providers.sonar.ProvidedSonarScan;
9+
import io.codemodder.providers.sonar.RuleIssue;
10+
import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
11+
import io.codemodder.remediation.FixCandidateSearcher;
12+
import io.codemodder.remediation.GenericRemediationMetadata;
13+
import io.codemodder.remediation.Remediator;
14+
import io.codemodder.remediation.SearcherStrategyRemediator;
15+
import io.codemodder.remediation.sqlinjection.SQLInjectionFixComposer;
16+
import io.codemodder.sonar.model.Issue;
17+
import io.codemodder.sonar.model.SonarFinding;
18+
import io.codemodder.sonar.model.TextRange;
19+
import java.util.List;
20+
import java.util.Objects;
21+
import java.util.Optional;
22+
import javax.inject.Inject;
23+
24+
@Codemod(
25+
id = "sonar:java/sonar-sql-injection-s3649",
26+
reviewGuidance = ReviewGuidance.MERGE_AFTER_REVIEW,
27+
importance = Importance.HIGH,
28+
executionPriority = CodemodExecutionPriority.HIGH)
29+
public final class SonarSQLInjectionIssueCodemod extends SonarRemediatingJavaParserChanger {
30+
31+
private final Remediator<Issue> remediationStrategy;
32+
private final RuleIssue issues;
33+
34+
@Inject
35+
public SonarSQLInjectionIssueCodemod(
36+
@ProvidedSonarScan(ruleId = "javasecurity:S3649") final RuleIssue issues) {
37+
super(GenericRemediationMetadata.SQL_INJECTION.reporter(), issues);
38+
this.issues = Objects.requireNonNull(issues);
39+
this.remediationStrategy =
40+
new SearcherStrategyRemediator.Builder<Issue>()
41+
.withSearcherStrategyPair(
42+
new FixCandidateSearcher.Builder<Issue>()
43+
.withMatcher(
44+
n ->
45+
Optional.empty()
46+
// is the argument of the call
47+
.or(
48+
() ->
49+
Optional.of(n)
50+
.map(
51+
m ->
52+
m instanceof Expression ? (Expression) m : null)
53+
.flatMap(ASTs::isArgumentOfMethodCall)
54+
.filter(SQLInjectionFixComposer::match))
55+
.isPresent())
56+
.build(),
57+
new SQLInjectionFixComposer())
58+
.build();
59+
}
60+
61+
@Override
62+
public DetectorRule detectorRule() {
63+
return new DetectorRule(
64+
"javasecurity:S3649",
65+
"Database queries should not be vulnerable to injection attacks",
66+
"https://rules.sonarsource.com/java/RSPEC-3649/");
67+
}
68+
69+
@Override
70+
public CodemodFileScanningResult visit(
71+
final CodemodInvocationContext context, final CompilationUnit cu) {
72+
List<Issue> issuesForFile = issues.getResultsByPath(context.path());
73+
return remediationStrategy.remediateAll(
74+
cu,
75+
context.path().toString(),
76+
detectorRule(),
77+
issuesForFile,
78+
SonarFinding::getKey,
79+
i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(),
80+
i -> Optional.ofNullable(i.getTextRange()).map(TextRange::getEndLine),
81+
i -> Optional.ofNullable(i.getTextRange()).map(tr -> tr.getStartOffset() + 1));
82+
}
83+
}

core-codemods/src/test/java/io/codemodder/codemods/sonar/SonarSQLInjectionCodemodTest.java renamed to core-codemods/src/test/java/io/codemodder/codemods/sonar/SonarSQLInjectionCodemodsTest.java

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@
44
import io.codemodder.testutils.Metadata;
55
import org.junit.jupiter.api.Nested;
66

7-
final class SonarSQLInjectionCodemodTest {
7+
final class SonarSQLInjectionCodemodsTest {
88

99
@Nested
1010
@Metadata(
11-
codemodType = SonarSQLInjectionCodemod.class,
11+
codemodType = SonarSQLInjectionHotspotCodemod.class,
1212
testResourceDir = "sonar-sql-injection-s2077/unsupported",
1313
renameTestFile = "src/main/java/org/owasp/webgoat/container/users/UserService.java",
1414
expectingFailedFixesAtLines = {52}, // we don't support this method
@@ -17,17 +17,27 @@ class UnsupportedTest implements CodemodTestMixin {}
1717

1818
@Nested
1919
@Metadata(
20-
codemodType = SonarSQLInjectionCodemod.class,
20+
codemodType = SonarSQLInjectionIssueCodemod.class,
21+
testResourceDir = "sonar-sql-injection-s3649",
22+
renameTestFile =
23+
"src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallenge.java",
24+
expectingFixesAtLines = {69},
25+
dependencies = {})
26+
class FromIssueRatherThanHotspotTest implements CodemodTestMixin {}
27+
28+
@Nested
29+
@Metadata(
30+
codemodType = SonarSQLInjectionHotspotCodemod.class,
2131
testResourceDir = "sonar-sql-injection-s2077/supported",
2232
renameTestFile =
2333
"src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallenge.java",
2434
expectingFixesAtLines = {69},
2535
dependencies = {})
26-
class SupportedTest implements CodemodTestMixin {}
36+
class SupportedHotspotTest implements CodemodTestMixin {}
2737

2838
@Nested
2939
@Metadata(
30-
codemodType = SonarSQLInjectionCodemod.class,
40+
codemodType = SonarSQLInjectionHotspotCodemod.class,
3141
testResourceDir = "sonar-sql-injection-s2077/supportedTableInjection",
3242
renameTestFile = "core-codemods/src/main/java/io/codemodder/codemods/SQLTest.java",
3343
expectingFixesAtLines = {19, 25, 33, 40},
@@ -36,7 +46,7 @@ class SupportedTableInjectionTest implements CodemodTestMixin {}
3646

3747
@Nested
3848
@Metadata(
39-
codemodType = SonarSQLInjectionCodemod.class,
49+
codemodType = SonarSQLInjectionHotspotCodemod.class,
4050
testResourceDir = "sonar-sql-injection-s2077/supportedMixedInjections",
4151
renameTestFile = "core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java",
4252
expectingFixesAtLines = {21},

‎core-codemods/src/test/resources/sonar-sql-injection-s3649/SqlInjectionChallenge.java.after‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
/*
2+
* This file is part of WebGoat, an Open Web Application Security Project utility. For details, please see http://www.owasp.org/
3+
*
4+
* Copyright (c) 2002 - 2019 Bruce Mayhew
5+
*
6+
* This program is free software; you can redistribute it and/or modify it under the terms of the
7+
* GNU General Public License as published by the Free Software Foundation; either version 2 of the
8+
* License, or (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without
11+
* even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
12+
* General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU General Public License along with this program; if
15+
* not, write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
16+
* 02111-1307, USA.
17+
*
18+
* Getting Source ==============
19+
*
20+
* Source for this application is maintained at https://github.com/WebGoat/WebGoat, a repository for free software projects.
21+
*/
22+
23+
package org.owasp.webgoat.lessons.sqlinjection.advanced;
24+
25+
import java.sql.*;
26+
import java.sql.PreparedStatement;
27+
import lombok.extern.slf4j.Slf4j;
28+
import org.owasp.webgoat.container.LessonDataSource;
29+
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
30+
import org.owasp.webgoat.container.assignments.AssignmentHints;
31+
import org.owasp.webgoat.container.assignments.AttackResult;
32+
import org.springframework.util.StringUtils;
33+
import org.springframework.web.bind.annotation.PutMapping;
34+
import org.springframework.web.bind.annotation.RequestParam;
35+
import org.springframework.web.bind.annotation.ResponseBody;
36+
import org.springframework.web.bind.annotation.RestController;
37+
38+
/**
39+
* @author nbaars
40+
* @since 4/8/17.
41+
*/
42+
@RestController
43+
@AssignmentHints(
44+
value = {"SqlInjectionChallenge1", "SqlInjectionChallenge2", "SqlInjectionChallenge3"})
45+
@Slf4j
46+
public class SqlInjectionChallenge extends AssignmentEndpoint {
47+
48+
private final LessonDataSource dataSource;
49+
50+
public SqlInjectionChallenge(LessonDataSource dataSource) {
51+
this.dataSource = dataSource;
52+
}
53+
54+
@PutMapping("/SqlInjectionAdvanced/challenge")
55+
// assignment path is bounded to class so we use different http method :-)
56+
@ResponseBody
57+
public AttackResult registerNewUser(
58+
@RequestParam String username_reg,
59+
@RequestParam String email_reg,
60+
@RequestParam String password_reg)
61+
throws Exception {
62+
AttackResult attackResult = checkArguments(username_reg, email_reg, password_reg);
63+
64+
if (attackResult == null) {
65+
66+
try (Connection connection = dataSource.getConnection()) {
67+
String checkUserQuery =
68+
"select userid from sql_challenge_users where userid = ?";
69+
PreparedStatement statement = connection.prepareStatement(checkUserQuery);
70+
statement.setString(1, username_reg);
71+
72+
ResultSet resultSet = statement.execute();
73+
if (resultSet.next()) {
74+
if (username_reg.contains("tom'")) {
75+
attackResult = success(this).feedback("user.exists").build();
76+
} else {
77+
attackResult = failed(this).feedback("user.exists").feedbackArgs(username_reg).build();
78+
}
79+
} else {
80+
PreparedStatement preparedStatement =
81+
connection.prepareStatement("INSERT INTO sql_challenge_users VALUES (?, ?, ?)");
82+
preparedStatement.setString(1, username_reg);
83+
preparedStatement.setString(2, email_reg);
84+
preparedStatement.setString(3, password_reg);
85+
preparedStatement.execute();
86+
attackResult = success(this).feedback("user.created").feedbackArgs(username_reg).build();
87+
}
88+
} catch (SQLException e) {
89+
attackResult = failed(this).output("Something went wrong").build();
90+
}
91+
}
92+
return attackResult;
93+
}
94+
95+
private AttackResult checkArguments(String username_reg, String email_reg, String password_reg) {
96+
if (StringUtils.isEmpty(username_reg)
97+
|| StringUtils.isEmpty(email_reg)
98+
|| StringUtils.isEmpty(password_reg)) {
99+
return failed(this).feedback("input.invalid").build();
100+
}
101+
if (username_reg.length() > 250 || email_reg.length() > 30 || password_reg.length() > 30) {
102+
return failed(this).feedback("input.invalid").build();
103+
}
104+
return null;
105+
}
106+
}

‎core-codemods/src/test/resources/sonar-sql-injection-s3649/SqlInjectionChallenge.java.before‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/*
2+
* This file is part of WebGoat, an Open Web Application Security Project utility. For details, please see http://www.owasp.org/
3+
*
4+
* Copyright (c) 2002 - 2019 Bruce Mayhew
5+
*
6+
* This program is free software; you can redistribute it and/or modify it under the terms of the
7+
* GNU General Public License as published by the Free Software Foundation; either version 2 of the
8+
* License, or (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without
11+
* even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
12+
* General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU General Public License along with this program; if
15+
* not, write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
16+
* 02111-1307, USA.
17+
*
18+
* Getting Source ==============
19+
*
20+
* Source for this application is maintained at https://github.com/WebGoat/WebGoat, a repository for free software projects.
21+
*/
22+
23+
package org.owasp.webgoat.lessons.sqlinjection.advanced;
24+
25+
import java.sql.*;
26+
import lombok.extern.slf4j.Slf4j;
27+
import org.owasp.webgoat.container.LessonDataSource;
28+
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
29+
import org.owasp.webgoat.container.assignments.AssignmentHints;
30+
import org.owasp.webgoat.container.assignments.AttackResult;
31+
import org.springframework.util.StringUtils;
32+
import org.springframework.web.bind.annotation.PutMapping;
33+
import org.springframework.web.bind.annotation.RequestParam;
34+
import org.springframework.web.bind.annotation.ResponseBody;
35+
import org.springframework.web.bind.annotation.RestController;
36+
37+
/**
38+
* @author nbaars
39+
* @since 4/8/17.
40+
*/
41+
@RestController
42+
@AssignmentHints(
43+
value = {"SqlInjectionChallenge1", "SqlInjectionChallenge2", "SqlInjectionChallenge3"})
44+
@Slf4j
45+
public class SqlInjectionChallenge extends AssignmentEndpoint {
46+
47+
private final LessonDataSource dataSource;
48+
49+
public SqlInjectionChallenge(LessonDataSource dataSource) {
50+
this.dataSource = dataSource;
51+
}
52+
53+
@PutMapping("/SqlInjectionAdvanced/challenge")
54+
// assignment path is bounded to class so we use different http method :-)
55+
@ResponseBody
56+
public AttackResult registerNewUser(
57+
@RequestParam String username_reg,
58+
@RequestParam String email_reg,
59+
@RequestParam String password_reg)
60+
throws Exception {
61+
AttackResult attackResult = checkArguments(username_reg, email_reg, password_reg);
62+
63+
if (attackResult == null) {
64+
65+
try (Connection connection = dataSource.getConnection()) {
66+
String checkUserQuery =
67+
"select userid from sql_challenge_users where userid = '" + username_reg + "'";
68+
Statement statement = connection.createStatement();
69+
ResultSet resultSet = statement.executeQuery(checkUserQuery);
70+
71+
if (resultSet.next()) {
72+
if (username_reg.contains("tom'")) {
73+
attackResult = success(this).feedback("user.exists").build();
74+
} else {
75+
attackResult = failed(this).feedback("user.exists").feedbackArgs(username_reg).build();
76+
}
77+
} else {
78+
PreparedStatement preparedStatement =
79+
connection.prepareStatement("INSERT INTO sql_challenge_users VALUES (?, ?, ?)");
80+
preparedStatement.setString(1, username_reg);
81+
preparedStatement.setString(2, email_reg);
82+
preparedStatement.setString(3, password_reg);
83+
preparedStatement.execute();
84+
attackResult = success(this).feedback("user.created").feedbackArgs(username_reg).build();
85+
}
86+
} catch (SQLException e) {
87+
attackResult = failed(this).output("Something went wrong").build();
88+
}
89+
}
90+
return attackResult;
91+
}
92+
93+
private AttackResult checkArguments(String username_reg, String email_reg, String password_reg) {
94+
if (StringUtils.isEmpty(username_reg)
95+
|| StringUtils.isEmpty(email_reg)
96+
|| StringUtils.isEmpty(password_reg)) {
97+
return failed(this).feedback("input.invalid").build();
98+
}
99+
if (username_reg.length() > 250 || email_reg.length() > 30 || password_reg.length() > 30) {
100+
return failed(this).feedback("input.invalid").build();
101+
}
102+
return null;
103+
}
104+
}

0 commit comments

Comments
 (0)