Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go: Fix false positives when logging using %T #19053

Merged
merged 13 commits into from
Mar 25, 2025
Merged
17 changes: 17 additions & 0 deletions go/ql/lib/semmle/go/Concepts.qll
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,23 @@ module RegexpReplaceFunction {
class LoggerCall extends DataFlow::Node instanceof LoggerCall::Range {
/** Gets a node that is a part of the logged message. */
DataFlow::Node getAMessageComponent() { result = super.getAMessageComponent() }

/**
* Gets a node whose value is a part of the logged message.
*
* Components corresponding to the format specifier "%T" are excluded as
* their type is logged rather than their value.
*/
DataFlow::Node getAValueFormattedMessageComponent() {
result = this.getAMessageComponent() and
not exists(string formatSpecifier |
result = this.(StringOps::Formatting::StringFormatCall).getOperand(_, formatSpecifier) and
// We already know that `formatSpecifier` starts with `%`, so we check
// that it ends with `T` to confirm that it is `%T` or possibly some
// variation on it.
formatSpecifier.matches("%T")
)
}
}

/** Provides a class for modeling new logging APIs. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ module CleartextLogging {
* An argument to a logging mechanism.
*/
class LoggerSink extends Sink {
LoggerSink() { this = any(LoggerCall log).getAMessageComponent() }
LoggerSink() { this = any(LoggerCall log).getAValueFormattedMessageComponent() }
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ module LogInjection {

/** An argument to a logging mechanism. */
class LoggerSink extends Sink {
LoggerSink() { this = any(LoggerCall log).getAMessageComponent() }
LoggerSink() { this = any(LoggerCall log).getAValueFormattedMessageComponent() }
}

/**
Expand Down
4 changes: 3 additions & 1 deletion go/ql/src/Security/CWE-352/ConstantOauth2State.ql
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ predicate privateUrlFlowsToAuthCodeUrlCall(DataFlow::CallNode call) {

module FlowToPrintConfig implements DataFlow::ConfigSig {
additional predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
exists(LoggerCall logCall | call = logCall | sink = logCall.getAMessageComponent())
exists(LoggerCall logCall | call = logCall |
sink = logCall.getAValueFormattedMessageComponent()
)
}

predicate isSource(DataFlow::Node source) { source = any(AuthCodeUrl m).getACall().getResult() }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* False positives in "Log entries created from user input" (`go/log-injection`) and "Clear-text logging of sensitive information" (`go/clear-text-logging`) which involved the verb `%T` in a format specifier have been fixed. As a result, some users may also see more alerts from the "Use of constant `state` value in OAuth 2.0 URL" (`go/constant-oauth2-state`) query.
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,20 @@ import ModelValidation
import utils.test.InlineExpectationsTest

module LoggerTest implements TestSig {
string getARelevantTag() { result = "logger" }
string getARelevantTag() { result = ["type-logger", "logger"] }

predicate hasActualResult(Location location, string element, string tag, string value) {
exists(LoggerCall log |
log.getLocation() = location and
element = log.toString() and
value = log.getAMessageComponent().toString() and
tag = "logger"
(
value = log.getAValueFormattedMessageComponent().toString() and
tag = "logger"
or
value = log.getAMessageComponent().toString() and
not value = log.getAValueFormattedMessageComponent().toString() and
tag = "type-logger"
)
)
}
}
Expand Down
14 changes: 14 additions & 0 deletions go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ func glogTest() {
glog.Warningf(fmt, text) // $ logger=fmt logger=text
glog.Warningln(text) // $ logger=text

// components corresponding to the format specifier "%T" are not considered vulnerable
glog.Errorf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
glog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
glog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
glog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
glog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v

klog.Error(text) // $ logger=text
klog.ErrorDepth(0, text) // $ logger=text
klog.Errorf(fmt, text) // $ logger=fmt logger=text
Expand All @@ -50,4 +57,11 @@ func glogTest() {
klog.WarningDepth(0, text) // $ logger=text
klog.Warningf(fmt, text) // $ logger=fmt logger=text
klog.Warningln(text) // $ logger=text

// components corresponding to the format specifier "%T" are not considered vulnerable
klog.Errorf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
klog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
klog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
klog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
klog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,8 @@ func logrusCalls() {
logrus.Panicln(text) // $ logger=text
logrus.Infof(fmt, text) // $ logger=fmt logger=text
logrus.FatalFn(fn) // $ logger=fn

// components corresponding to the format specifier "%T" are not considered vulnerable
logrus.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
logrus.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package main
const fmt = "formatted %s string"
const text = "test"

func main() {
var v []byte

func main() {
stdlib()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove linter warnings about lots of unused things 😆 .

}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ func stdlib() {
logger.Printf(fmt, text) // $ logger=fmt logger=text
logger.Println(text) // $ logger=text

// components corresponding to the format specifier "%T" are not considered vulnerable
logger.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
logger.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
logger.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v

log.SetPrefix("prefix: ")
log.Fatal(text) // $ logger=text
log.Fatalf(fmt, text) // $ logger=fmt logger=text
Expand All @@ -27,4 +32,9 @@ func stdlib() {
log.Print(text) // $ logger=text
log.Printf(fmt, text) // $ logger=fmt logger=text
log.Println(text) // $ logger=text

// components corresponding to the format specifier "%T" are not considered vulnerable
log.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
log.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
log.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text type-logger=v
}
Loading