-
Notifications
You must be signed in to change notification settings - Fork 726
Add Java path traversal detection for archive and derived paths #5591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| import java.io.File; | ||
| import java.io.FileOutputStream; | ||
| import java.util.zip.ZipEntry; | ||
|
|
||
| class ArchiveEntryPathTraversalCheckSample { | ||
| void bad(ZipEntry ze, File destDir) throws Exception { | ||
| String fileName = ze.getName(); | ||
| File newFile = new File(destDir, fileName); // Noncompliant | ||
| FileOutputStream fos = new FileOutputStream(newFile); | ||
| } | ||
|
|
||
| void good(File destDir) throws Exception { | ||
| String fileName = "safe.txt"; | ||
| File newFile = new File(destDir, fileName); | ||
| FileOutputStream fos = new FileOutputStream(newFile); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| import java.net.URL; | ||
|
|
||
| class CVE_2022_29253_ClassLoaderPathTraversal { | ||
| URL getClassloaderTemplate(ClassLoader classloader, String suffixPath, String templateName) { | ||
| String templatePath = suffixPath + templateName; | ||
| return classloader.getResource(templatePath); // Noncompliant | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| import java.io.File; | ||
|
|
||
| class CVE_2022_31194_RequestPathTraversal { | ||
| void upload(Request request, String tempDir) { | ||
| String resumableIdentifier = request.getParameter("resumableIdentifier"); | ||
| tempDir = tempDir + File.separator + resumableIdentifier; | ||
| File fileDir = new File(tempDir); // Noncompliant | ||
| if (!fileDir.exists()) { | ||
| fileDir.mkdir(); | ||
| } | ||
| } | ||
| interface Request { String getParameter(String name); } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| import java.io.File; | ||
| import java.io.FileOutputStream; | ||
| import java.util.zip.ZipEntry; | ||
| import java.util.zip.ZipInputStream; | ||
|
|
||
| class CVE_2022_39367_ArchiveEntryPathTraversal { | ||
| void unpack(ZipInputStream zipInputStream, File importSandboxDirectory) throws Exception { | ||
| ZipEntry zipEntry; | ||
| while ((zipEntry = zipInputStream.getNextEntry()) != null) { | ||
| final File destFile = new File(importSandboxDirectory, zipEntry.getName()); // Noncompliant | ||
| ensureFileCreated(destFile); | ||
| final FileOutputStream destOutputStream = new FileOutputStream(destFile); | ||
| destOutputStream.write(1); | ||
| destOutputStream.close(); | ||
| } | ||
| } | ||
| void ensureFileCreated(File f) {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| import java.io.File; | ||
| import java.io.FileOutputStream; | ||
| import java.util.zip.ZipEntry; | ||
|
|
||
| class CVE_2022_4494_ArchiveEntryPathTraversal { | ||
| void extract(ZipEntry ze, File destDir) throws Exception { | ||
| String fileName = ze.getName(); | ||
| File newFile = new File(destDir, fileName); // Noncompliant | ||
| if (ze.isDirectory()) { | ||
| newFile.mkdirs(); | ||
| } else { | ||
| FileOutputStream fos = new FileOutputStream(newFile); | ||
| fos.write(1); | ||
| fos.close(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,174 @@ | ||||||||||
| /* | ||||||||||
| * SonarQube Java | ||||||||||
| * Copyright (C) SonarSource Sàrl | ||||||||||
| * mailto:info AT sonarsource DOT com | ||||||||||
| * | ||||||||||
| * You can redistribute and/or modify this program under the terms of | ||||||||||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||||||||||
| * | ||||||||||
| * This program is distributed in the hope that it will be useful, | ||||||||||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||||||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||||||||||
| * See the Sonar Source-Available License for more details. | ||||||||||
| * | ||||||||||
| * You should have received a copy of the Sonar Source-Available License | ||||||||||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||||||||||
| */ | ||||||||||
| package org.sonar.java.checks; | ||||||||||
|
|
||||||||||
| import java.util.HashSet; | ||||||||||
| import java.util.List; | ||||||||||
| import java.util.Set; | ||||||||||
| import org.sonar.check.Rule; | ||||||||||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||||||||||
| import org.sonar.plugins.java.api.tree.MethodTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.BinaryExpressionTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.ExpressionTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.IdentifierTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.MethodInvocationTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.NewClassTree; | ||||||||||
| import org.sonar.plugins.java.api.tree.Tree; | ||||||||||
| import org.sonar.plugins.java.api.tree.VariableTree; | ||||||||||
|
|
||||||||||
| @Rule(key = "S7099") | ||||||||||
| public class ArchiveEntryPathTraversalCheck extends IssuableSubscriptionVisitor { | ||||||||||
|
|
||||||||||
| private static final String MESSAGE = "Validate attacker-controlled path components before using them in file paths or resource lookups."; | ||||||||||
| private static final Set<String> PATH_LIKE_PARAMETER_NAMES = Set.of( | ||||||||||
| "pathname", "path", "filename", "file", "dirname", "dir", "templatename", "template", "resource", "url"); | ||||||||||
|
|
||||||||||
| private final Set<String> taintedIdentifiers = new HashSet<>(); | ||||||||||
|
|
||||||||||
| @Override | ||||||||||
| public List<Tree.Kind> nodesToVisit() { | ||||||||||
| return List.of(Tree.Kind.METHOD, Tree.Kind.NEW_CLASS, Tree.Kind.VARIABLE, Tree.Kind.ASSIGNMENT, Tree.Kind.METHOD_INVOCATION); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| @Override | ||||||||||
| public void setContext(org.sonar.plugins.java.api.JavaFileScannerContext context) { | ||||||||||
| super.setContext(context); | ||||||||||
| taintedIdentifiers.clear(); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| @Override | ||||||||||
| public void visitNode(Tree tree) { | ||||||||||
| if (tree.is(Tree.Kind.METHOD)) { | ||||||||||
| checkMethod((MethodTree) tree); | ||||||||||
| } else if (tree.is(Tree.Kind.NEW_CLASS)) { | ||||||||||
| checkNewClass((NewClassTree) tree); | ||||||||||
| } else if (tree.is(Tree.Kind.VARIABLE)) { | ||||||||||
| checkVariable((VariableTree) tree); | ||||||||||
| } else if (tree.is(Tree.Kind.ASSIGNMENT)) { | ||||||||||
| checkAssignment((AssignmentExpressionTree) tree); | ||||||||||
| } else if (tree.is(Tree.Kind.METHOD_INVOCATION)) { | ||||||||||
| checkMethodInvocation((MethodInvocationTree) tree); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private void checkMethod(MethodTree tree) { | ||||||||||
| taintedIdentifiers.clear(); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. State corruption with nested methods and lambdas. Example pattern this silently misses: void unzip(ZipEntry ze, File dir) {
String name = ze.getName(); // tainted
list.forEach(item -> {}); // anonymous method fires checkMethod() → taintedIdentifiers cleared
File f = new File(dir, name); // name is no longer considered tainted — missed!
}The conventional fix is to use a stack: push the current taint set when entering a method and pop it when leaving via
|
||||||||||
| tree.parameters().stream() | ||||||||||
| .map(VariableTree::simpleName) | ||||||||||
| .filter(id -> id != null && isPathLikeName(id.name())) | ||||||||||
| .forEach(id -> taintedIdentifiers.add(id.name())); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private void checkNewClass(NewClassTree tree) { | ||||||||||
| String type = tree.identifier().symbolType().fullyQualifiedName(); | ||||||||||
| if ("java.io.File".equals(type) && !tree.arguments().isEmpty()) { | ||||||||||
| ExpressionTree pathArg = tree.arguments().get(tree.arguments().size() - 1); | ||||||||||
| if (isTainted(pathArg)) { | ||||||||||
| reportIssue(pathArg, MESSAGE); | ||||||||||
| } | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| if (("java.io.FileOutputStream".equals(type) || "java.io.FileInputStream".equals(type)) && !tree.arguments().isEmpty()) { | ||||||||||
| ExpressionTree pathArg = tree.arguments().get(0); | ||||||||||
| if (isTainted(pathArg)) { | ||||||||||
| reportIssue(pathArg, MESSAGE); | ||||||||||
| } | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private void checkVariable(VariableTree tree) { | ||||||||||
| ExpressionTree initializer = tree.initializer(); | ||||||||||
| if (initializer != null && tree.simpleName() != null && isTainted(initializer)) { | ||||||||||
| taintedIdentifiers.add(tree.simpleName().name()); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private void checkAssignment(AssignmentExpressionTree tree) { | ||||||||||
| if (tree.variable() instanceof IdentifierTree identifier && isTainted(tree.expression())) { | ||||||||||
| taintedIdentifiers.add(identifier.name()); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private void checkMethodInvocation(MethodInvocationTree tree) { | ||||||||||
| if (!(tree.methodSelect() instanceof MemberSelectExpressionTree mse)) { | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| String methodName = mse.identifier().name(); | ||||||||||
| if (("mkdir".equals(methodName) || "mkdirs".equals(methodName) || "createNewFile".equals(methodName)) | ||||||||||
| && isTainted(mse.expression())) { | ||||||||||
| reportIssue(mse.identifier(), MESSAGE); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| if ("getResource".equals(methodName) && !tree.arguments().isEmpty() && isTainted(tree.arguments().get(0))) { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||
| reportIssue(tree.arguments().get(0), MESSAGE); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private boolean isTainted(ExpressionTree expr) { | ||||||||||
| if (expr == null) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| if (isArchiveEntryGetName(expr) || isRequestGetParameter(expr)) { | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logic duplication: if (expr instanceof MethodInvocationTree mit) {
return mit.arguments().stream().anyMatch(this::isTainted);
}(Note: the argument-propagation line itself should also be reconsidered per the separate comment above.)
Suggested change
|
||||||||||
| return true; | ||||||||||
| } | ||||||||||
| if (expr instanceof IdentifierTree identifier) { | ||||||||||
| return taintedIdentifiers.contains(identifier.name()); | ||||||||||
| } | ||||||||||
| if (expr instanceof BinaryExpressionTree binary) { | ||||||||||
| return isTainted(binary.leftOperand()) || isTainted(binary.rightOperand()); | ||||||||||
| } | ||||||||||
| if (expr instanceof MethodInvocationTree mit) { | ||||||||||
| if (isArchiveEntryGetName(mit) || isRequestGetParameter(mit)) { | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
| return mit.arguments().stream().anyMatch(this::isTainted); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taint propagates through sanitization methods, causing false positives. String safe = FilenameUtils.getName(zipEntry.getName()); // sanitizes, strips path separators
File f = new File(destDir, safe); // Noncompliant — false positiveThis heuristic will fire for any helper method, logger, or utility that happens to receive a tainted value. It should be removed. Taint should only propagate through explicit data-flow nodes (identifier reads, binary concatenation), not through opaque method calls.
|
||||||||||
| } | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private static boolean isArchiveEntryGetName(ExpressionTree expr) { | ||||||||||
| if (!(expr instanceof MethodInvocationTree mit)) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| if (!(mit.methodSelect() instanceof MemberSelectExpressionTree mse)) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| if (!"getName".equals(mse.identifier().name())) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| String owner = mse.expression().symbolType().fullyQualifiedName(); | ||||||||||
| return "java.util.zip.ZipEntry".equals(owner) || "java.util.jar.JarEntry".equals(owner); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private static boolean isRequestGetParameter(ExpressionTree expr) { | ||||||||||
| if (!(expr instanceof MethodInvocationTree mit)) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| if (!(mit.methodSelect() instanceof MemberSelectExpressionTree mse)) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| return "getParameter".equals(mse.identifier().name()); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Use private static final MethodMatchers REQUEST_GET_PARAMETER = MethodMatchers.create()
.ofSubTypes("javax.servlet.ServletRequest", "jakarta.servlet.ServletRequest")
.names("getParameter")
.withAnyParameters()
.build();
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| private static boolean isPathLikeName(String name) { | ||||||||||
| name = name.toLowerCase(); | ||||||||||
| return PATH_LIKE_PARAMETER_NAMES.stream().anyMatch(name::contains); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In any real enterprise codebase, this will generate a significant volume of false positives for parameters that have nothing to do with file paths. Either switch to whole-word matching (compare after splitting on camelCase boundaries) or tighten the list to fewer, more precise tokens like
|
||||||||||
| } | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.sonar.java.checks.verifier.CheckVerifier; | ||
|
|
||
| import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; | ||
|
|
||
| class ArchiveEntryPathTraversalCheckTest { | ||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/ArchiveEntryPathTraversalCheckSample.java")) | ||
| .withCheck(new ArchiveEntryPathTraversalCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void cve_2022_4494() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/CVE_2022_4494_ArchiveEntryPathTraversal.java")) | ||
| .withCheck(new ArchiveEntryPathTraversalCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void cve_2022_39367() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/CVE_2022_39367_ArchiveEntryPathTraversal.java")) | ||
| .withCheck(new ArchiveEntryPathTraversalCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void cve_2022_31194() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/CVE_2022_31194_RequestPathTraversal.java")) | ||
| .withCheck(new ArchiveEntryPathTraversalCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void cve_2022_29253() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/CVE_2022_29253_ClassLoaderPathTraversal.java")) | ||
| .withCheck(new ArchiveEntryPathTraversalCheck()) | ||
| .verifyIssues(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
good()method tests the safe case, but there is no// Compliantannotation and the test class does not callverifyNoIssues()on a separate file (orverifyIssues()with only the bad case). Looking at the test inArchiveEntryPathTraversalCheckTest, all five test methods callverifyIssues()— which asserts that every// Noncompliantmarker matches a reported issue AND that no additional issues exist in the file. So thegood()path is implicitly validated (an unexpected issue there would fail the test). This is fine, but it relies onverifyIssues()semantics rather than an explicitverifyNoIssues()call. Add a// Compliantcomment to make the intent self-documenting.