Skip to content

Commit e167d3c

Browse files
atorralbaatorralba
committed
Add line break sanitizers
1 parent 4e49df1 commit e167d3c

File tree

3 files changed

+200
-2
lines changed

3 files changed

+200
-2
lines changed

‎java/ql/lib/change-notes/2022-10-06-log-injection-sanitizers.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added sanitizers that recognize line breaks to the query `java/log-injection`.

‎java/ql/lib/semmle/code/java/security/LogInjection.qll

Lines changed: 87 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
/** Provides classes and predicates related to Log Injection vulnerabilities. */
22

33
import java
4-
import semmle.code.java.dataflow.DataFlow
4+
private import semmle.code.java.dataflow.DataFlow
55
private import semmle.code.java.dataflow.ExternalFlow
6+
private import semmle.code.java.controlflow.Guards
67

78
/** A data flow sink for unvalidated user input that is used to log messages. */
89
abstract class LogInjectionSink extends DataFlow::Node { }
@@ -31,6 +32,90 @@ private class DefaultLogInjectionSink extends LogInjectionSink {
3132

3233
private class DefaultLogInjectionSanitizer extends LogInjectionSanitizer {
3334
DefaultLogInjectionSanitizer() {
34-
this.getType() instanceof BoxedType or this.getType() instanceof PrimitiveType
35+
this.getType() instanceof BoxedType or
36+
this.getType() instanceof PrimitiveType or
37+
this.getType() instanceof NumericType
3538
}
3639
}
40+
41+
private class LineBreaksLogInjectionSanitizer extends LogInjectionSanitizer {
42+
LineBreaksLogInjectionSanitizer() {
43+
logInjectionSanitizer(this.asExpr())
44+
or
45+
this = DataFlow::BarrierGuard<logInjectionGuard/3>::getABarrierNode()
46+
}
47+
}
48+
49+
/**
50+
* Holds if the return value of `ma` is sanitized against log injection attacks
51+
* by removing line breaks from it.
52+
*/
53+
private predicate logInjectionSanitizer(MethodAccess ma) {
54+
exists(CompileTimeConstantExpr target, CompileTimeConstantExpr replacement |
55+
ma.getMethod().getDeclaringType() instanceof TypeString and
56+
target = ma.getArgument(0) and
57+
replacement = ma.getArgument(1) and
58+
not replacement.getStringValue().matches(["%\n%", "%\r%"])
59+
|
60+
ma.getMethod().hasName("replace") and
61+
not replacement.getIntValue() = [10, 13] and
62+
(
63+
target.getIntValue() = [10, 13] or // 10 == '\n', 13 == '\r'
64+
target.getStringValue() = ["\n", "\r"]
65+
)
66+
or
67+
ma.getMethod().hasName("replaceAll") and
68+
(
69+
// Replace anything not in an allow list
70+
target.getStringValue().matches("[^%]") and
71+
not target.getStringValue().matches(["%\n%", "%\r%"])
72+
or
73+
// Replace line breaks
74+
target.getStringValue() = ["\n", "\r"]
75+
)
76+
)
77+
}
78+
79+
/**
80+
* Holds if `g` guards `e` in branch `branch` against log injection attacks
81+
* by checking if there are line breaks in `e`.
82+
*/
83+
private predicate logInjectionGuard(Guard g, Expr e, boolean branch) {
84+
exists(MethodAccess ma, CompileTimeConstantExpr target |
85+
ma = g and
86+
target = ma.getArgument(0)
87+
|
88+
ma.getMethod().getDeclaringType() instanceof TypeString and
89+
ma.getMethod().hasName("contains") and
90+
target.getStringValue() = ["\n", "\r"] and
91+
e = ma.getQualifier() and
92+
branch = false
93+
or
94+
ma.getMethod().hasName("matches") and
95+
(
96+
ma.getMethod().getDeclaringType() instanceof TypeString and
97+
e = ma.getQualifier()
98+
or
99+
ma.getMethod().getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and
100+
e = ma.getArgument(1)
101+
) and
102+
(
103+
// Allow anything except line breaks
104+
(
105+
not target.getStringValue().matches("%[^%]%") and
106+
not target.getStringValue().matches(["%\n%", "%\r%"])
107+
or
108+
target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"])
109+
) and
110+
branch = true
111+
or
112+
// Disallow line breaks
113+
(
114+
not target.getStringValue().matches(["%[^%\n%]%", "%[^%\r%]%"]) and
115+
// Assuming a regex containing line breaks is correctly matching line breaks in a string
116+
target.getStringValue().matches(["%\n%", "%\r%"])
117+
) and
118+
branch = false
119+
)
120+
)
121+
}

‎java/ql/test/query-tests/security/CWE-117/LogInjectionTest.java

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import java.util.ResourceBundle;
22
import java.util.logging.LogRecord;
3+
import java.util.regex.Pattern;
34
import com.google.common.flogger.LoggingApi;
45
import org.apache.commons.logging.Log;
56
import org.apache.log4j.Category;
@@ -19,6 +20,114 @@ public Object source() {
1920
return null;
2021
}
2122

23+
public void testSanitizers() {
24+
String source = (String) source();
25+
Logger logger = null;
26+
logger.debug(source.replace("\n", "")); // Safe
27+
logger.debug(source.replace("\n", "\n")); // $ hasTaintFlow
28+
logger.debug(source.replace("\n", "\r")); // $ hasTaintFlow
29+
logger.debug(source.replace("\r", "")); // Safe
30+
logger.debug(source.replace("\r", "\n")); // $ hasTaintFlow
31+
logger.debug(source.replace("\r", "\r")); // $ hasTaintFlow
32+
logger.debug(source.replace("something_else", "")); // $ hasTaintFlow
33+
logger.debug(source.replace('\n', '_')); // Safe
34+
logger.debug(source.replace('\n', '\n')); // $ hasTaintFlow
35+
logger.debug(source.replace('\n', '\r')); // $ hasTaintFlow
36+
logger.debug(source.replace('\r', '_')); // Safe
37+
logger.debug(source.replace('\r', '\n')); // $ hasTaintFlow
38+
logger.debug(source.replace('\r', '\r')); // $ hasTaintFlow
39+
logger.debug(source.replace('-', '_')); // $ hasTaintFlow
40+
logger.debug(source.replaceAll("\n", "")); // Safe
41+
logger.debug(source.replaceAll("\n", "\n")); // $ hasTaintFlow
42+
logger.debug(source.replaceAll("\n", "\r")); // $ hasTaintFlow
43+
logger.debug(source.replaceAll("\r", "")); // Safe
44+
logger.debug(source.replaceAll("\r", "\n")); // $ hasTaintFlow
45+
logger.debug(source.replaceAll("\r", "\r")); // $ hasTaintFlow
46+
logger.debug(source.replaceAll("[^a-zA-Z]", "")); // Safe
47+
logger.debug(source.replaceAll("[^a-zA-Z]", "\n")); // $ hasTaintFlow
48+
logger.debug(source.replaceAll("[^a-zA-Z]", "\r")); // $ hasTaintFlow
49+
logger.debug(source.replaceAll("[^a-zA-Z\n]", "")); // $ hasTaintFlow
50+
logger.debug(source.replaceAll("[^a-zA-Z\r]", "")); // $ hasTaintFlow
51+
}
52+
53+
public void testGuards() {
54+
String source = (String) source();
55+
Logger logger = null;
56+
57+
if (source.matches(".*\n.*")) {
58+
logger.debug(source); // $ hasTaintFlow
59+
} else {
60+
logger.debug(source); // Safe
61+
}
62+
63+
if (Pattern.matches(".*\n.*", source)) {
64+
logger.debug(source); // $ hasTaintFlow
65+
} else {
66+
logger.debug(source); // Safe
67+
}
68+
69+
if (source.matches(".*\r.*")) {
70+
logger.debug(source); // $ hasTaintFlow
71+
} else {
72+
logger.debug(source); // Safe
73+
}
74+
75+
if (Pattern.matches(".*\r.*", source)) {
76+
logger.debug(source); // $ hasTaintFlow
77+
} else {
78+
logger.debug(source); // Safe
79+
}
80+
81+
if (source.matches(".*")) {
82+
logger.debug(source); // Safe (assuming not DOTALL)
83+
} else {
84+
logger.debug(source); // $ hasTaintFlow
85+
}
86+
87+
if (Pattern.matches(".*", source)) {
88+
logger.debug(source); // Safe (assuming not DOTALL)
89+
} else {
90+
logger.debug(source); // $ hasTaintFlow
91+
}
92+
93+
if (source.matches("[^\n\r]*")) {
94+
logger.debug(source); // Safe
95+
} else {
96+
logger.debug(source); // $ hasTaintFlow
97+
}
98+
99+
if (Pattern.matches("[^\n\r]*", source)) {
100+
logger.debug(source); // Safe
101+
} else {
102+
logger.debug(source); // $ hasTaintFlow
103+
}
104+
105+
if (source.matches("[^a-zA-Z]*")) {
106+
logger.debug(source); // $ hasTaintFlow
107+
} else {
108+
logger.debug(source); // $ hasTaintFlow
109+
}
110+
111+
if (Pattern.matches("[^a-zA-Z]*", source)) {
112+
logger.debug(source); // $ hasTaintFlow
113+
} else {
114+
logger.debug(source); // $ hasTaintFlow
115+
}
116+
117+
if (source.matches("[\n]*")) {
118+
logger.debug(source); // $ hasTaintFlow
119+
} else {
120+
logger.debug(source); // $ MISSING: $ hasTaintFlow
121+
}
122+
123+
if (Pattern.matches("[\n]*", source)) {
124+
logger.debug(source); // $ hasTaintFlow
125+
} else {
126+
logger.debug(source); // $ MISSING: $ hasTaintFlow
127+
}
128+
129+
}
130+
22131
public void test() {
23132
{
24133
Category category = null;

0 commit comments

Comments
 (0)