Skip to content

Commit 9d88795

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 1904daf commit 9d88795

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
@@ -35,7 +35,15 @@ module LogInjection {
3535

3636
/** An argument to a logging mechanism. */
3737
class LoggerSink extends Sink {
38-
LoggerSink() { this = any(LoggerCall log).getAMessageComponent() }
38+
LoggerSink() {
39+
exists(LoggerCall call |
40+
this = call.getAMessageComponent() and
41+
// exclude arguments to `call` which have a safe format argument, which
42+
// aren't caught by SafeFormatArgumentSanitizer as that sanitizes the
43+
// result of the call.
44+
not safeFormatArgument(this, call)
45+
)
46+
}
3947
}
4048

4149
/**
@@ -47,6 +55,22 @@ module LogInjection {
4755
ReplaceSanitizer() { this.getReplacedString() = ["\r", "\n"] }
4856
}
4957

58+
/**
59+
* Holds if `arg` is an argument to `call` that is formatted using the `%q`
60+
* directive. This formatting directive replaces newline characters with
61+
* escape sequences, so `arg` would not be a sink for log injection.
62+
*/
63+
private predicate safeFormatArgument(
64+
DataFlow::Node arg, StringOps::Formatting::StringFormatCall call
65+
) {
66+
exists(string safeDirective |
67+
// Mark "%q" formats as safe, but not "%#q", which would preserve newline characters.
68+
safeDirective.regexpMatch("%[^%#]*q")
69+
|
70+
arg = call.getOperand(_, safeDirective)
71+
)
72+
}
73+
5074
/**
5175
* An argument that is formatted using the `%q` directive, considered as a sanitizer
5276
* for log injection.
@@ -55,10 +79,10 @@ module LogInjection {
5579
*/
5680
private class SafeFormatArgumentSanitizer extends Sanitizer {
5781
SafeFormatArgumentSanitizer() {
58-
exists(StringOps::Formatting::StringFormatCall call, string safeDirective |
59-
this = call.getOperand(_, safeDirective) and
60-
// Mark "%q" formats as safe, but not "%#q", which would preserve newline characters.
61-
safeDirective.regexpMatch("%[^%#]*q")
82+
exists(DataFlow::Node arg, StringOps::Formatting::StringFormatCall call |
83+
safeFormatArgument(arg, call)
84+
|
85+
this = call.getAResult()
6286
)
6387
}
6488
}

0 commit comments

Comments
 (0)