Merge pull request #658 from smowton/smowton/feature/q-format-directive-is-safe

Note that the %q format directive escapes newlines, and therefore prevents log injection

Author: smowton

ql/lib/semmle/go/StringOps.qll

@@ -166,6 +166,75 @@ 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, not including 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 +302,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 +318,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
+      }
     }
 
     /**

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() }
   }

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()))
     }
   }
 }

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 }
+  }
 }

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()))
     }
   }
 

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,24 +15,36 @@ 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.
    *
    * Functions which add data to be included the next time a direct logging
    * 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() }
   }