From ef94cde0b3e99111da07fad7bcc82812fb7ef55a Mon Sep 17 00:00:00 2001
From: Owen Mansel-Chan <owen-mc@github.com>
Date: Mon, 15 Feb 2021 00:57:30 +0000
Subject: [PATCH] Simplify Logrus model

Make methods which add data to entries sinks in their own right, rather
than trying to track the data flow of the entry to a later logging call.

This may cause some false positives, but only in the situation that
tainted data is added to an entry and that entry is never logged. It will
save us from false negatives when tainted data is added to an entry
which flows across a function boundary to a logging call.
---
 ql/src/semmle/go/frameworks/Logrus.qll        | 38 +++++++------------
 .../semmle/go/concepts/LoggerCall/logrus.go   | 17 +++++----
 2 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/ql/src/semmle/go/frameworks/Logrus.qll b/ql/src/semmle/go/frameworks/Logrus.qll
index 4be9c1c2f84..7c9ec711197 100644
--- a/ql/src/semmle/go/frameworks/Logrus.qll
+++ b/ql/src/semmle/go/frameworks/Logrus.qll
@@ -15,33 +15,21 @@ module Logrus {
     result.matches(["Debug%", "Error%", "Fatal%", "Info%", "Panic%", "Print%", "Trace%", "Warn%"])
   }
 
+  bindingset[result]
+  private string getAnEntryUpdatingMethodName() {
+    result.regexpMatch("With(Context|Error|Fields?|Time)")
+  }
+
   private class LogCall extends LoggerCall::Range, DataFlow::CallNode {
-    LogCall() { this.getTarget().hasQualifiedName(packagePath(), getALogResultName()) }
+    LogCall() {
+      this.getTarget().hasQualifiedName(packagePath(), getALogResultName()) or
+      this.getTarget().(Method).hasQualifiedName(packagePath(), "Entry", getALogResultName()) or
+      this.getTarget().hasQualifiedName(packagePath(), getAnEntryUpdatingMethodName()) or
+      this.getTarget()
+          .(Method)
+          .hasQualifiedName(packagePath(), "Entry", getAnEntryUpdatingMethodName())
+    }
 
     override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
-
-  private class LogEntryCall extends LoggerCall::Range, DataFlow::MethodCallNode {
-    LogEntryCall() {
-      this.getTarget().(Method).hasQualifiedName(packagePath(), "Entry", getALogResultName())
-    }
-
-    override DataFlow::Node getAMessageComponent() {
-      result = this.getAnArgument()
-      or
-      exists(DataFlow::MethodCallNode addFieldCall, DataFlow::SsaNode entry |
-        entry.getAUse() = this.getReceiver() and
-        entry.getAUse() = addFieldCall.getReceiver()
-      |
-        addFieldCall.getCalleeName().regexpMatch("With(Context|Error|Fields?|Time)") and
-        result = addFieldCall.getAnArgument()
-      )
-      or
-      exists(DataFlow::CallNode entryBuild |
-        entryBuild.getASuccessor*() = this.getReceiver() and
-        entryBuild.getCalleeName().regexpMatch("With(Context|Error|Fields?|Time)") and
-        result = entryBuild.getAnArgument()
-      )
-    }
-  }
 }
diff --git a/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go b/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go
index c2e65ff6788..3dd4da3ef0d 100644
--- a/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go
+++ b/ql/test/library-tests/semmle/go/concepts/LoggerCall/logrus.go
@@ -5,11 +5,12 @@ package main
 import (
 	"context"
 	"errors"
+
 	"github.com/sirupsen/logrus"
 )
 
 func logSomething(entry *logrus.Entry) {
-	entry.Traceln(text) // $logger=text $f-:logger=fields
+	entry.Traceln(text) // $logger=text
 }
 
 func logrusCalls() {
@@ -17,13 +18,13 @@ func logrusCalls() {
 	var fields logrus.Fields = nil
 	var fn logrus.LogFunction = nil
 	var ctx context.Context
-	tmp := logrus.WithContext(ctx)  //
-	tmp.Debugf(fmt, text)           // $logger=ctx $logger=fmt $logger=text
-	tmp = logrus.WithError(err)     //
-	tmp.Warn(text)                  // $logger=err $logger=text
-	tmp = logrus.WithFields(fields) //
-	tmp.Infoln(text)                // $logger=fields $logger=text
-	tmp = logrus.WithFields(fields) //
+	tmp := logrus.WithContext(ctx)  // $logger=ctx
+	tmp.Debugf(fmt, text)           // $logger=fmt $logger=text
+	tmp = logrus.WithError(err)     // $logger=err
+	tmp.Warn(text)                  // $logger=text
+	tmp = logrus.WithFields(fields) // $logger=fields
+	tmp.Infoln(text)                // $logger=text
+	tmp = logrus.WithFields(fields) // $logger=fields
 	logSomething(tmp)
 
 	logrus.Error(text)       // $logger=text