From 4c7d476280b88b11642529e6ca8a4b3e33c9b8b6 Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Tue, 28 Jun 2022 13:52:41 -0400 Subject: [PATCH 01/34] [JAVA] Partial Path Traversal Vuln Query --- .../CWE/CWE-023/PartialPathTraversal.expected | 0 .../CWE/CWE-023/PartialPathTraversal.ql | 24 ++ .../tests/PartialPathTraversal.expected | 17 ++ .../semmle/tests/PartialPathTraversal.qlref | 1 + .../tests/PartialPathTraversalTest.java | 218 ++++++++++++++++++ 5 files changed, 260 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.expected create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql create mode 100644 java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected create mode 100644 java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.qlref create mode 100644 java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.expected b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql new file mode 100644 index 000000000000..5c5a74371bf8 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql @@ -0,0 +1,24 @@ +/** + * @name Partial Path Traversal Vulnerability + * @description A misuse of the String `startsWith` method as a guard to protect against path traversal is insufficient. + * @kind problem + * @problem.severity error + * @security-severity 9.3 + * @precision high + * @id java/partial-path-traversal + * @tags security + * external/cwe/cwe-023 + */ + +import java + + +class MethodStringStartsWith extends Method { + MethodStringStartsWith() { + this.hasName("startsWith") + } +} + +from MethodAccess ma +where ma.getMethod() instanceof MethodStringStartsWith +select ma, "Partial Path Traversal Vulnerability due to insufficient guard against path traversal" \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected new file mode 100644 index 000000000000..a19847468cd5 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected @@ -0,0 +1,17 @@ +| PartialPathTraversalTest.java:10:14:10:73 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:17:9:17:72 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:29:14:29:58 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:35:14:35:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:42:14:42:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:49:14:49:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:53:14:53:65 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:61:14:61:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:64:14:64:65 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:75:14:75:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:94:14:94:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:102:14:102:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:105:14:105:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:150:9:150:43 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:173:14:173:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:191:18:191:87 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | +| PartialPathTraversalTest.java:209:14:209:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.qlref b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.qlref new file mode 100644 index 000000000000..431556c90afa --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-023/PartialPathTraversal.ql \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java new file mode 100644 index 000000000000..d6f02ce0e5ce --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java @@ -0,0 +1,218 @@ +import java.io.IOException; +import java.io.File; +import java.io.InputStream; +import static java.io.File.separatorChar; +import java.nio.file.Files; + + +public class PartialPathTraversalTest { + public void esapiExample(File dir, File parent) throws IOException { + if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + @SuppressWarnings("ResultOfMethodCallIgnored") + void foo1(File dir, File parent) throws IOException { + (dir.getCanonicalPath()).startsWith((parent.getCanonicalPath())); + } + + void foo2(File dir, File parent) throws IOException { + dir.getCanonicalPath(); + if ("potato".startsWith(parent.getCanonicalPath())) { + System.out.println("Hello!"); + } + } + + void foo3(File dir, File parent) throws IOException { + String parentPath = parent.getCanonicalPath(); + if (!dir.getCanonicalPath().startsWith(parentPath)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo4(File dir) throws IOException { + if (!dir.getCanonicalPath().startsWith("/usr" + "/dir")) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo5(File dir, File parent) throws IOException { + String canonicalPath = dir.getCanonicalPath(); + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo6(File dir, File parent) throws IOException { + String canonicalPath = dir.getCanonicalPath(); + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + String canonicalPath2 = dir.getCanonicalPath(); + if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo7(File dir, File parent) throws IOException { + String canonicalPath = dir.getCanonicalPath(); + String canonicalPath2 = dir.getCanonicalPath(); + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + File getChild() { + return null; + } + + void foo8(File parent) throws IOException { + String canonicalPath = getChild().getCanonicalPath(); + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + getChild().getCanonicalPath()); + } + } + + void foo9(File dir, File parent) throws IOException { + if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separator)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo10(File dir, File parent) throws IOException { + if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separatorChar)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo11(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath(); + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo12(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath(); + String parentCanonical2 = parent.getCanonicalPath(); + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + if (!dir.getCanonicalPath().startsWith(parentCanonical2)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo13(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath() + File.separatorChar; + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo14(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath() + separatorChar; + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo15(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath() + File.separatorChar; + String parentCanonical2 = parent.getCanonicalPath() + File.separatorChar; + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + if (!dir.getCanonicalPath().startsWith(parentCanonical2)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo16(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath() + File.separator; + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + @SuppressWarnings({ + "IfStatementWithIdenticalBranches", + "MismatchedStringCase", + "UnusedAssignment", + "ResultOfMethodCallIgnored" + }) + void foo17(File dir, File parent, boolean branch) throws IOException { + String parentCanonical = null; + "test ".startsWith("somethingElse"); + if (branch) { + parentCanonical = parent.getCanonicalPath() + File.separatorChar; + } else { + parentCanonical = parent.getCanonicalPath() + File.separatorChar; + } + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo18(File dir, File parent, boolean branch) throws IOException { + String parentCanonical = parent.getCanonicalPath(); + if (branch) { + parentCanonical = parent.getCanonicalPath() + File.separatorChar; + } + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo19(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath() + "/potato"; + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + private File cacheDir; + + InputStream foo20(String... path) { + StringBuilder sb = new StringBuilder(); + sb.append(cacheDir.getAbsolutePath()); + for (String p : path) { + sb.append(File.separatorChar); + sb.append(p); + } + sb.append(".gz"); + String filePath = sb.toString(); + File encodedFile = new File(filePath); + try { + if (!encodedFile.getCanonicalPath().startsWith(cacheDir.getCanonicalPath())) { + return null; + } + return Files.newInputStream(encodedFile.toPath()); + } catch (Exception e) { + return null; + } + } + + void foo21(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath(); + if (!dir.getCanonicalPath().startsWith(parentCanonical + File.separator)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + void foo22(File dir, File dir2, File parent, boolean conditional) throws IOException { + String canonicalPath = conditional ? dir.getCanonicalPath() : dir2.getCanonicalPath(); + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + + public void doesNotFlag() { + "hello".startsWith("goodbye"); + } + +} \ No newline at end of file From 7122f2929666e17f171853165bfadab9c65cbfad Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Tue, 28 Jun 2022 15:02:06 -0400 Subject: [PATCH 02/34] Finish Partial Path Traversal Query --- .../CWE/CWE-023/PartialPathTraversal.expected | 0 .../CWE/CWE-023/PartialPathTraversal.ql | 48 +++++++++++++++---- .../tests/PartialPathTraversal.expected | 1 - .../tests/PartialPathTraversalTest.java | 7 +++ 4 files changed, 47 insertions(+), 9 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.expected diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.expected b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.expected deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql index 5c5a74371bf8..e19a95ac9952 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql @@ -10,15 +10,47 @@ * external/cwe/cwe-023 */ -import java - +import java +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.environment.SystemProperty class MethodStringStartsWith extends Method { - MethodStringStartsWith() { - this.hasName("startsWith") - } + MethodStringStartsWith() { + this.getDeclaringType() instanceof TypeString and + this.hasName("startsWith") + } +} + +class MethodFileGetCanonicalPath extends Method { + MethodFileGetCanonicalPath() { + this.getDeclaringType() instanceof TypeFile and + this.hasName("getCanonicalPath") + } } -from MethodAccess ma -where ma.getMethod() instanceof MethodStringStartsWith -select ma, "Partial Path Traversal Vulnerability due to insufficient guard against path traversal" \ No newline at end of file +class MethodAccessFileGetCanonicalPath extends MethodAccess { + MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath } +} + +abstract class FileSeparatorExpr extends Expr { } + +class SystemPropFileSeparatorExpr extends FileSeparatorExpr { + SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") } +} + +class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { + StringLiteralFileSeparatorExpr() { this.getValue() = "/" } +} + +class FileSeparatorAppend extends AddExpr { + FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr } +} + +predicate isSafe(Expr expr) { DataFlow::localExprFlow(any(FileSeparatorAppend fsa), expr) } + +from MethodAccess ma +where + ma.getMethod() instanceof MethodStringStartsWith and + DataFlow::localExprFlow(any(MethodAccessFileGetCanonicalPath gcpma), ma.getQualifier()) and + not isSafe(ma.getArgument(0)) +select ma, "Partial Path Traversal Vulnerability due to insufficient guard against path traversal" diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected index a19847468cd5..52a49e65a69a 100644 --- a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversal.expected @@ -11,7 +11,6 @@ | PartialPathTraversalTest.java:94:14:94:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | | PartialPathTraversalTest.java:102:14:102:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | | PartialPathTraversalTest.java:105:14:105:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | -| PartialPathTraversalTest.java:150:9:150:43 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | | PartialPathTraversalTest.java:173:14:173:63 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | | PartialPathTraversalTest.java:191:18:191:87 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | | PartialPathTraversalTest.java:209:14:209:64 | startsWith(...) | Partial Path Traversal Vulnerability due to insufficient guard against path traversal | diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java index d6f02ce0e5ce..afcc6d078cfd 100644 --- a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java @@ -211,6 +211,13 @@ void foo22(File dir, File dir2, File parent, boolean conditional) throws IOExcep } } + void foo23(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath(); + if (!dir.getCanonicalPath().startsWith(parentCanonical + "/")) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + public void doesNotFlag() { "hello".startsWith("goodbye"); } From b5ca2c3d9d9e97d4fb3b8eb707b2476a6827481e Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Tue, 28 Jun 2022 17:32:20 -0400 Subject: [PATCH 03/34] Add additional tests from real world query run --- .../CWE/CWE-023/PartialPathTraversal.qhelp | 46 +++++++++++++++++++ .../CWE/CWE-023/PartialPathTraversal.ql | 14 +++++- .../tests/PartialPathTraversalTest.java | 14 ++++++ 3 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp new file mode 100644 index 000000000000..d49214b530a4 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -0,0 +1,46 @@ + + + +

Accessing paths controlled by users can allow an attacker to access unexpected resources. This +can result in sensitive information being revealed or deleted, or an attacker being able to influence +behavior by modifying unexpected files.

+ +

Paths that are naively constructed from data controlled by a user may contain unexpected special characters, +such as "..". Such a path may potentially point to any directory on the file system.

+ + + + +

Validate user input before using it to construct a file path. Ideally, follow these rules:

+ +
    +
  • Do not allow more than a single "." character.
    • +
  • Do not allow directory separators such as "/" or "\" (depending on the file system).
    • +
  • Do not rely on simply replacing problematic sequences such as "../". For example, after applying this filter to +".../...//" the resulting string would still be "../".
    • +
  • Ideally use a whitelist of known good patterns.
    • +
+ + + + +

In this example, a file name is read from a java.net.Socket and then used to access a file in the +user's home directory and send it back over the socket. However, a malicious user could enter a file name which contains special +characters. For example, the string "../../etc/passwd" will result in the code reading the file located at +"/home/[user]/../../etc/passwd", which is the system's password file. This file would then be sent back to the user, +giving them access to all the system's passwords.

+ + + + + + +
  • +OWASP: +Path Traversal. +
    • + +     +     diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql index e19a95ac9952..a6588bf195c0 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql @@ -39,14 +39,24 @@ class SystemPropFileSeparatorExpr extends FileSeparatorExpr { } class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { - StringLiteralFileSeparatorExpr() { this.getValue() = "/" } + StringLiteralFileSeparatorExpr() { + this.getValue().matches("%/") or this.getValue().matches("%\\") + } +} + +class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral { + CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" } } class FileSeparatorAppend extends AddExpr { FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr } } -predicate isSafe(Expr expr) { DataFlow::localExprFlow(any(FileSeparatorAppend fsa), expr) } +predicate isSafe(Expr expr) { + DataFlow::localExprFlow(any(Expr e | + e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr + ), expr) +} from MethodAccess ma where diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java index afcc6d078cfd..8b25897bc8ef 100644 --- a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java @@ -218,8 +218,22 @@ void foo23(File dir, File parent) throws IOException { } } + void foo24(File dir, File parent) throws IOException { + String parentCanonical = parent.getCanonicalPath(); + if (!dir.getCanonicalPath().startsWith(parentCanonical + '/')) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } + public void doesNotFlag() { "hello".startsWith("goodbye"); } + public void doesNotFlagBackslash(File file) throws IOException { + // https://github.com/jenkinsci/jenkins/blob/be3cf6bffe7aa2fe2307c424fa418519f3bbd73b/core/src/main/java/hudson/util/jna/Kernel32Utils.java#L77-L77 + if (!file.getCanonicalPath().startsWith("\\\\")) { + throw new RuntimeException("Boom"); + } + } + } \ No newline at end of file From 955e61456300d31582c00a2cfff43357021604ef Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Wed, 29 Jun 2022 17:31:04 -0400 Subject: [PATCH 04/34] Add documentation of the Partial Path Traversal vuln --- .../CWE/CWE-023/PartialPathTraversal.qhelp | 55 ++++++++++++------- .../CWE/CWE-023/PartialPathTraversalBad.java | 7 +++ .../CWE/CWE-023/PartialPathTraversalGood.java | 9 +++ 3 files changed, 50 insertions(+), 21 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index d49214b530a4..f747c483ac97 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,43 +3,56 @@ "qhelp.dtd"> -

    Accessing paths controlled by users can allow an attacker to access unexpected resources. This -can result in sensitive information being revealed or deleted, or an attacker being able to influence -behavior by modifying unexpected files.

    +

    User inputted file paths can often pose security risks if a program does not correctly handle them. In particular, if a user +is meant to access files under a certain directory but does not enters a path under that directory, they can gain access to +(and potentially modify/delete) unexpected, possibly sensitive resources.

    -

    Paths that are naively constructed from data controlled by a user may contain unexpected special characters, -such as "..". Such a path may potentially point to any directory on the file system.

    +

    Suppose a program is to only accept paths that point to files/folders within directory DIR. +To ensure that a user inputted path, say SUBDIR, is a subdirectory of DIR, the +program verifies that DIR is a prefix of SUBDIR. +However, this check is not satisfactory: unless DIR is not slash-terminated, +SUBDIR may be allowed to also access siblings of DIR and not +just children of DIR, which is a security issue.

    -

    Validate user input before using it to construct a file path. Ideally, follow these rules:

    - -
      -
    • Do not allow more than a single "." character.
      • -
    • Do not allow directory separators such as "/" or "\" (depending on the file system).
      • -
    • Do not rely on simply replacing problematic sequences such as "../". For example, after applying this filter to -".../...//" the resulting string would still be "../".
      • -
    • Ideally use a whitelist of known good patterns.
      • -
    +

    If the user should only access items within a certain directory DIR, first ensure that DIR is slash-terminated, +and then proceed (as normal) to verify that DIR is a prefix of the user-provided path, SUBDIR. Note, Java's getCanonicalPath() +returns a non-slash-terminated path string, so a "/" must be added to DIR if that method is used.

    -

    In this example, a file name is read from a java.net.Socket and then used to access a file in the -user's home directory and send it back over the socket. However, a malicious user could enter a file name which contains special -characters. For example, the string "../../etc/passwd" will result in the code reading the file located at -"/home/[user]/../../etc/passwd", which is the system's password file. This file would then be sent back to the user, -giving them access to all the system's passwords.

    +

    + + +In this example, the if statement checks if parent.getCanonicalPath() +is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is +not slash-terminated. So, the user that supplies dir may be allowed to access siblings of parent +and not just children of parent, which is a security issue. + +

    + + + +

    + +In this example, the if statement checks if parent.getCanonicalPath() + File.separator +is a prefix of dir.getCanonicalPath(). Because parent.getCanonicalPath() + File.separator is +indeed slash-terminated, the user supplying dir can only access children of +parent, as desired. + +

    - +
  • OWASP: -Path Traversal. +Partial Path Traversal.
    • diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java new file mode 100644 index 000000000000..633a9046f0a4 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java @@ -0,0 +1,7 @@ +public class PartialPathTraversalBad { + public void esapiExample(File dir, File parent) throws IOException { + if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } +} diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java new file mode 100644 index 000000000000..365c71d70c8c --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java @@ -0,0 +1,9 @@ +import java.io.File; + +public class PartialPathTraversalBad { + public void esapiExample(File dir, File parent) throws IOException { + if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separator)) { + throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + } + } +} From 7ab8f0262c533e0f3f3cfe7b5cf3f32b7d6484ad Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Wed, 29 Jun 2022 18:01:12 -0400 Subject: [PATCH 05/34] Fix duplicate class header and better fix using toPath() --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 2 +- .../ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index f747c483ac97..be21879b3e26 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -39,7 +39,7 @@ and not just children of parent, which is a security issue.

    In this example, the if statement checks if parent.getCanonicalPath() + File.separator -is a prefix of dir.getCanonicalPath(). Because parent.getCanonicalPath() + File.separator is +is a prefix of dir.getCanonicalPath(). Because parent.getCanonicalPath().toPath() is indeed slash-terminated, the user supplying dir can only access children of parent, as desired. diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java index 365c71d70c8c..daf4adeca577 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java @@ -1,8 +1,8 @@ import java.io.File; -public class PartialPathTraversalBad { +public class PartialPathTraversalGood { public void esapiExample(File dir, File parent) throws IOException { - if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separator)) { + if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath().toPath())) { throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } From 16814071df1d58a82479986362fc37bc9167c71b Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Wed, 29 Jun 2022 18:03:57 -0400 Subject: [PATCH 06/34] Fix typo in .qhelp --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index be21879b3e26..741690bc9047 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -28,7 +28,7 @@ returns a non-slash-terminated path string, so a "/" must be In this example, the if statement checks if parent.getCanonicalPath() -is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is +is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is not slash-terminated. So, the user that supplies dir may be allowed to access siblings of parent and not just children of parent, which is a security issue. From c6f2f61bfb75e0286469065a2e5a7f6c12db640d Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Fri, 1 Jul 2022 10:39:46 -0400 Subject: [PATCH 07/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java Co-authored-by: Jonathan Leitschuh --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java index 633a9046f0a4..e933679aa449 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java @@ -1,5 +1,5 @@ public class PartialPathTraversalBad { - public void esapiExample(File dir, File parent) throws IOException { + public void example(File dir, File parent) throws IOException { if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } From 209a21655ade3906cbf0b10f9c43f1c389716cd6 Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Fri, 1 Jul 2022 10:40:38 -0400 Subject: [PATCH 08/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java Co-authored-by: Jonathan Leitschuh --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java index daf4adeca577..2051460c8ba9 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java @@ -1,7 +1,7 @@ import java.io.File; public class PartialPathTraversalGood { - public void esapiExample(File dir, File parent) throws IOException { + public void example(File dir, File parent) throws IOException { if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath().toPath())) { throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } From 300a14c35cddbfe08c21af6ba71f6c9ed2cec7ec Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Fri, 1 Jul 2022 10:43:59 -0400 Subject: [PATCH 09/34] Add ESAPI reference --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 741690bc9047..8cf0bc6fd769 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -53,6 +53,7 @@ indeed slash-terminated, the user supplying dir can only access chi

  • OWASP: Partial Path Traversal. + ESAPI
    • From 1a41d4c3794927d694c47995bcd826eceefc9441 Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Fri, 1 Jul 2022 10:51:33 -0400 Subject: [PATCH 10/34] Add CVE number --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 8cf0bc6fd769..18f7648a570a 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -53,7 +53,8 @@ indeed slash-terminated, the user supplying dir can only access chi
  • OWASP: Partial Path Traversal. - ESAPI +CVE-2022-23457: + ESAPI Vulnerability Report
    • From 48e16e52b5601b573b481370c9c92de718e1db76 Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Fri, 1 Jul 2022 10:52:41 -0400 Subject: [PATCH 11/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Co-authored-by: Jonathan Leitschuh --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 18f7648a570a..4e900e383efc 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,7 +3,7 @@ "qhelp.dtd"> -

    User inputted file paths can often pose security risks if a program does not correctly handle them. In particular, if a user +

    User supplied file paths can often pose security risks if a program does not correctly handle them. In particular, if a user is meant to access files under a certain directory but does not enters a path under that directory, they can gain access to (and potentially modify/delete) unexpected, possibly sensitive resources.

    From ebe48ec30a337de6bde644b513f8d2a2c6b3a5d2 Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Fri, 1 Jul 2022 10:53:43 -0400 Subject: [PATCH 12/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Co-authored-by: Jonathan Leitschuh --- .../Security/CWE/CWE-023/PartialPathTraversal.qhelp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 4e900e383efc..e70b27a86282 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -7,12 +7,12 @@ is meant to access files under a certain directory but does not enters a path under that directory, they can gain access to (and potentially modify/delete) unexpected, possibly sensitive resources.

    -

    Suppose a program is to only accept paths that point to files/folders within directory DIR. -To ensure that a user inputted path, say SUBDIR, is a subdirectory of DIR, the -program verifies that DIR is a prefix of SUBDIR. -However, this check is not satisfactory: unless DIR is not slash-terminated, +

    Suppose a program is to only accept paths that point to files/folders within directory DIR. +To ensure that a user supplied path, say SUBDIR, is a subdirectory of DIR, the +program verifies, using string comparisons, that DIR is a prefix of SUBDIR. +However, if DIR is not slash-terminated, such a check would not be sufficient. SUBDIR may be allowed to also access siblings of DIR and not -just children of DIR, which is a security issue.

    +just children of DIR, which is a security vulnerability.

     From 391dd5b38d29d4db421dec26f4474514596b932d Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Fri, 1 Jul 2022 10:55:58 -0400 Subject: [PATCH 13/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java Co-authored-by: Jonathan Leitschuh --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java index 2051460c8ba9..859e759e088d 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java @@ -2,7 +2,7 @@ public class PartialPathTraversalGood { public void example(File dir, File parent) throws IOException { - if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath().toPath())) { + if (!dir.getCanonicalPath().toPath().startsWith(parent.getCanonicalPath().toPath())) { throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } From 39f885413f1d149ca2a23b7fb011a6e37ac67c1e Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Fri, 1 Jul 2022 11:34:27 -0400 Subject: [PATCH 14/34] Change log --- .../ql/src/change-notes/2022-07-01-partial-path-traversal.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 java/ql/src/change-notes/2022-07-01-partial-path-traversal.md diff --git a/java/ql/src/change-notes/2022-07-01-partial-path-traversal.md b/java/ql/src/change-notes/2022-07-01-partial-path-traversal.md new file mode 100644 index 000000000000..13bc1cdb902d --- /dev/null +++ b/java/ql/src/change-notes/2022-07-01-partial-path-traversal.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* A new query `java/partial-path-traversal` finds partial path traversal vulnerabilities resulting from incorrectly using +`String#startsWith` to compare canonical files. From 65b9947428fffd8496989bbbc39152f189bef8b9 Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Tue, 12 Jul 2022 01:58:33 -0400 Subject: [PATCH 15/34] Incorporate jksco's feedback --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index e70b27a86282..7a11f3a0f71e 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,9 +3,9 @@ "qhelp.dtd"> -

    User supplied file paths can often pose security risks if a program does not correctly handle them. In particular, if a user -is meant to access files under a certain directory but does not enters a path under that directory, they can gain access to -(and potentially modify/delete) unexpected, possibly sensitive resources.

    +

    User supplied file paths can often pose security risks if a program does not handle them correctly. In particular, if a user +is meant to access files under a certain directory but does not enter a path under that directory, they can +unexpectedly gain access to (and potentially modify/delete) possibly sensitive resources.

    Suppose a program is to only accept paths that point to files/folders within directory DIR. To ensure that a user supplied path, say SUBDIR, is a subdirectory of DIR, the From 01cec0490beca9b8fc5b92a2f6a1be03902ed616 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 13 Jul 2022 20:24:44 +0100 Subject: [PATCH 16/34] Abbreviate qhelp --- .../CWE/CWE-023/PartialPathTraversal.qhelp | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 7a11f3a0f71e..902d0fc682cf 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,23 +3,15 @@ "qhelp.dtd"> -

    User supplied file paths can often pose security risks if a program does not handle them correctly. In particular, if a user -is meant to access files under a certain directory but does not enter a path under that directory, they can -unexpectedly gain access to (and potentially modify/delete) possibly sensitive resources.

    - -

    Suppose a program is to only accept paths that point to files/folders within directory DIR. -To ensure that a user supplied path, say SUBDIR, is a subdirectory of DIR, the -program verifies, using string comparisons, that DIR is a prefix of SUBDIR. -However, if DIR is not slash-terminated, such a check would not be sufficient. -SUBDIR may be allowed to also access siblings of DIR and not -just children of DIR, which is a security vulnerability.

    - +

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR +is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR +is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

     -

    If the user should only access items within a certain directory DIR, first ensure that DIR is slash-terminated, -and then proceed (as normal) to verify that DIR is a prefix of the user-provided path, SUBDIR. Note, Java's getCanonicalPath() -returns a non-slash-terminated path string, so a "/" must be added to DIR if that method is used.

    +

    If the user should only access items within a certain directory DIR, ensure that DIR is slash-terminated +before checking that DIR is a prefix of the user-provided path, SUBDIR. Note, Java's getCanonicalPath() +returns a non-slash-terminated path string, so a slash must be added to DIR if that method is used.

     From a6970638cb6ef50581c76dfb025ce2ad69a4d3b4 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 13 Jul 2022 20:27:10 +0100 Subject: [PATCH 17/34] Improve description --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql index a6588bf195c0..1caacbe8f2bd 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql @@ -1,6 +1,6 @@ /** * @name Partial Path Traversal Vulnerability - * @description A misuse of the String `startsWith` method as a guard to protect against path traversal is insufficient. + * @description A prefix used to check that a canonicalised path falls within another must be slash-terminated. * @kind problem * @problem.severity error * @security-severity 9.3 From b7e522749f68d3c1ecc9a19b281d66a4416911f2 Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Wed, 20 Jul 2022 15:32:59 -0400 Subject: [PATCH 18/34] Apply suggestions from code review Co-authored-by: Chris Smowton Co-authored-by: Anders Schack-Mulligen --- .../ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java | 2 -- java/ql/src/change-notes/2022-07-01-partial-path-traversal.md | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java index 859e759e088d..a36a7dccb8ad 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java @@ -1,5 +1,3 @@ -import java.io.File; - public class PartialPathTraversalGood { public void example(File dir, File parent) throws IOException { if (!dir.getCanonicalPath().toPath().startsWith(parent.getCanonicalPath().toPath())) { diff --git a/java/ql/src/change-notes/2022-07-01-partial-path-traversal.md b/java/ql/src/change-notes/2022-07-01-partial-path-traversal.md index 13bc1cdb902d..4dc9762bdd7c 100644 --- a/java/ql/src/change-notes/2022-07-01-partial-path-traversal.md +++ b/java/ql/src/change-notes/2022-07-01-partial-path-traversal.md @@ -1,5 +1,5 @@ --- -category: minorAnalysis +category: newQuery --- * A new query `java/partial-path-traversal` finds partial path traversal vulnerabilities resulting from incorrectly using -`String#startsWith` to compare canonical files. +`String#startsWith` to compare canonical paths. From 09ec37943c8904df1faec7543eda2385382df9dd Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Wed, 20 Jul 2022 17:39:57 -0400 Subject: [PATCH 19/34] Partial Path Traversal split into 2 queries --- .../java/security/PartialPathTraversal.qll | 55 +++++++++++++++++++ .../security/PartialPathTraversalQuery.qll | 16 ++++++ .../CWE/CWE-023/PartialPathTraversal.ql | 54 +----------------- .../CWE-023/PartialPathTraversalFromRemote.ql | 18 ++++++ ...artialPathTraversalFromRemoteTest.expected | 0 .../PartialPathTraversalFromRemoteTest.ql | 17 ++++++ .../tests/PartialPathTraversalTest.java | 34 ++++++------ 7 files changed, 125 insertions(+), 69 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll create mode 100644 java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql create mode 100644 java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll new file mode 100644 index 000000000000..0997bfb7f24a --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll @@ -0,0 +1,55 @@ +import java +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.environment.SystemProperty + +class MethodStringStartsWith extends Method { + MethodStringStartsWith() { + this.getDeclaringType() instanceof TypeString and + this.hasName("startsWith") + } +} + +class MethodFileGetCanonicalPath extends Method { + MethodFileGetCanonicalPath() { + this.getDeclaringType() instanceof TypeFile and + this.hasName("getCanonicalPath") + } +} + +class MethodAccessFileGetCanonicalPath extends MethodAccess { + MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath } +} + +abstract class FileSeparatorExpr extends Expr { } + +class SystemPropFileSeparatorExpr extends FileSeparatorExpr { + SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") } +} + +class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { + StringLiteralFileSeparatorExpr() { + this.getValue().matches("%/") or this.getValue().matches("%\\") + } +} + +class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral { + CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" } +} + +class FileSeparatorAppend extends AddExpr { + FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr } +} + +predicate isSafe(Expr expr) { + DataFlow::localExprFlow(any(Expr e | + e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr + ), expr) +} + +class PartialPathTraversalMethodAccess extends MethodAccess { + PartialPathTraversalMethodAccess() { + this.getMethod() instanceof MethodStringStartsWith and + DataFlow::localExprFlow(any(MethodAccessFileGetCanonicalPath gcpma), this.getQualifier()) and + not isSafe(this.getArgument(0)) + } +} diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll new file mode 100644 index 000000000000..3ca8aaa10740 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll @@ -0,0 +1,16 @@ +import java +import semmle.code.java.security.PartialPathTraversal +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.FlowSources + +class PartialPathTraversalFromRemoteConfig extends TaintTracking::Configuration { + PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" } + + override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node node) { + any(PartialPathTraversalMethodAccess ma).getQualifier() = node.asExpr() + } +} diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql index 1caacbe8f2bd..de426bf0d696 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql @@ -10,57 +10,7 @@ * external/cwe/cwe-023 */ -import java -private import semmle.code.java.dataflow.DataFlow -private import semmle.code.java.environment.SystemProperty +import semmle.code.java.security.PartialPathTraversal -class MethodStringStartsWith extends Method { - MethodStringStartsWith() { - this.getDeclaringType() instanceof TypeString and - this.hasName("startsWith") - } -} - -class MethodFileGetCanonicalPath extends Method { - MethodFileGetCanonicalPath() { - this.getDeclaringType() instanceof TypeFile and - this.hasName("getCanonicalPath") - } -} - -class MethodAccessFileGetCanonicalPath extends MethodAccess { - MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath } -} - -abstract class FileSeparatorExpr extends Expr { } - -class SystemPropFileSeparatorExpr extends FileSeparatorExpr { - SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") } -} - -class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { - StringLiteralFileSeparatorExpr() { - this.getValue().matches("%/") or this.getValue().matches("%\\") - } -} - -class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral { - CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" } -} - -class FileSeparatorAppend extends AddExpr { - FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr } -} - -predicate isSafe(Expr expr) { - DataFlow::localExprFlow(any(Expr e | - e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr - ), expr) -} - -from MethodAccess ma -where - ma.getMethod() instanceof MethodStringStartsWith and - DataFlow::localExprFlow(any(MethodAccessFileGetCanonicalPath gcpma), ma.getQualifier()) and - not isSafe(ma.getArgument(0)) +from PartialPathTraversalMethodAccess ma select ma, "Partial Path Traversal Vulnerability due to insufficient guard against path traversal" diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql new file mode 100644 index 000000000000..9003003f4a26 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql @@ -0,0 +1,18 @@ +/** + * @name Partial Path Traversal Vulnerability From Remote + * @description A prefix used to check that a canonicalised path falls within another must be slash-terminated. + * @kind path-problem + * @problem.severity error + * @security-severity 9.3 + * @precision high + * @id java/partial-path-traversal-from-remote + * @tags security + * external/cwe/cwe-023 + */ + +import semmle.code.java.security.PartialPathTraversalQuery + +from DataFlow::PathNode source, DataFlow::PathNode sink +where any(PartialPathTraversalFromRemoteConfig config).hasFlowPath(source, sink) +select sink.getNode(), source, sink, + "Partial Path Traversal Vulnerability due to insufficient guard against path traversal from user-supplied data" diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.expected b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql new file mode 100644 index 000000000000..a0f82a60cc33 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql @@ -0,0 +1,17 @@ +import java +import TestUtilities.InlineFlowTest +import semmle.code.java.security.PartialPathTraversalQuery + +class TestRemoteSource extends RemoteFlowSource { + TestRemoteSource() { this.asParameter().hasName(["dir", "path"]) } + + override string getSourceType() { result = "TestSource" } +} + +class Test extends InlineFlowTest { + override DataFlow::Configuration getValueFlowConfig() { none() } + + override TaintTracking::Configuration getTaintFlowConfig() { + result instanceof PartialPathTraversalFromRemoteConfig + } +} diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java index 8b25897bc8ef..b7c0256d0756 100644 --- a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalTest.java @@ -7,14 +7,14 @@ public class PartialPathTraversalTest { public void esapiExample(File dir, File parent) throws IOException { - if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { + if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } @SuppressWarnings("ResultOfMethodCallIgnored") void foo1(File dir, File parent) throws IOException { - (dir.getCanonicalPath()).startsWith((parent.getCanonicalPath())); + (dir.getCanonicalPath()).startsWith((parent.getCanonicalPath())); // $hasTaintFlow } void foo2(File dir, File parent) throws IOException { @@ -26,31 +26,31 @@ void foo2(File dir, File parent) throws IOException { void foo3(File dir, File parent) throws IOException { String parentPath = parent.getCanonicalPath(); - if (!dir.getCanonicalPath().startsWith(parentPath)) { + if (!dir.getCanonicalPath().startsWith(parentPath)) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } void foo4(File dir) throws IOException { - if (!dir.getCanonicalPath().startsWith("/usr" + "/dir")) { + if (!dir.getCanonicalPath().startsWith("/usr" + "/dir")) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } void foo5(File dir, File parent) throws IOException { String canonicalPath = dir.getCanonicalPath(); - if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } void foo6(File dir, File parent) throws IOException { String canonicalPath = dir.getCanonicalPath(); - if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } String canonicalPath2 = dir.getCanonicalPath(); - if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { + if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } @@ -58,10 +58,10 @@ void foo6(File dir, File parent) throws IOException { void foo7(File dir, File parent) throws IOException { String canonicalPath = dir.getCanonicalPath(); String canonicalPath2 = dir.getCanonicalPath(); - if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } - if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { + if (!canonicalPath2.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } @@ -72,7 +72,7 @@ File getChild() { void foo8(File parent) throws IOException { String canonicalPath = getChild().getCanonicalPath(); - if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { throw new IOException("Invalid directory: " + getChild().getCanonicalPath()); } } @@ -91,7 +91,7 @@ void foo10(File dir, File parent) throws IOException { void foo11(File dir, File parent) throws IOException { String parentCanonical = parent.getCanonicalPath(); - if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } @@ -99,10 +99,10 @@ void foo11(File dir, File parent) throws IOException { void foo12(File dir, File parent) throws IOException { String parentCanonical = parent.getCanonicalPath(); String parentCanonical2 = parent.getCanonicalPath(); - if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } - if (!dir.getCanonicalPath().startsWith(parentCanonical2)) { + if (!dir.getCanonicalPath().startsWith(parentCanonical2)) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } @@ -116,7 +116,7 @@ void foo13(File dir, File parent) throws IOException { void foo14(File dir, File parent) throws IOException { String parentCanonical = parent.getCanonicalPath() + separatorChar; - if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } @@ -170,7 +170,7 @@ void foo18(File dir, File parent, boolean branch) throws IOException { void foo19(File dir, File parent) throws IOException { String parentCanonical = parent.getCanonicalPath() + "/potato"; - if (!dir.getCanonicalPath().startsWith(parentCanonical)) { + if (!dir.getCanonicalPath().startsWith(parentCanonical)) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } @@ -188,7 +188,7 @@ InputStream foo20(String... path) { String filePath = sb.toString(); File encodedFile = new File(filePath); try { - if (!encodedFile.getCanonicalPath().startsWith(cacheDir.getCanonicalPath())) { + if (!encodedFile.getCanonicalPath().startsWith(cacheDir.getCanonicalPath())) { // $hasTaintFlow return null; } return Files.newInputStream(encodedFile.toPath()); @@ -206,7 +206,7 @@ void foo21(File dir, File parent) throws IOException { void foo22(File dir, File dir2, File parent, boolean conditional) throws IOException { String canonicalPath = conditional ? dir.getCanonicalPath() : dir2.getCanonicalPath(); - if (!canonicalPath.startsWith(parent.getCanonicalPath())) { + if (!canonicalPath.startsWith(parent.getCanonicalPath())) { // $hasTaintFlow throw new IOException("Invalid directory: " + dir.getCanonicalPath()); } } From 76cecc170ef5242a0bf699ae108b3daea6a06f32 Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Wed, 3 Aug 2022 14:30:17 -0400 Subject: [PATCH 20/34] Fix documentation --- .../java/security/PartialPathTraversal.qll | 23 +++++---- .../security/PartialPathTraversalQuery.qll | 9 +++- .../CWE/CWE-023/PartialPathTraversal.qhelp | 47 ++--------------- .../PartialPathTraversalFromRemote.qhelp | 11 ++++ .../PartialPathTraversalOverview.inc.qhelp | 10 ++++ .../PartialPathTraversalRemainder.inc.qhelp | 51 +++++++++++++++++++ 6 files changed, 97 insertions(+), 54 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll index 0997bfb7f24a..956efbeaeb91 100644 --- a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll +++ b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll @@ -1,51 +1,56 @@ +/** Provides classes to reason about partial path traversal vulnerabilities. */ + import java private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.environment.SystemProperty -class MethodStringStartsWith extends Method { +private class MethodStringStartsWith extends Method { MethodStringStartsWith() { this.getDeclaringType() instanceof TypeString and this.hasName("startsWith") } } -class MethodFileGetCanonicalPath extends Method { +private class MethodFileGetCanonicalPath extends Method { MethodFileGetCanonicalPath() { this.getDeclaringType() instanceof TypeFile and this.hasName("getCanonicalPath") } } -class MethodAccessFileGetCanonicalPath extends MethodAccess { +private class MethodAccessFileGetCanonicalPath extends MethodAccess { MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath } } -abstract class FileSeparatorExpr extends Expr { } +private abstract class FileSeparatorExpr extends Expr { } -class SystemPropFileSeparatorExpr extends FileSeparatorExpr { +private class SystemPropFileSeparatorExpr extends FileSeparatorExpr { SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") } } -class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { +private class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { StringLiteralFileSeparatorExpr() { this.getValue().matches("%/") or this.getValue().matches("%\\") } } -class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral { +private class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral { CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" } } -class FileSeparatorAppend extends AddExpr { +private class FileSeparatorAppend extends AddExpr { FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr } } -predicate isSafe(Expr expr) { +private predicate isSafe(Expr expr) { DataFlow::localExprFlow(any(Expr e | e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr ), expr) } +/** + * A method access that returns a boolean that incorrectly guards against Partial Path Traversal. + */ class PartialPathTraversalMethodAccess extends MethodAccess { PartialPathTraversalMethodAccess() { this.getMethod() instanceof MethodStringStartsWith and diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll index 3ca8aaa10740..70416546b96e 100644 --- a/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll +++ b/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll @@ -1,3 +1,5 @@ +/** Provides taint tracking configurations to be used in partial path traversal queries. */ + import java import semmle.code.java.security.PartialPathTraversal import semmle.code.java.dataflow.DataFlow @@ -5,8 +7,13 @@ import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources +/** + * A taint-tracking configuration for unsafe user input + * that is used to validate against path traversal, but is insufficient + * and remains vulnerable to Partial Path Traversal. + */ class PartialPathTraversalFromRemoteConfig extends TaintTracking::Configuration { - PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" } + PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" } override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 902d0fc682cf..462e15d8ccc6 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,51 +3,10 @@ "qhelp.dtd"> -

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR -is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR -is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    + +

    See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability

     - -

    If the user should only access items within a certain directory DIR, ensure that DIR is slash-terminated -before checking that DIR is a prefix of the user-provided path, SUBDIR. Note, Java's getCanonicalPath() -returns a non-slash-terminated path string, so a slash must be added to DIR if that method is used.

    + -     - - -

    - - -In this example, the if statement checks if parent.getCanonicalPath() -is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is -not slash-terminated. So, the user that supplies dir may be allowed to access siblings of parent -and not just children of parent, which is a security issue. - -

    - - - -

    - -In this example, the if statement checks if parent.getCanonicalPath() + File.separator -is a prefix of dir.getCanonicalPath(). Because parent.getCanonicalPath().toPath() is -indeed slash-terminated, the user supplying dir can only access children of -parent, as desired. - -

    - - - -     - - -
  • -OWASP: -Partial Path Traversal. -CVE-2022-23457: - ESAPI Vulnerability Report -
    • - -         diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp new file mode 100644 index 000000000000..ef9802ae45d8 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -0,0 +1,11 @@ + + + + + + + + + diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp new file mode 100644 index 000000000000..63f17afb3f63 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp @@ -0,0 +1,10 @@ + + + +

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR +is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR +is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    + +     \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp new file mode 100644 index 000000000000..94562d332c67 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp @@ -0,0 +1,51 @@ + + + + + +

    If the user should only access items within a certain directory DIR, ensure that DIR is slash-terminated +before checking that DIR is a prefix of the user-provided path, SUBDIR. Note, Java's getCanonicalPath() +returns a non-slash-terminated path string, so a slash must be added to DIR if that method is used.

    + +     + + +

    + + +In this example, the if statement checks if parent.getCanonicalPath() +is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is +not slash-terminated. So, the user that supplies dir may be allowed to access siblings of parent +and not just children of parent, which is a security issue. + +

    + + + +

    + +In this example, the if statement checks if parent.getCanonicalPath() + File.separator +is a prefix of dir.getCanonicalPath(). Because parent.getCanonicalPath().toPath() is +indeed slash-terminated, the user supplying dir can only access children of +parent, as desired. + +

    + + + +     + + +
  • +OWASP: +Partial Path Traversal. +CVE-2022-23457: + ESAPI Vulnerability Report +
    • + +     + + +     \ No newline at end of file From 4f1bc3022c358e9013fd8912d9430b3e97aadfd1 Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Mon, 8 Aug 2022 17:09:43 -0400 Subject: [PATCH 21/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql Co-authored-by: Chris Smowton --- .../src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql index 9003003f4a26..b5b41011dcf1 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql @@ -4,7 +4,7 @@ * @kind path-problem * @problem.severity error * @security-severity 9.3 - * @precision high + * @precision medium * @id java/partial-path-traversal-from-remote * @tags security * external/cwe/cwe-023 From 9d3e8ec4757e7264ebcdd0c36bb74399fa25c81a Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Mon, 8 Aug 2022 17:34:06 -0400 Subject: [PATCH 22/34] Update PartialPathTraversalFromRemote.qhelp --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql | 2 +- .../Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql index de426bf0d696..3b607d665937 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql @@ -4,7 +4,7 @@ * @kind problem * @problem.severity error * @security-severity 9.3 - * @precision high + * @precision medium * @id java/partial-path-traversal * @tags security * external/cwe/cwe-023 diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp index ef9802ae45d8..15b6f6eb962c 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -4,6 +4,8 @@ +

    See also java/partial-path-traversal, which is similar to this query, +but may also flag non-exploitable instances of Partial Path Traversal.

     From 50b4df52f0e38a8b04da89e8ba4b6b00fa0ef1a2 Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Mon, 8 Aug 2022 17:36:04 -0400 Subject: [PATCH 23/34] Fixed precision labels --- .../src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql index b5b41011dcf1..9003003f4a26 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql @@ -4,7 +4,7 @@ * @kind path-problem * @problem.severity error * @security-severity 9.3 - * @precision medium + * @precision high * @id java/partial-path-traversal-from-remote * @tags security * external/cwe/cwe-023 From af92fc389b8df1670fe09c144687fd9561a92761 Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Mon, 8 Aug 2022 17:37:57 -0400 Subject: [PATCH 24/34] Update PartialPathTraversalFromRemote.qhelp --- .../Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp index 15b6f6eb962c..354cec1fa7ba 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -5,7 +5,7 @@

    See also java/partial-path-traversal, which is similar to this query, -but may also flag non-exploitable instances of Partial Path Traversal.

    +but may also flag non-remotely-exploitable instances of Partial Path Traversal.

     From c2b670eff8f8b2d78651ca3b35e2bd2e7d13ce2f Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Tue, 9 Aug 2022 11:58:55 -0700 Subject: [PATCH 25/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp index 354cec1fa7ba..acd693c7faf9 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -3,7 +3,7 @@ "qhelp.dtd"> - +

    See also java/partial-path-traversal, which is similar to this query, but may also flag non-remotely-exploitable instances of Partial Path Traversal.

     From 7da07400ead6039d6bf534c131c29dfc6cb82d3b Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Tue, 9 Aug 2022 11:59:03 -0700 Subject: [PATCH 26/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp index acd693c7faf9..f5f71109018d 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -8,6 +8,6 @@ but may also flag non-remotely-exploitable instances of Partial Path Traversal.

    - +     From 4d80fd0b00dc2089aff18e5a1a158c01bbcfc963 Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Tue, 9 Aug 2022 11:59:14 -0700 Subject: [PATCH 27/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Co-authored-by: Chris Smowton --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 462e15d8ccc6..7893a06e30d8 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -7,6 +7,6 @@

    See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability

    - +     From cf68a1126715a89a8f6eda237f0c861e715adeee Mon Sep 17 00:00:00 2001 From: smehta23 <47726705+smehta23@users.noreply.github.com> Date: Tue, 9 Aug 2022 11:59:28 -0700 Subject: [PATCH 28/34] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Co-authored-by: Chris Smowton --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 7893a06e30d8..f83ee17b5500 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,7 +3,7 @@ "qhelp.dtd"> - +

    See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability

     From 2ca0b0c6b5c1a900f6159a749f1fead041a40dc2 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 9 Aug 2022 18:58:27 +0100 Subject: [PATCH 29/34] Inline qhelp overview A

    at the top isn't allowed, and for some reason the inclusion is required to be a valid qhelp file. --- .../Security/CWE/CWE-023/PartialPathTraversal.qhelp | 5 ++++- .../CWE/CWE-023/PartialPathTraversalFromRemote.qhelp | 5 ++++- .../CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp | 10 ---------- 3 files changed, 8 insertions(+), 12 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index f83ee17b5500..afb8847a7d48 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,7 +3,10 @@ "qhelp.dtd"> - +

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR +is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR +is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    +

    See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability

    diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp index f5f71109018d..d43cf4b3daf0 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -3,7 +3,10 @@ "qhelp.dtd"> - +

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR +is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR +is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    +

    See also java/partial-path-traversal, which is similar to this query, but may also flag non-remotely-exploitable instances of Partial Path Traversal.

     diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp deleted file mode 100644 index 63f17afb3f63..000000000000 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp +++ /dev/null @@ -1,10 +0,0 @@ - - - -

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR -is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR -is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    - -     \ No newline at end of file From 09e4c6b66b45c965bb3ebf3360b90196879ef448 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 10 Aug 2022 10:33:30 +0100 Subject: [PATCH 30/34] Add dataflow path-graph --- .../src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql index 9003003f4a26..0c6731bf43e3 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql @@ -11,6 +11,7 @@ */ import semmle.code.java.security.PartialPathTraversalQuery +import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink where any(PartialPathTraversalFromRemoteConfig config).hasFlowPath(source, sink) From e9df675f88f3378a6fa5d50e5a7c955e93bd5b5f Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 11 Aug 2022 09:55:46 +0100 Subject: [PATCH 31/34] Autoformat ql --- java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll | 2 +- .../CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll index 956efbeaeb91..ea3acb2cc92a 100644 --- a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll +++ b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll @@ -22,7 +22,7 @@ private class MethodAccessFileGetCanonicalPath extends MethodAccess { MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath } } -private abstract class FileSeparatorExpr extends Expr { } +abstract private class FileSeparatorExpr extends Expr { } private class SystemPropFileSeparatorExpr extends FileSeparatorExpr { SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") } diff --git a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql index a0f82a60cc33..b4009b55244e 100644 --- a/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql +++ b/java/ql/test/query-tests/security/CWE-023/semmle/tests/PartialPathTraversalFromRemoteTest.ql @@ -3,7 +3,7 @@ import TestUtilities.InlineFlowTest import semmle.code.java.security.PartialPathTraversalQuery class TestRemoteSource extends RemoteFlowSource { - TestRemoteSource() { this.asParameter().hasName(["dir", "path"]) } + TestRemoteSource() { this.asParameter().hasName(["dir", "path"]) } override string getSourceType() { result = "TestSource" } } From 3cf871e9e5b535073bab9042cc4f02aaf67c16c1 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Mon, 15 Aug 2022 10:34:55 +0100 Subject: [PATCH 32/34] Apply docs suggestions Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- .../src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 2 +- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql | 2 +- .../CWE/CWE-023/PartialPathTraversalFromRemote.qhelp | 2 +- .../CWE/CWE-023/PartialPathTraversalFromRemote.ql | 2 +- .../CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp | 8 ++++---- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index afb8847a7d48..1f90ff358433 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -7,7 +7,7 @@ is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    -

    See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability

    +

    See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability.

    diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql index 3b607d665937..5ea391b031b4 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql @@ -1,5 +1,5 @@ /** - * @name Partial Path Traversal Vulnerability + * @name Partial path traversal vulnerability * @description A prefix used to check that a canonicalised path falls within another must be slash-terminated. * @kind problem * @problem.severity error diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp index d43cf4b3daf0..4e73b3e13952 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -8,7 +8,7 @@ is to use getCanonicalPath() to remove any path-traversal elements is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    See also java/partial-path-traversal, which is similar to this query, -but may also flag non-remotely-exploitable instances of Partial Path Traversal.

    +but may also flag non-remotely-exploitable instances of partial path traversal vulnerabilities.

    diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql index 0c6731bf43e3..f8fed8fd5290 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql @@ -1,5 +1,5 @@ /** - * @name Partial Path Traversal Vulnerability From Remote + * @name Partial path traversal vulnerability from remote * @description A prefix used to check that a canonicalised path falls within another must be slash-terminated. * @kind path-problem * @problem.severity error diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp index 94562d332c67..67838c85f585 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp @@ -17,7 +17,7 @@ returns a non-slash-terminated path string, so a slash must be added to < In this example, the if statement checks if parent.getCanonicalPath() is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is -not slash-terminated. So, the user that supplies dir may be allowed to access siblings of parent +not slash-terminated. This means that users that supply dir may be also allowed to access siblings of parent and not just children of parent, which is a security issue.

    @@ -40,9 +40,9 @@ indeed slash-terminated, the user supplying dir can only access chi
  • OWASP: -Partial Path Traversal. -CVE-2022-23457: - ESAPI Vulnerability Report +Partial Path Traversal.
    • +
  • CVE-2022-23457: + ESAPI Vulnerability Report.
    • From 5677e38994f6d9e6bc87619048f4b11632262a82 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Mon, 15 Aug 2022 10:37:55 +0100 Subject: [PATCH 33/34] Style edit --- java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 1f90ff358433..baef9da52d76 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -5,7 +5,7 @@

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR -is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    +is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow access to siblings of DIR.

    See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability.

     From 1a3dc1d6ebf7ca03b91cbe44687fcd661746a9ab Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Mon, 15 Aug 2022 11:31:53 +0100 Subject: [PATCH 34/34] Remove extra closing tag --- .../CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp index 67838c85f585..a20663dd162d 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp @@ -38,14 +38,12 @@ indeed slash-terminated, the user supplying dir can only access chi     -
  • -OWASP: +
  • OWASP: Partial Path Traversal.
  • CVE-2022-23457: ESAPI Vulnerability Report.
    • -     -     \ No newline at end of file +