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 d120ccb4d..36232b293 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 @@ -20,6 +20,7 @@ public enum GenericRemediationMetadata { REGEX_INJECTION("regex-injection"), ERROR_MESSAGE_EXPOSURE("error-message-exposure"), LOG_INJECTION("log-injection"), + PATH_TRAVERSAL("path-traversal"), WEAK_CRYPTO_ALGORITHM("weak-crypto-algorithm"); private final CodemodReporterStrategy reporter; diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/pathtraversal/PathTraversalRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/pathtraversal/PathTraversalRemediator.java new file mode 100644 index 000000000..828165c81 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/pathtraversal/PathTraversalRemediator.java @@ -0,0 +1,60 @@ +package io.codemodder.remediation.pathtraversal; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.MethodCallExpr; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.SearcherStrategyRemediator; +import java.util.Collection; +import java.util.Optional; +import java.util.function.Function; + +/** Remediate path traversal vulns. */ +public final class PathTraversalRemediator implements Remediator { + + private final SearcherStrategyRemediator searchStrategyRemediator; + + public PathTraversalRemediator() { + this.searchStrategyRemediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher( + node -> + Optional.of(node) + .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null) + .filter(PathTraversalRemediator::isSpringMultipartFilenameCall) + .isPresent()) + .build(), + new SpringMultipartFixStrategy()) + .build(); + } + + @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) { + return searchStrategyRemediator.remediateAll( + cu, + path, + detectorRule, + findingsForPath, + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingStartColumnExtractor); + } + + private static boolean isSpringMultipartFilenameCall(final MethodCallExpr methodCallExpr) { + return methodCallExpr.hasScope() + && "getOriginalFilename".equals(methodCallExpr.getNameAsString()); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/pathtraversal/SpringMultipartFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/pathtraversal/SpringMultipartFixStrategy.java new file mode 100644 index 000000000..b1cef487f --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/pathtraversal/SpringMultipartFixStrategy.java @@ -0,0 +1,24 @@ +package io.codemodder.remediation.pathtraversal; + +import static io.codemodder.javaparser.JavaParserTransformer.wrap; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.MethodCallExpr; +import io.codemodder.remediation.RemediationStrategy; +import io.codemodder.remediation.SuccessOrReason; +import io.github.pixee.security.Filenames; + +/** + * Fix strategy for Spring MultipartFile getOriginalFilename() calls which wraps with + * java-security-toolkit API for guaranteeing a simple file name. + */ +final class SpringMultipartFixStrategy implements RemediationStrategy { + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + MethodCallExpr methodCallExpr = (MethodCallExpr) node; + boolean success = + wrap(methodCallExpr).withStaticMethod(Filenames.class.getName(), "toSimpleFileName", false); + return success ? SuccessOrReason.success() : SuccessOrReason.reason("Wrap failed"); + } +} diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/path-traversal/description.md b/framework/codemodder-base/src/main/resources/generic-remediation-reports/path-traversal/description.md new file mode 100644 index 000000000..732666deb --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/path-traversal/description.md @@ -0,0 +1,13 @@ +This change attempts to remediate path traversal (also called directory traversal, local file include, etc.) vulnerabilities. + +Our changes may introduce sanitization up front, if the input looks like it's a file name (like from a multipart HTTP request), or validation if it appears to be a piece of path. + + +```diff ++ import io.github.pixee.security.Filenames; + + ... + +- String fileName = request.getFileName(); ++ String fileName = Filenames.toSimpleFileName(request.getFileName()); +``` diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/path-traversal/report.json b/framework/codemodder-base/src/main/resources/generic-remediation-reports/path-traversal/report.json new file mode 100644 index 000000000..17ba7976d --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/path-traversal/report.json @@ -0,0 +1,8 @@ +{ + "summary" : "Remediate path traversal", + "change": "Added a validator/sanitizer", + "reviewGuidanceIJustification" : "These changes should be reviewed to ensure that the validator/sanitizer is correctly implemented and won't disrupt the application's functionality.", + "references" : [ + "https://cwe.mitre.org/data/definitions/35.html" + ] +} diff --git a/framework/codemodder-base/src/test/java/io/codemodder/RemediatorTestMixin.java b/framework/codemodder-base/src/test/java/io/codemodder/RemediatorTestMixin.java new file mode 100644 index 000000000..819ed0db5 --- /dev/null +++ b/framework/codemodder-base/src/test/java/io/codemodder/RemediatorTestMixin.java @@ -0,0 +1,89 @@ +package io.codemodder; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.Remediator; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.stream.Stream; +import org.junit.jupiter.api.DynamicTest; +import org.junit.jupiter.api.TestFactory; + +/** A mixin which provides basic structure for testing remediators. */ +public interface RemediatorTestMixin { + + @TestFactory + default Stream it_fixes_the_code() { + Stream fixableSamples = createFixableSamples(); + + return fixableSamples.map( + sample -> { + String beforeCode = sample.beforeCode(); + String afterCode = sample.afterCode(); + int line = sample.line(); + + return DynamicTest.dynamicTest( + beforeCode, + () -> { + CompilationUnit cu = StaticJavaParser.parse(beforeCode); + LexicalPreservingPrinter.setup(cu); + Remediator remediator = createRemediator(); + + DetectorRule rule = new DetectorRule("rule-123", "my-remediation-rule", null); + + CodemodFileScanningResult result = + remediator.remediateAll( + cu, + "foo", + rule, + List.of(new Object()), + f -> "123", + f -> line, + x -> Optional.empty(), + x -> Optional.empty()); + assertThat(result.unfixedFindings()).isEmpty(); + assertThat(result.changes()).hasSize(1); + CodemodChange change = result.changes().get(0); + assertThat(change.lineNumber()).isEqualTo(line); + + String actualCode = LexicalPreservingPrinter.print(cu); + assertThat(actualCode).isEqualToIgnoringCase(afterCode); + }); + }); + } + + /** Build the remediator to test. */ + Remediator createRemediator(); + + /** Create samples to test. */ + Stream createFixableSamples(); + + /** Create unfixable samples. */ + Stream createUnfixableSamples(); + + /** Represents a finding that can be fixed. */ + record FixableSample(String beforeCode, String afterCode, int line) { + public FixableSample { + Objects.requireNonNull(beforeCode); + Objects.requireNonNull(afterCode); + if (line < 0) { + throw new IllegalArgumentException("Line number must be non-negative"); + } + } + } + + /** Represents a finding that can't be fixed for some reason. */ + record UnfixableSample(String code, int line) { + public UnfixableSample { + Objects.requireNonNull(code); + if (line < 0) { + throw new IllegalArgumentException("Line number must be non-negative"); + } + } + } +} diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/pathtraversal/PathTraversalRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/pathtraversal/PathTraversalRemediatorTest.java new file mode 100644 index 000000000..7e034527a --- /dev/null +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/pathtraversal/PathTraversalRemediatorTest.java @@ -0,0 +1,72 @@ +package io.codemodder.remediation.pathtraversal; + +import io.codemodder.RemediatorTestMixin; +import io.codemodder.remediation.Remediator; +import java.util.stream.Stream; + +final class PathTraversalRemediatorTest implements RemediatorTestMixin { + + @Override + public Remediator createRemediator() { + return new PathTraversalRemediator<>(); + } + + @Override + public Stream createFixableSamples() { + return Stream.of( + new FixableSample( + """ + import org.springframework.web.multipart.MultipartFile; + public class Test { + public void test(MultipartFile file) { + String filename = file.getOriginalFilename(); + } + } + """, + """ + import io.github.pixee.security.Filenames; + import org.springframework.web.multipart.MultipartFile; + public class Test { + public void test(MultipartFile file) { + String filename = Filenames.toSimpleFilename(file.getOriginalFilename()); + } + } + """, + 4), + new FixableSample( + """ + import org.springframework.web.multipart.MultipartFile; + public class Test { + public void test(MultipartFile file) { + String filename = new File(dir, file.getOriginalFilename()); + } + } + """, + """ + import io.github.pixee.security.Filenames; + import org.springframework.web.multipart.MultipartFile; + public class Test { + public void test(MultipartFile file) { + String filename = new File(dir, Filenames.toSimpleFilename(file.getOriginalFilename())); + } + } + """, + 4)); + } + + @Override + public Stream createUnfixableSamples() { + return Stream.of( + // no getOriginalFilename() call + new UnfixableSample( + """ + import org.springframework.web.multipart.MultipartFile; + public class Test { + public void test(MultipartFile file) { + String filename = file.filename(); + } + } + """, + 4)); + } +}