Merge pull request #18646 from jcogs33/jcogs33/java/directory-chars-path-sanitizer

Java: path sanitizer for `replace`, `replaceAll`, and `matches`
2025-12-16 16:53:25 +01:00 · 2025-03-18 13:26:01 -04:00
parent 1324c11044 0d2e9ae469
commit 2750d1d889
4 changed files with 465 additions and 0 deletions
--- a/java/ql/lib/change-notes/2025-03-10-matches-replace-path-sanitizer.md
+++ b/java/ql/lib/change-notes/2025-03-10-matches-replace-path-sanitizer.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added a path injection sanitizer for calls to `java.lang.String.matches`, `java.lang.String.replace`, and `java.lang.String.replaceAll` that make sure '/', '\', '..' are not in the path.
--- a/java/ql/lib/semmle/code/java/JDK.qll
+++ b/java/ql/lib/semmle/code/java/JDK.qll
@@ -45,6 +45,36 @@ class StringContainsMethod extends Method {
  }
 }

+/** A call to the `java.lang.String.matches` method. */
+class StringMatchesCall extends MethodCall {
+  StringMatchesCall() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypeString and
+      m.hasName("matches")
+    )
+  }
+}
+
+/** A call to the `java.lang.String.replaceAll` method. */
+class StringReplaceAllCall extends MethodCall {
+  StringReplaceAllCall() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypeString and
+      m.hasName("replaceAll")
+    )
+  }
+}
+
+/** A call to the `java.lang.String.replace` method. */
+class StringReplaceCall extends MethodCall {
+  StringReplaceCall() {
+    exists(Method m | m = this.getMethod() |
+      m.getDeclaringType() instanceof TypeString and
+      m.hasName("replace")
+    )
+  }
+}
+
 /**
 * The methods on the class `java.lang.String` that are used to perform partial matches with a specified substring or char.
 */
--- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
+++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
@@ -383,3 +383,178 @@ private class FileConstructorChildArgumentStep extends AdditionalTaintStep {
    )
  }
 }
+
+/** A call to `java.lang.String.replace` or `java.lang.String.replaceAll`. */
+private class StringReplaceOrReplaceAllCall extends MethodCall {
+  StringReplaceOrReplaceAllCall() {
+    this instanceof StringReplaceCall or
+    this instanceof StringReplaceAllCall
+  }
+}
+
+/** Gets a character used for replacement. */
+private string getAReplacementChar() { result = ["", "_", "-"] }
+
+/** Gets a directory character represented as regex. */
+private string getADirRegexChar() { result = ["\\.", "/", "\\\\"] }
+
+/** Gets a directory character represented as a char. */
+private string getADirChar() { result = [".", "/", "\\"] }
+
+/** Holds if `target` is the first argument of `replaceAllCall`. */
+private predicate isReplaceAllTarget(
+  StringReplaceAllCall replaceAllCall, CompileTimeConstantExpr target
+) {
+  target = replaceAllCall.getArgument(0)
+}
+
+/** Holds if `target` is the first argument of `replaceCall`. */
+private predicate isReplaceTarget(StringReplaceCall replaceCall, CompileTimeConstantExpr target) {
+  target = replaceCall.getArgument(0)
+}
+
+/** Holds if a single `replaceAllCall` replaces all directory characters. */
+private predicate replacesDirectoryCharactersWithSingleReplaceAll(
+  StringReplaceAllCall replaceAllCall
+) {
+  exists(CompileTimeConstantExpr target, string targetValue |
+    isReplaceAllTarget(replaceAllCall, target) and
+    target.getStringValue() = targetValue and
+    replaceAllCall.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar()
+  |
+    not targetValue.matches("%[^%]%") and
+    targetValue.matches("[%.%]") and
+    targetValue.matches("[%/%]") and
+    // Search for "\\\\" (needs extra backslashes to avoid escaping the '%')
+    targetValue.matches("[%\\\\\\\\%]")
+    or
+    targetValue.matches("%|%") and
+    targetValue.matches("%" + ["[.]", "\\."] + "%") and
+    targetValue.matches("%/%") and
+    targetValue.matches("%\\\\\\\\%")
+  )
+}
+
+/**
+ * Holds if there are two chained replacement calls, `rc1` and `rc2`, that replace
+ * '.' and one of '/' or '\'.
+ */
+private predicate replacesDirectoryCharactersWithDoubleReplaceOrReplaceAll(
+  StringReplaceOrReplaceAllCall rc1
+) {
+  exists(
+    CompileTimeConstantExpr target1, string targetValue1, StringReplaceOrReplaceAllCall rc2,
+    CompileTimeConstantExpr target2, string targetValue2
+  |
+    rc1 instanceof StringReplaceAllCall and
+    isReplaceAllTarget(rc1, target1) and
+    isReplaceAllTarget(rc2, target2) and
+    targetValue1 = getADirRegexChar() and
+    targetValue2 = getADirRegexChar()
+    or
+    rc1 instanceof StringReplaceCall and
+    isReplaceTarget(rc1, target1) and
+    isReplaceTarget(rc2, target2) and
+    targetValue1 = getADirChar() and
+    targetValue2 = getADirChar()
+  |
+    rc2.getQualifier() = rc1 and
+    target1.getStringValue() = targetValue1 and
+    target2.getStringValue() = targetValue2 and
+    rc1.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
+    rc2.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() and
+    // make sure the calls replace different characters
+    targetValue2 != targetValue1 and
+    // make sure one of the calls replaces '.'
+    // then the other call must replace one of '/' or '\' if they are not equal
+    (targetValue2.matches("%.%") or targetValue1.matches("%.%"))
+  )
+}
+
+/**
+ * A sanitizer that protects against path injection vulnerabilities by replacing
+ * directory characters ('..', '/', and '\') with safe characters.
+ */
+private class ReplaceDirectoryCharactersSanitizer extends StringReplaceOrReplaceAllCall {
+  ReplaceDirectoryCharactersSanitizer() {
+    replacesDirectoryCharactersWithSingleReplaceAll(this) or
+    replacesDirectoryCharactersWithDoubleReplaceOrReplaceAll(this)
+  }
+}
+
+/** Holds if `target` is the first argument of `matchesCall`. */
+private predicate isMatchesTarget(StringMatchesCall matchesCall, CompileTimeConstantExpr target) {
+  target = matchesCall.getArgument(0)
+}
+
+/**
+ * Holds if `matchesCall` confirms that `checkedExpr` does not contain any directory characters
+ * on the given `branch`.
+ */
+private predicate isMatchesCall(StringMatchesCall matchesCall, Expr checkedExpr, boolean branch) {
+  exists(CompileTimeConstantExpr target, string targetValue |
+    isMatchesTarget(matchesCall, target) and
+    target.getStringValue() = targetValue and
+    checkedExpr = matchesCall.getQualifier()
+  |
+    (
+      // Allow anything except `.`, '/', '\'
+      targetValue.matches(["[%]*", "[%]+", "[%]{%}"]) and
+      (
+        // Note: we do not account for when '.', '/', '\' are inside a character range
+        not targetValue.matches("[%" + [".", "/", "\\\\\\\\"] + "%]%") and
+        not targetValue.matches("%[^%]%")
+        or
+        targetValue.matches("[^%.%]%") and
+        targetValue.matches("[^%/%]%") and
+        targetValue.matches("[^%\\\\\\\\%]%")
+      ) and
+      branch = true
+      or
+      // Disallow `.`, '/', '\'
+      targetValue.matches([".*[%].*", ".+[%].+"]) and
+      targetValue.matches("%[%.%]%") and
+      targetValue.matches("%[%/%]%") and
+      targetValue.matches("%[%\\\\\\\\%]%") and
+      not targetValue.matches("%[^%]%") and
+      branch = false
+    )
+  )
+}
+
+/**
+ * A guard that protects against path traversal by looking for patterns
+ * that exclude directory characters: `..`, '/', and '\'.
+ */
+private class DirectoryCharactersGuard extends PathGuard {
+  Expr checkedExpr;
+  boolean branch;
+
+  DirectoryCharactersGuard() { isMatchesCall(this, checkedExpr, branch) }
+
+  override Expr getCheckedExpr() { result = checkedExpr }
+
+  boolean getBranch() { result = branch }
+}
+
+/**
+ * Holds if `g` is a guard that considers a path safe because it is checked to make
+ * sure it does not contain any directory characters: '..', '/', and '\'.
+ */
+private predicate directoryCharactersGuard(Guard g, Expr e, boolean branch) {
+  branch = g.(DirectoryCharactersGuard).getBranch() and
+  localTaintFlowToPathGuard(e, g)
+}
+
+/**
+ * A sanitizer that protects against path injection vulnerabilities
+ * by ensuring that the path does not contain any directory characters:
+ *  '..', '/', and '\'.
+ */
+private class DirectoryCharactersSanitizer extends PathInjectionSanitizer {
+  DirectoryCharactersSanitizer() {
+    this.asExpr() instanceof ReplaceDirectoryCharactersSanitizer or
+    this = DataFlow::BarrierGuard<directoryCharactersGuard/3>::getABarrierNode() or
+    this = ValidationMethod<directoryCharactersGuard/3>::getAValidatedNode()
+  }
+}
--- a/java/ql/test/library-tests/pathsanitizer/Test.java
+++ b/java/ql/test/library-tests/pathsanitizer/Test.java
@@ -604,4 +604,260 @@ public class Test {
            sink(normalized); // $ hasTaintFlow
        }
    }
+
+    private void directoryCharsValidation(String path) throws Exception {
+        if (!path.matches("[0-9a-fA-F]{20,}")) {
+            throw new Exception();
+        }
+    }
+
+    public void directoryCharsSanitizer() throws Exception {
+        // DirectoryCharactersGuard
+        // Ensures that directory characters (/, \ and ..) cannot possibly be in the payload
+        // branch = true
+        {
+            String source = (String) source();
+            if (source.matches("[0-9a-fA-F]{20,}")) {
+                sink(source); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            if (source.matches("[0-9a-fA-F]*")) {
+                sink(source); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            if (source.matches("[0-9a-fA-F]+")) {
+                sink(source); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            if (source.matches("[0-9a-fA-F\\.]+")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            if (source.matches("[0-9a-fA-F/]+")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            if (source.matches("[0-9a-fA-F\\\\]+")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            // exclude '.', '/', '\'
+            if (source.matches("[^0-9./\\\\a-f]+")) {
+                sink(source); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            // '/' is not excluded
+            if (source.matches("[^0-9.\\\\a-f]+")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        // branch = false
+        {
+            String source = (String) source();
+            if (source.matches(".*[\\./\\\\].*")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // Safe
+            }
+        }
+        {
+            String source = (String) source();
+            if (source.matches(".+[\\./\\\\].+")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // Safe
+            }
+        }
+        {
+            String source = (String) source();
+            // does not match whole string
+            if (source.matches("[\\./\\\\]+")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            // not a complete sanitizer since it doesn't protect against absolute path injection
+            if (source.matches(".+[\\.].+")) {
+                sink(source); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        // validation method
+        {
+            String source = (String) source();
+            directoryCharsValidation(source);
+            sink(source); // Safe
+        }
+
+        // ReplaceDirectoryCharactersSanitizer
+        // Removes ".." sequences and path separators from the payload
+        // single `replaceAll` call
+        {
+            String source = (String) source();
+            source = source.replaceAll("\\.\\.|[/\\\\]", "");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll("\\.|[/\\\\]", "-");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll("[.][.]|[/\\\\]", "_");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character
+            source = source.replaceAll("[.][.]|[/\\\\]", "/");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll(".|[/\\\\]", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll("\\.|/|\\\\", "");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll("[\\./\\\\]", "");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll("[\\.\\\\]", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll("[^\\.\\\\/]", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // Bypassable with ".../...//"
+            source = source.replaceAll("\\.\\./", "");
+            sink(source); // $ hasTaintFlow
+        }
+        // multiple `replaceAll` or `replace` calls
+        {
+            String source = (String) source();
+            source = source.replaceAll("\\.", "").replaceAll("/", "");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character in each call
+            source = source.replaceAll("\\.", "/").replaceAll("/", ".");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character in first call
+            source = source.replaceAll("\\.", "/").replaceAll("/", "-");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // test a not-accepted replacement character in second call
+            source = source.replaceAll("\\.", "_").replaceAll("/", ".");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            source = source.replaceAll("\\.", "").replaceAll("/", "").replaceAll("\\\\", "");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            // '/' or '\' are not replaced
+            source = source.replaceAll("\\.", "").replaceAll("\\.", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // '.' is not replaced
+            source = source.replaceAll("/", "").replaceAll("\\\\", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // Bypassable with ".....///"
+            source = source.replaceAll("\\.\\./", "").replaceAll("\\./", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            source = source.replace(".", "").replace("/", "");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            source = source.replace(".", "").replace("/", "").replace("\\", "");
+            sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            // '/' or '\' are not replaced
+            source = source.replace(".", "").replace(".", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // '.' is not replaced
+            source = source.replace("/", "").replace("\\", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // Bypassable with ".....///"
+            source = source.replace("../", "").replace("./", "");
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            // Bypassable with ".../...//"
+            source = source.replace("../", "");
+            sink(source); // $ hasTaintFlow
+        }
+    }
 }