Skip to content

Commit

Permalink
Merge pull request #10707 from atorralba/atorralba/log-injection-sani…
Browse files Browse the repository at this point in the history 
…tizers

Java: Add line break sanitizers to java/log-injection
  • Loading branch information
@atorralba
atorralba authored Apr 19, 2023
2 parents 9aca2d8 + f5702f5 commit 62f5a5d
Show file tree
Hide file tree
Showing 3 changed files with 258 additions and 2 deletions.
4 changes: 4 additions & 0 deletions java/ql/lib/change-notes/2022-10-06-log-injection-sanitizers.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Added sanitizers that recognize line breaks to the query `java/log-injection`.
89 changes: 87 additions & 2 deletions java/ql/lib/semmle/code/java/security/LogInjection.qll
View file
Original file line number Diff line number Diff line change
@@ -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 { }
Expand Down Expand Up @@ -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<logInjectionGuard/3>::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
)
)
}
167 changes: 167 additions & 0 deletions java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand Down

0 comments on commit 62f5a5d

Please sign in to comment.