From 92d3da5e56d8bc2d3668dab4f4e7e430b730c4cd Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Fri, 17 Dec 2021 17:07:58 +0000
Subject: [PATCH 1/5] Declassify fmt.Fprintf as a log sink

In future we could try harder to find out whether you're Fprintf'ing to stdout, a file named xyz.log etc, but for now this causes Fprintf'ing to an HTTP writer to be mistaken for log-injection rather than just XSS.
---
 ql/lib/semmle/go/frameworks/stdlib/Fmt.qll         | 14 +++-----------
 .../query-tests/Security/CWE-117/LogInjection.go   | 12 ++++++------
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll b/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
index e634a82a863..171c4d0ce76 100644
--- a/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
+++ b/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
@@ -29,19 +29,11 @@ module Fmt {
     Printer() { this.hasQualifiedName("fmt", ["Print", "Printf", "Println"]) }
   }
 
-  /** A call to `Print`, `Fprint`, or similar. */
+  /** A call to `Print` or similar. */
   private class PrintCall extends LoggerCall::Range, DataFlow::CallNode {
-    int firstPrintedArg;
+    PrintCall() { this.getTarget() instanceof Printer }
 
-    PrintCall() {
-      this.getTarget() instanceof Printer and firstPrintedArg = 0
-      or
-      this.getTarget() instanceof Fprinter and firstPrintedArg = 1
-    }
-
-    override DataFlow::Node getAMessageComponent() {
-      result = this.getArgument(any(int i | i >= firstPrintedArg))
-    }
+    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
 
   /** The `Fprint` function or one of its variants. */
diff --git a/ql/test/query-tests/Security/CWE-117/LogInjection.go b/ql/test/query-tests/Security/CWE-117/LogInjection.go
index 7beb055ad9a..f386d2fb910 100644
--- a/ql/test/query-tests/Security/CWE-117/LogInjection.go
+++ b/ql/test/query-tests/Security/CWE-117/LogInjection.go
@@ -32,12 +32,12 @@ func handler(req *http.Request, ctx *goproxy.ProxyCtx) {
 	testFlag := req.URL.Query()["testFlag"][0]
 
 	{
-		fmt.Print(username)         // $ hasTaintFlow="username"
-		fmt.Printf(username)        // $ hasTaintFlow="username"
-		fmt.Println(username)       // $ hasTaintFlow="username"
-		fmt.Fprint(nil, username)   // $ hasTaintFlow="username"
-		fmt.Fprintf(nil, username)  // $ hasTaintFlow="username"
-		fmt.Fprintln(nil, username) // $ hasTaintFlow="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)
 	}
 	// log
 	{

From 749698759acf5e8189e18ca5fd636d0ba839f3cc Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Wed, 5 Jan 2022 16:04:20 +0000
Subject: [PATCH 2/5] Note that the %q format directive escapes newlines, and
 therefore prevents log injection

---
 ql/lib/semmle/go/StringOps.qll                | 124 +++++++-----
 .../semmle/go/frameworks/ElazarlGoproxy.qll   |  11 +-
 ql/lib/semmle/go/frameworks/Glog.qll          |  35 +++-
 ql/lib/semmle/go/frameworks/Logrus.qll        |  26 ++-
 ql/lib/semmle/go/frameworks/Spew.qll          |  25 ++-
 ql/lib/semmle/go/frameworks/Zap.qll           |  35 ++--
 ql/lib/semmle/go/frameworks/stdlib/Fmt.qll    |  20 ++
 ql/lib/semmle/go/frameworks/stdlib/Log.qll    |  21 +-
 .../security/LogInjectionCustomizations.qll   |  17 ++
 .../Security/CWE-117/LogInjection.go          | 187 ++++++++++++++++++
 10 files changed, 422 insertions(+), 79 deletions(-)

diff --git a/ql/lib/semmle/go/StringOps.qll b/ql/lib/semmle/go/StringOps.qll
index 45fdca6a7f7..130d7d83b3f 100644
--- a/ql/lib/semmle/go/StringOps.qll
+++ b/ql/lib/semmle/go/StringOps.qll
@@ -166,6 +166,76 @@ module StringOps {
     }
   }
 
+  /** Provides predicates and classes for working with Printf-style formatters. */
+  module Formatting {
+    /**
+     * Gets a regular expression for matching simple format-string components, including flags,
+     * width and precision specifiers, but not including `*` specifiers or explicit argument
+     * indices.
+     */
+    pragma[noinline]
+    private string getFormatComponentRegex() {
+      exists(
+        string literal, string opt_flag, string width, string prec, string opt_width_and_prec,
+        string operator, string verb
+      |
+        literal = "([^%]|%%)+" and
+        opt_flag = "[-+ #0]?" and
+        width = "\\d+|\\*" and
+        prec = "\\.(\\d+|\\*)" and
+        opt_width_and_prec = "(" + width + ")?(" + prec + ")?" and
+        operator = "[bcdeEfFgGoOpqstTxXUv]" and
+        verb = "(%" + opt_flag + opt_width_and_prec + operator + ")"
+      |
+        result = "(" + literal + "|" + verb + ")"
+      )
+    }
+
+    /**
+     * A function that performs string formatting in the same manner as `fmt.Printf` etc.
+     */
+    abstract class Range extends Function {
+      /**
+       * Gets the parameter index of the format string.
+       */
+      abstract int getFormatStringIndex();
+
+      /**
+       * Gets the parameter index of the first parameter to be formatted.
+       */
+      abstract int getFirstFormattedParameterIndex();
+    }
+
+    /**
+     * A call to a `fmt.Printf`-style string formatting function.
+     */
+    class StringFormatCall extends DataFlow::CallNode {
+      string fmt;
+      Range f;
+
+      StringFormatCall() {
+        this = f.getACall() and
+        fmt = this.getArgument(f.getFormatStringIndex()).getStringValue() and
+        fmt.regexpMatch(getFormatComponentRegex() + "*")
+      }
+
+      /**
+       * Gets the `n`th component of this format string.
+       */
+      string getComponent(int n) { result = fmt.regexpFind(getFormatComponentRegex(), n, _) }
+
+      /**
+       * Gets the `n`th argument formatted by this format call, where `formatDirective` specifies how it will be formatted.
+       */
+      DataFlow::Node getOperand(int n, string formatDirective) {
+        formatDirective = this.getComponent(n) and
+        formatDirective.charAt(0) = "%" and
+        formatDirective.charAt(1) != "%" and
+        result = this.getArgument((n / 2) + f.getFirstFormattedParameterIndex())
+      }
+    }
+  }
+
   /**
    * A data-flow node that performs string concatenation.
    *
@@ -233,29 +303,6 @@ module StringOps {
       }
     }
 
-    /**
-     * Gets a regular expression for matching simple format-string components, including flags,
-     * width and precision specifiers, but not including `*` specifiers or explicit argument
-     * indices.
-     */
-    pragma[noinline]
-    private string getFormatComponentRegex() {
-      exists(
-        string literal, string opt_flag, string width, string prec, string opt_width_and_prec,
-        string operator, string verb
-      |
-        literal = "([^%]|%%)+" and
-        opt_flag = "[-+ #0]?" and
-        width = "\\d+|\\*" and
-        prec = "\\.(\\d+|\\*)" and
-        opt_width_and_prec = "(" + width + ")?(" + prec + ")?" and
-        operator = "[bcdeEfFgGoOpqstTxXUv]" and
-        verb = "(%" + opt_flag + opt_width_and_prec + operator + ")"
-      |
-        result = "(" + literal + "|" + verb + ")"
-      )
-    }
-
     /**
      * A call to `fmt.Sprintf`, considered as a string concatenation.
      *
@@ -272,42 +319,25 @@ module StringOps {
      * node nor a string value. This is because verbs like `%q` perform additional string
      * transformations that we cannot easily represent.
      */
-    private class SprintfConcat extends Range, DataFlow::CallNode {
-      string fmt;
-
-      SprintfConcat() {
-        exists(Function sprintf | sprintf.hasQualifiedName("fmt", "Sprintf") |
-          this = sprintf.getACall() and
-          fmt = this.getArgument(0).getStringValue() and
-          fmt.regexpMatch(getFormatComponentRegex() + "*")
-        )
-      }
-
-      /**
-       * Gets the `n`th component of this format string.
-       */
-      private string getComponent(int n) {
-        result = fmt.regexpFind(getFormatComponentRegex(), n, _)
-      }
+    private class SprintfConcat extends Range instanceof Formatting::StringFormatCall {
+      SprintfConcat() { this = any(Function f | f.hasQualifiedName("fmt", "Sprintf")).getACall() }
 
       override DataFlow::Node getOperand(int n) {
-        exists(int i, string part | part = "%s" or part = "%v" |
-          part = this.getComponent(n) and
-          i = n / 2 and
-          result = this.getArgument(i + 1)
-        )
+        result = Formatting::StringFormatCall.super.getOperand(n, ["%s", "%v"])
       }
 
       override string getOperandStringValue(int n) {
         result = Range.super.getOperandStringValue(n)
         or
-        exists(string cmp | cmp = this.getComponent(n) |
+        exists(string cmp | cmp = Formatting::StringFormatCall.super.getComponent(n) |
           (cmp.charAt(0) != "%" or cmp.charAt(1) = "%") and
           result = cmp.replaceAll("%%", "%")
         )
       }
 
-      override int getNumOperand() { result = max(int i | exists(this.getComponent(i))) + 1 }
+      override int getNumOperand() {
+        result = max(int i | exists(Formatting::StringFormatCall.super.getComponent(i))) + 1
+      }
     }
 
     /**
diff --git a/ql/lib/semmle/go/frameworks/ElazarlGoproxy.qll b/ql/lib/semmle/go/frameworks/ElazarlGoproxy.qll
index 0ab4e48375b..0d49431e72b 100644
--- a/ql/lib/semmle/go/frameworks/ElazarlGoproxy.qll
+++ b/ql/lib/semmle/go/frameworks/ElazarlGoproxy.qll
@@ -3,6 +3,7 @@
  */
 
 import go
+private import semmle.go.StringOps
 
 /**
  * Provides classes for working with concepts relating to the [github.com/elazarl/goproxy](https://pkg.go.dev/github.com/elazarl/goproxy) package.
@@ -108,8 +109,16 @@ module ElazarlGoproxy {
     }
   }
 
+  private class ProxyLogFunction extends StringOps::Formatting::Range, Method {
+    ProxyLogFunction() { this.hasQualifiedName(packagePath(), "ProxyCtx", ["Logf", "Warnf"]) }
+
+    override int getFormatStringIndex() { result = 0 }
+
+    override int getFirstFormattedParameterIndex() { result = 1 }
+  }
+
   private class ProxyLog extends LoggerCall::Range, DataFlow::MethodCallNode {
-    ProxyLog() { this.getTarget().hasQualifiedName(packagePath(), "ProxyCtx", ["Logf", "Warnf"]) }
+    ProxyLog() { this.getTarget() instanceof ProxyLogFunction }
 
     override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
diff --git a/ql/lib/semmle/go/frameworks/Glog.qll b/ql/lib/semmle/go/frameworks/Glog.qll
index fbcda396d82..70b117a71e6 100644
--- a/ql/lib/semmle/go/frameworks/Glog.qll
+++ b/ql/lib/semmle/go/frameworks/Glog.qll
@@ -4,34 +4,53 @@
  */
 
 import go
+private import semmle.go.StringOps
 
 /**
  * Provides models of commonly used functions in the `github.com/golang/glog` packages and its
  * forks.
  */
 module Glog {
-  private class GlogCall extends LoggerCall::Range, DataFlow::CallNode {
+  private class GlogFunction extends Function {
     int firstPrintedArg;
 
-    GlogCall() {
-      exists(string pkg, Function f, string fn, string level |
+    GlogFunction() {
+      exists(string pkg, string fn, string level |
         pkg = package(["github.com/golang/glog", "gopkg.in/glog", "k8s.io/klog"], "") 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)
+        this.hasQualifiedName(pkg, fn)
         or
-        f.(Method).hasQualifiedName(pkg, "Verbose", fn)
+        this.(Method).hasQualifiedName(pkg, "Verbose", fn)
       )
     }
 
+    /**
+     * Gets the index of the first argument that may be output, including a format string if one is present.
+     */
+    int getFirstPrintedArg() { result = firstPrintedArg }
+  }
+
+  private class StringFormatter extends StringOps::Formatting::Range instanceof GlogFunction {
+    StringFormatter() { this.getName().matches("%f") }
+
+    override int getFormatStringIndex() { result = super.getFirstPrintedArg() }
+
+    override int getFirstFormattedParameterIndex() { result = super.getFirstPrintedArg() + 1 }
+  }
+
+  private class GlogCall extends LoggerCall::Range, DataFlow::CallNode {
+    GlogFunction callee;
+
+    GlogCall() { this = callee.getACall() }
+
     override DataFlow::Node getAMessageComponent() {
-      result = this.getArgument(any(int i | i >= firstPrintedArg))
+      result = this.getArgument(any(int i | i >= callee.getFirstPrintedArg()))
     }
   }
 }
diff --git a/ql/lib/semmle/go/frameworks/Logrus.qll b/ql/lib/semmle/go/frameworks/Logrus.qll
index 2daa1370a93..9255b6767b5 100644
--- a/ql/lib/semmle/go/frameworks/Logrus.qll
+++ b/ql/lib/semmle/go/frameworks/Logrus.qll
@@ -1,6 +1,7 @@
 /** Provides models of commonly used functions in the `github.com/sirupsen/logrus` package. */
 
 import go
+private import semmle.go.StringOps
 
 /** Provides models of commonly used functions in the `github.com/sirupsen/logrus` package. */
 module Logrus {
@@ -22,14 +23,31 @@ module Logrus {
     result.regexpMatch("With(Context|Error|Fields?|Time)")
   }
 
-  private class LogCall extends LoggerCall::Range, DataFlow::CallNode {
-    LogCall() {
+  private class LogFunction extends Function {
+    LogFunction() {
       exists(string name | name = getALogResultName() or name = getAnEntryUpdatingMethodName() |
-        this.getTarget().hasQualifiedName(packagePath(), name) or
-        this.getTarget().(Method).hasQualifiedName(packagePath(), ["Entry", "Logger"], name)
+        this.hasQualifiedName(packagePath(), name) or
+        this.(Method).hasQualifiedName(packagePath(), ["Entry", "Logger"], name)
       )
     }
+  }
+
+  private class LogCall extends LoggerCall::Range, DataFlow::CallNode {
+    LogCall() { this = any(LogFunction f).getACall() }
 
     override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
+
+  private class StringFormatters extends StringOps::Formatting::Range instanceof LogFunction {
+    int argOffset;
+
+    StringFormatters() {
+      this.getName().matches("%f") and
+      if this.getName() = "Logf" then argOffset = 1 else argOffset = 0
+    }
+
+    override int getFormatStringIndex() { result = argOffset }
+
+    override int getFirstFormattedParameterIndex() { result = argOffset + 1 }
+  }
 }
diff --git a/ql/lib/semmle/go/frameworks/Spew.qll b/ql/lib/semmle/go/frameworks/Spew.qll
index ec722a1e86c..92c96d27c73 100644
--- a/ql/lib/semmle/go/frameworks/Spew.qll
+++ b/ql/lib/semmle/go/frameworks/Spew.qll
@@ -3,6 +3,7 @@
  */
 
 import go
+private import semmle.go.StringOps
 
 /**
  * Provides models of commonly used functions in the `github.com/davecgh/go-spew/spew` package.
@@ -11,21 +12,37 @@ module Spew {
   /** Gets the package path `github.com/davecgh/go-spew/spew`. */
   private string packagePath() { result = package("github.com/davecgh/go-spew", "spew") }
 
-  private class SpewCall extends LoggerCall::Range, DataFlow::CallNode {
+  private class SpewFunction extends Function {
     int firstPrintedArg;
 
-    SpewCall() {
+    SpewFunction() {
       exists(string fn |
         fn in ["Dump", "Errorf", "Print", "Printf", "Println"] and firstPrintedArg = 0
         or
         fn in ["Fdump", "Fprint", "Fprintf", "Fprintln"] and firstPrintedArg = 1
       |
-        this.getTarget().hasQualifiedName(packagePath(), fn)
+        this.hasQualifiedName(packagePath(), fn)
       )
     }
 
+    int getFirstPrintedArg() { result = firstPrintedArg }
+  }
+
+  private class StringFormatter extends StringOps::Formatting::Range instanceof SpewFunction {
+    StringFormatter() { this.getName().matches("%f") }
+
+    override int getFormatStringIndex() { result = super.getFirstPrintedArg() }
+
+    override int getFirstFormattedParameterIndex() { result = super.getFirstPrintedArg() + 1 }
+  }
+
+  private class SpewCall extends LoggerCall::Range, DataFlow::CallNode {
+    SpewFunction target;
+
+    SpewCall() { this = target.getACall() }
+
     override DataFlow::Node getAMessageComponent() {
-      result = this.getArgument(any(int i | i >= firstPrintedArg))
+      result = this.getArgument(any(int i | i >= target.getFirstPrintedArg()))
     }
   }
 
diff --git a/ql/lib/semmle/go/frameworks/Zap.qll b/ql/lib/semmle/go/frameworks/Zap.qll
index c66e120e6ee..75366c18962 100644
--- a/ql/lib/semmle/go/frameworks/Zap.qll
+++ b/ql/lib/semmle/go/frameworks/Zap.qll
@@ -3,6 +3,7 @@
  */
 
 import go
+private import semmle.go.StringOps
 
 /**
  * Provides models of commonly used functions in the `go.uber.org/zap` package.
@@ -14,6 +15,28 @@ module Zap {
   /** Gets a suffix for a method on `zap.SugaredLogger`. */
   private string getSuffix() { result in ["", "f", "w"] }
 
+  private class ZapFunction extends Method {
+    ZapFunction() {
+      exists(string fn | fn in ["DPanic", "Debug", "Error", "Fatal", "Info", "Panic", "Warn"] |
+        this.hasQualifiedName(packagePath(), "Logger", fn)
+        or
+        this.hasQualifiedName(packagePath(), "SugaredLogger", fn + getSuffix())
+      )
+      or
+      this.hasQualifiedName(packagePath(), "Logger", ["Named", "With", "WithOptions"])
+      or
+      this.hasQualifiedName(packagePath(), "SugaredLogger", ["Named", "With"])
+    }
+  }
+
+  private class ZapFormatter extends StringOps::Formatting::Range instanceof ZapFunction {
+    ZapFormatter() { this.getName().matches("%f") }
+
+    override int getFormatStringIndex() { result = 0 }
+
+    override int getFirstFormattedParameterIndex() { result = 1 }
+  }
+
   /**
    * A call to a logger function in Zap.
    *
@@ -21,17 +44,7 @@ module Zap {
    * function is called are included.
    */
   private class ZapCall extends LoggerCall::Range, DataFlow::MethodCallNode {
-    ZapCall() {
-      exists(string fn | fn in ["DPanic", "Debug", "Error", "Fatal", "Info", "Panic", "Warn"] |
-        this.getTarget().hasQualifiedName(packagePath(), "Logger", fn)
-        or
-        this.getTarget().hasQualifiedName(packagePath(), "SugaredLogger", fn + getSuffix())
-      )
-      or
-      this.getTarget().hasQualifiedName(packagePath(), "Logger", ["Named", "With", "WithOptions"])
-      or
-      this.getTarget().hasQualifiedName(packagePath(), "SugaredLogger", ["Named", "With"])
-    }
+    ZapCall() { this = any(ZapFunction f).getACall() }
 
     override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
diff --git a/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll b/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
index 171c4d0ce76..0f4dc079d79 100644
--- a/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
+++ b/ql/lib/semmle/go/frameworks/stdlib/Fmt.qll
@@ -3,6 +3,7 @@
  */
 
 import go
+private import semmle.go.StringOps
 
 /** Provides models of commonly used functions in the `fmt` package. */
 module Fmt {
@@ -54,6 +55,25 @@ module Fmt {
     }
   }
 
+  private class FmtStringFormatter extends StringOps::Formatting::Range {
+    int argOffset;
+
+    FmtStringFormatter() {
+      exists(string fname |
+        this.hasQualifiedName("fmt", fname) and
+        (
+          fname = ["Printf", "Sprintf"] and argOffset = 0
+          or
+          fname = "Fprintf" and argOffset = 1
+        )
+      )
+    }
+
+    override int getFormatStringIndex() { result = argOffset }
+
+    override int getFirstFormattedParameterIndex() { result = argOffset + 1 }
+  }
+
   /** The `Sscan` function or one of its variants. */
   private class Sscanner extends TaintTracking::FunctionModel {
     FunctionInput inp;
diff --git a/ql/lib/semmle/go/frameworks/stdlib/Log.qll b/ql/lib/semmle/go/frameworks/stdlib/Log.qll
index 11e1814c3ab..5cd81a1997d 100644
--- a/ql/lib/semmle/go/frameworks/stdlib/Log.qll
+++ b/ql/lib/semmle/go/frameworks/stdlib/Log.qll
@@ -3,17 +3,30 @@
  */
 
 import go
+private import semmle.go.StringOps
 
 /** Provides models of commonly used functions in the `log` package. */
 module Log {
-  private class LogCall extends LoggerCall::Range, DataFlow::CallNode {
-    LogCall() {
+  private class LogFunction extends Function {
+    LogFunction() {
       exists(string fn | fn.matches(["Fatal%", "Panic%", "Print%"]) |
-        this.getTarget().hasQualifiedName("log", fn)
+        this.hasQualifiedName("log", fn)
         or
-        this.getTarget().(Method).hasQualifiedName("log", "Logger", fn)
+        this.(Method).hasQualifiedName("log", "Logger", fn)
       )
     }
+  }
+
+  private class ZapFormatter extends StringOps::Formatting::Range instanceof LogFunction {
+    ZapFormatter() { this.getName().matches("%f") }
+
+    override int getFormatStringIndex() { result = 0 }
+
+    override int getFirstFormattedParameterIndex() { result = 1 }
+  }
+
+  private class LogCall extends LoggerCall::Range, DataFlow::CallNode {
+    LogCall() { this = any(LogFunction f).getACall() }
 
     override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
   }
diff --git a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll
index 091d8a65b8e..af0d07f288b 100644
--- a/ql/lib/semmle/go/security/LogInjectionCustomizations.qll
+++ b/ql/lib/semmle/go/security/LogInjectionCustomizations.qll
@@ -4,6 +4,7 @@
  */
 
 import go
+private import semmle.go.StringOps
 
 /**
  * Provides extension points for customizing the data-flow tracking configuration for reasoning
@@ -58,4 +59,20 @@ module LogInjection {
       )
     }
   }
+
+  /**
+   * An argument that is formatted using the `%q` directive, considered as a sanitizer
+   * for log injection.
+   *
+   * This formatting directive replaces newline characters with escape sequences.
+   */
+  private class SafeFormatArgumentSanitizer extends Sanitizer {
+    SafeFormatArgumentSanitizer() {
+      exists(StringOps::Formatting::StringFormatCall call, string safeDirective |
+        this = call.getOperand(_, safeDirective) and
+        // Mark "%q" formats as safe, but not "%#q", which would preserve newline characters.
+        safeDirective.regexpMatch("%[^%#]*q")
+      )
+    }
+  }
 }
diff --git a/ql/test/query-tests/Security/CWE-117/LogInjection.go b/ql/test/query-tests/Security/CWE-117/LogInjection.go
index f386d2fb910..b6dc9724759 100644
--- a/ql/test/query-tests/Security/CWE-117/LogInjection.go
+++ b/ql/test/query-tests/Security/CWE-117/LogInjection.go
@@ -377,3 +377,190 @@ func handlerGood2(req *http.Request) {
 	escapedUsername = strings.ReplaceAll(escapedUsername, "\r", "")
 	log.Printf("user %s logged in.\n", escapedUsername)
 }
+
+// GOOD: User-provided values formatted using a %q directive, which escapes newlines
+func handlerGood3(req *http.Request, ctx *goproxy.ProxyCtx) {
+	username := req.URL.Query()["username"][0]
+	testFlag := req.URL.Query()["testFlag"][0]
+	log.Printf("user %q logged in.\n", username)
+	// Flags shouldn't make a difference...
+	log.Printf("user %-50q logged in.\n", username)
+	// Except for the '#' flag that retains newlines, emitting a backtick-delimited string:
+	log.Printf("user %#10q logged in.\n", username) // $ hasTaintFlow="username"
+
+	// Check this works with fmt:
+	log.Print(fmt.Sprintf("user %q logged in.\n", username))
+	log.Print(fmt.Sprintf("user %-50q logged in.\n", username))
+	log.Print(fmt.Sprintf("user %#10q logged in.\n", username)) // $ hasTaintFlow="call to Sprintf"
+
+	// Check this works with a variety of other loggers:
+	// k8s.io/klog
+	{
+		verbose := klog.V(0)
+		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)
+	}
+	// elazarl/goproxy
+	{
+		ctx.Logf("user %q logged in.\n", username)
+		ctx.Warnf("user %q logged in.\n", username)
+	}
+	// golang/glog
+	{
+		verbose := glog.V(0)
+		verbose.Infof("user %q logged in.\n", username)
+
+		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)
+	}
+	// 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)
+		logrus.Infof("user %q logged in.\n", username)
+		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)
+		logrus.Warningf("user %q logged in.\n", username)
+
+		fields := make(logrus.Fields)
+		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)
+		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)
+		entry.Printf("user %q logged in.\n", username)
+		entry.Tracef("user %q logged in.\n", username)
+		entry.Warnf("user %q logged in.\n", username)
+		entry.Warningf("user %q logged in.\n", username)
+
+		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)
+		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)
+		logger.Printf("user %q logged in.\n", username)
+		logger.Tracef("user %q logged in.\n", username)
+		logger.Warnf("user %q logged in.\n", username)
+		logger.Warningf("user %q logged in.\n", username)
+	}
+	// davecgh/go-spew/spew
+	{
+		spew.Errorf("user %q logged in.\n", username)
+		spew.Printf("user %q logged in.\n", username)
+		spew.Fprintf(nil, "user %q logged in.\n", username)
+	}
+	// zap
+	{
+		logger, _ := zap.NewProduction()
+		sLogger := logger.Sugar()
+		sLogger.DPanicf("user %q logged in.\n", username)
+		sLogger.Debugf("user %q logged in.\n", username)
+		sLogger.Errorf("user %q logged in.\n", username)
+		if testFlag == " true" {
+			sLogger.Fatalf("user %q logged in.\n", username)
+		}
+		sLogger.Infof("user %q logged in.\n", username)
+		if testFlag == " true" {
+			sLogger.Panicf("user %q logged in.\n", username)
+		}
+		sLogger.Warnf("user %q logged in.\n", username)
+	}
+
+	// Check those same loggers recognise that %#q is still dangerous:
+	// k8s.io/klog
+	{
+		verbose := klog.V(0)
+		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"
+	}
+	// elazarl/goproxy
+	{
+		ctx.Logf("user %#q logged in.\n", username)  // $ hasTaintFlow="username"
+		ctx.Warnf("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+	}
+	// golang/glog
+	{
+		verbose := glog.V(0)
+		verbose.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+
+		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"
+	}
+	// 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.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"
+		logrus.Warningf("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+
+		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.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.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"
+		logger.Warningf("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+	}
+	// davecgh/go-spew/spew
+	{
+		spew.Errorf("user %#q logged in.\n", username)       // $ hasTaintFlow="username"
+		spew.Printf("user %#q logged in.\n", username)       // $ hasTaintFlow="username"
+		spew.Fprintf(nil, "user %#q logged in.\n", username) // $ hasTaintFlow="username"
+	}
+	// zap
+	{
+		logger, _ := zap.NewProduction()
+		sLogger := logger.Sugar()
+		sLogger.DPanicf("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+		sLogger.Debugf("user %#q logged in.\n", username)  // $ hasTaintFlow="username"
+		sLogger.Errorf("user %#q logged in.\n", username)  // $ hasTaintFlow="username"
+		if testFlag == " true" {
+			sLogger.Fatalf("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+		}
+		sLogger.Infof("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+		if testFlag == " true" {
+			sLogger.Panicf("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+		}
+		sLogger.Warnf("user %#q logged in.\n", username) // $ hasTaintFlow="username"
+	}
+
+}

From e0a3ec85f32de3f9e836ec15a58b4674c0494d10 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Wed, 5 Jan 2022 16:31:31 +0000
Subject: [PATCH 3/5] Path transformer: use fully resolved path

This makes source locations consistent between databases that do and don't use the `SEMMLE_PATH_TRANSFORMER` option in the case where the original source location isn't its own realpath (i.e, some parent directory is a symbolic link).
---
 extractor/cli/go-autobuilder/go-autobuilder.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extractor/cli/go-autobuilder/go-autobuilder.go b/extractor/cli/go-autobuilder/go-autobuilder.go
index d2306191370..daaf733f88a 100644
--- a/extractor/cli/go-autobuilder/go-autobuilder.go
+++ b/extractor/cli/go-autobuilder/go-autobuilder.go
@@ -374,7 +374,7 @@ func main() {
 			log.Fatalf("Unable to create path transformer file: %s.", err.Error())
 		}
 		defer os.Remove(pt.Name())
-		_, err = pt.WriteString("#" + srcdir + "\n" + newdir + "//\n")
+		_, err = pt.WriteString("#" + realSrc + "\n" + newdir + "//\n")
 		if err != nil {
 			log.Fatalf("Unable to write path transformer file: %s.", err.Error())
 		}

From ae5eadef2876abf1718c9d6b66dd69d579f393f7 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Mon, 10 Jan 2022 10:24:30 +0000
Subject: [PATCH 4/5] Update ql/lib/semmle/go/frameworks/stdlib/Log.qll

Rename class

Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
---
 ql/lib/semmle/go/frameworks/stdlib/Log.qll | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ql/lib/semmle/go/frameworks/stdlib/Log.qll b/ql/lib/semmle/go/frameworks/stdlib/Log.qll
index 5cd81a1997d..54b1612e6eb 100644
--- a/ql/lib/semmle/go/frameworks/stdlib/Log.qll
+++ b/ql/lib/semmle/go/frameworks/stdlib/Log.qll
@@ -17,8 +17,8 @@ module Log {
     }
   }
 
-  private class ZapFormatter extends StringOps::Formatting::Range instanceof LogFunction {
-    ZapFormatter() { this.getName().matches("%f") }
+  private class LogFormatter extends StringOps::Formatting::Range instanceof LogFunction {
+    LogFormatter() { this.getName().matches("%f") }
 
     override int getFormatStringIndex() { result = 0 }
 

From 6f598a69723abf2bce1c85cf6989348efdf99e07 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Mon, 10 Jan 2022 10:49:12 +0000
Subject: [PATCH 5/5] Fix formatting regex comment

---
 ql/lib/semmle/go/StringOps.qll | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ql/lib/semmle/go/StringOps.qll b/ql/lib/semmle/go/StringOps.qll
index 130d7d83b3f..70f83328c05 100644
--- a/ql/lib/semmle/go/StringOps.qll
+++ b/ql/lib/semmle/go/StringOps.qll
@@ -170,8 +170,7 @@ module StringOps {
   module Formatting {
     /**
      * Gets a regular expression for matching simple format-string components, including flags,
-     * width and precision specifiers, but not including `*` specifiers or explicit argument
-     * indices.
+     * width and precision specifiers, not including explicit argument indices.
      */
     pragma[noinline]
     private string getFormatComponentRegex() {