Skip to content

Commit 7742a56

Browse files
authored
Merge pull request #21326 from owen-mc/java/log-injection-regex-match
Java: Recognise `@Pattern` annotation as sanitizer for log injection
2 parents 7d2b40c + cf73d96 commit 7742a56

File tree

6 files changed

+6711
-6684
lines changed

6 files changed

+6711
-6684
lines changed
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Using a regular expression to check that a string doesn't contain any line breaks is already a sanitizer for `java/log-injection`. Additional ways of doing the regular expression check are now recognised, including annotation with `@javax.validation.constraints.Pattern`.

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

Lines changed: 57 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,11 @@ private class LineBreaksLogInjectionSanitizer extends LogInjectionSanitizer {
4545
}
4646

4747
private predicate stringMethodCall(
48-
MethodCall ma, CompileTimeConstantExpr arg0, CompileTimeConstantExpr arg1
48+
MethodCall mc, CompileTimeConstantExpr arg0, CompileTimeConstantExpr arg1
4949
) {
50-
ma.getMethod().getDeclaringType() instanceof TypeString and
51-
arg0 = ma.getArgument(0) and
52-
arg1 = ma.getArgument(1)
50+
mc.getMethod().getDeclaringType() instanceof TypeString and
51+
arg0 = mc.getArgument(0) and
52+
arg1 = mc.getArgument(1)
5353
}
5454

5555
private predicate stringMethodArgument(CompileTimeConstantExpr arg) {
@@ -64,22 +64,23 @@ private predicate stringMethodArgumentValueMatches(CompileTimeConstantExpr const
6464
}
6565

6666
/**
67-
* Holds if the return value of `ma` is sanitized against log injection attacks
68-
* by removing line breaks from it.
67+
* Holds if `e` is sanitized against log injection attacks by removing line
68+
* breaks from it.
6969
*/
70-
private predicate logInjectionSanitizer(MethodCall ma) {
71-
exists(CompileTimeConstantExpr target, CompileTimeConstantExpr replacement |
72-
stringMethodCall(ma, target, replacement) and
70+
private predicate logInjectionSanitizer(Expr e) {
71+
exists(MethodCall mc, CompileTimeConstantExpr target, CompileTimeConstantExpr replacement |
72+
e = mc and
73+
stringMethodCall(mc, target, replacement) and
7374
not stringMethodArgumentValueMatches(replacement, ["%\n%", "%\r%"])
7475
|
75-
ma.getMethod().hasName("replace") and
76+
mc.getMethod().hasName("replace") and
7677
not replacement.getIntValue() = [10, 13] and
7778
(
7879
target.getIntValue() = [10, 13] or // 10 == '\n', 13 == '\r'
7980
target.getStringValue() = ["\n", "\r"]
8081
)
8182
or
82-
ma.getMethod().hasName("replaceAll") and
83+
mc.getMethod().hasName("replaceAll") and
8384
(
8485
// Replace anything not in an allow list
8586
target.getStringValue().matches("[^%]") and
@@ -89,48 +90,58 @@ private predicate logInjectionSanitizer(MethodCall ma) {
8990
target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\R"]
9091
)
9192
)
93+
or
94+
exists(RegexMatch rm, CompileTimeConstantExpr target |
95+
rm instanceof Annotation and
96+
e = rm.getASanitizedExpr() and
97+
target = rm.getRegex() and
98+
regexPreventsLogInjection(target.getStringValue(), true)
99+
)
92100
}
93101

94102
/**
95103
* Holds if `g` guards `e` in branch `branch` against log injection attacks
96104
* by checking if there are line breaks in `e`.
97105
*/
98106
private predicate logInjectionGuard(Guard g, Expr e, boolean branch) {
99-
exists(MethodCall ma, CompileTimeConstantExpr target |
100-
ma = g and
101-
target = ma.getArgument(0)
102-
|
103-
ma.getMethod().getDeclaringType() instanceof TypeString and
104-
ma.getMethod().hasName("contains") and
105-
target.getStringValue() = ["\n", "\r"] and
106-
e = ma.getQualifier() and
107+
exists(MethodCall mc | mc = g |
108+
mc.getMethod() instanceof StringContainsMethod and
109+
mc.getArgument(0).(CompileTimeConstantExpr).getStringValue() = ["\n", "\r"] and
110+
e = mc.getQualifier() and
107111
branch = false
108-
or
109-
ma.getMethod().hasName("matches") and
110-
(
111-
ma.getMethod().getDeclaringType() instanceof TypeString and
112-
e = ma.getQualifier()
113-
or
114-
ma.getMethod().getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and
115-
e = ma.getArgument(1)
116-
) and
117-
(
118-
// Allow anything except line breaks
119-
(
120-
not target.getStringValue().matches("%[^%]%") and
121-
not target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
122-
or
123-
target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%")
124-
) and
125-
branch = true
126-
or
127-
// Disallow line breaks
128-
(
129-
not target.getStringValue().matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") and
130-
// Assuming a regex containing line breaks is correctly matching line breaks in a string
131-
target.getStringValue().matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
132-
) and
133-
branch = false
134-
)
135112
)
113+
or
114+
exists(RegexMatch rm, CompileTimeConstantExpr target |
115+
rm = g and
116+
not rm instanceof Annotation and
117+
target = rm.getRegex() and
118+
e = rm.getASanitizedExpr()
119+
|
120+
regexPreventsLogInjection(target.getStringValue(), branch)
121+
)
122+
}
123+
124+
/**
125+
* Holds if `regex` matches against a pattern that allows anything except
126+
* line breaks when `branch` is `true`, or a pattern that matches line breaks
127+
* when `branch` is `false`.
128+
*/
129+
bindingset[regex]
130+
private predicate regexPreventsLogInjection(string regex, boolean branch) {
131+
// Allow anything except line breaks
132+
(
133+
not regex.matches("%[^%]%") and
134+
not regex.matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
135+
or
136+
regex.matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%")
137+
) and
138+
branch = true
139+
or
140+
// Disallow line breaks
141+
(
142+
not regex.matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") and
143+
// Assuming a regex containing line breaks is correctly matching line breaks in a string
144+
regex.matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
145+
) and
146+
branch = false
136147
}

0 commit comments

Comments
 (0)