github · atorralba · Nov 24, 2021 · Nov 19, 2021 · Nov 19, 2021 · Nov 22, 2021
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* A new query "Log entries created from user input" (`go/log-injection`) has been added. The query reports user-provided data reaching calls to logging methods.
@@ -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)") 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))
+    }
   }
 }
@@ -11,7 +11,10 @@ module Logrus {
 
   bindingset[result]
   private string getALogResultName() {
-    result.matches(["Debug%", "Error%", "Fatal%", "Info%", "Panic%", "Print%", "Trace%", "Warn%"])
+    result
+        .matches([
+            "Debug%", "Error%", "Fatal%", "Info%", "Log%", "Panic%", "Print%", "Trace%", "Warn%"
+          ])
   }
 
   bindingset[result]
@@ -23,7 +26,7 @@ module Logrus {
     LogCall() {
       exists(string name | name = getALogResultName() or name = getAnEntryUpdatingMethodName() |
         this.getTarget().hasQualifiedName(packagePath(), name) or
-        this.getTarget().(Method).hasQualifiedName(packagePath(), "Entry", name)
+        this.getTarget().(Method).hasQualifiedName(packagePath(), ["Entry", "Logger"], name)
       )
     }
 

@@ -8,13 +8,7 @@ import go
 module Log {
   private class LogCall extends LoggerCall::Range, DataFlow::CallNode {
     LogCall() {
-      exists(string fn |
-        fn.matches("Fatal%")
-        or
-        fn.matches("Panic%")
-        or
-        fn.matches("Print%")
-      |
+      exists(string fn | fn.matches(["Fatal%", "Panic%", "Print%"]) |
         this.getTarget().hasQualifiedName("log", fn)
         or
         this.getTarget().(Method).hasQualifiedName("log", "Logger", fn)

@@ -0,0 +1,33 @@
+/**
+ * Provides a taint tracking configuration for reasoning about log injection vulnerabilities.
+ *
+ * Note: for performance reasons, only import this file if `LogInjection::Configuration` is needed,
+ * otherwise `LogInjectionCustomizations` should be imported instead.
+ */
+
+import go
+
+/**
+ * Provides a taint-tracking configuration for reasoning about
+ * log injection vulnerabilities.
+ */
+module LogInjection {
+  import LogInjectionCustomizations::LogInjection
+
+  /**
+   * A taint-tracking configuration for reasoning about log injection vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "LogInjection" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer }
+
+    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+      guard instanceof SanitizerGuard
+    }
+  }
+}
@@ -0,0 +1,42 @@
+/**
+ * Provides default sources, sinks, and sanitizers for reasoning about
+ * log injection vulnerabilities, as well as extension points for adding your own.
+ */
+
+import go
+
+/**
+ * Provides extension points for customizing the data-flow tracking configuration for reasoning
+ * about log injection.
+ */
+module LogInjection {
+  /**
+   * A data flow source for log injection vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for log injection vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for log injection vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for log injection vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /** A source of untrusted data, considered as a taint source for log injection. */
+  class UntrustedFlowAsSource extends Source {
+    UntrustedFlowAsSource() { this instanceof UntrustedFlowSource }
+  }
+
+  /** An argument to a logging mechanism. */
+  class LoggerSink extends Sink {
+    LoggerSink() { this = any(LoggerCall log).getAMessageComponent() }
+  }
+}
@@ -0,0 +1,12 @@
+package main
+
+import (
+	"log"
+	"net/http"
+)
+
+// BAD: A user-provided value is written directly to a log.
+func handler(req *http.Request) {
+	username := req.URL.Query()["username"][0]
+	log.Printf("user %s logged in.\n", username)
+}
@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>If unsanitized user input is written to a log entry, a malicious user may
+be able to forge new log entries.</p>
+
+<p>Forgery can occur if a user provides some input with characters that are interpreted
+when the log output is displayed. If the log is displayed as a plain text file, then new
+line characters can be used by a malicious user. If the log is displayed as HTML, then
+arbitrary HTML may be include to spoof log entries.</p>
+</overview>
+
+<recommendation>
+<p>
+User input should be suitably encoded before it is logged.
+</p>
+<p>
+If the log entries are plain text then line breaks should be removed from user input, using
+<code>strings.Replace</code> or similar. Care should also be taken that user input is clearly marked
+in log entries, and that a malicious user cannot cause confusion in other ways.
+</p>
+<p>
+For log entries that will be displayed in HTML, user input should be HTML encoded using
+<code>html.EscapeString</code> or similar before being logged, to prevent forgery and
+other forms of HTML injection.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In the following example, a user name, provided by the user, is logged using a logging framework without any sanitization.
+</p>
+<sample src="LogInjection.go" />
+<p>
+In the next example, <code>strings.Replace</code> is used to ensure no line endings are present in the user input.
+</p>
+<sample src="LogInjectionGood.go" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>
+</references>
+</qhelp>
@@ -0,0 +1,21 @@
+/**
+ * @name Log entries created from user input
+ * @description Building log entries from user-controlled sources is vulnerable to
+ *              insertion of forged log entries by a malicious user.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.8
+ * @precision high
+ * @id go/log-injection
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import go
+import semmle.go.security.LogInjection
+import DataFlow::PathGraph
+
+from LogInjection::Configuration c, DataFlow::PathNode source, DataFlow::PathNode sink
+where c.hasFlowPath(source, sink)
+select sink, source, sink, "This log write receives unsanitized user input from $@.",
+  source.getNode(), "here"
@@ -0,0 +1,15 @@
+package main
+
+import (
+	"log"
+	"net/http"
+	"strings"
+)
+
+// GOOD: The user-provided value is escaped before being written to the log.
+func handlerGood(req *http.Request) {
+	username := req.URL.Query()["username"][0]
+	escapedUsername := strings.Replace(username, "\n", "", -1)
+	escapedUsername = strings.Replace(escapedUsername, "\r", "", -1)
+	log.Printf("user %s logged in.\n", escapedUsername)
+}
@@ -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
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		lgtm,codescanning
		* A new query "Log entries created from user input" (`go/log-injection`) has been added. The query reports user-provided data reaching calls to logging methods.