From f1733056294ff78df0a0b9f4733b3b33950bbc87 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 11:19:24 +0000 Subject: [PATCH 01/13] Add tests for %T (passing but marked SPURIOUS) --- .../semmle/go/concepts/LoggerCall/glog.go | 14 ++++++++++++++ .../semmle/go/concepts/LoggerCall/logrus.go | 4 ++++ .../semmle/go/concepts/LoggerCall/main.go | 4 +++- .../semmle/go/concepts/LoggerCall/stdlib.go | 10 ++++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go index 6913bfc95760..4856fb5ea6b2 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go @@ -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 SPURIOUS: logger=v + glog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + glog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + glog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + glog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + klog.Error(text) // $ logger=text klog.ErrorDepth(0, text) // $ logger=text klog.Errorf(fmt, text) // $ logger=fmt logger=text @@ -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 SPURIOUS: logger=v + klog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + klog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + klog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + klog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go index ce2d3ba4e253..c5f77de934b5 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go @@ -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 SPURIOUS: logger=v + logrus.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/main.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/main.go index bb2111afbec1..5353d9155ccb 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/main.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/main.go @@ -3,6 +3,8 @@ package main const fmt = "formatted %s string" const text = "test" -func main() { +var v []byte +func main() { + stdlib() } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go index f8401865b490..50a7e19223a2 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go @@ -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 SPURIOUS: logger=v + logger.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + logger.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + log.SetPrefix("prefix: ") log.Fatal(text) // $ logger=text log.Fatalf(fmt, text) // $ logger=fmt logger=text @@ -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 SPURIOUS: logger=v + log.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + log.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v } From 009e0e17b2e6b35d0e433cacefb55dec99abbfc6 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 11:27:51 +0000 Subject: [PATCH 02/13] Don't consider arguments with %T as logger call components --- go/ql/lib/semmle/go/Concepts.qll | 15 ++++++++++++-- .../semmle/go/concepts/LoggerCall/glog.go | 20 +++++++++---------- .../semmle/go/concepts/LoggerCall/logrus.go | 4 ++-- .../semmle/go/concepts/LoggerCall/stdlib.go | 12 +++++------ 4 files changed, 31 insertions(+), 20 deletions(-) diff --git a/go/ql/lib/semmle/go/Concepts.qll b/go/ql/lib/semmle/go/Concepts.qll index 3f0cd0f8885e..2239291565d2 100644 --- a/go/ql/lib/semmle/go/Concepts.qll +++ b/go/ql/lib/semmle/go/Concepts.qll @@ -355,8 +355,19 @@ module RegexpReplaceFunction { * extend `LoggerCall::Range` instead. */ 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 that is a part of the logged message. + * + * Exclude components corresponding to the format specifier "%T" as this + * prints the type of the argument, which is not considered vulnerable. + */ + DataFlow::Node getAMessageComponent() { + result = super.getAMessageComponent() and + not exists(string formatSpecifier | + formatSpecifier.regexpMatch("%[^%]*T") and + result = this.(StringOps::Formatting::StringFormatCall).getOperand(_, formatSpecifier) + ) + } } /** Provides a class for modeling new logging APIs. */ diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go index 4856fb5ea6b2..b45e1796d897 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go @@ -31,11 +31,11 @@ func glogTest() { 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 SPURIOUS: logger=v - glog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - glog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - glog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - glog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + 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 klog.Error(text) // $ logger=text klog.ErrorDepth(0, text) // $ logger=text @@ -59,9 +59,9 @@ func glogTest() { 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 SPURIOUS: logger=v - klog.Exitf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - klog.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - klog.Infof("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - klog.Warningf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + 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 } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go index c5f77de934b5..613ace5f1ee9 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go @@ -34,6 +34,6 @@ func logrusCalls() { 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 SPURIOUS: logger=v - logrus.Fatalf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + 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 } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go index 50a7e19223a2..d0f1ca181369 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go @@ -18,9 +18,9 @@ func stdlib() { 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 SPURIOUS: logger=v - logger.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - logger.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + 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 @@ -34,7 +34,7 @@ func stdlib() { 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 SPURIOUS: logger=v - log.Panicf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v - log.Printf("%s: found type %T", text, v) // $ logger="%s: found type %T" logger=text SPURIOUS: logger=v + 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 } From 59d82b3b62a48a8d423e4e3ab3d6312c9a6f6d3d Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 10:24:48 +0000 Subject: [PATCH 03/13] Make log injection tests more realistic --- .../Security/CWE-117/LogInjection.go | 155 +++++++++++------- 1 file changed, 98 insertions(+), 57 deletions(-) diff --git a/go/ql/test/query-tests/Security/CWE-117/LogInjection.go b/go/ql/test/query-tests/Security/CWE-117/LogInjection.go index 6fb628c4cc38..9efd516fb531 100644 --- a/go/ql/test/query-tests/Security/CWE-117/LogInjection.go +++ b/go/ql/test/query-tests/Security/CWE-117/LogInjection.go @@ -30,52 +30,54 @@ import ( func handler(req *http.Request, ctx *goproxy.ProxyCtx) { username := req.URL.Query()["username"][0] - slice := []any{"username", username} + password := req.URL.Query()["password"][0] + formatString := req.URL.Query()["formatString"][0] testFlag := req.URL.Query()["testFlag"][0] + slice := []any{"username", username} { - fmt.Print(username) // $ hasTaintFlow="username" - fmt.Printf(username) // $ hasTaintFlow="username" - fmt.Println(username) // $ hasTaintFlow="username" - fmt.Fprint(nil, username) // Fprint functions are only loggers if they target stdout/stderr - fmt.Fprintf(nil, username) - fmt.Fprintln(nil, username) + fmt.Print(username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + fmt.Printf(formatString, username, password) // $ hasTaintFlow="formatString" hasTaintFlow="username" hasTaintFlow="password" + fmt.Println(username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + fmt.Fprint(nil, username, password) // Fprint functions are only loggers if they target stdout/stderr + fmt.Fprintf(nil, formatString, username, password) + fmt.Fprintln(nil, username, password) } // log { - log.Print("user %s logged in.\n", username) // $ hasTaintFlow="username" - log.Printf("user %s logged in.\n", username) // $ hasTaintFlow="username" - log.Println("user %s logged in.\n", username) // $ hasTaintFlow="username" + log.Print("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + log.Printf(formatString, username, password) // $ hasTaintFlow="formatString" hasTaintFlow="username" hasTaintFlow="password" + log.Println("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" if testFlag == "true" { - log.Fatal("user %s logged in.\n", username) // $ hasTaintFlow="username" + log.Fatal("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" } if testFlag == "true" { - log.Fatalf("user %s logged in.\n", username) // $ hasTaintFlow="username" + log.Fatalf(formatString, username, password) // $ hasTaintFlow="formatString" hasTaintFlow="username" hasTaintFlow="password" } if testFlag == "true" { - log.Fatalln("user %s logged in.\n", username) // $ hasTaintFlow="username" + log.Fatalln("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" } if testFlag == "true" { - log.Panic("user %s logged in.\n", username) // $ hasTaintFlow="username" + log.Panic("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" } if testFlag == "true" { - log.Panicf("user %s logged in.\n", username) // $ hasTaintFlow="username" + log.Panicf(formatString, username, password) // $ hasTaintFlow="formatString" hasTaintFlow="username" hasTaintFlow="password" } if testFlag == "true" { - log.Panicln("user %s logged in.\n", username) // $ hasTaintFlow="username" + log.Panicln("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" } logger := log.Default() - logger.Print("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Printf("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Println("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Fatal("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Fatalf("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Fatalln("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Panic("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Panicf("user %s logged in.\n", username) // $ hasTaintFlow="username" - logger.Panicln("user %s logged in.\n", username) // $ hasTaintFlow="username" + logger.Print("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + logger.Printf(formatString, username, password) // $ hasTaintFlow="formatString" hasTaintFlow="username" hasTaintFlow="password" + logger.Println("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + logger.Fatal("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + logger.Fatalf(formatString, username, password) // $ hasTaintFlow="formatString" hasTaintFlow="username" hasTaintFlow="password" + logger.Fatalln("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + logger.Panic("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" + logger.Panicf(formatString, username, password) // $ hasTaintFlow="formatString" hasTaintFlow="username" hasTaintFlow="password" + logger.Panicln("user is logged in:", username, password) // $ hasTaintFlow="username" hasTaintFlow="password" } // k8s.io/klog { @@ -421,7 +423,6 @@ func handler(req *http.Request, ctx *goproxy.ProxyCtx) { simpleLogger.Tracew("%s", username) // $ hasTaintFlow="username" simpleLogger.Debugw("%s %s", slice...) // $ hasTaintFlow="slice" } - } type Logger interface { @@ -514,8 +515,12 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { verbose.Infof("user %q logged in.\n", username) klog.Infof("user %q logged in.\n", username) klog.Errorf("user %q logged in.\n", username) - klog.Fatalf("user %q logged in.\n", username) - klog.Exitf("user %q logged in.\n", username) + if testFlag == " true" { + klog.Fatalf("user %q logged in.\n", username) + } + if testFlag == " true" { + klog.Exitf("user %q logged in.\n", username) + } } // elazarl/goproxy { @@ -529,16 +534,24 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { glog.Infof("user %q logged in.\n", username) glog.Errorf("user %q logged in.\n", username) - glog.Fatalf("user %q logged in.\n", username) - glog.Exitf("user %q logged in.\n", username) + if testFlag == " true" { + glog.Fatalf("user %q logged in.\n", username) + } + if testFlag == " true" { + glog.Exitf("user %q logged in.\n", username) + } } // sirupsen/logrus { logrus.Debugf("user %q logged in.\n", username) logrus.Errorf("user %q logged in.\n", username) - logrus.Fatalf("user %q logged in.\n", username) + if testFlag == " true" { + logrus.Fatalf("user %q logged in.\n", username) + } logrus.Infof("user %q logged in.\n", username) - logrus.Panicf("user %q logged in.\n", username) + if testFlag == " true" { + logrus.Panicf("user %q logged in.\n", username) + } logrus.Printf("user %q logged in.\n", username) logrus.Tracef("user %q logged in.\n", username) logrus.Warnf("user %q logged in.\n", username) @@ -548,10 +561,14 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { entry := logrus.WithFields(fields) entry.Debugf("user %q logged in.\n", username) entry.Errorf("user %q logged in.\n", username) - entry.Fatalf("user %q logged in.\n", username) + if testFlag == " true" { + entry.Fatalf("user %q logged in.\n", username) + } entry.Infof("user %q logged in.\n", username) entry.Logf(0, "user %q logged in.\n", username) - entry.Panicf("user %q logged in.\n", username) + if testFlag == " true" { + entry.Panicf("user %q logged in.\n", username) + } entry.Printf("user %q logged in.\n", username) entry.Tracef("user %q logged in.\n", username) entry.Warnf("user %q logged in.\n", username) @@ -560,10 +577,14 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { logger := entry.Logger logger.Debugf("user %q logged in.\n", username) logger.Errorf("user %q logged in.\n", username) - logger.Fatalf("user %q logged in.\n", username) + if testFlag == " true" { + logger.Fatalf("user %q logged in.\n", username) + } logger.Infof("user %q logged in.\n", username) logger.Logf(0, "user %q logged in.\n", username) - logger.Panicf("user %q logged in.\n", username) + if testFlag == " true" { + logger.Panicf("user %q logged in.\n", username) + } logger.Printf("user %q logged in.\n", username) logger.Tracef("user %q logged in.\n", username) logger.Warnf("user %q logged in.\n", username) @@ -599,8 +620,12 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { verbose.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" klog.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" klog.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - klog.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - klog.Exitf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + klog.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } + if testFlag == " true" { + klog.Exitf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } } // elazarl/goproxy { @@ -614,16 +639,24 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { glog.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" glog.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - glog.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - glog.Exitf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + glog.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } + if testFlag == " true" { + glog.Exitf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } } // sirupsen/logrus { - logrus.Debugf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logrus.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logrus.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logrus.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logrus.Panicf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + logrus.Debugf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + logrus.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + logrus.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } + logrus.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + logrus.Panicf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } logrus.Printf("user %#q logged in.\n", username) // $ hasTaintFlow="username" logrus.Tracef("user %#q logged in.\n", username) // $ hasTaintFlow="username" logrus.Warnf("user %#q logged in.\n", username) // $ hasTaintFlow="username" @@ -631,24 +664,32 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { fields := make(logrus.Fields) entry := logrus.WithFields(fields) - entry.Debugf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - entry.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - entry.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - entry.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" - entry.Logf(0, "user %#q logged in.\n", username) // $ hasTaintFlow="username" - entry.Panicf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + entry.Debugf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + entry.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + entry.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } + entry.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" + entry.Logf(0, "user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + entry.Panicf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } entry.Printf("user %#q logged in.\n", username) // $ hasTaintFlow="username" entry.Tracef("user %#q logged in.\n", username) // $ hasTaintFlow="username" entry.Warnf("user %#q logged in.\n", username) // $ hasTaintFlow="username" entry.Warningf("user %#q logged in.\n", username) // $ hasTaintFlow="username" logger := entry.Logger - logger.Debugf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logger.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logger.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logger.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" - logger.Logf(0, "user %#q logged in.\n", username) // $ hasTaintFlow="username" - logger.Panicf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + logger.Debugf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + logger.Errorf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + logger.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } + logger.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username" + logger.Logf(0, "user %#q logged in.\n", username) // $ hasTaintFlow="username" + if testFlag == " true" { + logger.Panicf("user %#q logged in.\n", username) // $ hasTaintFlow="username" + } logger.Printf("user %#q logged in.\n", username) // $ hasTaintFlow="username" logger.Tracef("user %#q logged in.\n", username) // $ hasTaintFlow="username" logger.Warnf("user %#q logged in.\n", username) // $ hasTaintFlow="username" From 94c812cbe6805faeee7f2269193bb095cef43ccc Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 14:35:40 +0000 Subject: [PATCH 04/13] Convert cleartext logging tests to inline expectations --- .../CWE-312/CleartextLogging.expected | 28 ++++----- .../Security/CWE-312/CleartextLogging.qlref | 4 +- .../test/query-tests/Security/CWE-312/klog.go | 9 +-- .../test/query-tests/Security/CWE-312/main.go | 60 +++++++++---------- .../query-tests/Security/CWE-312/overrides.go | 4 +- .../query-tests/Security/CWE-312/passwords.go | 58 +++++++++--------- .../query-tests/Security/CWE-312/protobuf.go | 4 +- 7 files changed, 85 insertions(+), 82 deletions(-) diff --git a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected index 3435eff77754..9a259408cf24 100644 --- a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected +++ b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected @@ -1,6 +1,6 @@ #select -| klog.go:22:15:22:20 | header | klog.go:20:30:20:37 | selection of Header | klog.go:22:15:22:20 | header | $@ flows to a logging call. | klog.go:20:30:20:37 | selection of Header | Sensitive data returned by HTTP request headers | -| klog.go:28:13:28:41 | call to Get | klog.go:28:13:28:20 | selection of Header | klog.go:28:13:28:41 | call to Get | $@ flows to a logging call. | klog.go:28:13:28:20 | selection of Header | Sensitive data returned by HTTP request headers | +| klog.go:23:15:23:20 | header | klog.go:21:30:21:37 | selection of Header | klog.go:23:15:23:20 | header | $@ flows to a logging call. | klog.go:21:30:21:37 | selection of Header | Sensitive data returned by HTTP request headers | +| klog.go:29:13:29:41 | call to Get | klog.go:29:13:29:20 | selection of Header | klog.go:29:13:29:41 | call to Get | $@ flows to a logging call. | klog.go:29:13:29:20 | selection of Header | Sensitive data returned by HTTP request headers | | main.go:15:12:15:19 | password | main.go:15:12:15:19 | password | main.go:15:12:15:19 | password | $@ flows to a logging call. | main.go:15:12:15:19 | password | Sensitive data returned by an access to password | | main.go:16:17:16:24 | password | main.go:16:17:16:24 | password | main.go:16:17:16:24 | password | $@ flows to a logging call. | main.go:16:17:16:24 | password | Sensitive data returned by an access to password | | main.go:17:13:17:20 | password | main.go:17:13:17:20 | password | main.go:17:13:17:20 | password | $@ flows to a logging call. | main.go:17:13:17:20 | password | Sensitive data returned by an access to password | @@ -55,11 +55,11 @@ | passwords.go:127:14:127:21 | selection of y | passwords.go:122:13:122:25 | call to getPassword | passwords.go:127:14:127:21 | selection of y | $@ flows to a logging call. | passwords.go:122:13:122:25 | call to getPassword | Sensitive data returned by a call to getPassword | | protobuf.go:14:14:14:35 | call to GetDescription | protobuf.go:12:22:12:29 | password | protobuf.go:14:14:14:35 | call to GetDescription | $@ flows to a logging call. | protobuf.go:12:22:12:29 | password | Sensitive data returned by an access to password | edges -| klog.go:20:3:25:3 | range statement[1] | klog.go:21:27:21:33 | headers | provenance | | -| klog.go:20:30:20:37 | selection of Header | klog.go:20:3:25:3 | range statement[1] | provenance | Src:MaD:1 Config | -| klog.go:21:4:24:4 | range statement[1] | klog.go:22:15:22:20 | header | provenance | | -| klog.go:21:27:21:33 | headers | klog.go:21:4:24:4 | range statement[1] | provenance | Config | -| klog.go:28:13:28:20 | selection of Header | klog.go:28:13:28:41 | call to Get | provenance | Src:MaD:1 Config | +| klog.go:21:3:26:3 | range statement[1] | klog.go:22:27:22:33 | headers | provenance | | +| klog.go:21:30:21:37 | selection of Header | klog.go:21:3:26:3 | range statement[1] | provenance | Src:MaD:1 Config | +| klog.go:22:4:25:4 | range statement[1] | klog.go:23:15:23:20 | header | provenance | | +| klog.go:22:27:22:33 | headers | klog.go:22:4:25:4 | range statement[1] | provenance | Config | +| klog.go:29:13:29:20 | selection of Header | klog.go:29:13:29:41 | call to Get | provenance | Src:MaD:1 Config | | overrides.go:9:9:9:16 | password | overrides.go:13:14:13:23 | call to String | provenance | | | passwords.go:8:12:8:12 | definition of x | passwords.go:9:14:9:14 | x | provenance | | | passwords.go:30:8:30:15 | password | passwords.go:8:12:8:12 | definition of x | provenance | | @@ -101,13 +101,13 @@ edges models | 1 | Source: net/http; Request; true; Header; ; ; ; remote; manual | nodes -| klog.go:20:3:25:3 | range statement[1] | semmle.label | range statement[1] | -| klog.go:20:30:20:37 | selection of Header | semmle.label | selection of Header | -| klog.go:21:4:24:4 | range statement[1] | semmle.label | range statement[1] | -| klog.go:21:27:21:33 | headers | semmle.label | headers | -| klog.go:22:15:22:20 | header | semmle.label | header | -| klog.go:28:13:28:20 | selection of Header | semmle.label | selection of Header | -| klog.go:28:13:28:41 | call to Get | semmle.label | call to Get | +| klog.go:21:3:26:3 | range statement[1] | semmle.label | range statement[1] | +| klog.go:21:30:21:37 | selection of Header | semmle.label | selection of Header | +| klog.go:22:4:25:4 | range statement[1] | semmle.label | range statement[1] | +| klog.go:22:27:22:33 | headers | semmle.label | headers | +| klog.go:23:15:23:20 | header | semmle.label | header | +| klog.go:29:13:29:20 | selection of Header | semmle.label | selection of Header | +| klog.go:29:13:29:41 | call to Get | semmle.label | call to Get | | main.go:15:12:15:19 | password | semmle.label | password | | main.go:16:17:16:24 | password | semmle.label | password | | main.go:17:13:17:20 | password | semmle.label | password | diff --git a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.qlref b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.qlref index b540e0ddc002..693299c33a21 100644 --- a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.qlref +++ b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.qlref @@ -1,2 +1,4 @@ query: Security/CWE-312/CleartextLogging.ql -postprocess: utils/test/PrettyPrintModels.ql +postprocess: + - utils/test/PrettyPrintModels.ql + - utils/test/InlineExpectationsTestQuery.ql diff --git a/go/ql/test/query-tests/Security/CWE-312/klog.go b/go/ql/test/query-tests/Security/CWE-312/klog.go index 70265c7d4719..126dc65b6966 100644 --- a/go/ql/test/query-tests/Security/CWE-312/klog.go +++ b/go/ql/test/query-tests/Security/CWE-312/klog.go @@ -3,9 +3,10 @@ package main //go:generate depstubber -vendor k8s.io/klog "" Info import ( - "k8s.io/klog" "net/http" "strings" + + "k8s.io/klog" ) func mask(key, value string) string { @@ -17,15 +18,15 @@ func mask(key, value string) string { func klogTest() { http.HandleFunc("/klog", func(w http.ResponseWriter, r *http.Request) { - for name, headers := range r.Header { + for name, headers := range r.Header { // $ Source for _, header := range headers { - klog.Info(header) // NOT OK + klog.Info(header) // $ Alert klog.Info(mask(name, header)) // OK } } klog.Info(r.Header.Get("Accept")) // OK klog.Info(r.Header["Content-Type"]) // OK - klog.Info(r.Header.Get("Authorization")) // NOT OK + klog.Info(r.Header.Get("Authorization")) // $ Alert }) http.ListenAndServe(":80", nil) } diff --git a/go/ql/test/query-tests/Security/CWE-312/main.go b/go/ql/test/query-tests/Security/CWE-312/main.go index d91166455571..5f746311c996 100644 --- a/go/ql/test/query-tests/Security/CWE-312/main.go +++ b/go/ql/test/query-tests/Security/CWE-312/main.go @@ -12,43 +12,43 @@ import ( func main() { password := "P4ssw0rd" - log.Print(password) - log.Printf("", password) - log.Printf(password, "") - log.Println(password) - log.Fatal(password) - log.Fatalf("", password) - log.Fatalf(password, "") - log.Fatalln(password) - log.Panic(password) - log.Panicf("", password) - log.Panicf(password, "") - log.Panicln(password) - log.Output(0, password) + log.Print(password) // $ Alert + log.Printf("", password) // $ Alert + log.Printf(password, "") // $ Alert + log.Println(password) // $ Alert + log.Fatal(password) // $ Alert + log.Fatalf("", password) // $ Alert + log.Fatalf(password, "") // $ Alert + log.Fatalln(password) // $ Alert + log.Panic(password) // $ Alert + log.Panicf("", password) // $ Alert + log.Panicf(password, "") // $ Alert + log.Panicln(password) // $ Alert + log.Output(0, password) // $ Alert l := log.Default() - l.Print(password) - l.Printf("", password) - l.Printf(password, "") - l.Println(password) - l.Fatal(password) - l.Fatalf("", password) - l.Fatalf(password, "") - l.Fatalln(password) - l.Panic(password) - l.Panicf("", password) - l.Panicf(password, "") - l.Panicln(password) - l.Output(0, password) - - glog.Info(password) - logrus.Warning(password) + l.Print(password) // $ Alert + l.Printf("", password) // $ Alert + l.Printf(password, "") // $ Alert + l.Println(password) // $ Alert + l.Fatal(password) // $ Alert + l.Fatalf("", password) // $ Alert + l.Fatalf(password, "") // $ Alert + l.Fatalln(password) // $ Alert + l.Panic(password) // $ Alert + l.Panicf("", password) // $ Alert + l.Panicf(password, "") // $ Alert + l.Panicln(password) // $ Alert + l.Output(0, password) // $ Alert + + glog.Info(password) // $ Alert + logrus.Warning(password) // $ Alert fields := make(logrus.Fields) fields["pass"] = password entry := logrus.WithFields(fields) entry.Errorf("") - entry = logrus.WithField("pass", password) + entry = logrus.WithField("pass", password) // $ Alert entry.Panic("") } diff --git a/go/ql/test/query-tests/Security/CWE-312/overrides.go b/go/ql/test/query-tests/Security/CWE-312/overrides.go index cd94b1b84b54..98fbdad9e77d 100644 --- a/go/ql/test/query-tests/Security/CWE-312/overrides.go +++ b/go/ql/test/query-tests/Security/CWE-312/overrides.go @@ -6,10 +6,10 @@ type s struct{} func (_ s) String() string { password := "horsebatterystaplecorrect" - return password + return password // $ Source } func overrideTest(x s, y fmt.Stringer) { - fmt.Println(x.String()) // NOT OK + fmt.Println(x.String()) // $ Alert fmt.Println(y.String()) // OK } diff --git a/go/ql/test/query-tests/Security/CWE-312/passwords.go b/go/ql/test/query-tests/Security/CWE-312/passwords.go index 5f0b291016db..f99178f0fae0 100644 --- a/go/ql/test/query-tests/Security/CWE-312/passwords.go +++ b/go/ql/test/query-tests/Security/CWE-312/passwords.go @@ -6,7 +6,7 @@ import ( ) func myLog(x string) { - log.Println(x) // NOT OK + log.Println(x) // $ Alert } func redact(kind, value string) string { @@ -22,33 +22,33 @@ func test() { x := "horsebatterystapleincorrect" var o passStruct - log.Println(password) // NOT OK - log.Println(o.password) // NOT OK - log.Println(getPassword()) // NOT OK - log.Println(o.getPassword()) // NOT OK + log.Println(password) // $ Alert + log.Println(o.password) // $ Alert + log.Println(getPassword()) // $ Alert + log.Println(o.getPassword()) // $ Alert - myLog(password) + myLog(password) // $ Source - log.Panic(password) // NOT OK + log.Panic(password) // $ Alert - log.Println(name + ", " + password) // NOT OK + log.Println(name + ", " + password) // $ Alert obj1 := passStruct{ - password: x, + password: x, // $ Source } - log.Println(obj1) // NOT OK + log.Println(obj1) // $ Alert obj2 := xStruct{ - x: password, + x: password, // $ Source } - log.Println(obj2) // NOT OK + log.Println(obj2) // $ Alert var obj3 xStruct - log.Println(obj3) // caught because of the below line - obj3.x = password // NOT OK + log.Println(obj3) // $ SPURIOUS: Alert // caught because of the below line and def-use flow + obj3.x = password // $ Source fixed_password := "cowbatterystaplecorrect" - log.Println(fixed_password) // Probably OK, but caught + log.Println(fixed_password) // $ Alert // Probably OK log.Println(IncorrectPasswordError) // OK @@ -83,12 +83,12 @@ func test() { log.Println(password_sha) // OK utilityObject := passSetStruct{ - passwordSet: make(map[string]bool), + passwordSet: make(map[string]bool), // $ Source } - log.Println(utilityObject) // NOT OK + log.Println(utilityObject) // $ Alert - secret := password - log.Printf("pw: %s", secret) // NOT OK + secret := password // $ Source + log.Printf("pw: %s", secret) // $ Alert log.Println("Password is: " + redact("password", password)) @@ -98,33 +98,33 @@ func test() { if t.test(y) { f() // ... - log.Println("Password is: " + password) // NOT OK + log.Println("Password is: " + password) // $ Alert // ... } if t.test(y) { if f() { - log.Println("Password is: " + password) // NOT OK + log.Println("Password is: " + password) // $ Alert } } if os.Getenv("APP_ENV") != "production" { - log.Println("Password is: " + password) // OK, but still flagged + log.Println("Password is: " + password) // $ SPURIOUS: Alert } var password1 stringable = stringable{"arstneio"} - log.Println(name + ", " + password1.String()) // NOT OK + log.Println(name + ", " + password1.String()) // $ Alert config := Config{ - password: x, + password: x, // $ Source hostname: "tarski", - x: password, - y: getPassword(), + x: password, // $ Source + y: getPassword(), // $ Source } log.Println(config.hostname) // OK - log.Println(config) // NOT OK - log.Println(config.x) // NOT OK - log.Println(config.y) // NOT OK + log.Println(config) // $ Alert + log.Println(config.x) // $ Alert + log.Println(config.y) // $ Alert obj4 := xStruct{ x: "aaaaa", diff --git a/go/ql/test/query-tests/Security/CWE-312/protobuf.go b/go/ql/test/query-tests/Security/CWE-312/protobuf.go index ce8e2218842b..a995f0d7cb8b 100644 --- a/go/ql/test/query-tests/Security/CWE-312/protobuf.go +++ b/go/ql/test/query-tests/Security/CWE-312/protobuf.go @@ -9,8 +9,8 @@ func testProtobuf() { password := "P@ssw0rd" query := &query.Query{} - query.Description = password + query.Description = password // $ Source - log.Println(query.GetDescription()) // NOT OK + log.Println(query.GetDescription()) // $ Alert log.Println(query.GetId()) // OK } From 646d28feeba954591fa080c3abb007ad99b8273a Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 14:41:23 +0000 Subject: [PATCH 05/13] Make cleartext logging tests more realistic --- .../CWE-312/CleartextLogging.expected | 116 +++++++++--------- .../test/query-tests/Security/CWE-312/main.go | 55 +++++---- 2 files changed, 86 insertions(+), 85 deletions(-) diff --git a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected index 9a259408cf24..8cba6b476a87 100644 --- a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected +++ b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected @@ -1,35 +1,35 @@ #select | klog.go:23:15:23:20 | header | klog.go:21:30:21:37 | selection of Header | klog.go:23:15:23:20 | header | $@ flows to a logging call. | klog.go:21:30:21:37 | selection of Header | Sensitive data returned by HTTP request headers | | klog.go:29:13:29:41 | call to Get | klog.go:29:13:29:20 | selection of Header | klog.go:29:13:29:41 | call to Get | $@ flows to a logging call. | klog.go:29:13:29:20 | selection of Header | Sensitive data returned by HTTP request headers | -| main.go:15:12:15:19 | password | main.go:15:12:15:19 | password | main.go:15:12:15:19 | password | $@ flows to a logging call. | main.go:15:12:15:19 | password | Sensitive data returned by an access to password | -| main.go:16:17:16:24 | password | main.go:16:17:16:24 | password | main.go:16:17:16:24 | password | $@ flows to a logging call. | main.go:16:17:16:24 | password | Sensitive data returned by an access to password | -| main.go:17:13:17:20 | password | main.go:17:13:17:20 | password | main.go:17:13:17:20 | password | $@ flows to a logging call. | main.go:17:13:17:20 | password | Sensitive data returned by an access to password | -| main.go:18:14:18:21 | password | main.go:18:14:18:21 | password | main.go:18:14:18:21 | password | $@ flows to a logging call. | main.go:18:14:18:21 | password | Sensitive data returned by an access to password | -| main.go:19:12:19:19 | password | main.go:19:12:19:19 | password | main.go:19:12:19:19 | password | $@ flows to a logging call. | main.go:19:12:19:19 | password | Sensitive data returned by an access to password | -| main.go:20:17:20:24 | password | main.go:20:17:20:24 | password | main.go:20:17:20:24 | password | $@ flows to a logging call. | main.go:20:17:20:24 | password | Sensitive data returned by an access to password | -| main.go:21:13:21:20 | password | main.go:21:13:21:20 | password | main.go:21:13:21:20 | password | $@ flows to a logging call. | main.go:21:13:21:20 | password | Sensitive data returned by an access to password | -| main.go:22:14:22:21 | password | main.go:22:14:22:21 | password | main.go:22:14:22:21 | password | $@ flows to a logging call. | main.go:22:14:22:21 | password | Sensitive data returned by an access to password | -| main.go:23:12:23:19 | password | main.go:23:12:23:19 | password | main.go:23:12:23:19 | password | $@ flows to a logging call. | main.go:23:12:23:19 | password | Sensitive data returned by an access to password | -| main.go:24:17:24:24 | password | main.go:24:17:24:24 | password | main.go:24:17:24:24 | password | $@ flows to a logging call. | main.go:24:17:24:24 | password | Sensitive data returned by an access to password | -| main.go:25:13:25:20 | password | main.go:25:13:25:20 | password | main.go:25:13:25:20 | password | $@ flows to a logging call. | main.go:25:13:25:20 | password | Sensitive data returned by an access to password | -| main.go:26:14:26:21 | password | main.go:26:14:26:21 | password | main.go:26:14:26:21 | password | $@ flows to a logging call. | main.go:26:14:26:21 | password | Sensitive data returned by an access to password | -| main.go:27:16:27:23 | password | main.go:27:16:27:23 | password | main.go:27:16:27:23 | password | $@ flows to a logging call. | main.go:27:16:27:23 | password | Sensitive data returned by an access to password | -| main.go:30:10:30:17 | password | main.go:30:10:30:17 | password | main.go:30:10:30:17 | password | $@ flows to a logging call. | main.go:30:10:30:17 | password | Sensitive data returned by an access to password | -| main.go:31:15:31:22 | password | main.go:31:15:31:22 | password | main.go:31:15:31:22 | password | $@ flows to a logging call. | main.go:31:15:31:22 | password | Sensitive data returned by an access to password | -| main.go:32:11:32:18 | password | main.go:32:11:32:18 | password | main.go:32:11:32:18 | password | $@ flows to a logging call. | main.go:32:11:32:18 | password | Sensitive data returned by an access to password | -| main.go:33:12:33:19 | password | main.go:33:12:33:19 | password | main.go:33:12:33:19 | password | $@ flows to a logging call. | main.go:33:12:33:19 | password | Sensitive data returned by an access to password | -| main.go:34:10:34:17 | password | main.go:34:10:34:17 | password | main.go:34:10:34:17 | password | $@ flows to a logging call. | main.go:34:10:34:17 | password | Sensitive data returned by an access to password | -| main.go:35:15:35:22 | password | main.go:35:15:35:22 | password | main.go:35:15:35:22 | password | $@ flows to a logging call. | main.go:35:15:35:22 | password | Sensitive data returned by an access to password | -| main.go:36:11:36:18 | password | main.go:36:11:36:18 | password | main.go:36:11:36:18 | password | $@ flows to a logging call. | main.go:36:11:36:18 | password | Sensitive data returned by an access to password | -| main.go:37:12:37:19 | password | main.go:37:12:37:19 | password | main.go:37:12:37:19 | password | $@ flows to a logging call. | main.go:37:12:37:19 | password | Sensitive data returned by an access to password | -| main.go:38:10:38:17 | password | main.go:38:10:38:17 | password | main.go:38:10:38:17 | password | $@ flows to a logging call. | main.go:38:10:38:17 | password | Sensitive data returned by an access to password | -| main.go:39:15:39:22 | password | main.go:39:15:39:22 | password | main.go:39:15:39:22 | password | $@ flows to a logging call. | main.go:39:15:39:22 | password | Sensitive data returned by an access to password | -| main.go:40:11:40:18 | password | main.go:40:11:40:18 | password | main.go:40:11:40:18 | password | $@ flows to a logging call. | main.go:40:11:40:18 | password | Sensitive data returned by an access to password | -| main.go:41:12:41:19 | password | main.go:41:12:41:19 | password | main.go:41:12:41:19 | password | $@ flows to a logging call. | main.go:41:12:41:19 | password | Sensitive data returned by an access to password | -| main.go:42:14:42:21 | password | main.go:42:14:42:21 | password | main.go:42:14:42:21 | password | $@ flows to a logging call. | main.go:42:14:42:21 | password | Sensitive data returned by an access to password | -| main.go:44:12:44:19 | password | main.go:44:12:44:19 | password | main.go:44:12:44:19 | password | $@ flows to a logging call. | main.go:44:12:44:19 | password | Sensitive data returned by an access to password | -| main.go:45:17:45:24 | password | main.go:45:17:45:24 | password | main.go:45:17:45:24 | password | $@ flows to a logging call. | main.go:45:17:45:24 | password | Sensitive data returned by an access to password | -| main.go:52:35:52:42 | password | main.go:52:35:52:42 | password | main.go:52:35:52:42 | password | $@ flows to a logging call. | main.go:52:35:52:42 | password | Sensitive data returned by an access to password | +| main.go:16:12:16:19 | password | main.go:16:12:16:19 | password | main.go:16:12:16:19 | password | $@ flows to a logging call. | main.go:16:12:16:19 | password | Sensitive data returned by an access to password | +| main.go:17:19:17:26 | password | main.go:17:19:17:26 | password | main.go:17:19:17:26 | password | $@ flows to a logging call. | main.go:17:19:17:26 | password | Sensitive data returned by an access to password | +| main.go:18:13:18:20 | password | main.go:18:13:18:20 | password | main.go:18:13:18:20 | password | $@ flows to a logging call. | main.go:18:13:18:20 | password | Sensitive data returned by an access to password | +| main.go:19:14:19:21 | password | main.go:19:14:19:21 | password | main.go:19:14:19:21 | password | $@ flows to a logging call. | main.go:19:14:19:21 | password | Sensitive data returned by an access to password | +| main.go:20:12:20:19 | password | main.go:20:12:20:19 | password | main.go:20:12:20:19 | password | $@ flows to a logging call. | main.go:20:12:20:19 | password | Sensitive data returned by an access to password | +| main.go:21:19:21:26 | password | main.go:21:19:21:26 | password | main.go:21:19:21:26 | password | $@ flows to a logging call. | main.go:21:19:21:26 | password | Sensitive data returned by an access to password | +| main.go:22:13:22:20 | password | main.go:22:13:22:20 | password | main.go:22:13:22:20 | password | $@ flows to a logging call. | main.go:22:13:22:20 | password | Sensitive data returned by an access to password | +| main.go:23:14:23:21 | password | main.go:23:14:23:21 | password | main.go:23:14:23:21 | password | $@ flows to a logging call. | main.go:23:14:23:21 | password | Sensitive data returned by an access to password | +| main.go:24:12:24:19 | password | main.go:24:12:24:19 | password | main.go:24:12:24:19 | password | $@ flows to a logging call. | main.go:24:12:24:19 | password | Sensitive data returned by an access to password | +| main.go:25:19:25:26 | password | main.go:25:19:25:26 | password | main.go:25:19:25:26 | password | $@ flows to a logging call. | main.go:25:19:25:26 | password | Sensitive data returned by an access to password | +| main.go:26:13:26:20 | password | main.go:26:13:26:20 | password | main.go:26:13:26:20 | password | $@ flows to a logging call. | main.go:26:13:26:20 | password | Sensitive data returned by an access to password | +| main.go:27:14:27:21 | password | main.go:27:14:27:21 | password | main.go:27:14:27:21 | password | $@ flows to a logging call. | main.go:27:14:27:21 | password | Sensitive data returned by an access to password | +| main.go:28:16:28:23 | password | main.go:28:16:28:23 | password | main.go:28:16:28:23 | password | $@ flows to a logging call. | main.go:28:16:28:23 | password | Sensitive data returned by an access to password | +| main.go:31:10:31:17 | password | main.go:31:10:31:17 | password | main.go:31:10:31:17 | password | $@ flows to a logging call. | main.go:31:10:31:17 | password | Sensitive data returned by an access to password | +| main.go:32:17:32:24 | password | main.go:32:17:32:24 | password | main.go:32:17:32:24 | password | $@ flows to a logging call. | main.go:32:17:32:24 | password | Sensitive data returned by an access to password | +| main.go:33:11:33:18 | password | main.go:33:11:33:18 | password | main.go:33:11:33:18 | password | $@ flows to a logging call. | main.go:33:11:33:18 | password | Sensitive data returned by an access to password | +| main.go:34:12:34:19 | password | main.go:34:12:34:19 | password | main.go:34:12:34:19 | password | $@ flows to a logging call. | main.go:34:12:34:19 | password | Sensitive data returned by an access to password | +| main.go:35:10:35:17 | password | main.go:35:10:35:17 | password | main.go:35:10:35:17 | password | $@ flows to a logging call. | main.go:35:10:35:17 | password | Sensitive data returned by an access to password | +| main.go:36:17:36:24 | password | main.go:36:17:36:24 | password | main.go:36:17:36:24 | password | $@ flows to a logging call. | main.go:36:17:36:24 | password | Sensitive data returned by an access to password | +| main.go:37:11:37:18 | password | main.go:37:11:37:18 | password | main.go:37:11:37:18 | password | $@ flows to a logging call. | main.go:37:11:37:18 | password | Sensitive data returned by an access to password | +| main.go:38:12:38:19 | password | main.go:38:12:38:19 | password | main.go:38:12:38:19 | password | $@ flows to a logging call. | main.go:38:12:38:19 | password | Sensitive data returned by an access to password | +| main.go:39:10:39:17 | password | main.go:39:10:39:17 | password | main.go:39:10:39:17 | password | $@ flows to a logging call. | main.go:39:10:39:17 | password | Sensitive data returned by an access to password | +| main.go:40:17:40:24 | password | main.go:40:17:40:24 | password | main.go:40:17:40:24 | password | $@ flows to a logging call. | main.go:40:17:40:24 | password | Sensitive data returned by an access to password | +| main.go:41:11:41:18 | password | main.go:41:11:41:18 | password | main.go:41:11:41:18 | password | $@ flows to a logging call. | main.go:41:11:41:18 | password | Sensitive data returned by an access to password | +| main.go:42:12:42:19 | password | main.go:42:12:42:19 | password | main.go:42:12:42:19 | password | $@ flows to a logging call. | main.go:42:12:42:19 | password | Sensitive data returned by an access to password | +| main.go:43:14:43:21 | password | main.go:43:14:43:21 | password | main.go:43:14:43:21 | password | $@ flows to a logging call. | main.go:43:14:43:21 | password | Sensitive data returned by an access to password | +| main.go:45:12:45:19 | password | main.go:45:12:45:19 | password | main.go:45:12:45:19 | password | $@ flows to a logging call. | main.go:45:12:45:19 | password | Sensitive data returned by an access to password | +| main.go:46:17:46:24 | password | main.go:46:17:46:24 | password | main.go:46:17:46:24 | password | $@ flows to a logging call. | main.go:46:17:46:24 | password | Sensitive data returned by an access to password | +| main.go:53:35:53:42 | password | main.go:53:35:53:42 | password | main.go:53:35:53:42 | password | $@ flows to a logging call. | main.go:53:35:53:42 | password | Sensitive data returned by an access to password | | overrides.go:13:14:13:23 | call to String | overrides.go:9:9:9:16 | password | overrides.go:13:14:13:23 | call to String | $@ flows to a logging call. | overrides.go:9:9:9:16 | password | Sensitive data returned by an access to password | | passwords.go:9:14:9:14 | x | passwords.go:30:8:30:15 | password | passwords.go:9:14:9:14 | x | $@ flows to a logging call. | passwords.go:30:8:30:15 | password | Sensitive data returned by an access to password | | passwords.go:25:14:25:21 | password | passwords.go:25:14:25:21 | password | passwords.go:25:14:25:21 | password | $@ flows to a logging call. | passwords.go:25:14:25:21 | password | Sensitive data returned by an access to password | @@ -108,35 +108,35 @@ nodes | klog.go:23:15:23:20 | header | semmle.label | header | | klog.go:29:13:29:20 | selection of Header | semmle.label | selection of Header | | klog.go:29:13:29:41 | call to Get | semmle.label | call to Get | -| main.go:15:12:15:19 | password | semmle.label | password | -| main.go:16:17:16:24 | password | semmle.label | password | -| main.go:17:13:17:20 | password | semmle.label | password | -| main.go:18:14:18:21 | password | semmle.label | password | -| main.go:19:12:19:19 | password | semmle.label | password | -| main.go:20:17:20:24 | password | semmle.label | password | -| main.go:21:13:21:20 | password | semmle.label | password | -| main.go:22:14:22:21 | password | semmle.label | password | -| main.go:23:12:23:19 | password | semmle.label | password | -| main.go:24:17:24:24 | password | semmle.label | password | -| main.go:25:13:25:20 | password | semmle.label | password | -| main.go:26:14:26:21 | password | semmle.label | password | -| main.go:27:16:27:23 | password | semmle.label | password | -| main.go:30:10:30:17 | password | semmle.label | password | -| main.go:31:15:31:22 | password | semmle.label | password | -| main.go:32:11:32:18 | password | semmle.label | password | -| main.go:33:12:33:19 | password | semmle.label | password | -| main.go:34:10:34:17 | password | semmle.label | password | -| main.go:35:15:35:22 | password | semmle.label | password | -| main.go:36:11:36:18 | password | semmle.label | password | -| main.go:37:12:37:19 | password | semmle.label | password | -| main.go:38:10:38:17 | password | semmle.label | password | -| main.go:39:15:39:22 | password | semmle.label | password | -| main.go:40:11:40:18 | password | semmle.label | password | -| main.go:41:12:41:19 | password | semmle.label | password | -| main.go:42:14:42:21 | password | semmle.label | password | -| main.go:44:12:44:19 | password | semmle.label | password | -| main.go:45:17:45:24 | password | semmle.label | password | -| main.go:52:35:52:42 | password | semmle.label | password | +| main.go:16:12:16:19 | password | semmle.label | password | +| main.go:17:19:17:26 | password | semmle.label | password | +| main.go:18:13:18:20 | password | semmle.label | password | +| main.go:19:14:19:21 | password | semmle.label | password | +| main.go:20:12:20:19 | password | semmle.label | password | +| main.go:21:19:21:26 | password | semmle.label | password | +| main.go:22:13:22:20 | password | semmle.label | password | +| main.go:23:14:23:21 | password | semmle.label | password | +| main.go:24:12:24:19 | password | semmle.label | password | +| main.go:25:19:25:26 | password | semmle.label | password | +| main.go:26:13:26:20 | password | semmle.label | password | +| main.go:27:14:27:21 | password | semmle.label | password | +| main.go:28:16:28:23 | password | semmle.label | password | +| main.go:31:10:31:17 | password | semmle.label | password | +| main.go:32:17:32:24 | password | semmle.label | password | +| main.go:33:11:33:18 | password | semmle.label | password | +| main.go:34:12:34:19 | password | semmle.label | password | +| main.go:35:10:35:17 | password | semmle.label | password | +| main.go:36:17:36:24 | password | semmle.label | password | +| main.go:37:11:37:18 | password | semmle.label | password | +| main.go:38:12:38:19 | password | semmle.label | password | +| main.go:39:10:39:17 | password | semmle.label | password | +| main.go:40:17:40:24 | password | semmle.label | password | +| main.go:41:11:41:18 | password | semmle.label | password | +| main.go:42:12:42:19 | password | semmle.label | password | +| main.go:43:14:43:21 | password | semmle.label | password | +| main.go:45:12:45:19 | password | semmle.label | password | +| main.go:46:17:46:24 | password | semmle.label | password | +| main.go:53:35:53:42 | password | semmle.label | password | | overrides.go:9:9:9:16 | password | semmle.label | password | | overrides.go:13:14:13:23 | call to String | semmle.label | call to String | | passwords.go:8:12:8:12 | definition of x | semmle.label | definition of x | diff --git a/go/ql/test/query-tests/Security/CWE-312/main.go b/go/ql/test/query-tests/Security/CWE-312/main.go index 5f746311c996..2d4121d3606c 100644 --- a/go/ql/test/query-tests/Security/CWE-312/main.go +++ b/go/ql/test/query-tests/Security/CWE-312/main.go @@ -4,42 +4,43 @@ package main //go:generate depstubber -vendor github.com/golang/glog "" Info import ( + "log" + "github.com/golang/glog" "github.com/sirupsen/logrus" - "log" ) func main() { password := "P4ssw0rd" - log.Print(password) // $ Alert - log.Printf("", password) // $ Alert - log.Printf(password, "") // $ Alert - log.Println(password) // $ Alert - log.Fatal(password) // $ Alert - log.Fatalf("", password) // $ Alert - log.Fatalf(password, "") // $ Alert - log.Fatalln(password) // $ Alert - log.Panic(password) // $ Alert - log.Panicf("", password) // $ Alert - log.Panicf(password, "") // $ Alert - log.Panicln(password) // $ Alert - log.Output(0, password) // $ Alert + log.Print(password) // $ Alert + log.Printf("%s", password) // $ Alert + log.Printf(password, "") // $ Alert + log.Println(password) // $ Alert + log.Fatal(password) // $ Alert + log.Fatalf("%s", password) // $ Alert + log.Fatalf(password, "") // $ Alert + log.Fatalln(password) // $ Alert + log.Panic(password) // $ Alert + log.Panicf("%s", password) // $ Alert + log.Panicf(password, "") // $ Alert + log.Panicln(password) // $ Alert + log.Output(0, password) // $ Alert l := log.Default() - l.Print(password) // $ Alert - l.Printf("", password) // $ Alert - l.Printf(password, "") // $ Alert - l.Println(password) // $ Alert - l.Fatal(password) // $ Alert - l.Fatalf("", password) // $ Alert - l.Fatalf(password, "") // $ Alert - l.Fatalln(password) // $ Alert - l.Panic(password) // $ Alert - l.Panicf("", password) // $ Alert - l.Panicf(password, "") // $ Alert - l.Panicln(password) // $ Alert - l.Output(0, password) // $ Alert + l.Print(password) // $ Alert + l.Printf("%s", password) // $ Alert + l.Printf(password, "") // $ Alert + l.Println(password) // $ Alert + l.Fatal(password) // $ Alert + l.Fatalf("%s", password) // $ Alert + l.Fatalf(password, "") // $ Alert + l.Fatalln(password) // $ Alert + l.Panic(password) // $ Alert + l.Panicf("%s", password) // $ Alert + l.Panicf(password, "") // $ Alert + l.Panicln(password) // $ Alert + l.Output(0, password) // $ Alert glog.Info(password) // $ Alert logrus.Warning(password) // $ Alert From 11ff0a08f3abfd7e10b88bbb93bffca9bfee8b5b Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 14:41:47 +0000 Subject: [PATCH 06/13] Add log injection and cleartext logging tests for %T --- .../Security/CWE-117/LogInjection.go | 6 ++ .../CWE-312/CleartextLogging.expected | 64 +++++++++---------- .../test/query-tests/Security/CWE-312/main.go | 2 + 3 files changed, 40 insertions(+), 32 deletions(-) diff --git a/go/ql/test/query-tests/Security/CWE-117/LogInjection.go b/go/ql/test/query-tests/Security/CWE-117/LogInjection.go index 9efd516fb531..fc9d71791582 100644 --- a/go/ql/test/query-tests/Security/CWE-117/LogInjection.go +++ b/go/ql/test/query-tests/Security/CWE-117/LogInjection.go @@ -718,3 +718,9 @@ func handlerGood4(req *http.Request, ctx *goproxy.ProxyCtx) { sLogger.Warnf("user %#q logged in.\n", username) // $ hasTaintFlow="username" } } + +// GOOD: User-provided values formatted using a %T directive, which prints the type of the argument +func handlerGood5(req *http.Request) { + object := req.URL.Query()["username"][0] + log.Printf("found object of type %T.\n", object) +} diff --git a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected index 8cba6b476a87..a7f7f83a9ffe 100644 --- a/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected +++ b/go/ql/test/query-tests/Security/CWE-312/CleartextLogging.expected @@ -14,22 +14,22 @@ | main.go:26:13:26:20 | password | main.go:26:13:26:20 | password | main.go:26:13:26:20 | password | $@ flows to a logging call. | main.go:26:13:26:20 | password | Sensitive data returned by an access to password | | main.go:27:14:27:21 | password | main.go:27:14:27:21 | password | main.go:27:14:27:21 | password | $@ flows to a logging call. | main.go:27:14:27:21 | password | Sensitive data returned by an access to password | | main.go:28:16:28:23 | password | main.go:28:16:28:23 | password | main.go:28:16:28:23 | password | $@ flows to a logging call. | main.go:28:16:28:23 | password | Sensitive data returned by an access to password | -| main.go:31:10:31:17 | password | main.go:31:10:31:17 | password | main.go:31:10:31:17 | password | $@ flows to a logging call. | main.go:31:10:31:17 | password | Sensitive data returned by an access to password | -| main.go:32:17:32:24 | password | main.go:32:17:32:24 | password | main.go:32:17:32:24 | password | $@ flows to a logging call. | main.go:32:17:32:24 | password | Sensitive data returned by an access to password | -| main.go:33:11:33:18 | password | main.go:33:11:33:18 | password | main.go:33:11:33:18 | password | $@ flows to a logging call. | main.go:33:11:33:18 | password | Sensitive data returned by an access to password | -| main.go:34:12:34:19 | password | main.go:34:12:34:19 | password | main.go:34:12:34:19 | password | $@ flows to a logging call. | main.go:34:12:34:19 | password | Sensitive data returned by an access to password | -| main.go:35:10:35:17 | password | main.go:35:10:35:17 | password | main.go:35:10:35:17 | password | $@ flows to a logging call. | main.go:35:10:35:17 | password | Sensitive data returned by an access to password | -| main.go:36:17:36:24 | password | main.go:36:17:36:24 | password | main.go:36:17:36:24 | password | $@ flows to a logging call. | main.go:36:17:36:24 | password | Sensitive data returned by an access to password | -| main.go:37:11:37:18 | password | main.go:37:11:37:18 | password | main.go:37:11:37:18 | password | $@ flows to a logging call. | main.go:37:11:37:18 | password | Sensitive data returned by an access to password | -| main.go:38:12:38:19 | password | main.go:38:12:38:19 | password | main.go:38:12:38:19 | password | $@ flows to a logging call. | main.go:38:12:38:19 | password | Sensitive data returned by an access to password | -| main.go:39:10:39:17 | password | main.go:39:10:39:17 | password | main.go:39:10:39:17 | password | $@ flows to a logging call. | main.go:39:10:39:17 | password | Sensitive data returned by an access to password | -| main.go:40:17:40:24 | password | main.go:40:17:40:24 | password | main.go:40:17:40:24 | password | $@ flows to a logging call. | main.go:40:17:40:24 | password | Sensitive data returned by an access to password | -| main.go:41:11:41:18 | password | main.go:41:11:41:18 | password | main.go:41:11:41:18 | password | $@ flows to a logging call. | main.go:41:11:41:18 | password | Sensitive data returned by an access to password | -| main.go:42:12:42:19 | password | main.go:42:12:42:19 | password | main.go:42:12:42:19 | password | $@ flows to a logging call. | main.go:42:12:42:19 | password | Sensitive data returned by an access to password | -| main.go:43:14:43:21 | password | main.go:43:14:43:21 | password | main.go:43:14:43:21 | password | $@ flows to a logging call. | main.go:43:14:43:21 | password | Sensitive data returned by an access to password | -| main.go:45:12:45:19 | password | main.go:45:12:45:19 | password | main.go:45:12:45:19 | password | $@ flows to a logging call. | main.go:45:12:45:19 | password | Sensitive data returned by an access to password | -| main.go:46:17:46:24 | password | main.go:46:17:46:24 | password | main.go:46:17:46:24 | password | $@ flows to a logging call. | main.go:46:17:46:24 | password | Sensitive data returned by an access to password | -| main.go:53:35:53:42 | password | main.go:53:35:53:42 | password | main.go:53:35:53:42 | password | $@ flows to a logging call. | main.go:53:35:53:42 | password | Sensitive data returned by an access to password | +| main.go:32:10:32:17 | password | main.go:32:10:32:17 | password | main.go:32:10:32:17 | password | $@ flows to a logging call. | main.go:32:10:32:17 | password | Sensitive data returned by an access to password | +| main.go:33:17:33:24 | password | main.go:33:17:33:24 | password | main.go:33:17:33:24 | password | $@ flows to a logging call. | main.go:33:17:33:24 | password | Sensitive data returned by an access to password | +| main.go:34:11:34:18 | password | main.go:34:11:34:18 | password | main.go:34:11:34:18 | password | $@ flows to a logging call. | main.go:34:11:34:18 | password | Sensitive data returned by an access to password | +| main.go:35:12:35:19 | password | main.go:35:12:35:19 | password | main.go:35:12:35:19 | password | $@ flows to a logging call. | main.go:35:12:35:19 | password | Sensitive data returned by an access to password | +| main.go:36:10:36:17 | password | main.go:36:10:36:17 | password | main.go:36:10:36:17 | password | $@ flows to a logging call. | main.go:36:10:36:17 | password | Sensitive data returned by an access to password | +| main.go:37:17:37:24 | password | main.go:37:17:37:24 | password | main.go:37:17:37:24 | password | $@ flows to a logging call. | main.go:37:17:37:24 | password | Sensitive data returned by an access to password | +| main.go:38:11:38:18 | password | main.go:38:11:38:18 | password | main.go:38:11:38:18 | password | $@ flows to a logging call. | main.go:38:11:38:18 | password | Sensitive data returned by an access to password | +| main.go:39:12:39:19 | password | main.go:39:12:39:19 | password | main.go:39:12:39:19 | password | $@ flows to a logging call. | main.go:39:12:39:19 | password | Sensitive data returned by an access to password | +| main.go:40:10:40:17 | password | main.go:40:10:40:17 | password | main.go:40:10:40:17 | password | $@ flows to a logging call. | main.go:40:10:40:17 | password | Sensitive data returned by an access to password | +| main.go:41:17:41:24 | password | main.go:41:17:41:24 | password | main.go:41:17:41:24 | password | $@ flows to a logging call. | main.go:41:17:41:24 | password | Sensitive data returned by an access to password | +| main.go:42:11:42:18 | password | main.go:42:11:42:18 | password | main.go:42:11:42:18 | password | $@ flows to a logging call. | main.go:42:11:42:18 | password | Sensitive data returned by an access to password | +| main.go:43:12:43:19 | password | main.go:43:12:43:19 | password | main.go:43:12:43:19 | password | $@ flows to a logging call. | main.go:43:12:43:19 | password | Sensitive data returned by an access to password | +| main.go:44:14:44:21 | password | main.go:44:14:44:21 | password | main.go:44:14:44:21 | password | $@ flows to a logging call. | main.go:44:14:44:21 | password | Sensitive data returned by an access to password | +| main.go:47:12:47:19 | password | main.go:47:12:47:19 | password | main.go:47:12:47:19 | password | $@ flows to a logging call. | main.go:47:12:47:19 | password | Sensitive data returned by an access to password | +| main.go:48:17:48:24 | password | main.go:48:17:48:24 | password | main.go:48:17:48:24 | password | $@ flows to a logging call. | main.go:48:17:48:24 | password | Sensitive data returned by an access to password | +| main.go:55:35:55:42 | password | main.go:55:35:55:42 | password | main.go:55:35:55:42 | password | $@ flows to a logging call. | main.go:55:35:55:42 | password | Sensitive data returned by an access to password | | overrides.go:13:14:13:23 | call to String | overrides.go:9:9:9:16 | password | overrides.go:13:14:13:23 | call to String | $@ flows to a logging call. | overrides.go:9:9:9:16 | password | Sensitive data returned by an access to password | | passwords.go:9:14:9:14 | x | passwords.go:30:8:30:15 | password | passwords.go:9:14:9:14 | x | $@ flows to a logging call. | passwords.go:30:8:30:15 | password | Sensitive data returned by an access to password | | passwords.go:25:14:25:21 | password | passwords.go:25:14:25:21 | password | passwords.go:25:14:25:21 | password | $@ flows to a logging call. | passwords.go:25:14:25:21 | password | Sensitive data returned by an access to password | @@ -121,22 +121,22 @@ nodes | main.go:26:13:26:20 | password | semmle.label | password | | main.go:27:14:27:21 | password | semmle.label | password | | main.go:28:16:28:23 | password | semmle.label | password | -| main.go:31:10:31:17 | password | semmle.label | password | -| main.go:32:17:32:24 | password | semmle.label | password | -| main.go:33:11:33:18 | password | semmle.label | password | -| main.go:34:12:34:19 | password | semmle.label | password | -| main.go:35:10:35:17 | password | semmle.label | password | -| main.go:36:17:36:24 | password | semmle.label | password | -| main.go:37:11:37:18 | password | semmle.label | password | -| main.go:38:12:38:19 | password | semmle.label | password | -| main.go:39:10:39:17 | password | semmle.label | password | -| main.go:40:17:40:24 | password | semmle.label | password | -| main.go:41:11:41:18 | password | semmle.label | password | -| main.go:42:12:42:19 | password | semmle.label | password | -| main.go:43:14:43:21 | password | semmle.label | password | -| main.go:45:12:45:19 | password | semmle.label | password | -| main.go:46:17:46:24 | password | semmle.label | password | -| main.go:53:35:53:42 | password | semmle.label | password | +| main.go:32:10:32:17 | password | semmle.label | password | +| main.go:33:17:33:24 | password | semmle.label | password | +| main.go:34:11:34:18 | password | semmle.label | password | +| main.go:35:12:35:19 | password | semmle.label | password | +| main.go:36:10:36:17 | password | semmle.label | password | +| main.go:37:17:37:24 | password | semmle.label | password | +| main.go:38:11:38:18 | password | semmle.label | password | +| main.go:39:12:39:19 | password | semmle.label | password | +| main.go:40:10:40:17 | password | semmle.label | password | +| main.go:41:17:41:24 | password | semmle.label | password | +| main.go:42:11:42:18 | password | semmle.label | password | +| main.go:43:12:43:19 | password | semmle.label | password | +| main.go:44:14:44:21 | password | semmle.label | password | +| main.go:47:12:47:19 | password | semmle.label | password | +| main.go:48:17:48:24 | password | semmle.label | password | +| main.go:55:35:55:42 | password | semmle.label | password | | overrides.go:9:9:9:16 | password | semmle.label | password | | overrides.go:13:14:13:23 | call to String | semmle.label | call to String | | passwords.go:8:12:8:12 | definition of x | semmle.label | definition of x | diff --git a/go/ql/test/query-tests/Security/CWE-312/main.go b/go/ql/test/query-tests/Security/CWE-312/main.go index 2d4121d3606c..17a183ff2096 100644 --- a/go/ql/test/query-tests/Security/CWE-312/main.go +++ b/go/ql/test/query-tests/Security/CWE-312/main.go @@ -26,6 +26,7 @@ func main() { log.Panicf(password, "") // $ Alert log.Panicln(password) // $ Alert log.Output(0, password) // $ Alert + log.Printf("%T", password) l := log.Default() l.Print(password) // $ Alert @@ -41,6 +42,7 @@ func main() { l.Panicf(password, "") // $ Alert l.Panicln(password) // $ Alert l.Output(0, password) // $ Alert + l.Printf("%T", password) glog.Info(password) // $ Alert logrus.Warning(password) // $ Alert From bf78160830b4723ccbaef60ee8b3219506a311c9 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 15:02:29 +0000 Subject: [PATCH 07/13] Add change note --- .../2025-03-18-loggercall-type-format-specifier.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md diff --git a/go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md b/go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md new file mode 100644 index 000000000000..c3879a6ffca6 --- /dev/null +++ b/go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* `LoggerCall::getAMessageComponent` no longer returns arguments to logger calls which correspond to the verb `%T` in a format specifier. This will remove false positives in "Log entries created from user input" (`go/log-injection`) and "Clear-text logging of sensitive information" (`go/clear-text-logging`), and it may lead to more results in "Use of constant `state` value in OAuth 2.0 URL" (`go/constant-oauth2-state`). From 05a94807e130cf2beec9dea47b2b7b8937231976 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 18 Mar 2025 16:48:02 +0000 Subject: [PATCH 08/13] Make comment clearer --- go/ql/lib/semmle/go/Concepts.qll | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/go/ql/lib/semmle/go/Concepts.qll b/go/ql/lib/semmle/go/Concepts.qll index 2239291565d2..3c4f326cba4e 100644 --- a/go/ql/lib/semmle/go/Concepts.qll +++ b/go/ql/lib/semmle/go/Concepts.qll @@ -356,10 +356,9 @@ module RegexpReplaceFunction { */ class LoggerCall extends DataFlow::Node instanceof LoggerCall::Range { /** - * Gets a node that is a part of the logged message. - * - * Exclude components corresponding to the format specifier "%T" as this - * prints the type of the argument, which is not considered vulnerable. + * Gets a node whose value is a part of the logged message. Note that + * components corresponding to the format specifier "%T" are excluded as + * their type is logged rather than their value. */ DataFlow::Node getAMessageComponent() { result = super.getAMessageComponent() and From f944ff4d782d382a0f1ad2e7dcd514d2de269a5e Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 20 Mar 2025 14:48:24 +0000 Subject: [PATCH 09/13] Create `getAValueFormattedMessageComponent` --- go/ql/lib/semmle/go/Concepts.qll | 12 ++++++++---- .../go/security/CleartextLoggingCustomizations.qll | 2 +- .../go/security/LogInjectionCustomizations.qll | 2 +- go/ql/src/Security/CWE-352/ConstantOauth2State.ql | 4 +++- .../semmle/go/concepts/LoggerCall/LoggerCall.ql | 12 +++++++++--- 5 files changed, 22 insertions(+), 10 deletions(-) diff --git a/go/ql/lib/semmle/go/Concepts.qll b/go/ql/lib/semmle/go/Concepts.qll index 3c4f326cba4e..36a5bf44c9b5 100644 --- a/go/ql/lib/semmle/go/Concepts.qll +++ b/go/ql/lib/semmle/go/Concepts.qll @@ -355,13 +355,17 @@ module RegexpReplaceFunction { * extend `LoggerCall::Range` instead. */ 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. Note that - * components corresponding to the format specifier "%T" are excluded as + * 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 getAMessageComponent() { - result = super.getAMessageComponent() and + DataFlow::Node getAValueFormattedMessageComponent() { + result = this.getAMessageComponent() and not exists(string formatSpecifier | formatSpecifier.regexpMatch("%[^%]*T") and result = this.(StringOps::Formatting::StringFormatCall).getOperand(_, formatSpecifier) diff --git a/go/ql/lib/semmle/go/security/CleartextLoggingCustomizations.qll b/go/ql/lib/semmle/go/security/CleartextLoggingCustomizations.qll index 17a7345b23e7..6c95686cb8c8 100644 --- a/go/ql/lib/semmle/go/security/CleartextLoggingCustomizations.qll +++ b/go/ql/lib/semmle/go/security/CleartextLoggingCustomizations.qll @@ -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() } } /** diff --git a/go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll b/go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll index 188256f9643b..565cf29a4508 100644 --- a/go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll +++ b/go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll @@ -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() } } /** diff --git a/go/ql/src/Security/CWE-352/ConstantOauth2State.ql b/go/ql/src/Security/CWE-352/ConstantOauth2State.ql index daaac1ce4f3b..31b6907ffddf 100644 --- a/go/ql/src/Security/CWE-352/ConstantOauth2State.ql +++ b/go/ql/src/Security/CWE-352/ConstantOauth2State.ql @@ -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() } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/LoggerCall.ql b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/LoggerCall.ql index 7eef263ec830..11680579012a 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/LoggerCall.ql +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/LoggerCall.ql @@ -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" + ) ) } } From bc40a4289cc0b607dfe60003d8b21e36a86551ad Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 20 Mar 2025 14:56:44 +0000 Subject: [PATCH 10/13] Do not use full regex match for `%T` --- go/ql/lib/semmle/go/Concepts.qll | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/go/ql/lib/semmle/go/Concepts.qll b/go/ql/lib/semmle/go/Concepts.qll index 36a5bf44c9b5..1931f16871ab 100644 --- a/go/ql/lib/semmle/go/Concepts.qll +++ b/go/ql/lib/semmle/go/Concepts.qll @@ -367,8 +367,11 @@ class LoggerCall extends DataFlow::Node instanceof LoggerCall::Range { DataFlow::Node getAValueFormattedMessageComponent() { result = this.getAMessageComponent() and not exists(string formatSpecifier | - formatSpecifier.regexpMatch("%[^%]*T") and - result = this.(StringOps::Formatting::StringFormatCall).getOperand(_, 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") ) } } From da8ae84422ac9b97f1ac6ed967b8db294f10160d Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 20 Mar 2025 15:02:30 +0000 Subject: [PATCH 11/13] Change change note to query change note --- .../2025-03-18-loggercall-type-format-specifier.md | 4 ---- ...2025-03-20-logging-false-positive-type-format-specifier.md | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) delete mode 100644 go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md create mode 100644 go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md diff --git a/go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md b/go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md deleted file mode 100644 index c3879a6ffca6..000000000000 --- a/go/ql/lib/change-notes/2025-03-18-loggercall-type-format-specifier.md +++ /dev/null @@ -1,4 +0,0 @@ ---- -category: minorAnalysis ---- -* `LoggerCall::getAMessageComponent` no longer returns arguments to logger calls which correspond to the verb `%T` in a format specifier. This will remove false positives in "Log entries created from user input" (`go/log-injection`) and "Clear-text logging of sensitive information" (`go/clear-text-logging`), and it may lead to more results in "Use of constant `state` value in OAuth 2.0 URL" (`go/constant-oauth2-state`). diff --git a/go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md b/go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md new file mode 100644 index 000000000000..88c20db04a9d --- /dev/null +++ b/go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md @@ -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`). From 662af6e2480983f1434837295eb1a1f300653b48 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 20 Mar 2025 15:49:20 +0000 Subject: [PATCH 12/13] Update test expectations --- .../semmle/go/concepts/LoggerCall/glog.go | 20 +++++++++---------- .../semmle/go/concepts/LoggerCall/logrus.go | 4 ++-- .../semmle/go/concepts/LoggerCall/stdlib.go | 12 +++++------ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go index b45e1796d897..ab82527b5e0c 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go @@ -31,11 +31,11 @@ func glogTest() { 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 + 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 @@ -59,9 +59,9 @@ func glogTest() { 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 + 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 } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go index 613ace5f1ee9..bdb57aae2e1b 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go @@ -34,6 +34,6 @@ func logrusCalls() { 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 + 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 } diff --git a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go index d0f1ca181369..6fbf3c43fd35 100644 --- a/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go +++ b/go/ql/test/library-tests/semmle/go/concepts/LoggerCall/stdlib.go @@ -18,9 +18,9 @@ func stdlib() { 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 + 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 @@ -34,7 +34,7 @@ func stdlib() { 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 + 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 } From f677ddda260610e3c51bc7629c770a99263a0e35 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Fri, 21 Mar 2025 11:26:50 +0000 Subject: [PATCH 13/13] Update wording of change note (accepting review suggestion) Co-authored-by: Michael B. Gale --- .../2025-03-20-logging-false-positive-type-format-specifier.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md b/go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md index 88c20db04a9d..43478a70097e 100644 --- a/go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md +++ b/go/ql/src/change-notes/2025-03-20-logging-false-positive-type-format-specifier.md @@ -1,4 +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`). +* 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.