hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-28 10:15:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Make block lists work with substring matching too

Browse Source 
A block list approach doesn't need to restrict itself to prefix matching
This commit is contained in:
Tony Torralba
2022-09-01 11:32:52 +02:00
parent 69d1895175
commit 90020b6aab
2 changed files with 14 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -72,7 +72,6 @@ private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
private class AllowListGuard extends Guard instanceof MethodAccess {
AllowListGuard() {
(isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
not isDisallowedPrefix(super.getAnArgument()) and
not isDisallowedWord(super.getAnArgument())
}
@@ -127,10 +126,7 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer {
private class BlockListGuard extends Guard instanceof MethodAccess {
BlockListGuard() {
(isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
isDisallowedPrefix(super.getAnArgument())
or
isStringPartialMatch(this) and
(isStringPartialMatch(this) or isPathPrefixMatch(this)) and
isDisallowedWord(super.getAnArgument())
}
@@ -178,6 +174,8 @@ private predicate isStringPrefixMatch(MethodAccess ma) {
* Holds if `ma` is a call to a method that checks a partial string match.
*/
private predicate isStringPartialMatch(MethodAccess ma) {
isStringPrefixMatch(ma)
or
ma.getMethod().getDeclaringType() instanceof TypeString and
ma.getMethod().hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"])
}
@@ -196,12 +194,8 @@ private predicate isPathPrefixMatch(MethodAccess ma) {
)
}
private predicate isDisallowedPrefix(CompileTimeConstantExpr prefix) {
prefix.getStringValue().matches(["%WEB-INF%", "/data%"])
}
private predicate isDisallowedWord(CompileTimeConstantExpr word) {
word.getStringValue().matches(["/", "\\"])
word.getStringValue().matches(["/", "\\", "%WEB-INF%", "%/data%"])
}
/** A complementary guard that protects against path traversal, by looking for the literal `..`. */

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

@@ -421,13 +421,13 @@ public class Test {
sink(normalized); // $ hasTaintFlow
}
}
// PathInjectionSanitizer + partial string match with prefixes is considered unsafe
// PathInjectionSanitizer + partial string match with disallowed prefixes
{
String source = (String) source();
String normalized = Paths.get(source).normalize().toString();
if (normalized.contains("/data")) {
sink(source); // $ hasTaintFlow
sink(normalized); // $ hasTaintFlow
if (!normalized.contains("/data")) {
sink(source); // Safe
sink(normalized); // Safe
} else {
sink(source); // $ hasTaintFlow
sink(normalized); // $ hasTaintFlow
@@ -436,9 +436,9 @@ public class Test {
{
String source = (String) source();
String normalized = Paths.get(source).normalize().toString();
if (normalized.regionMatches(1, "/data", 0, 5)) {
sink(source); // $ hasTaintFlow
sink(normalized); // $ hasTaintFlow
if (!normalized.regionMatches(1, "/data", 0, 5)) {
sink(source); // Safe
sink(normalized); // Safe
} else {
sink(source); // $ hasTaintFlow
sink(normalized); // $ hasTaintFlow
@@ -447,9 +447,9 @@ public class Test {
{
String source = (String) source();
String normalized = Paths.get(source).normalize().toString();
if (normalized.matches(".*/data/.*")) {
sink(source); // $ hasTaintFlow
sink(normalized); // $ hasTaintFlow
if (!normalized.matches(".*/data/.*")) {
sink(source); // Safe
sink(normalized); // Safe
} else {
sink(source); // $ hasTaintFlow
sink(normalized); // $ hasTaintFlow