Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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
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 correspond to the verb `%T` in a format specifier have been removed. There may also be more results in "Use of constant `state` value in OAuth 2.0 URL" (`go/constant-oauth2-state`).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be less results, for the same reason?

Copy link
Copy Markdown
Contributor Author

@owen-mc owen-mc Mar 20, 2025

Choose a reason for hiding this comment

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

In go/constant-oauth2-state, arguments to loggercalls are used to rule out some alerts. So less arguments to loggercalls means more alerts, potentially. (In practice I doubt there will be any alert changes for that query.)

Original file line number Diff line number Diff line change
@@ -1,17 +1,23 @@
import go
import semmle.go.dataflow.ExternalFlow
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
glog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
glog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
glog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
glog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this change because the tests in the first commit were written so that they pass without the fix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I guess I could have marked them as SPURIOUS in the first commit.

Copy link
Copy Markdown
Member

@mbg mbg Mar 18, 2025

Choose a reason for hiding this comment

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

I think it would be good to make sure that if we add tests before a fix is implemented (even in the same PR) to either mark them as SPURIOUS or just have them fail. Ideally the latter if there's a fix in the same PR (as is the case here) so that you don't add the tests in one commit and then change every line again in the next.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I marked them as SPURIOUS.


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
klog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
klog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
klog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
klog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
}
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
logrus.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
}
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
Copy Markdown
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
Copy Markdown
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
logger.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
logger.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text

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
log.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
log.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text
}
Loading
Loading