Java: generalize sanitizer and add tests

Author: Jami Cogswell

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.
  */

java/ql/lib/semmle/code/java/security/PathSanitizer.qll

@@ -384,30 +384,135 @@ 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 isSingleReplaceAll(StringReplaceAllCall replaceAllCall) {
+  exists(CompileTimeConstantExpr target, string targetValue |
+    isReplaceAllTarget(replaceAllCall, target) and
+    target.getStringValue() = targetValue
+  |
+    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 isDoubleReplaceOrReplaceAll(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
+    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 complementary sanitizer that protects against path injection vulnerabilities
- * by replacing all directory characters ('..', '/', and '\') with safe characters.
+ * by replacing directory characters ('..', '/', and '\') with safe characters.
  */
-private class ReplaceDirectoryCharactersSanitizer extends MethodCall {
+private class ReplaceDirectoryCharactersSanitizer extends StringReplaceOrReplaceAllCall {
   ReplaceDirectoryCharactersSanitizer() {
-    exists(MethodCall mc |
-      // TODO: "java.lang.String.replace" as well
-      mc.getMethod().hasQualifiedName("java.lang", "String", "replaceAll") and
-      // TODO: unhardcode all of the below to handle more valid replacements and several calls
+    isSingleReplaceAll(this) or
+    isDoubleReplaceOrReplaceAll(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()
+  |
+    targetValue.matches(["[%]*", "[%]+", "[%]{%}"]) and
+    (
+      // Allow anything except `.`, '/', '\'
       (
-        mc.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "\\.\\.|[/\\\\]"
+        // Note: we do not account for when '.', '/', '\' are inside a character range
+        not targetValue.matches("[%" + [".", "/", "\\\\\\\\"] + "%]%") and
+        not targetValue.matches("%[^%]%")
         or
-        exists(MethodCall mc2 |
-          mc2.getMethod().hasQualifiedName("java.lang", "String", "replaceAll") and
-          mc.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "\\." and
-          mc2.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "/"
-        )
+        targetValue.matches("[^%.%]%") and
+        targetValue.matches("[^%/%]%") and
+        targetValue.matches("[^%\\\\\\\\%]%")
       ) and
-      // TODO: accept more replacement characters?
-      mc.getArgument(1).(CompileTimeConstantExpr).getStringValue() = ["", "_"] and
-      this = mc
+      branch = true
+      or
+      // Disallow `.`, '/', '\'
+      targetValue.matches("[%.%]%") and
+      targetValue.matches("[%/%]%") and
+      targetValue.matches("[%\\\\\\\\%]%") and
+      not targetValue.matches("%[^%]%") and
+      branch = false
     )
-  }
+  )
 }
 
 /**
@@ -416,28 +521,21 @@ private class ReplaceDirectoryCharactersSanitizer extends MethodCall {
  */
 private class DirectoryCharactersGuard extends PathGuard {
   Expr checkedExpr;
+  boolean branch;
 
-  DirectoryCharactersGuard() {
-    exists(MethodCall mc, Method m | m = mc.getMethod() |
-      m.getDeclaringType() instanceof TypeString and
-      m.hasName("matches") and
-      // TODO: unhardcode to handle more valid matches
-      mc.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "[0-9a-fA-F]{20,}" and
-      checkedExpr = mc.getQualifier() and
-      this = mc
-    )
-  }
+  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 = true and
-  g instanceof DirectoryCharactersGuard and
+  branch = g.(DirectoryCharactersGuard).getBranch() and
   localTaintFlowToPathGuard(e, g)
 }