diff --git a/java/ql/lib/change-notes/2022-10-06-log-injection-sanitizers.md b/java/ql/lib/change-notes/2022-10-06-log-injection-sanitizers.md new file mode 100644 index 000000000000..a7acd34df8bf --- /dev/null +++ b/java/ql/lib/change-notes/2022-10-06-log-injection-sanitizers.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added sanitizers that recognize line breaks to the query `java/log-injection`. diff --git a/java/ql/lib/semmle/code/java/security/LogInjection.qll b/java/ql/lib/semmle/code/java/security/LogInjection.qll index 55e4a785d4bd..e60e6ed9a7f3 100644 --- a/java/ql/lib/semmle/code/java/security/LogInjection.qll +++ b/java/ql/lib/semmle/code/java/security/LogInjection.qll @@ -1,8 +1,9 @@ /** Provides classes and predicates related to Log Injection vulnerabilities. */ import java -import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow +private import semmle.code.java.controlflow.Guards /** A data flow sink for unvalidated user input that is used to log messages. */ abstract class LogInjectionSink extends DataFlow::Node { } @@ -31,6 +32,90 @@ private class DefaultLogInjectionSink extends LogInjectionSink { private class DefaultLogInjectionSanitizer extends LogInjectionSanitizer { DefaultLogInjectionSanitizer() { - this.getType() instanceof BoxedType or this.getType() instanceof PrimitiveType + this.getType() instanceof BoxedType or + this.getType() instanceof PrimitiveType or + this.getType() instanceof NumericType } } + +private class LineBreaksLogInjectionSanitizer extends LogInjectionSanitizer { + LineBreaksLogInjectionSanitizer() { + logInjectionSanitizer(this.asExpr()) + or + this = DataFlow::BarrierGuard::getABarrierNode() + } +} + +/** + * Holds if the return value of `ma` is sanitized against log injection attacks + * by removing line breaks from it. + */ +private predicate logInjectionSanitizer(MethodAccess ma) { + exists(CompileTimeConstantExpr target, CompileTimeConstantExpr replacement | + ma.getMethod().getDeclaringType() instanceof TypeString and + target = ma.getArgument(0) and + replacement = ma.getArgument(1) and + not replacement.getStringValue().matches(["%\n%", "%\r%"]) + | + ma.getMethod().hasName("replace") and + not replacement.getIntValue() = [10, 13] and + ( + target.getIntValue() = [10, 13] or // 10 == '\n', 13 == '\r' + target.getStringValue() = ["\n", "\r"] + ) + or + ma.getMethod().hasName("replaceAll") and + ( + // Replace anything not in an allow list + target.getStringValue().matches("[^%]") and + not target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%") + or + // Replace line breaks + target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\R"] + ) + ) +} + +/** + * Holds if `g` guards `e` in branch `branch` against log injection attacks + * by checking if there are line breaks in `e`. + */ +private predicate logInjectionGuard(Guard g, Expr e, boolean branch) { + exists(MethodAccess ma, CompileTimeConstantExpr target | + ma = g and + target = ma.getArgument(0) + | + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName("contains") and + target.getStringValue() = ["\n", "\r"] and + e = ma.getQualifier() and + branch = false + or + ma.getMethod().hasName("matches") and + ( + ma.getMethod().getDeclaringType() instanceof TypeString and + e = ma.getQualifier() + or + ma.getMethod().getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and + e = ma.getArgument(1) + ) and + ( + // Allow anything except line breaks + ( + not target.getStringValue().matches("%[^%]%") and + not target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%") + or + target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") + ) and + branch = true + or + // Disallow line breaks + ( + 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", "\\n", "\\r", "\\R"] + "%") + ) and + branch = false + ) + ) +} diff --git a/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java b/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java index 13d4901fc675..4bcdb197e152 100644 --- a/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java +++ b/java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java @@ -1,5 +1,6 @@ import java.util.ResourceBundle; import java.util.logging.LogRecord; +import java.util.regex.Pattern; import com.google.common.flogger.LoggingApi; import org.apache.commons.logging.Log; import org.apache.log4j.Category; @@ -19,6 +20,172 @@ public Object source() { return null; } + public void testSanitizers() { + String source = (String) source(); + Logger logger = null; + logger.debug(source.replace("\n", "")); // Safe + logger.debug(source.replace("\n", "\n")); // $ hasTaintFlow + logger.debug(source.replace("\n", "\r")); // $ hasTaintFlow + logger.debug(source.replace("\r", "")); // Safe + logger.debug(source.replace("\r", "\n")); // $ hasTaintFlow + logger.debug(source.replace("\r", "\r")); // $ hasTaintFlow + logger.debug(source.replace("something_else", "")); // $ hasTaintFlow + logger.debug(source.replace('\n', '_')); // Safe + logger.debug(source.replace('\n', '\n')); // $ hasTaintFlow + logger.debug(source.replace('\n', '\r')); // $ hasTaintFlow + logger.debug(source.replace('\r', '_')); // Safe + logger.debug(source.replace('\r', '\n')); // $ hasTaintFlow + logger.debug(source.replace('\r', '\r')); // $ hasTaintFlow + logger.debug(source.replace('-', '_')); // $ 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("\\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() { + String source = (String) source(); + Logger logger = null; + + 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(".*\\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 { + 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(".*\\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 { + logger.debug(source); // $ hasTaintFlow + } + + if (Pattern.matches(".*", source)) { + logger.debug(source); // Safe (assuming not DOTALL) + } else { + logger.debug(source); // $ hasTaintFlow + } + + if (source.matches("[^\n\r]*")) { + logger.debug(source); // Safe + } else { + logger.debug(source); // $ hasTaintFlow + } + + if (Pattern.matches("[^\n\r]*", source)) { + logger.debug(source); // Safe + } else { + 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 { + logger.debug(source); // $ hasTaintFlow + } + + if (Pattern.matches("[^a-zA-Z]*", source)) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // $ hasTaintFlow + } + + if (source.matches("[\n]*")) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // $ MISSING: $ hasTaintFlow + } + + if (Pattern.matches("[\n]*", source)) { + logger.debug(source); // $ hasTaintFlow + } else { + logger.debug(source); // $ MISSING: $ hasTaintFlow + } + + } + public void test() { { Category category = null;