Skip to content

Commit 49a00c1

Browse files
committed
Adjust SafeFormatArgumentSanitizer to use-use flow
Make it sanitize the result of the call rather than the input, so that further uses of the input are still tainted. This means that it catches things like `log.Print(fmt.Sprintf("user %q logged in.\n", username))` where the argument to the LoggerCall contains a StringFormatCall, but it misses things like `log.Printf("user %q logged in.\n", username)`. So we extract the logic into a predicate and apply it as a condition in the sink as well. The downside of this approach is that if there are two tainted inputs and only one has a safe format argument then we still sanitize the result. Hopefully this is rare.
1 parent 1573f24 commit 49a00c1

File tree

1 file changed

+29
-5
lines changed

1 file changed

+29
-5
lines changed

go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,15 @@ module LogInjection {
3030

3131
/** An argument to a logging mechanism. */
3232
class LoggerSink extends Sink {
33-
LoggerSink() { this = any(LoggerCall log).getAMessageComponent() }
33+
LoggerSink() {
34+
exists(LoggerCall call |
35+
this = call.getAMessageComponent() and
36+
// exclude arguments to `call` which have a safe format argument, which
37+
// aren't caught by SafeFormatArgumentSanitizer as that sanitizes the
38+
// result of the call.
39+
not safeFormatArgument(this, call)
40+
)
41+
}
3442
}
3543

3644
/**
@@ -42,6 +50,22 @@ module LogInjection {
4250
ReplaceSanitizer() { this.getReplacedString() = ["\r", "\n"] }
4351
}
4452

53+
/**
54+
* Holds if `arg` is an argument to `call` that is formatted using the `%q`
55+
* directive. This formatting directive replaces newline characters with
56+
* escape sequences, so `arg` would not be a sink for log injection.
57+
*/
58+
private predicate safeFormatArgument(
59+
DataFlow::Node arg, StringOps::Formatting::StringFormatCall call
60+
) {
61+
exists(string safeDirective |
62+
// Mark "%q" formats as safe, but not "%#q", which would preserve newline characters.
63+
safeDirective.regexpMatch("%[^%#]*q")
64+
|
65+
arg = call.getOperand(_, safeDirective)
66+
)
67+
}
68+
4569
/**
4670
* An argument that is formatted using the `%q` directive, considered as a sanitizer
4771
* for log injection.
@@ -50,10 +74,10 @@ module LogInjection {
5074
*/
5175
private class SafeFormatArgumentSanitizer extends Sanitizer {
5276
SafeFormatArgumentSanitizer() {
53-
exists(StringOps::Formatting::StringFormatCall call, string safeDirective |
54-
this = call.getOperand(_, safeDirective) and
55-
// Mark "%q" formats as safe, but not "%#q", which would preserve newline characters.
56-
safeDirective.regexpMatch("%[^%#]*q")
77+
exists(DataFlow::Node arg, StringOps::Formatting::StringFormatCall call |
78+
safeFormatArgument(arg, call)
79+
|
80+
this = call.getAResult()
5781
)
5882
}
5983
}

0 commit comments

Comments
 (0)