hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-22 03:36:30 +01:00
Code Issues Packages Projects Releases Wiki Activity

Address review comment

Browse Source 
Handle more regex cases that cover line breaks
This commit is contained in:
Tony Torralba
2022-10-07 10:13:39 +02:00
parent e167d3ce00
commit f5702f5c69
2 changed files with 64 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
java/ql/lib/semmle/code/java/security/LogInjection.qll
View File

@@ -68,10 +68,10 @@ private predicate logInjectionSanitizer(MethodAccess ma) {
(
// Replace anything not in an allow list
target.getStringValue().matches("[^%]") and
not target.getStringValue().matches(["%\n%", "%\r%"])
not target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
or
// Replace line breaks
target.getStringValue() = ["\n", "\r"]
target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\R"]
)
)
}
@@ -103,17 +103,17 @@ private predicate logInjectionGuard(Guard g, Expr e, boolean branch) {
// Allow anything except line breaks
(
not target.getStringValue().matches("%[^%]%") and
not target.getStringValue().matches(["%\n%", "%\r%"])
not target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
or
target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"])
target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%")
) and
branch = true
or
// Disallow line breaks
(
not target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"]) and
not target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") and
// Assuming a regex containing line breaks is correctly matching line breaks in a string
target.getStringValue().matches(["%\n%", "%\r%"])
target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
) and
branch = false
)

58
java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java
View File

@@ -43,11 +43,21 @@ public class LogInjectionTest {
logger.debug(source.replaceAll("\r", "")); // Safe
logger.debug(source.replaceAll("\r", "\n")); // $ hasTaintFlow
logger.debug(source.replaceAll("\r", "\r")); // $ hasTaintFlow
logger.debug(source.replaceAll("\\n", "")); // Safe
logger.debug(source.replaceAll("\\n", "\n")); // $ hasTaintFlow
logger.debug(source.replaceAll("\\n", "\r")); // $ hasTaintFlow
logger.debug(source.replaceAll("\\r", "")); // Safe
logger.debug(source.replaceAll("\\r", "\n")); // $ hasTaintFlow
logger.debug(source.replaceAll("\\r", "\r")); // $ hasTaintFlow
logger.debug(source.replaceAll("\\R", "")); // Safe
logger.debug(source.replaceAll("\\R", "\n")); // $ hasTaintFlow
logger.debug(source.replaceAll("\\R", "\r")); // $ hasTaintFlow
logger.debug(source.replaceAll("[^a-zA-Z]", "")); // Safe
logger.debug(source.replaceAll("[^a-zA-Z]", "\n")); // $ hasTaintFlow
logger.debug(source.replaceAll("[^a-zA-Z]", "\r")); // $ hasTaintFlow
logger.debug(source.replaceAll("[^a-zA-Z\n]", "")); // $ hasTaintFlow
logger.debug(source.replaceAll("[^a-zA-Z\r]", "")); // $ hasTaintFlow
logger.debug(source.replaceAll("[^a-zA-Z\\R]", "")); // $ hasTaintFlow
}
public void testGuards() {
@@ -66,6 +76,18 @@ public class LogInjectionTest {
logger.debug(source); // Safe
}
if (source.matches(".*\\n.*")) {
logger.debug(source); // $ hasTaintFlow
} else {
logger.debug(source); // Safe
}
if (Pattern.matches(".*\\n.*", source)) {
logger.debug(source); // $ hasTaintFlow
} else {
logger.debug(source); // Safe
}
if (source.matches(".*\r.*")) {
logger.debug(source); // $ hasTaintFlow
} else {
@@ -78,6 +100,30 @@ public class LogInjectionTest {
logger.debug(source); // Safe
}
if (source.matches(".*\\r.*")) {
logger.debug(source); // $ hasTaintFlow
} else {
logger.debug(source); // Safe
}
if (Pattern.matches(".*\\r.*", source)) {
logger.debug(source); // $ hasTaintFlow
} else {
logger.debug(source); // Safe
}
if (source.matches(".*\\R.*")) {
logger.debug(source); // $ hasTaintFlow
} else {
logger.debug(source); // Safe
}
if (Pattern.matches(".*\\R.*", source)) {
logger.debug(source); // $ hasTaintFlow
} else {
logger.debug(source); // Safe
}
if (source.matches(".*")) {
logger.debug(source); // Safe (assuming not DOTALL)
} else {
@@ -102,6 +148,18 @@ public class LogInjectionTest {
logger.debug(source); // $ hasTaintFlow
}
if (source.matches("[^\\R]*")) {
logger.debug(source); // Safe
} else {
logger.debug(source); // $ hasTaintFlow
}
if (Pattern.matches("[^\\R]*", source)) {
logger.debug(source); // Safe
} else {
logger.debug(source); // $ hasTaintFlow
}
if (source.matches("[^a-zA-Z]*")) {
logger.debug(source); // $ hasTaintFlow
} else {