From c9332cdccb2523567979a23fafcab300933fd522 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 22 Nov 2021 09:15:01 +0100 Subject: [PATCH] Fix *Depth log levels in glog and klog --- ql/lib/semmle/go/frameworks/Glog.qll | 15 +++- .../semmle/go/concepts/LoggerCall/glog.go | 20 +++--- .../Security/CWE-117/LogInjection.go | 70 +++++++++---------- 3 files changed, 57 insertions(+), 48 deletions(-) diff --git a/ql/lib/semmle/go/frameworks/Glog.qll b/ql/lib/semmle/go/frameworks/Glog.qll index 0756a6ad608..fbcda396d82 100644 --- a/ql/lib/semmle/go/frameworks/Glog.qll +++ b/ql/lib/semmle/go/frameworks/Glog.qll @@ -11,10 +11,17 @@ import go */ module Glog { private class GlogCall extends LoggerCall::Range, DataFlow::CallNode { + int firstPrintedArg; + GlogCall() { - exists(string pkg, Function f, string fn | + exists(string pkg, Function f, string fn, string level | pkg = package(["github.com/golang/glog", "gopkg.in/glog", "k8s.io/klog"], "") and - fn.regexpMatch("(Error|Exit|Fatal|Info|Warning)(|f|ln|Depth)") and + level = ["Error", "Exit", "Fatal", "Info", "Warning"] and + ( + fn = level + ["", "f", "ln"] and firstPrintedArg = 0 + or + fn = level + "Depth" and firstPrintedArg = 1 + ) and this = f.getACall() | f.hasQualifiedName(pkg, fn) @@ -23,6 +30,8 @@ module Glog { ) } - override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() } + override DataFlow::Node getAMessageComponent() { + result = this.getArgument(any(int i | i >= firstPrintedArg)) + } } } diff --git a/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go b/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go index 28f5be2b067..6913bfc9576 100644 --- a/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go +++ b/ql/test/library-tests/semmle/go/concepts/LoggerCall/glog.go @@ -10,44 +10,44 @@ import ( func glogTest() { glog.Error(text) // $ logger=text - glog.ErrorDepth(0, text) // $ MISSING: logger=text + glog.ErrorDepth(0, text) // $ logger=text glog.Errorf(fmt, text) // $ logger=fmt logger=text glog.Errorln(text) // $ logger=text glog.Exit(text) // $ logger=text - glog.ExitDepth(0, text) // $ MISSING: logger=text + glog.ExitDepth(0, text) // $ logger=text glog.Exitf(fmt, text) // $ logger=fmt logger=text glog.Exitln(text) // $ logger=text glog.Fatal(text) // $ logger=text - glog.FatalDepth(0, text) // $ MISSING: logger=text + glog.FatalDepth(0, text) // $ logger=text glog.Fatalf(fmt, text) // $ logger=fmt logger=text glog.Fatalln(text) // $ logger=text glog.Info(text) // $ logger=text - glog.InfoDepth(0, text) // $ MISSING: logger=text + glog.InfoDepth(0, text) // $ logger=text glog.Infof(fmt, text) // $ logger=fmt logger=text glog.Infoln(text) // $ logger=text glog.Warning(text) // $ logger=text - glog.WarningDepth(0, text) // $ MISSING: logger=text + glog.WarningDepth(0, text) // $ logger=text glog.Warningf(fmt, text) // $ logger=fmt logger=text glog.Warningln(text) // $ logger=text klog.Error(text) // $ logger=text - klog.ErrorDepth(0, text) // $ MISSING: logger=text + klog.ErrorDepth(0, text) // $ logger=text klog.Errorf(fmt, text) // $ logger=fmt logger=text klog.Errorln(text) // $ logger=text klog.Exit(text) // $ logger=text - klog.ExitDepth(0, text) // $ MISSING: logger=text + klog.ExitDepth(0, text) // $ logger=text klog.Exitf(fmt, text) // $ logger=fmt logger=text klog.Exitln(text) // $ logger=text klog.Fatal(text) // $ logger=text - klog.FatalDepth(0, text) // $ MISSING: logger=text + klog.FatalDepth(0, text) // $ logger=text klog.Fatalf(fmt, text) // $ logger=fmt logger=text klog.Fatalln(text) // $ logger=text klog.Info(text) // $ logger=text - klog.InfoDepth(0, text) // $ MISSING: logger=text + klog.InfoDepth(0, text) // $ logger=text klog.Infof(fmt, text) // $ logger=fmt logger=text klog.Infoln(text) // $ logger=text klog.Warning(text) // $ logger=text - klog.WarningDepth(0, text) // $ MISSING: logger=text + klog.WarningDepth(0, text) // $ logger=text klog.Warningf(fmt, text) // $ logger=fmt logger=text klog.Warningln(text) // $ logger=text } diff --git a/ql/test/query-tests/Security/CWE-117/LogInjection.go b/ql/test/query-tests/Security/CWE-117/LogInjection.go index 203575d3bda..a82fb0ab178 100644 --- a/ql/test/query-tests/Security/CWE-117/LogInjection.go +++ b/ql/test/query-tests/Security/CWE-117/LogInjection.go @@ -77,25 +77,25 @@ func handler(req *http.Request, ctx *goproxy.ProxyCtx) { // k8s.io/klog { verbose := klog.V(0) - verbose.Info(username) // $ hasTaintFlow="username" - verbose.Infof(username) // $ hasTaintFlow="username" - verbose.Infoln(username) // $ hasTaintFlow="username" - klog.Info(username) // $ hasTaintFlow="username" - klog.InfoDepth(username) // $ hasTaintFlow="username" - klog.Infof(username) // $ hasTaintFlow="username" - klog.Infoln(username) // $ hasTaintFlow="username" - klog.Error(username) // $ hasTaintFlow="username" - klog.ErrorDepth(username) // $ hasTaintFlow="username" - klog.Errorf(username) // $ hasTaintFlow="username" - klog.Errorln(username) // $ hasTaintFlow="username" - klog.Fatal(username) // $ hasTaintFlow="username" - klog.FatalDepth(username) // $ hasTaintFlow="username" - klog.Fatalf(username) // $ hasTaintFlow="username" - klog.Fatalln(username) // $ hasTaintFlow="username" - klog.Exit(username) // $ hasTaintFlow="username" - klog.ExitDepth(username) // $ hasTaintFlow="username" - klog.Exitf(username) // $ hasTaintFlow="username" - klog.Exitln(username) // $ hasTaintFlow="username" + verbose.Info(username) // $ hasTaintFlow="username" + verbose.Infof(username) // $ hasTaintFlow="username" + verbose.Infoln(username) // $ hasTaintFlow="username" + klog.Info(username) // $ hasTaintFlow="username" + klog.InfoDepth(0, username) // $ hasTaintFlow="username" + klog.Infof(username) // $ hasTaintFlow="username" + klog.Infoln(username) // $ hasTaintFlow="username" + klog.Error(username) // $ hasTaintFlow="username" + klog.ErrorDepth(0, username) // $ hasTaintFlow="username" + klog.Errorf(username) // $ hasTaintFlow="username" + klog.Errorln(username) // $ hasTaintFlow="username" + klog.Fatal(username) // $ hasTaintFlow="username" + klog.FatalDepth(0, username) // $ hasTaintFlow="username" + klog.Fatalf(username) // $ hasTaintFlow="username" + klog.Fatalln(username) // $ hasTaintFlow="username" + klog.Exit(username) // $ hasTaintFlow="username" + klog.ExitDepth(0, username) // $ hasTaintFlow="username" + klog.Exitf(username) // $ hasTaintFlow="username" + klog.Exitln(username) // $ hasTaintFlow="username" } // astaxie/beego { @@ -152,22 +152,22 @@ func handler(req *http.Request, ctx *goproxy.ProxyCtx) { verbose.Infof(username) // $ hasTaintFlow="username" verbose.Infoln(username) // $ hasTaintFlow="username" - glog.Info(username) // $ hasTaintFlow="username" - glog.InfoDepth(username) // $ hasTaintFlow="username" - glog.Infof(username) // $ hasTaintFlow="username" - glog.Infoln(username) // $ hasTaintFlow="username" - glog.Error(username) // $ hasTaintFlow="username" - glog.ErrorDepth(username) // $ hasTaintFlow="username" - glog.Errorf(username) // $ hasTaintFlow="username" - glog.Errorln(username) // $ hasTaintFlow="username" - glog.Fatal(username) // $ hasTaintFlow="username" - glog.FatalDepth(username) // $ hasTaintFlow="username" - glog.Fatalf(username) // $ hasTaintFlow="username" - glog.Fatalln(username) // $ hasTaintFlow="username" - glog.Exit(username) // $ hasTaintFlow="username" - glog.ExitDepth(username) // $ hasTaintFlow="username" - glog.Exitf(username) // $ hasTaintFlow="username" - glog.Exitln(username) // $ hasTaintFlow="username" + glog.Info(username) // $ hasTaintFlow="username" + glog.InfoDepth(0, username) // $ hasTaintFlow="username" + glog.Infof(username) // $ hasTaintFlow="username" + glog.Infoln(username) // $ hasTaintFlow="username" + glog.Error(username) // $ hasTaintFlow="username" + glog.ErrorDepth(0, username) // $ hasTaintFlow="username" + glog.Errorf(username) // $ hasTaintFlow="username" + glog.Errorln(username) // $ hasTaintFlow="username" + glog.Fatal(username) // $ hasTaintFlow="username" + glog.FatalDepth(0, username) // $ hasTaintFlow="username" + glog.Fatalf(username) // $ hasTaintFlow="username" + glog.Fatalln(username) // $ hasTaintFlow="username" + glog.Exit(username) // $ hasTaintFlow="username" + glog.ExitDepth(0, username) // $ hasTaintFlow="username" + glog.Exitf(username) // $ hasTaintFlow="username" + glog.Exitln(username) // $ hasTaintFlow="username" } // sirupsen/logrus