From 94080a6e4718c808ccfe801ebc6a4840e9e7c116 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Sun, 12 Jan 2025 21:31:48 -0500 Subject: [PATCH 01/10] Java: initial tests --- .../CWE-022/semmle/tests/TaintedPath.java | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java index 00447364bb38..0f18eeac3674 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java @@ -6,6 +6,7 @@ import java.net.Socket; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.FileSystems; public class TaintedPath { public void sendUserFile(Socket sock, String user) throws IOException { @@ -86,4 +87,51 @@ public void sendUserFileGood4(Socket sock, String user) throws IOException { fileLine = fileReader.readLine(); } } + + // TODO : New tests + + public void sendUserFileGood5(Socket sock, String user) throws IOException { + BufferedReader filenameReader = new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); + // GOOD: remove all ".." sequences and path separators from the filename + String filename = filenameReader.readLine().replaceAll("\\.", "").replaceAll("/", ""); + BufferedReader fileReader = new BufferedReader(new FileReader(filename)); // GOOD + String fileLine = fileReader.readLine(); + while(fileLine != null) { + sock.getOutputStream().write(fileLine.getBytes()); + fileLine = fileReader.readLine(); + } + } + + public void sendUserFileGood6(Socket sock, String user) throws IOException { + BufferedReader filenameReader = new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); + String filename = filenameReader.readLine(); + // GOOD: remove all ".." sequences and path separators from the filename + filename = filename.replaceAll("\\.\\.|[/\\\\]", ""); + BufferedReader fileReader = new BufferedReader(new FileReader(filename)); // GOOD + String fileLine = fileReader.readLine(); + while(fileLine != null) { + sock.getOutputStream().write(fileLine.getBytes()); + fileLine = fileReader.readLine(); + } + } + + public void sendUserFileGood7(Socket sock, String user) throws Exception { + BufferedReader filenameReader = + new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); + String filename = filenameReader.readLine(); + + // GOOD: ensure that that /, \ and .. cannot possibly be in the payload + if (filename.matches("[0-9a-fA-F]{20,}")) { + final Path pathObject = FileSystems.getDefault().getPath(filename); // summary now, see https://github.com/github/codeql/commit/19cb7adb6db17a3131b7db93482abc6a0d93ceff#diff-4b91db1bd2a19ab607f83fbe858f0ceffd942d1fb246739c731112367c865f88L8 + + BufferedReader fileReader = new BufferedReader(new FileReader(pathObject.toString())); // GOOD + String fileLine = fileReader.readLine(); + while (fileLine != null) { + sock.getOutputStream().write(fileLine.getBytes()); + fileLine = fileReader.readLine(); + } + } + + } + } From ab3690f6662a2a50d92680c6dde9fed61d3d6149 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 31 Jan 2025 16:26:29 -0500 Subject: [PATCH 02/10] Java: initial sanitizer --- .../code/java/security/PathSanitizer.qll | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index cd173823f2dc..e66ed5d5e2ce 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -383,3 +383,73 @@ private class FileConstructorChildArgumentStep extends AdditionalTaintStep { ) } } + +/** + * A complementary sanitizer that protects against path injection vulnerabilities + * by replacing all directory characters ('..', '/', and '\') with safe characters. + */ +private class ReplaceDirectoryCharactersSanitizer extends MethodCall { + 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 + ( + mc.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "\\.\\.|[/\\\\]" + or + exists(MethodCall mc2 | + mc2.getMethod().hasQualifiedName("java.lang", "String", "replaceAll") and + mc.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "\\." and + mc2.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "/" + ) + ) and + // TODO: accept more replacement characters? + mc.getArgument(1).(CompileTimeConstantExpr).getStringValue() = ["", "_"] and + this = mc + ) + } +} + +/** + * A complementary guard that protects against path traversal by looking + * for patterns that exclude directory characters: `..`, '/', and '\'. + */ +private class DirectoryCharactersGuard extends PathGuard { + Expr checkedExpr; + + 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 + ) + } + + override Expr getCheckedExpr() { result = checkedExpr } +} + +/** + * 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 + 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() + } +} From 76433a31f73236c994a0251da3e4388a02c46d3e Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 19 Feb 2025 12:35:38 -0500 Subject: [PATCH 03/10] Java: generalize sanitizer and add tests --- java/ql/lib/semmle/code/java/JDK.qll | 30 +++ .../code/java/security/PathSanitizer.qll | 154 ++++++++++--- .../library-tests/pathsanitizer/Test.java | 210 ++++++++++++++++++ .../CWE-022/semmle/tests/TaintedPath.java | 48 ---- 4 files changed, 366 insertions(+), 76 deletions(-) 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 e66ed5d5e2ce..c9373509386b 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/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,19 +521,13 @@ 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 } } /** @@ -436,8 +535,7 @@ private class DirectoryCharactersGuard extends PathGuard { * 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) } diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java index d3285352fa38..5c5557b02927 100644 --- a/java/ql/test/library-tests/pathsanitizer/Test.java +++ b/java/ql/test/library-tests/pathsanitizer/Test.java @@ -604,4 +604,214 @@ 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(); + // 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(); + 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); // $ 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(); + 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 + } + } } diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java index 0f18eeac3674..00447364bb38 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java @@ -6,7 +6,6 @@ import java.net.Socket; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.FileSystems; public class TaintedPath { public void sendUserFile(Socket sock, String user) throws IOException { @@ -87,51 +86,4 @@ public void sendUserFileGood4(Socket sock, String user) throws IOException { fileLine = fileReader.readLine(); } } - - // TODO : New tests - - public void sendUserFileGood5(Socket sock, String user) throws IOException { - BufferedReader filenameReader = new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); - // GOOD: remove all ".." sequences and path separators from the filename - String filename = filenameReader.readLine().replaceAll("\\.", "").replaceAll("/", ""); - BufferedReader fileReader = new BufferedReader(new FileReader(filename)); // GOOD - String fileLine = fileReader.readLine(); - while(fileLine != null) { - sock.getOutputStream().write(fileLine.getBytes()); - fileLine = fileReader.readLine(); - } - } - - public void sendUserFileGood6(Socket sock, String user) throws IOException { - BufferedReader filenameReader = new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); - String filename = filenameReader.readLine(); - // GOOD: remove all ".." sequences and path separators from the filename - filename = filename.replaceAll("\\.\\.|[/\\\\]", ""); - BufferedReader fileReader = new BufferedReader(new FileReader(filename)); // GOOD - String fileLine = fileReader.readLine(); - while(fileLine != null) { - sock.getOutputStream().write(fileLine.getBytes()); - fileLine = fileReader.readLine(); - } - } - - public void sendUserFileGood7(Socket sock, String user) throws Exception { - BufferedReader filenameReader = - new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8")); - String filename = filenameReader.readLine(); - - // GOOD: ensure that that /, \ and .. cannot possibly be in the payload - if (filename.matches("[0-9a-fA-F]{20,}")) { - final Path pathObject = FileSystems.getDefault().getPath(filename); // summary now, see https://github.com/github/codeql/commit/19cb7adb6db17a3131b7db93482abc6a0d93ceff#diff-4b91db1bd2a19ab607f83fbe858f0ceffd942d1fb246739c731112367c865f88L8 - - BufferedReader fileReader = new BufferedReader(new FileReader(pathObject.toString())); // GOOD - String fileLine = fileReader.readLine(); - while (fileLine != null) { - sock.getOutputStream().write(fileLine.getBytes()); - fileLine = fileReader.readLine(); - } - } - - } - } From 41aeb874f1a9711425f6ad8450a349c7667b4202 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 10 Mar 2025 17:12:54 -0400 Subject: [PATCH 04/10] Java: add change note --- .../change-notes/2025-03-10-matches-replace-path-sanitizer.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/lib/change-notes/2025-03-10-matches-replace-path-sanitizer.md 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. From 9d6a10b601cd05661adc9e71bd510529a7995093 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 17 Mar 2025 15:07:10 -0400 Subject: [PATCH 05/10] Java: rename 'isSingleReplaceAll' and 'isDoubleReplaceOrReplaceAll' --- .../lib/semmle/code/java/security/PathSanitizer.qll | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index c9373509386b..70ee6ef3c8de 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -414,7 +414,9 @@ private predicate isReplaceTarget(StringReplaceCall replaceCall, CompileTimeCons } /** Holds if a single `replaceAllCall` replaces all directory characters. */ -private predicate isSingleReplaceAll(StringReplaceAllCall replaceAllCall) { +private predicate replacesDirectoryCharactersWithSingleReplaceAll( + StringReplaceAllCall replaceAllCall +) { exists(CompileTimeConstantExpr target, string targetValue | isReplaceAllTarget(replaceAllCall, target) and target.getStringValue() = targetValue @@ -436,7 +438,9 @@ private predicate isSingleReplaceAll(StringReplaceAllCall replaceAllCall) { * Holds if there are two chained replacement calls, `rc1` and `rc2`, that replace * '.' and one of '/' or '\'. */ -private predicate isDoubleReplaceOrReplaceAll(StringReplaceOrReplaceAllCall rc1) { +private predicate replacesDirectoryCharactersWithDoubleReplaceOrReplaceAll( + StringReplaceOrReplaceAllCall rc1 +) { exists( CompileTimeConstantExpr target1, string targetValue1, StringReplaceOrReplaceAllCall rc2, CompileTimeConstantExpr target2, string targetValue2 @@ -471,8 +475,8 @@ private predicate isDoubleReplaceOrReplaceAll(StringReplaceOrReplaceAllCall rc1) */ private class ReplaceDirectoryCharactersSanitizer extends StringReplaceOrReplaceAllCall { ReplaceDirectoryCharactersSanitizer() { - isSingleReplaceAll(this) or - isDoubleReplaceOrReplaceAll(this) + replacesDirectoryCharactersWithSingleReplaceAll(this) or + replacesDirectoryCharactersWithDoubleReplaceOrReplaceAll(this) } } From 2f6696e8a8816afd2ecb7236529ea3023978d3d1 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 17 Mar 2025 15:13:33 -0400 Subject: [PATCH 06/10] Java: add test --- java/ql/test/library-tests/pathsanitizer/Test.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java index 5c5557b02927..4fc4a80119eb 100644 --- a/java/ql/test/library-tests/pathsanitizer/Test.java +++ b/java/ql/test/library-tests/pathsanitizer/Test.java @@ -729,6 +729,11 @@ public void directoryCharsSanitizer() throws Exception { source = source.replaceAll(".|[/\\\\]", ""); sink(source); // $ hasTaintFlow } + { + String source = (String) source(); + source = source.replaceAll("\\.|/|\\\\", ""); + sink(source); // Safe + } { String source = (String) source(); source = source.replaceAll("[\\./\\\\]", ""); From b9f642f4aa119988208188c28f963a144a32087a Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 17 Mar 2025 15:20:14 -0400 Subject: [PATCH 07/10] Java: condense '.' matching --- java/ql/lib/semmle/code/java/security/PathSanitizer.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 70ee6ef3c8de..fff954a8a1a5 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -428,7 +428,7 @@ private predicate replacesDirectoryCharactersWithSingleReplaceAll( targetValue.matches("[%\\\\\\\\%]") or targetValue.matches("%|%") and - targetValue.matches("%" + ["\\.\\.", "[.][.]", "\\."] + "%") and + targetValue.matches("%" + ["[.]", "\\."] + "%") and targetValue.matches("%/%") and targetValue.matches("%\\\\\\\\%") ) From 3083360032b689c04cba3d73b464f2bb44ef63c2 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 17 Mar 2025 15:24:31 -0400 Subject: [PATCH 08/10] Java: remove 'complementary' from qldocs --- java/ql/lib/semmle/code/java/security/PathSanitizer.qll | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index fff954a8a1a5..35a226627258 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -470,8 +470,8 @@ private predicate replacesDirectoryCharactersWithDoubleReplaceOrReplaceAll( } /** - * A complementary sanitizer that protects against path injection vulnerabilities - * by replacing directory characters ('..', '/', and '\') with safe characters. + * A sanitizer that protects against path injection vulnerabilities by replacing + * directory characters ('..', '/', and '\') with safe characters. */ private class ReplaceDirectoryCharactersSanitizer extends StringReplaceOrReplaceAllCall { ReplaceDirectoryCharactersSanitizer() { @@ -520,8 +520,8 @@ private predicate isMatchesCall(StringMatchesCall matchesCall, Expr checkedExpr, } /** - * A complementary guard that protects against path traversal by looking - * for patterns that exclude directory characters: `..`, '/', and '\'. + * A guard that protects against path traversal by looking for patterns + * that exclude directory characters: `..`, '/', and '\'. */ private class DirectoryCharactersGuard extends PathGuard { Expr checkedExpr; From 49d37c517d2221a05e047524e8f56d088011476d Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 17 Mar 2025 16:02:13 -0400 Subject: [PATCH 09/10] Java: fix replacement char check and add tests --- .../code/java/security/PathSanitizer.qll | 4 ++- .../library-tests/pathsanitizer/Test.java | 28 +++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 35a226627258..a120cb4489f8 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -419,7 +419,8 @@ private predicate replacesDirectoryCharactersWithSingleReplaceAll( ) { exists(CompileTimeConstantExpr target, string targetValue | isReplaceAllTarget(replaceAllCall, target) and - target.getStringValue() = targetValue + target.getStringValue() = targetValue and + replaceAllCall.getArgument(1).(CompileTimeConstantExpr).getStringValue() = getAReplacementChar() | not targetValue.matches("%[^%]%") and targetValue.matches("[%.%]") and @@ -460,6 +461,7 @@ private predicate replacesDirectoryCharactersWithDoubleReplaceOrReplaceAll( 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 diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java index 4fc4a80119eb..db00508c56a1 100644 --- a/java/ql/test/library-tests/pathsanitizer/Test.java +++ b/java/ql/test/library-tests/pathsanitizer/Test.java @@ -716,14 +716,20 @@ public void directoryCharsSanitizer() throws Exception { } { String source = (String) source(); - source = source.replaceAll("\\.|[/\\\\]", ""); + source = source.replaceAll("\\.|[/\\\\]", "-"); sink(source); // Safe } { String source = (String) source(); - source = source.replaceAll("[.][.]|[/\\\\]", ""); + 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(".|[/\\\\]", ""); @@ -761,6 +767,24 @@ public void directoryCharsSanitizer() throws Exception { 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("\\\\", ""); From 0d2e9ae4691c7fa9b85a112dcab8cb74a9754cef Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 17 Mar 2025 18:48:44 -0400 Subject: [PATCH 10/10] Java: fix 'matches' false branch --- .../code/java/security/PathSanitizer.qll | 9 ++++---- .../library-tests/pathsanitizer/Test.java | 21 +++++++++++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index a120cb4489f8..8b08b5a78f2f 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -497,9 +497,9 @@ private predicate isMatchesCall(StringMatchesCall matchesCall, Expr checkedExpr, target.getStringValue() = targetValue and checkedExpr = matchesCall.getQualifier() | - targetValue.matches(["[%]*", "[%]+", "[%]{%}"]) and ( // Allow anything except `.`, '/', '\' + targetValue.matches(["[%]*", "[%]+", "[%]{%}"]) and ( // Note: we do not account for when '.', '/', '\' are inside a character range not targetValue.matches("[%" + [".", "/", "\\\\\\\\"] + "%]%") and @@ -512,9 +512,10 @@ private predicate isMatchesCall(StringMatchesCall matchesCall, Expr checkedExpr, branch = true or // Disallow `.`, '/', '\' - targetValue.matches("[%.%]%") and - targetValue.matches("[%/%]%") and - targetValue.matches("[%\\\\\\\\%]%") and + targetValue.matches([".*[%].*", ".+[%].+"]) and + targetValue.matches("%[%.%]%") and + targetValue.matches("%[%/%]%") and + targetValue.matches("%[%\\\\\\\\%]%") and not targetValue.matches("%[^%]%") and branch = false ) diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java index db00508c56a1..5943d29c144b 100644 --- a/java/ql/test/library-tests/pathsanitizer/Test.java +++ b/java/ql/test/library-tests/pathsanitizer/Test.java @@ -684,16 +684,33 @@ public void directoryCharsSanitizer() throws Exception { // 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); // $ Safe + sink(source); // $ hasTaintFlow } } { String source = (String) source(); // not a complete sanitizer since it doesn't protect against absolute path injection - if (source.matches("[\\.]+")) { + if (source.matches(".+[\\.].+")) { sink(source); // $ hasTaintFlow } else { sink(source); // $ hasTaintFlow