Skip to content

Commit 62f5a5d

Browse files
atorralbaatorralba
authored
Merge pull request #10707 from atorralba/atorralba/log-injection-sanitizers
Java: Add line break sanitizers to java/log-injection
2 parents 9aca2d8 + f5702f5 commit 62f5a5d

3 files changed

Lines changed: 258 additions & 2 deletions

File tree

‎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", "\\n", "\\r", "\\R"] + "%")
72+
or
73+
// Replace line breaks
74+
target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\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", "\\n", "\\r", "\\R"] + "%")
107+
or
108+
target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%")
109+
) and
110+
branch = true
111+
or
112+
// Disallow line breaks
113+
(
114+
not target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") and
115+
// Assuming a regex containing line breaks is correctly matching line breaks in a string
116+
target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
117+
) and
118+
branch = false
119+
)
120+
)
121+
}

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

Lines changed: 167 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,172 @@ 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("\\n", "")); // Safe
47+
logger.debug(source.replaceAll("\\n", "\n")); // $ hasTaintFlow
48+
logger.debug(source.replaceAll("\\n", "\r")); // $ hasTaintFlow
49+
logger.debug(source.replaceAll("\\r", "")); // Safe
50+
logger.debug(source.replaceAll("\\r", "\n")); // $ hasTaintFlow
51+
logger.debug(source.replaceAll("\\r", "\r")); // $ hasTaintFlow
52+
logger.debug(source.replaceAll("\\R", "")); // Safe
53+
logger.debug(source.replaceAll("\\R", "\n")); // $ hasTaintFlow
54+
logger.debug(source.replaceAll("\\R", "\r")); // $ hasTaintFlow
55+
logger.debug(source.replaceAll("[^a-zA-Z]", "")); // Safe
56+
logger.debug(source.replaceAll("[^a-zA-Z]", "\n")); // $ hasTaintFlow
57+
logger.debug(source.replaceAll("[^a-zA-Z]", "\r")); // $ hasTaintFlow
58+
logger.debug(source.replaceAll("[^a-zA-Z\n]", "")); // $ hasTaintFlow
59+
logger.debug(source.replaceAll("[^a-zA-Z\r]", "")); // $ hasTaintFlow
60+
logger.debug(source.replaceAll("[^a-zA-Z\\R]", "")); // $ hasTaintFlow
61+
}
62+
63+
public void testGuards() {
64+
String source = (String) source();
65+
Logger logger = null;
66+
67+
if (source.matches(".*\n.*")) {
68+
logger.debug(source); // $ hasTaintFlow
69+
} else {
70+
logger.debug(source); // Safe
71+
}
72+
73+
if (Pattern.matches(".*\n.*", source)) {
74+
logger.debug(source); // $ hasTaintFlow
75+
} else {
76+
logger.debug(source); // Safe
77+
}
78+
79+
if (source.matches(".*\\n.*")) {
80+
logger.debug(source); // $ hasTaintFlow
81+
} else {
82+
logger.debug(source); // Safe
83+
}
84+
85+
if (Pattern.matches(".*\\n.*", source)) {
86+
logger.debug(source); // $ hasTaintFlow
87+
} else {
88+
logger.debug(source); // Safe
89+
}
90+
91+
if (source.matches(".*\r.*")) {
92+
logger.debug(source); // $ hasTaintFlow
93+
} else {
94+
logger.debug(source); // Safe
95+
}
96+
97+
if (Pattern.matches(".*\r.*", source)) {
98+
logger.debug(source); // $ hasTaintFlow
99+
} else {
100+
logger.debug(source); // Safe
101+
}
102+
103+
if (source.matches(".*\\r.*")) {
104+
logger.debug(source); // $ hasTaintFlow
105+
} else {
106+
logger.debug(source); // Safe
107+
}
108+
109+
if (Pattern.matches(".*\\r.*", source)) {
110+
logger.debug(source); // $ hasTaintFlow
111+
} else {
112+
logger.debug(source); // Safe
113+
}
114+
115+
if (source.matches(".*\\R.*")) {
116+
logger.debug(source); // $ hasTaintFlow
117+
} else {
118+
logger.debug(source); // Safe
119+
}
120+
121+
if (Pattern.matches(".*\\R.*", source)) {
122+
logger.debug(source); // $ hasTaintFlow
123+
} else {
124+
logger.debug(source); // Safe
125+
}
126+
127+
if (source.matches(".*")) {
128+
logger.debug(source); // Safe (assuming not DOTALL)
129+
} else {
130+
logger.debug(source); // $ hasTaintFlow
131+
}
132+
133+
if (Pattern.matches(".*", source)) {
134+
logger.debug(source); // Safe (assuming not DOTALL)
135+
} else {
136+
logger.debug(source); // $ hasTaintFlow
137+
}
138+
139+
if (source.matches("[^\n\r]*")) {
140+
logger.debug(source); // Safe
141+
} else {
142+
logger.debug(source); // $ hasTaintFlow
143+
}
144+
145+
if (Pattern.matches("[^\n\r]*", source)) {
146+
logger.debug(source); // Safe
147+
} else {
148+
logger.debug(source); // $ hasTaintFlow
149+
}
150+
151+
if (source.matches("[^\\R]*")) {
152+
logger.debug(source); // Safe
153+
} else {
154+
logger.debug(source); // $ hasTaintFlow
155+
}
156+
157+
if (Pattern.matches("[^\\R]*", source)) {
158+
logger.debug(source); // Safe
159+
} else {
160+
logger.debug(source); // $ hasTaintFlow
161+
}
162+
163+
if (source.matches("[^a-zA-Z]*")) {
164+
logger.debug(source); // $ hasTaintFlow
165+
} else {
166+
logger.debug(source); // $ hasTaintFlow
167+
}
168+
169+
if (Pattern.matches("[^a-zA-Z]*", source)) {
170+
logger.debug(source); // $ hasTaintFlow
171+
} else {
172+
logger.debug(source); // $ hasTaintFlow
173+
}
174+
175+
if (source.matches("[\n]*")) {
176+
logger.debug(source); // $ hasTaintFlow
177+
} else {
178+
logger.debug(source); // $ MISSING: $ hasTaintFlow
179+
}
180+
181+
if (Pattern.matches("[\n]*", source)) {
182+
logger.debug(source); // $ hasTaintFlow
183+
} else {
184+
logger.debug(source); // $ MISSING: $ hasTaintFlow
185+
}
186+
187+
}
188+
22189
public void test() {
23190
{
24191
Category category = null;

0 commit comments

Comments
 (0)