hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-25 16:55:19 +02:00
Code Issues Packages Projects Releases Wiki Activity

Java: generalize sanitizer and add tests

Browse Source
This commit is contained in:
Jami Cogswell
2025-02-19 12:35:38 -05:00
parent ab3690f666
commit 76433a31f7
4 changed files with 369 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
java/ql/lib/semmle/code/java/JDK.qll
View File

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

160
java/ql/lib/semmle/code/java/security/PathSanitizer.qll
View File

@@ -384,51 +384,150 @@ 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
(
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
)
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 `.`, '/', '\'
(
// 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
not targetValue.matches("%[^%]%") and
branch = false
)
)
}
/**
* A complementary 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() {
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)
}

210
java/ql/test/library-tests/pathsanitizer/Test.java
View File

@@ -604,4 +604,214 @@ 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();
// 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
}
}
}

48
java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java
View File

@@ -6,7 +6,6 @@ import java.io.InputStreamReader;
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 class TaintedPath {
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();
}
}
}
}