Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* 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`.
103 changes: 57 additions & 46 deletions java/ql/lib/semmle/code/java/security/LogInjection.qll
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ private class LineBreaksLogInjectionSanitizer extends LogInjectionSanitizer {
}

private predicate stringMethodCall(
MethodCall ma, CompileTimeConstantExpr arg0, CompileTimeConstantExpr arg1
MethodCall mc, CompileTimeConstantExpr arg0, CompileTimeConstantExpr arg1
) {
ma.getMethod().getDeclaringType() instanceof TypeString and
arg0 = ma.getArgument(0) and
arg1 = ma.getArgument(1)
mc.getMethod().getDeclaringType() instanceof TypeString and
arg0 = mc.getArgument(0) and
arg1 = mc.getArgument(1)
}

private predicate stringMethodArgument(CompileTimeConstantExpr arg) {
Expand All @@ -64,22 +64,23 @@ private predicate stringMethodArgumentValueMatches(CompileTimeConstantExpr const
}

/**
* Holds if the return value of `ma` is sanitized against log injection attacks
* by removing line breaks from it.
* Holds if `e` is sanitized against log injection attacks by removing line
* breaks from it.
*/
private predicate logInjectionSanitizer(MethodCall ma) {
exists(CompileTimeConstantExpr target, CompileTimeConstantExpr replacement |
stringMethodCall(ma, target, replacement) and
private predicate logInjectionSanitizer(Expr e) {
exists(MethodCall mc, CompileTimeConstantExpr target, CompileTimeConstantExpr replacement |
e = mc and
stringMethodCall(mc, target, replacement) and
not stringMethodArgumentValueMatches(replacement, ["%\n%", "%\r%"])
|
ma.getMethod().hasName("replace") and
mc.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
mc.getMethod().hasName("replaceAll") and
(
// Replace anything not in an allow list
target.getStringValue().matches("[^%]") and
Expand All @@ -89,48 +90,58 @@ private predicate logInjectionSanitizer(MethodCall ma) {
target.getStringValue() = ["\n", "\r", "\\n", "\\r", "\\R"]
)
)
or
exists(RegexMatch rm, CompileTimeConstantExpr target |
rm instanceof Annotation and
e = rm.getASanitizedExpr() and
target = rm.getRegex() and
regexPreventsLogInjection(target.getStringValue(), true)
)
}

/**
* 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(MethodCall 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
exists(MethodCall mc | mc = g |
mc.getMethod() instanceof StringContainsMethod and
mc.getArgument(0).(CompileTimeConstantExpr).getStringValue() = ["\n", "\r"] and
e = mc.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
)
)
or
exists(RegexMatch rm, CompileTimeConstantExpr target |
rm = g and
not rm instanceof Annotation and
target = rm.getRegex() and
e = rm.getASanitizedExpr()
|
regexPreventsLogInjection(target.getStringValue(), branch)
)
}

/**
* Holds if `regex` matches against a pattern that allows anything except
* line breaks when `branch` is `true`, or a pattern that matches line breaks
* when `branch` is `false`.
*/
bindingset[regex]
private predicate regexPreventsLogInjection(string regex, boolean branch) {
// Allow anything except line breaks
(
not regex.matches("%[^%]%") and
not regex.matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
or
regex.matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%")
) and
branch = true
or
// Disallow line breaks
(
not regex.matches("%[^%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%]%") and
// Assuming a regex containing line breaks is correctly matching line breaks in a string
regex.matches("%" + ["\n", "\r", "\\n", "\\r", "\\R"] + "%")
) and
branch = false
}
Loading