From e641505361f23e8120429796e268f8907ccea66c Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 31 Mar 2023 11:05:28 -0400 Subject: [PATCH 1/3] Fix partial path traversal Java example Again The original wouldn't compile, and the fix made by #11899 is sub-optimal. This keeps the entire comparision using the Java `Path` object, which is optimal. Signed-off-by: Jonathan Leitschuh --- .../src/Security/CWE/CWE-023/PartialPathTraversalBad.java | 2 +- .../src/Security/CWE/CWE-023/PartialPathTraversalGood.java | 6 ++++-- .../CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp | 7 +++---- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java index e933679aa449..961d8c21c6b2 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java @@ -1,7 +1,7 @@ public class PartialPathTraversalBad { public void example(File dir, File parent) throws IOException { if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath())) { - throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + throw new IOException("Path traversal attempt: " + dir.getCanonicalPath()); } } } diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java index baf4ef8545ec..65392373a25e 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java @@ -1,7 +1,9 @@ +import java.io.File; + public class PartialPathTraversalGood { public void example(File dir, File parent) throws IOException { - if (!dir.getCanonicalPath().startsWith(parent.getCanonicalPath() + File.separator)) { - throw new IOException("Invalid directory: " + dir.getCanonicalPath()); + if (!dir.toPath().normalize().startsWith(parent.toPath())) { + throw new IOException("Path traversal attempt: " + dir.getCanonicalPath()); } } } 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 7703f5c52ff0..5338a98efc58 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp @@ -26,10 +26,9 @@ 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. +In this example, the if statement checks if parent.toPath() +is a prefix of dir.normalize(). Because Path#startsWith will do the correct check that +dir is ia child children of parent, as desired.

From b9d409279bf38b8e3200c07d453ea1581a78b0fe Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 31 Mar 2023 15:17:56 -0400 Subject: [PATCH 2/3] Update java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp Co-authored-by: Tony Torralba --- .../CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 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 5338a98efc58..342d38ac5c23 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp @@ -27,8 +27,8 @@ and not just children of parent, which is a security issue.

In this example, the if statement checks if parent.toPath() -is a prefix of dir.normalize(). Because Path#startsWith will do the correct check that -dir is ia child children of parent, as desired. +is a prefix of dir.normalize(). Because Path#startsWith does the correct check that +dir is a child of parent, users will not be able to access siblings of parent, as desired.

From 0d774a647ca4766f2db86163f3e6270d974a4969 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 31 Mar 2023 11:05:28 -0400 Subject: [PATCH 3/3] Fix partial path traversal Java example Again The original wouldn't compile, and the fix made by #11899 is sub-optimal. This keeps the entire comparision using the Java `Path` object, which is optimal. Signed-off-by: Jonathan Leitschuh --- .../CWE-023/semmle/tests/PartialPathTraversalTest.java | 6 ++++++ 1 file changed, 6 insertions(+) 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 b7c0256d0756..af0fd776de15 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 @@ -225,6 +225,12 @@ void foo24(File dir, File parent) throws IOException { } } + public void doesNotFlagOptimalSafeVersion(File dir, File parent) throws IOException { + if (!dir.toPath().normalize().startsWith(parent.toPath())) { // Safe + throw new IOException("Path traversal attempt: " + dir.getCanonicalPath()); + } + } + public void doesNotFlag() { "hello".startsWith("goodbye"); }