From a64d95c0f883ccabd8a59b6a01436c6516484c50 Mon Sep 17 00:00:00 2001 From: Arshan Dabirsiaghi Date: Fri, 18 Oct 2024 10:52:24 -0400 Subject: [PATCH] Add more tests and remove codemod we're not ready for (#459) * Moves remediators that were for some reason in core-codemods to the framework * Removes a codemod we're not ready to do yet (CodeQL reports vulnerabilities at odds locations in comparison to other tools) * Made tests harder to pass * Styling cleanup --- .../integration/WebGoat20238Test.java | 2 +- .../integration/WebGoat822Test.java | 1 - .../codemodder/codemods/DefaultCodemods.java | 2 +- .../codemodder/codemods/SonarSSRFCodemod.java | 2 +- .../CodeQLInsecureRandomnessCodemod.java | 48 ----------- .../codeql/CodeQLPredictableSeedCodemod.java | 56 +++++++++++++ .../codemods/codeql/CodeQLSSRFCodemod.java | 2 +- .../codemods/semgrep/SemgrepSSRFCodemod.java | 2 +- .../semgrep/SemgrepWeakRandomCodemod.java | 2 +- .../codemodder/CodemodFileScanningResult.java | 2 +- .../GenericRemediationMetadata.java | 4 +- .../io/codemodder/remediation/Remediator.java | 3 +- .../SearcherStrategyRemediator.java | 4 +- .../JNDIInjectionRemediator.java | 7 +- .../PredictableSeedRemediator.java | 80 +++++++++++++++++++ .../sqlinjection/SQLInjectionRemediator.java | 4 +- .../ssrf/DefaultSSRFRemediator.java | 2 +- .../remediation}/ssrf/SSRFRemediator.java | 2 +- .../DefaultWeakRandomRemediator.java | 2 +- .../weakrandom/WeakRandomRemediator.java | 2 +- .../predictable-seed/description.md | 11 +++ .../predictable-seed/report.json | 11 +++ .../zip-slip/description.md | 30 +++++++ .../zip-slip/report.json | 7 ++ .../io/codemodder/plugins/llm/LLMDiffs.java | 2 +- .../plugins/llm/SarifPluginLLMCodemod.java | 3 +- 26 files changed, 222 insertions(+), 71 deletions(-) delete mode 100644 core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLInsecureRandomnessCodemod.java create mode 100644 core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLPredictableSeedCodemod.java create mode 100644 framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java rename {core-codemods/src/main/java/io/codemodder/codemods/remediators => framework/codemodder-base/src/main/java/io/codemodder/remediation}/ssrf/DefaultSSRFRemediator.java (99%) rename {core-codemods/src/main/java/io/codemodder/codemods/remediators => framework/codemodder-base/src/main/java/io/codemodder/remediation}/ssrf/SSRFRemediator.java (93%) rename {core-codemods/src/main/java/io/codemodder/codemods/remediators => framework/codemodder-base/src/main/java/io/codemodder/remediation}/weakrandom/DefaultWeakRandomRemediator.java (97%) rename {core-codemods/src/main/java/io/codemodder/codemods/remediators => framework/codemodder-base/src/main/java/io/codemodder/remediation}/weakrandom/WeakRandomRemediator.java (93%) create mode 100644 framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/description.md create mode 100644 framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/report.json create mode 100644 framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/description.md create mode 100644 framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/report.json diff --git a/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat20238Test.java b/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat20238Test.java index d4f36e425..ac1f26ab8 100644 --- a/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat20238Test.java +++ b/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat20238Test.java @@ -68,10 +68,10 @@ void it_remediates_webgoat_2023_8() throws Exception { assertThat(jwtChange.getChanges().get(0).getLineNumber(), equalTo(113)); assertThat(jwtChange.getChanges().get(1).getLineNumber(), equalTo(140)); - verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-randomness", 0); verifyCodemodsHitWithChangesetCount(report, "codeql:java/ssrf", 1); verifyCodemodsHitWithChangesetCount(report, "codeql:java/xxe", 1); verifyCodemodsHitWithChangesetCount(report, "codeql:java/sql-injection", 6); verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-cookie", 2); + verifyCodemodsHitWithChangesetCount(report, "codeql:java/unsafe-deserialization", 2); } } diff --git a/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat822Test.java b/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat822Test.java index 2b44be617..3072f87d8 100644 --- a/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat822Test.java +++ b/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat822Test.java @@ -171,7 +171,6 @@ void it_transforms_webgoat_with_codeql() throws Exception { assertThat(ajaxJwtChange.getChanges().size(), equalTo(1)); assertThat(ajaxJwtChange.getChanges().get(0).getLineNumber(), equalTo(53)); - verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-randomness", 0); verifyCodemodsHitWithChangesetCount(report, "codeql:java/ssrf", 3); verifyCodemodsHitWithChangesetCount(report, "codeql:java/sql-injection", 5); verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-cookie", 1); diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java b/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java index 479506697..74ca78b41 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java @@ -30,12 +30,12 @@ public static List> asList() { CodeQLHttpResponseSplittingCodemod.class, CodeQLInputResourceLeakCodemod.class, CodeQLInsecureCookieCodemod.class, - CodeQLInsecureRandomnessCodemod.class, CodeQLJDBCResourceLeakCodemod.class, CodeQLJEXLInjectionCodemod.class, CodeQLJNDIInjectionCodemod.class, CodeQLMavenSecureURLCodemod.class, CodeQLOutputResourceLeakCodemod.class, + CodeQLPredictableSeedCodemod.class, CodeQLSQLInjectionCodemod.class, CodeQLSSRFCodemod.class, CodeQLStackTraceExposureCodemod.class, diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarSSRFCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarSSRFCodemod.java index 271cc5ec3..ab636fcaa 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/SonarSSRFCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/SonarSSRFCodemod.java @@ -2,12 +2,12 @@ import com.github.javaparser.ast.CompilationUnit; import io.codemodder.*; -import io.codemodder.codemods.remediators.ssrf.SSRFRemediator; import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sonar.ProvidedSonarScan; import io.codemodder.providers.sonar.RuleIssue; import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger; import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.ssrf.SSRFRemediator; import io.codemodder.sonar.model.Issue; import io.codemodder.sonar.model.SonarFinding; import java.util.List; diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLInsecureRandomnessCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLInsecureRandomnessCodemod.java deleted file mode 100644 index 6d2d5bed9..000000000 --- a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLInsecureRandomnessCodemod.java +++ /dev/null @@ -1,48 +0,0 @@ -package io.codemodder.codemods.codeql; - -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.*; -import io.codemodder.codemods.remediators.weakrandom.WeakRandomRemediator; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan; -import io.codemodder.remediation.GenericRemediationMetadata; -import javax.inject.Inject; - -/** A codemod for automatically fixing insecure randomness from CodeQL. */ -@Codemod( - id = "codeql:java/insecure-randomness", - reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW, - importance = Importance.HIGH, - executionPriority = CodemodExecutionPriority.HIGH) -public final class CodeQLInsecureRandomnessCodemod extends CodeQLRemediationCodemod { - - private final WeakRandomRemediator remediator; - - @Inject - public CodeQLInsecureRandomnessCodemod( - @ProvidedCodeQLScan(ruleId = "java/insecure-randomness") final RuleSarif sarif) { - super(GenericRemediationMetadata.WEAK_RANDOM.reporter(), sarif); - this.remediator = WeakRandomRemediator.DEFAULT; - } - - @Override - public DetectorRule detectorRule() { - return new DetectorRule( - "insecure-randomness", - "Insecure randomness", - "https://codeql.github.com/codeql-query-help/java/java-insecure-randomness/"); - } - - @Override - public CodemodFileScanningResult visit( - final CodemodInvocationContext context, final CompilationUnit cu) { - return remediator.remediateAll( - cu, - context.path().toString(), - detectorRule(), - ruleSarif.getResultsByLocationPath(context.path()), - SarifFindingKeyUtil::buildFindingId, - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()); - } -} diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLPredictableSeedCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLPredictableSeedCodemod.java new file mode 100644 index 000000000..6dda7c185 --- /dev/null +++ b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLPredictableSeedCodemod.java @@ -0,0 +1,56 @@ +package io.codemodder.codemods.codeql; + +import com.contrastsecurity.sarif.Result; +import com.github.javaparser.ast.CompilationUnit; +import io.codemodder.*; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan; +import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.predictableseed.PredictableSeedRemediator; +import java.util.Optional; +import javax.inject.Inject; + +/** A codemod for automatically fixing predictable seeds reported by CodeQL. */ +@Codemod( + id = "codeql:java/predictable-seed", + reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW, + importance = Importance.MEDIUM, + executionPriority = CodemodExecutionPriority.HIGH) +public final class CodeQLPredictableSeedCodemod extends CodeQLRemediationCodemod { + + private final Remediator remediator; + + @Inject + public CodeQLPredictableSeedCodemod( + @ProvidedCodeQLScan(ruleId = "java/predictable-seed") final RuleSarif sarif) { + super(GenericRemediationMetadata.PREDICTABLE_SEED.reporter(), sarif); + this.remediator = new PredictableSeedRemediator<>(); + } + + @Override + public DetectorRule detectorRule() { + return new DetectorRule( + "predictable-seed", + "Use of a predictable seed in a secure random number generator", + "https://codeql.github.com/codeql-query-help/java/java-predictable-seed/"); + } + + @Override + public CodemodFileScanningResult visit( + final CodemodInvocationContext context, final CompilationUnit cu) { + return remediator.remediateAll( + cu, + context.path().toString(), + detectorRule(), + ruleSarif.getResultsByLocationPath(context.path()), + SarifFindingKeyUtil::buildFindingId, + result -> result.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), + result -> + Optional.ofNullable( + result.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + result -> + Optional.ofNullable( + result.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn())); + } +} diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLSSRFCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLSSRFCodemod.java index 0b02e59c9..6eb6c4801 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLSSRFCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLSSRFCodemod.java @@ -2,10 +2,10 @@ import com.github.javaparser.ast.CompilationUnit; import io.codemodder.*; -import io.codemodder.codemods.remediators.ssrf.SSRFRemediator; import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan; import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.ssrf.SSRFRemediator; import javax.inject.Inject; /** A codemod for automatically fixing SQL injection from CodeQL. */ diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSSRFCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSSRFCodemod.java index 29c5a823d..851b65651 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSSRFCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSSRFCodemod.java @@ -9,10 +9,10 @@ import io.codemodder.ReviewGuidance; import io.codemodder.RuleSarif; import io.codemodder.SarifFindingKeyUtil; -import io.codemodder.codemods.remediators.ssrf.SSRFRemediator; import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.ssrf.SSRFRemediator; import javax.inject.Inject; /** diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java index 980f780dc..5b42288c3 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java @@ -9,9 +9,9 @@ import io.codemodder.ReviewGuidance; import io.codemodder.RuleSarif; import io.codemodder.SarifFindingKeyUtil; -import io.codemodder.codemods.remediators.weakrandom.WeakRandomRemediator; import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; +import io.codemodder.remediation.weakrandom.WeakRandomRemediator; import javax.inject.Inject; /** diff --git a/framework/codemodder-base/src/main/java/io/codemodder/CodemodFileScanningResult.java b/framework/codemodder-base/src/main/java/io/codemodder/CodemodFileScanningResult.java index dcdab6cb6..a784af6f4 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/CodemodFileScanningResult.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/CodemodFileScanningResult.java @@ -30,7 +30,7 @@ public List unfixedFindings() { static CodemodFileScanningResult from( final List changes, final List unfixedFindings, - CodeTFAiMetadata codeTFAiMetadata) { + final CodeTFAiMetadata codeTFAiMetadata) { return new AICodemodFileScanningResult(changes, unfixedFindings, codeTFAiMetadata); } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java index 022a7c582..848552990 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java @@ -14,7 +14,9 @@ public enum GenericRemediationMetadata { MISSING_SECURE_FLAG("missing-secure-flag"), SQL_INJECTION("sql-injection"), SSRF("ssrf"), - WEAK_RANDOM("weak-random"); + WEAK_RANDOM("weak-random"), + PREDICTABLE_SEED("predictable-seed"), + ZIP_SLIP("zip-slip"); private final CodemodReporterStrategy reporter; diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/Remediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/Remediator.java index 9eb8ca578..fe7ce11ca 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/Remediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/Remediator.java @@ -14,6 +14,7 @@ */ public interface Remediator { + /** Performs the work of performing all remediation, and returning reporting metadata. */ CodemodFileScanningResult remediateAll( final CompilationUnit cu, final String path, @@ -22,5 +23,5 @@ CodemodFileScanningResult remediateAll( final Function findingIdExtractor, final Function findingStartLineExtractor, final Function> findingEndLineExtractor, - final Function> findingColumnExtractor); + final Function> findingStartColumnExtractor); } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java index dcac0f6bf..5075a5991 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java @@ -135,7 +135,7 @@ public CodemodFileScanningResult remediateAll( final Function findingIdExtractor, final Function findingStartLineExtractor, final Function> findingEndLineExtractor, - final Function> findingColumnExtractor) { + final Function> findingStartColumnExtractor) { List allChanges = new ArrayList<>(); List allUnfixed = new ArrayList<>(); @@ -150,7 +150,7 @@ public CodemodFileScanningResult remediateAll( findingIdExtractor, findingStartLineExtractor, findingEndLineExtractor, - findingColumnExtractor, + findingStartColumnExtractor, searcherAndStrategy.getKey(), searcherAndStrategy.getValue()); allChanges.addAll(pairResult.getValue0()); diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java index f89c2ef7d..22c92f68d 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java @@ -9,7 +9,8 @@ import java.util.Optional; import java.util.function.Function; -public class JNDIInjectionRemediator implements Remediator { +/** Remediates JNDI injection vulnerabilities. */ +public final class JNDIInjectionRemediator implements Remediator { private final SearcherStrategyRemediator searchStrategyRemediator; @@ -51,7 +52,7 @@ public CodemodFileScanningResult remediateAll( Function findingIdExtractor, Function findingStartLineExtractor, Function> findingEndLineExtractor, - Function> findingColumnExtractor) { + Function> findingStartColumnExtractor) { return searchStrategyRemediator.remediateAll( cu, path, @@ -60,6 +61,6 @@ public CodemodFileScanningResult remediateAll( findingIdExtractor, findingStartLineExtractor, findingEndLineExtractor, - findingColumnExtractor); + findingStartColumnExtractor); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java new file mode 100644 index 000000000..78c1cbfee --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java @@ -0,0 +1,80 @@ +package io.codemodder.remediation.predictableseed; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.NodeList; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.LiteralExpr; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.NameExpr; +import io.codemodder.CodemodChange; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.codetf.FixedFinding; +import io.codemodder.remediation.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Optional; +import java.util.function.Function; + +/** Remediator for predictable seed weaknesses. */ +public final class PredictableSeedRemediator implements Remediator { + + @Override + public CodemodFileScanningResult remediateAll( + final CompilationUnit cu, + final String path, + final DetectorRule detectorRule, + final Collection findingsForPath, + final Function findingIdExtractor, + final Function findingStartLineExtractor, + final Function> findingEndLineExtractor, + final Function> findingStartColumnExtractor) { + + FixCandidateSearcher searcher = + new FixCandidateSearcher.Builder() + .withMatcher( + n -> + Optional.of(n) + .map(MethodOrConstructor::new) + .filter(mce -> mce.isMethodCallWithName("setSeed")) + .filter(mce -> mce.asNode().hasScope()) + .filter(mce -> mce.getArguments().size() == 1) + // technically, we don't need this, just to prevent a silly tool from + // reporting on hardcoded data + .filter(mce -> !(mce.getArguments().get(0) instanceof LiteralExpr)) + .isPresent()) + .build(); + + FixCandidateSearchResults candidateSearchResults = + searcher.search( + cu, + path, + detectorRule, + List.copyOf(findingsForPath), + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingStartColumnExtractor); + + List changes = new ArrayList<>(); + for (FixCandidate fixCandidate : candidateSearchResults.fixCandidates()) { + MethodCallExpr setSeedCall = (MethodCallExpr) fixCandidate.node(); + MethodCallExpr safeExpression = + new MethodCallExpr(new NameExpr(System.class.getSimpleName()), "currentTimeMillis"); + NodeList arguments = setSeedCall.getArguments(); + arguments.set(0, safeExpression); + + // should only ever be one, but just in case + List fixedFindings = + fixCandidate.issues().stream() + .map(issue -> new FixedFinding(findingIdExtractor.apply(issue), detectorRule)) + .toList(); + + int reportedLine = setSeedCall.getRange().get().begin.line; + changes.add(CodemodChange.from(reportedLine, List.of(), fixedFindings)); + } + + return CodemodFileScanningResult.from(changes, candidateSearchResults.unfixableFindings()); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java index 88cfecb1e..f043745a6 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java @@ -36,7 +36,7 @@ public CodemodFileScanningResult remediateAll( Function findingIdExtractor, Function findingStartLineExtractor, Function> findingEndLineExtractor, - Function> findingColumnExtractor) { + Function> findingStartColumnExtractor) { return searchStrategyRemediator.remediateAll( cu, path, @@ -45,6 +45,6 @@ public CodemodFileScanningResult remediateAll( findingIdExtractor, findingStartLineExtractor, findingEndLineExtractor, - findingColumnExtractor); + findingStartColumnExtractor); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/remediators/ssrf/DefaultSSRFRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/DefaultSSRFRemediator.java similarity index 99% rename from core-codemods/src/main/java/io/codemodder/codemods/remediators/ssrf/DefaultSSRFRemediator.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/DefaultSSRFRemediator.java index dbb370a17..52d9f5940 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/remediators/ssrf/DefaultSSRFRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/DefaultSSRFRemediator.java @@ -1,4 +1,4 @@ -package io.codemodder.codemods.remediators.ssrf; +package io.codemodder.remediation.ssrf; import static io.codemodder.ast.ASTTransforms.addImportIfMissing; diff --git a/core-codemods/src/main/java/io/codemodder/codemods/remediators/ssrf/SSRFRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/SSRFRemediator.java similarity index 93% rename from core-codemods/src/main/java/io/codemodder/codemods/remediators/ssrf/SSRFRemediator.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/SSRFRemediator.java index e9c3440c3..8469f611c 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/remediators/ssrf/SSRFRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/SSRFRemediator.java @@ -1,4 +1,4 @@ -package io.codemodder.codemods.remediators.ssrf; +package io.codemodder.remediation.ssrf; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.CodemodFileScanningResult; diff --git a/core-codemods/src/main/java/io/codemodder/codemods/remediators/weakrandom/DefaultWeakRandomRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java similarity index 97% rename from core-codemods/src/main/java/io/codemodder/codemods/remediators/weakrandom/DefaultWeakRandomRemediator.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java index 20770a12f..d746e7b8c 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/remediators/weakrandom/DefaultWeakRandomRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java @@ -1,4 +1,4 @@ -package io.codemodder.codemods.remediators.weakrandom; +package io.codemodder.remediation.weakrandom; import static io.codemodder.ast.ASTTransforms.addImportIfMissing; diff --git a/core-codemods/src/main/java/io/codemodder/codemods/remediators/weakrandom/WeakRandomRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java similarity index 93% rename from core-codemods/src/main/java/io/codemodder/codemods/remediators/weakrandom/WeakRandomRemediator.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java index dc2612c81..e301c0866 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/remediators/weakrandom/WeakRandomRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java @@ -1,4 +1,4 @@ -package io.codemodder.codemods.remediators.weakrandom; +package io.codemodder.remediation.weakrandom; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.CodemodFileScanningResult; diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/description.md b/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/description.md new file mode 100644 index 000000000..3b001c607 --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/description.md @@ -0,0 +1,11 @@ +This change replaces all the constant seeds passed to `Random#setSeed(long)` with a pseudo-random value, which will make it considerably more secure. + +A "seed" tells your pseudo-random number generator (PRNG) "where to start" in a deterministic (huge, but deterministic) set of numbers. If attackers can detect you're using a constant seed, they'll quickly be able to predict the next numbers you will generate. + +Our change replaces the constant with `System#currentTimeMillis()`. + +```diff + Random random = new Random(); +- random.setSeed(123); ++ random.setSeed(System.currentTimeMillis()); +``` diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/report.json b/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/report.json new file mode 100644 index 000000000..d3003fa09 --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/report.json @@ -0,0 +1,11 @@ +{ + "summary" : "Strengthened cipher seed with more unpredictable value", + "change" : "Added a better seed to the pseudorandom number generation to make numbers in successive runs more random", + "reviewGuidanceJustification" : "There should be no difference to the code what random numbers are generated. If there is, this change will surface that issue. This case could indicate a serious security weakness.", + "references" : [ + "https://wiki.sei.cmu.edu/confluence/display/c/MSC32-C.+Properly+seed+pseudorandom+number+generators", + "https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC51-CPP.+Ensure+your+random+number+generator+is+properly+seeded", + "https://cwe.mitre.org/data/definitions/337.html", + "https://en.wikipedia.org/wiki/Random_seed" + ] +} diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/description.md b/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/description.md new file mode 100644 index 000000000..58a479693 --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/description.md @@ -0,0 +1,30 @@ +This change fixes instances of [ZipInputStream](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/zip/ZipInputStream.html) to protect against malicious entries that attempt to escape their "file root" and overwrite other files on the running filesystem. + +Normally, when you're using `ZipInputStream` it's because you're processing zip files. That code might look like this: + +```java +File file = new File(unzipTargetDirectory, zipEntry.getName()); // use file name from zip entry +InputStream is = zip.getInputStream(zipEntry); // get the contents of the zip entry +IOUtils.copy(is, new FileOutputStream(file)); // write the contents to the provided file name +``` + +This looks fine when it encounters a normal zip entry within a zip file, looking something like this pseudo-data: +```binary +path: data/names.txt +contents: Zeus\nHelen\nLeda... +``` + +However, there's nothing to prevent an attacker from sending an evil entry in the zip that looks more like this: +```binary +path: ../../../../../etc/passwd +contents: root::0:0:root:/:/bin/sh +``` + +Yes, in the above code, which looks like [every](https://stackoverflow.com/a/23870468) [piece](https://stackoverflow.com/a/51285801) of [zip-processing](https://kodejava.org/how-do-i-decompress-a-zip-file-using-zipinputstream/) code you can [find](https://www.tabnine.com/code/java/classes/java.util.zip.ZipInputStream) on the [Internet](https://www.baeldung.com/java-compress-and-uncompress), attackers could overwrite any files to which the application has access. This rule replaces the standard `ZipInputStream` with a hardened subclass which prevents access to entry paths that attempt to traverse directories above the current directory (which no normal zip file should ever do.) Our changes end up looking something like this: + +```diff ++ import io.github.pixee.security.ZipSecurity; + ... +- var zip = new ZipInputStream(is, StandardCharsets.UTF_8); ++ var zip = ZipSecurity.createHardenedInputStream(is, StandardCharsets.UTF_8); +``` diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/report.json b/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/report.json new file mode 100644 index 000000000..15913b6aa --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/report.json @@ -0,0 +1,7 @@ +{ + "summary" : "Introduced protections against \"zip slip\" attacks", + "change" : "Created a hardened `java.io.ZipInputStream` wrapper type that prevents files from being written that escape the target directory", + "reviewGuidanceIJustification" : "We believe this change is safe and effective. The behavior of hardened `ZipInputStream` instances will only be different if malicious zip entries are encountered.", + "control" : "https://github.com/pixee/java-security-toolkit/blob/main/src/main/java/io/github/pixee/security/ZipSecurity.java", + "references": ["https://snyk.io/research/zip-slip-vulnerability", "https://github.com/snyk/zip-slip-vulnerability", "https://wiki.sei.cmu.edu/confluence/display/java/IDS04-J.+Safely+extract+files+from+ZipInputStream", "https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.path_manipulation_zip_entry_overwrite"] +} diff --git a/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/LLMDiffs.java b/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/LLMDiffs.java index d694d10b7..1910f4be1 100644 --- a/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/LLMDiffs.java +++ b/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/LLMDiffs.java @@ -79,7 +79,7 @@ private static List fixDiffWhitespace( // Save the starting line number for the new hunk. start = Integer.parseInt(m.group(1)); } else { - hunk.add(line.length() > 0 ? line : " "); + hunk.add(!line.isEmpty() ? line : " "); } } diff --git a/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/SarifPluginLLMCodemod.java b/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/SarifPluginLLMCodemod.java index 089ecc51f..8df96dfba 100644 --- a/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/SarifPluginLLMCodemod.java +++ b/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/SarifPluginLLMCodemod.java @@ -6,9 +6,10 @@ /** A base class for LLM codemods that process SARIF and use the OpenAI service. */ public abstract class SarifPluginLLMCodemod extends SarifPluginRawFileChanger { + protected final OpenAIService openAI; - public SarifPluginLLMCodemod(RuleSarif sarif, final OpenAIService openAI) { + protected SarifPluginLLMCodemod(RuleSarif sarif, final OpenAIService openAI) { super(sarif); this.openAI = Objects.requireNonNull(openAI); }