diff --git 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 new file mode 100644 index 000000000000..21d4c61f7c11 --- /dev/null +++ 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. diff --git a/java/ql/lib/semmle/code/java/JDK.qll b/java/ql/lib/semmle/code/java/JDK.qll index e1fbf9317465..27a8b2a9ca73 100644 --- 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. */ diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index cd173823f2dc..8b08b5a78f2f 100644 --- 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::getABarrierNode() or + this = ValidationMethod::getAValidatedNode() + } +} diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java index d3285352fa38..5943d29c144b 100644 --- a/java/ql/test/library-tests/pathsanitizer/Test.java +++ b/java/ql/test/library-tests/pathsanitizer/Test.java @@ -604,4 +604,260 @@ public void fileConstructorSanitizer() throws Exception { 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 + } + } }