Merge pull request #229 from max/string-break

Add query to find unsafe quoting

change-notes/1.24/analysis-go.md

@@ -14,6 +14,7 @@
 | Constant length comparison (`go/constant-length-comparison`)     | correctness | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. |
 | Impossible interface nil check (`go/impossible-interface-nil-check`) | correctness | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default. |
 | Incomplete URL scheme check (`go/incomplete-url-scheme-check`) | correctness, security, external/cwe/cwe-020 | Highlights checks for `javascript` URLs that do not take `data` or `vbscript` URLs into account. Results are shown on LGTM by default. |
+| Potentially unsafe quoting (`go/unsafe-quoting`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-089, external/cwe/cwe-094 | Highlights code that constructs a quoted string literal containing data that may itself contain quotes. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

ql/src/Security/CWE-089/StringBreak.go

@@ -0,0 +1,16 @@
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+	sq "github.com/Masterminds/squirrel"
+)
+
+func save(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr(fmt.Sprintf("md5('%s')", versionJSON))).
+		Exec()
+}

ql/src/Security/CWE-089/StringBreak.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Code that constructs a string containing a quoted substring needs to ensure that any user-provided
+data embedded in between the quotes does not itself contain a quote. Otherwise the embedded data
+could (accidentally or intentionally) change the structure of the overall string by terminating
+the quoted substring early, with potentially severe consequences. If, for example, the string is
+later interpreted as an operating-system command or database query, a malicious attacker may be
+able to craft input data that enables a command injection or SQL injection attack.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Sanitize the embedded data appropriately to ensure quotes are escaped, or use an API that does
+not rely on manually constructing quoted substrings.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following example, assume that <code>version</code> is an object from an untrusted source.
+The code snippet first uses <code>json.Marshal</code> to serialize this object into a string, and
+then embeds it into a SQL query built using the Squirrel library.
+</p>
+<sample src="StringBreak.go"/>
+<p>
+Note that while Squirrel provides a structured API for building SQL queries that mitigates against
+common causes of SQL injection vulnerabilities, this code is still vulnerable: if the JSON-encoded
+representation of <code>version</code> contains a single quote, this will prematurely close the
+surrounding string, changing the structure of the SQL expression being constructed. This could be
+exploited to mount a SQL injection attack.
+</p>
+<p>
+To fix this vulnerability, use Squirrel's placeholder syntax, which avoids the need to explicitly
+construct a quoted string.
+</p>
+<sample src="StringBreakGood.go"/>
+</example>
+
+<references>
+<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li>
+<li>OWASP: <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.</li>
+</references>
+</qhelp>

ql/src/Security/CWE-089/StringBreak.ql

@@ -0,0 +1,25 @@
+/**
+ * @name Potentially unsafe quoting
+ * @description If a quoted string literal is constructed from data that may itself contain quotes,
+ *              the embedded data could (accidentally or intentionally) change the structure of
+ *              the overall string.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id go/unsafe-quoting
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-089
+ *       external/cwe/cwe-094
+ */
+
+import go
+import semmle.go.security.StringBreak
+import DataFlow::PathGraph
+
+from StringBreak::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "If this $@ contains a " + cfg.getQuote().getType() + " quote, it could break out of " +
+    "the enclosing quotes.", source.getNode(), "JSON value"

ql/src/Security/CWE-089/StringBreakGood.go

@@ -0,0 +1,15 @@
+package main
+
+import (
+	"encoding/json"
+	sq "github.com/Masterminds/squirrel"
+)
+
+func saveGood(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr("md5(?)", versionJSON)).
+		Exec()
+}

ql/src/semmle/go/StringOps.qll

@@ -4,60 +4,6 @@
 
 import go
 
-module StringConcatenation {
-  /** Gets the `n`th operand to the string concatenation defining `node`. */
-  DataFlow::Node getOperand(DataFlow::Node node, int n) {
-    node.getType() instanceof StringType and
-    exists(DataFlow::BinaryOperationNode add | add = node and add.getOperator() = "+" |
-      n = 0 and result = add.getLeftOperand()
-      or
-      n = 1 and result = add.getRightOperand()
-    )
-  }
-
-  /** Gets an operand to the string concatenation defining `node`. */
-  DataFlow::Node getAnOperand(DataFlow::Node node) { result = getOperand(node, _) }
-
-  /** Gets the number of operands to the given concatenation. */
-  int getNumOperand(DataFlow::Node node) { result = strictcount(getAnOperand(node)) }
-
-  /** Gets the first operand to the string concatenation defining `node`. */
-  DataFlow::Node getFirstOperand(DataFlow::Node node) { result = getOperand(node, 0) }
-
-  /** Gets the last operand to the string concatenation defining `node`. */
-  DataFlow::Node getLastOperand(DataFlow::Node node) {
-    result = getOperand(node, getNumOperand(node) - 1)
-  }
-
-  /**
-   * Holds if `src` flows to `dst` through the `n`th operand of the given concatenation operator.
-   */
-  predicate taintStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::Node operator, int n) {
-    src = getOperand(dst, n) and
-    operator = dst
-  }
-
-  /**
-   * Holds if there is a taint step from `src` to `dst` through string concatenation.
-   */
-  predicate taintStep(DataFlow::Node src, DataFlow::Node dst) { taintStep(src, dst, _, _) }
-
-  /**
-   * Holds if `node` is the root of a concatenation tree, that is,
-   * it is a concatenation operator that is not itself the immediate operand to
-   * another concatenation operator.
-   */
-  predicate isRoot(DataFlow::Node node) {
-    exists(getAnOperand(node)) and
-    not node = getAnOperand(_)
-  }
-
-  /**
-   * Gets the root of the concatenation tree in which `node` is an operand or operator.
-   */
-  DataFlow::Node getRoot(DataFlow::Node node) { isRoot(result) and node = getAnOperand*(result) }
-}
-
 module StringOps {
   /**
    * An expression that is equivalent to `strings.HasPrefix(A, B)` or `!strings.HasPrefix(A, B)`.
@@ -196,4 +142,323 @@ module StringOps {
       override boolean getPolarity() { result = expr.getPolarity() }
     }
   }
+
+  /**
+   * A data-flow node that performs string concatenation.
+   *
+   * Extend this class to refine existing API models. If you want to model new APIs,
+   * extend `StringOps::Concatenation::Range` instead.
+   */
+  class Concatenation extends DataFlow::Node {
+    Concatenation::Range self;
+
+    Concatenation() { this = self }
+
+    /**
+     * Gets the `n`th operand of this string concatenation, if there is a data-flow node for it.
+     */
+    DataFlow::Node getOperand(int n) { result = self.getOperand(n) }
+
+    /**
+     * Gets the string value of the `n`th operand of this string concatenation, if it is a constant.
+     */
+    string getOperandStringValue(int n) { result = self.getOperandStringValue(n) }
+
+    /**
+     * Gets the number of operands of this string concatenation.
+     */
+    int getNumOperand() { result = self.getNumOperand() }
+  }
+
+  module Concatenation {
+    /**
+     * A data-flow node that performs string concatenation.
+     *
+     * Extend this class to model new APIs. If you want to refine existing API models,
+     * extend `StringOps::Concatenation` instead.
+     */
+    abstract class Range extends DataFlow::Node {
+      /**
+       * Gets the `n`th operand of this string concatenation, if there is a data-flow node for it.
+       */
+      abstract DataFlow::Node getOperand(int n);
+
+      /**
+       * Gets the string value of the `n`th operand of this string concatenation, if it is
+       * a constant.
+       */
+      string getOperandStringValue(int n) { result = getOperand(n).getStringValue() }
+
+      /**
+       * Gets the number of operands of this string concatenation.
+       */
+      int getNumOperand() { result = count(getOperand(_)) }
+    }
+
+    /** A string concatenation using the `+` or `+=` operator. */
+    private class PlusConcat extends Range, DataFlow::BinaryOperationNode {
+      PlusConcat() {
+        getType() instanceof StringType and
+        getOperator() = "+"
+      }
+
+      override DataFlow::Node getOperand(int n) {
+        n = 0 and result = getLeftOperand()
+        or
+        n = 1 and result = getRightOperand()
+      }
+    }
+
+    /**
+     * Gets a regular expression for matching simple format-string components, including flags,
+     * width and precision specifiers, but not including `*` specifiers or explicit argument
+     * indices.
+     */
+    private string getFormatComponentRegex() {
+      exists(string literal, string opt_flag, string opt_width, string operator, string verb |
+        literal = "([^%]|%%)+" and
+        opt_flag = "[-+ #0]?" and
+        opt_width = "((\\d*|\\*)(\\.(\\d*|\\*))?)?" and
+        operator = "[bcdeEfFgGoOpqstTxXUv]" and
+        verb = "(%" + opt_flag + opt_width + operator + ")"
+      |
+        result = "(" + literal + "|" + verb + ")"
+      )
+    }
+
+    /**
+     * A call to `fmt.Sprintf`, considered as a string concatenation.
+     *
+     * Only calls with simple format strings (no `*` specifiers, no explicit argument indices)
+     * are supported. Such format strings can be viewed as sequences of alternating literal and
+     * non-literal components. A literal component contains no `%` characters except `%%` pairs,
+     * while a non-literal component consists of `%`, a verb, and possibly flags and specifiers.
+     * Each non-literal component consumes exactly one argument.
+     *
+     * Literal components give rise to concatenation operands that have a string value but no
+     * data-flow node; non-literal `%s` or `%v` components give rise to concatenation operands
+     * that do have an associated data-flow node but possibly no string value; any other non-literal
+     * components give rise to concatenation operands that have neither an associated data-flow
+     * 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 = 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, _)
+      }
+
+      override DataFlow::Node getOperand(int n) {
+        exists(int i, string part | part = "%s" or part = "%v" |
+          part = getComponent(n) and
+          i = n / 2 and
+          result = getArgument(i + 1)
+        )
+      }
+
+      override string getOperandStringValue(int n) {
+        result = Range.super.getOperandStringValue(n)
+        or
+        exists(string cmp | cmp = getComponent(n) |
+          (cmp.charAt(0) != "%" or cmp.charAt(1) = "%") and
+          result = cmp.replaceAll("%%", "%")
+        )
+      }
+
+      override int getNumOperand() { result = max(int i | exists(getComponent(i))) + 1 }
+    }
+
+    /**
+     * Holds if `src` flows to `dst` through the `n`th operand of the given concatenation operator.
+     */
+    predicate taintStep(DataFlow::Node src, DataFlow::Node dst, Concatenation cat, int n) {
+      src = cat.getOperand(n) and
+      dst = cat
+    }
+
+    /**
+     * Holds if there is a taint step from `src` to `dst` through string concatenation.
+     */
+    predicate taintStep(DataFlow::Node src, DataFlow::Node dst) { taintStep(src, dst, _, _) }
+  }
+
+  newtype TConcatenationElement =
+    /** A root concatenation element that is not itself an operand of a string concatenation. */
+    MkConcatenationRoot(Concatenation cat) { not cat = any(Concatenation parent).getOperand(_) } or
+    /** A concatenation element that is an operand of a string concatenation. */
+    MkConcatenationOperand(Concatenation parent, int i) { i in [0 .. parent.getNumOperand() - 1] }
+
+  /**
+   * An element of a string concatenation, which either itself performs a string concatenation or
+   * occurs as an operand in a string concatenation.
+   *
+   * For example, the expression `x + y + z` contains the following concatenation
+   * elements:
+   *
+   * - The leaf elements `x`, `y`, and `z`
+   * - The intermediate element `x + y`, which is both a concatenation and an operand
+   * - The root element `x + y + z`
+   */
+  class ConcatenationElement extends TConcatenationElement {
+    /**
+     * Gets the data-flow node corresponding to this concatenation element, if any.
+     */
+    DataFlow::Node asNode() {
+      this = MkConcatenationRoot(result)
+      or
+      exists(Concatenation parent, int i | this = MkConcatenationOperand(parent, i) |
+        result = parent.getOperand(i)
+      )
+    }
+
+    /**
+     * Gets the string value of this concatenation element if it is a constant.
+     */
+    string getStringValue() {
+      result = asNode().getStringValue()
+      or
+      exists(Concatenation parent, int i | this = MkConcatenationOperand(parent, i) |
+        result = parent.getOperandStringValue(i)
+      )
+    }
+
+    /**
+     * Gets the `n`th operand of this string concatenation.
+     */
+    ConcatenationOperand getOperand(int n) { result = MkConcatenationOperand(asNode(), n) }
+
+    /**
+     * Gets an operand of this string concatenation.
+     */
+    ConcatenationOperand getAnOperand() { result = this.getOperand(_) }
+
+    /**
+     * Gets the number of operands of this string concatenation.
+     */
+    int getNumOperand() { result = count(this.getAnOperand()) }
+
+    /**
+     * Gets the first operand of this string concatenation.
+     *
+     * For example, the first operand of `(x + y) + z` is `(x + y)`.
+     */
+    ConcatenationOperand getFirstOperand() { result = getOperand(0) }
+
+    /**
+     * Gets the last operand of this string concatenation.
+     *
+     * For example, the last operand of `x + (y + z)` is `(y + z)`.
+     */
+    ConcatenationOperand getLastOperand() { result = getOperand(getNumOperand() - 1) }
+
+    /**
+     * Gets the root of the concatenation tree to which this element belongs.
+     */
+    ConcatenationRoot getConcatenationRoot() { this = result.getAnOperand*() }
+
+    /**
+     * Gets a leaf in the concatenation tree that this element is the root of.
+     */
+    ConcatenationLeaf getALeaf() { result = this.getAnOperand*() }
+
+    /**
+     * Gets the first leaf in this concatenation tree.
+     *
+     * For example, the first leaf of `(x + y) + z` is `x`.
+     */
+    ConcatenationLeaf getFirstLeaf() { result = getFirstOperand*() }
+
+    /**
+     * Gets the last leaf in this concatenation tree.
+     *
+     * For example, the last leaf of `x + (y + z)` is `z`.
+     */
+    ConcatenationLeaf getLastLeaf() { result = getLastOperand*() }
+
+    /** Gets a textual representation of this concatenation element. */
+    string toString() {
+      if exists(asNode())
+      then result = asNode().toString()
+      else
+        if exists(getStringValue())
+        then result = getStringValue()
+        else result = "concatenation element"
+    }
+
+    /**
+     * Holds if this element is at the specified location.
+     * The location spans column `startcolumn` of line `startline` to
+     * column `endcolumn` of line `endline` in file `filepath`.
+     * For more information, see
+     * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+     */
+    predicate hasLocationInfo(
+      string filepath, int startline, int startcolumn, int endline, int endcolumn
+    ) {
+      asNode().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+      or
+      // use dummy location for elements that don't have a corresponding node
+      not exists(asNode()) and
+      filepath = "" and
+      startline = 0 and
+      startcolumn = 0 and
+      endline = 0 and
+      endcolumn = 0
+    }
+  }
+
+  /**
+   * One of the operands in a string concatenation.
+   *
+   * See `ConcatenationElement` for more information.
+   */
+  class ConcatenationOperand extends ConcatenationElement, MkConcatenationOperand { }
+
+  /**
+   * A data-flow node that performs a string concatenation, and is not an
+   * immediate operand in a larger string concatenation.
+   *
+   * See `ConcatenationElement` for more information.
+   */
+  class ConcatenationRoot extends ConcatenationElement, MkConcatenationRoot { }
+
+  /**
+   * An operand to a concatenation that is not itself a concatenation.
+   *
+   * See `ConcatenationElement` for more information.
+   */
+  class ConcatenationLeaf extends ConcatenationOperand {
+    ConcatenationLeaf() { not exists(getAnOperand()) }
+
+    /**
+     * Gets the operand immediately preceding this one in its parent concatenation.
+     *
+     * For example, in `(x + y) + z`, the previous leaf for `z` is `y`.
+     */
+    ConcatenationLeaf getPreviousLeaf() {
+      exists(ConcatenationElement parent, int i |
+        result = parent.getOperand(i - 1).getLastLeaf() and
+        this = parent.getOperand(i).getFirstLeaf()
+      )
+    }
+
+    /**
+     * Gets the operand immediately succeeding this one in its parent concatenation.
+     *
+     * For example, in `(x + y) + z`, the previous leaf for `y` is `z`.
+     */
+    ConcatenationLeaf getNextLeaf() { this = result.getPreviousLeaf() }
+  }
 }

ql/src/semmle/go/security/StringBreak.qll

@@ -0,0 +1,31 @@
+/**
+ * Provides a taint tracking configuration for reasoning about unsafe-quoting vulnerabilities.
+ *
+ * Note: for performance reasons, only import this file if `StringBreak::Configuration` is needed,
+ * otherwise `StringBreakCustomizations` should be imported instead.
+ */
+
+import go
+
+module StringBreak {
+  import StringBreakCustomizations::StringBreak
+
+  /**
+   * A taint-tracking configuration for reasoning about unsafe-quoting vulnerabilities,
+   * parameterized with the type of quote being tracked.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Quote quote;
+
+    Configuration() { this = "StringBreak" + quote }
+
+    /** Gets the type of quote being tracked by this configuration. */
+    Quote getQuote() { result = quote }
+
+    override predicate isSource(DataFlow::Node nd) { nd instanceof Source }
+
+    override predicate isSink(DataFlow::Node nd) { quote = nd.(Sink).getQuote() }
+
+    override predicate isSanitizer(DataFlow::Node nd) { quote = nd.(Sanitizer).getQuote() }
+  }
+}

ql/src/semmle/go/security/StringBreakCustomizations.qll

@@ -0,0 +1,97 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about unsafe-quoting
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+
+import go
+
+module StringBreak {
+  /** A (single or double) quote. */
+  class Quote extends string {
+    Quote() { this = "'" or this = "\"" }
+
+    /** Gets the type of this quote, either single or double. */
+    string getType() { if this = "'" then result = "single" else result = "double" }
+  }
+
+  /**
+   * A data flow source for unsafe-quoting vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for unsafe-quoting vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /** Gets the quote character for which this is a sink. */
+    abstract Quote getQuote();
+  }
+
+  /**
+   * A sanitizer for unsafe-quoting vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node {
+    /** Gets the quote character for which this is a sanitizer. */
+    Quote getQuote() { any() }
+  }
+
+  /**
+   * A sanitizer guard for unsafe-quoting vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /** Holds if `l` contains a `quote` (either single or double). */
+  private predicate containsQuote(StringOps::ConcatenationLeaf l, Quote quote) {
+    quote = l.getStringValue().regexpFind("['\"]", _, _)
+  }
+
+  /** A call to `json.Marshal`, considered as a taint source for unsafe quoting. */
+  class JsonMarshalAsSource extends Source {
+    JsonMarshalAsSource() {
+      exists(Function jsonMarshal | jsonMarshal.hasQualifiedName("encoding/json", "Marshal") |
+        this = jsonMarshal.getACall()
+      )
+    }
+  }
+
+  /** A string concatenation with quotes, considered as a taint sink for unsafe quoting. */
+  class StringConcatenationAsSink extends Sink {
+    Quote quote;
+
+    StringConcatenationAsSink() {
+      exists(StringOps::ConcatenationLeaf lf | lf.asNode() = this |
+        containsQuote(lf.getPreviousLeaf(), quote) and
+        containsQuote(lf.getNextLeaf(), quote)
+      )
+    }
+
+    override Quote getQuote() { result = quote }
+  }
+
+  class UnmarshalSanitizer extends Sanitizer {
+    UnmarshalSanitizer() {
+      exists(Function jsonUnmarshal | jsonUnmarshal.hasQualifiedName("encoding/json", "Unmarshal") |
+        this = jsonUnmarshal.getACall()
+      )
+    }
+  }
+
+  class ReplaceSanitizer extends Sanitizer {
+    Quote quote;
+
+    ReplaceSanitizer() {
+      exists(string name, DataFlow::CallNode call |
+        this = call and
+        call.getTarget().hasQualifiedName("strings", name) and
+        call.getArgument(2).getStringValue().matches("%" + quote + "%")
+      |
+        name = "Replace" and
+        call.getArgument(3).getNumericValue() < 0
+        or
+        name = "ReplaceAll"
+      )
+    }
+
+    override Quote getQuote() { result = quote }
+  }
+}

ql/src/semmle/go/security/UrlConcatenation.qll

@@ -7,12 +7,17 @@
 import go
 
 /**
- * Holds if the given data flow node refers to a string that ends with a slash.
+ * Holds if the string value of `cat` prevents anything appended after it
+ * from affecting the hostname or path of a URL.
+ *
+ * Specifically, this holds if the string contains `?` or `#`.
  */
-private predicate endsWithSlash(DataFlow::Node nd) {
-  nd.getStringValue().matches("%/")
-  or
-  endsWithSlash(StringConcatenation::getLastOperand(nd))
+private predicate concatenationHasSanitizingSubstring(StringOps::ConcatenationElement cat) {
+  exists(StringOps::ConcatenationLeaf lf | lf = cat.getALeaf() |
+    lf.getStringValue().regexpMatch(".*[?#].*")
+    or
+    hasSanitizingSubstring(lf.asNode().getAPredecessor())
+  )
 }
 
 /**
@@ -22,9 +27,9 @@ private predicate endsWithSlash(DataFlow::Node nd) {
  * Specifically, this holds if the string contains `?` or `#`.
  */
 private predicate hasSanitizingSubstring(DataFlow::Node nd) {
-  nd.getStringValue().regexpMatch(".*[?#].*")
-  or
-  hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
+  exists(StringOps::ConcatenationElement cat | nd = cat.asNode() |
+    concatenationHasSanitizingSubstring(cat)
+  )
   or
   hasSanitizingSubstring(nd.getAPredecessor())
 }
@@ -36,9 +41,21 @@ private predicate hasSanitizingSubstring(DataFlow::Node nd) {
  * This is considered as a sanitizing edge for the URL redirection queries.
  */
 predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
-  exists(DataFlow::Node operator, int n |
-    StringConcatenation::taintStep(source, sink, operator, n) and
-    hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
+  exists(StringOps::ConcatenationElement cat, int n |
+    StringOps::Concatenation::taintStep(source, sink, cat.asNode(), n) and
+    concatenationHasSanitizingSubstring(cat.getOperand([0 .. n - 1]))
+  )
+}
+
+/**
+ * Holds if the string value of `cat` prevents anything appended after it
+ * from affecting the hostname of a URL.
+ */
+private predicate concatenationHasHostnameSanitizingSubstring(StringOps::ConcatenationElement cat) {
+  exists(StringOps::ConcatenationLeaf lf | lf = cat.getALeaf() |
+    lf.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*")
+    or
+    hasHostnameSanitizingSubstring(lf.asNode())
   )
 }
 
@@ -56,9 +73,9 @@ predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
  * the `//` separating the (optional) scheme from the hostname.
  */
 predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
-  nd.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*")
-  or
-  hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd))
+  exists(StringOps::ConcatenationElement cat | cat.asNode() = nd |
+    concatenationHasHostnameSanitizingSubstring(cat)
+  )
   or
   hasHostnameSanitizingSubstring(nd.getAPredecessor())
 }
@@ -70,8 +87,8 @@ predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
  * This is considered as a sanitizing edge for the URL redirection queries.
  */
 predicate hostnameSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
-  exists(DataFlow::Node operator, int n |
-    StringConcatenation::taintStep(source, sink, operator, n) and
-    hasHostnameSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
+  exists(StringOps::ConcatenationElement cat, int n |
+    StringOps::Concatenation::taintStep(source, sink, cat.asNode(), n) and
+    concatenationHasHostnameSanitizingSubstring(cat.getOperand([0 .. n - 1]))
   )
 }

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement.expected

@@ -0,0 +1,29 @@
+| file://:0:0:0:0 |  and  |
+| file://:0:0:0:0 | ! |
+| file://:0:0:0:0 | ' |
+| file://:0:0:0:0 | ' |
+| file://:0:0:0:0 | , quoted:  |
+| file://:0:0:0:0 | Here are  |
+| file://:0:0:0:0 | concatenation element |
+| main.go:6:14:6:15 | s1 |
+| main.go:6:14:6:20 | ...+... |
+| main.go:6:14:6:25 | ...+... |
+| main.go:6:19:6:20 | s2 |
+| main.go:6:24:6:25 | s3 |
+| main.go:7:14:7:15 | s1 |
+| main.go:7:14:7:27 | ...+... |
+| main.go:7:20:7:21 | s2 |
+| main.go:7:20:7:26 | ...+... |
+| main.go:7:25:7:26 | s3 |
+| main.go:8:14:8:27 | ...+... |
+| main.go:8:15:8:16 | s1 |
+| main.go:8:15:8:21 | ...+... |
+| main.go:8:20:8:21 | s2 |
+| main.go:8:26:8:27 | s3 |
+| main.go:9:2:9:24 | call to Sprintf |
+| main.go:9:22:9:23 | s1 |
+| main.go:10:2:10:43 | call to Sprintf |
+| main.go:10:37:10:38 | s1 |
+| main.go:10:41:10:42 | s2 |
+| main.go:11:2:11:38 | call to Sprintf |
+| main.go:11:32:11:33 | s1 |

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement.ql

@@ -0,0 +1,4 @@
+import go
+
+from StringOps::ConcatenationElement elt
+select elt

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_asNode.expected

@@ -0,0 +1,22 @@
+| main.go:6:14:6:15 | s1 | main.go:6:14:6:15 | s1 |
+| main.go:6:14:6:20 | ...+... | main.go:6:14:6:20 | ...+... |
+| main.go:6:14:6:25 | ...+... | main.go:6:14:6:25 | ...+... |
+| main.go:6:19:6:20 | s2 | main.go:6:19:6:20 | s2 |
+| main.go:6:24:6:25 | s3 | main.go:6:24:6:25 | s3 |
+| main.go:7:14:7:15 | s1 | main.go:7:14:7:15 | s1 |
+| main.go:7:14:7:27 | ...+... | main.go:7:14:7:27 | ...+... |
+| main.go:7:20:7:21 | s2 | main.go:7:20:7:21 | s2 |
+| main.go:7:20:7:26 | ...+... | main.go:7:20:7:26 | ...+... |
+| main.go:7:25:7:26 | s3 | main.go:7:25:7:26 | s3 |
+| main.go:8:14:8:27 | ...+... | main.go:8:14:8:27 | ...+... |
+| main.go:8:15:8:16 | s1 | main.go:8:15:8:16 | s1 |
+| main.go:8:15:8:21 | ...+... | main.go:8:15:8:21 | ...+... |
+| main.go:8:20:8:21 | s2 | main.go:8:20:8:21 | s2 |
+| main.go:8:26:8:27 | s3 | main.go:8:26:8:27 | s3 |
+| main.go:9:2:9:24 | call to Sprintf | main.go:9:2:9:24 | call to Sprintf |
+| main.go:9:22:9:23 | s1 | main.go:9:22:9:23 | s1 |
+| main.go:10:2:10:43 | call to Sprintf | main.go:10:2:10:43 | call to Sprintf |
+| main.go:10:37:10:38 | s1 | main.go:10:37:10:38 | s1 |
+| main.go:10:41:10:42 | s2 | main.go:10:41:10:42 | s2 |
+| main.go:11:2:11:38 | call to Sprintf | main.go:11:2:11:38 | call to Sprintf |
+| main.go:11:32:11:33 | s1 | main.go:11:32:11:33 | s1 |

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_asNode.ql

@@ -0,0 +1,4 @@
+import go
+
+from StringOps::ConcatenationElement elt
+select elt, elt.asNode()

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getOperand.expected

@@ -0,0 +1,23 @@
+| main.go:6:14:6:20 | ...+... | 0 | main.go:6:14:6:15 | s1 |
+| main.go:6:14:6:20 | ...+... | 1 | main.go:6:19:6:20 | s2 |
+| main.go:6:14:6:25 | ...+... | 0 | main.go:6:14:6:20 | ...+... |
+| main.go:6:14:6:25 | ...+... | 1 | main.go:6:24:6:25 | s3 |
+| main.go:7:14:7:27 | ...+... | 0 | main.go:7:14:7:15 | s1 |
+| main.go:7:14:7:27 | ...+... | 1 | main.go:7:20:7:26 | ...+... |
+| main.go:7:20:7:26 | ...+... | 0 | main.go:7:20:7:21 | s2 |
+| main.go:7:20:7:26 | ...+... | 1 | main.go:7:25:7:26 | s3 |
+| main.go:8:14:8:27 | ...+... | 0 | main.go:8:15:8:21 | ...+... |
+| main.go:8:14:8:27 | ...+... | 1 | main.go:8:26:8:27 | s3 |
+| main.go:8:15:8:21 | ...+... | 0 | main.go:8:15:8:16 | s1 |
+| main.go:8:15:8:21 | ...+... | 1 | main.go:8:20:8:21 | s2 |
+| main.go:9:2:9:24 | call to Sprintf | 0 | file://:0:0:0:0 | ' |
+| main.go:9:2:9:24 | call to Sprintf | 1 | main.go:9:22:9:23 | s1 |
+| main.go:9:2:9:24 | call to Sprintf | 2 | file://:0:0:0:0 | ' |
+| main.go:10:2:10:43 | call to Sprintf | 0 | file://:0:0:0:0 | Here are  |
+| main.go:10:2:10:43 | call to Sprintf | 1 | main.go:10:37:10:38 | s1 |
+| main.go:10:2:10:43 | call to Sprintf | 2 | file://:0:0:0:0 |  and  |
+| main.go:10:2:10:43 | call to Sprintf | 3 | main.go:10:41:10:42 | s2 |
+| main.go:10:2:10:43 | call to Sprintf | 4 | file://:0:0:0:0 | ! |
+| main.go:11:2:11:38 | call to Sprintf | 0 | main.go:11:32:11:33 | s1 |
+| main.go:11:2:11:38 | call to Sprintf | 1 | file://:0:0:0:0 | , quoted:  |
+| main.go:11:2:11:38 | call to Sprintf | 2 | file://:0:0:0:0 | concatenation element |

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getOperand.ql

@@ -0,0 +1,4 @@
+import go
+
+from StringOps::ConcatenationElement elt, int i
+select elt, i, elt.getOperand(i)

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getStringValue.expected

@@ -0,0 +1,6 @@
+| file://:0:0:0:0 |  and  |  and  |
+| file://:0:0:0:0 | ! | ! |
+| file://:0:0:0:0 | ' | ' |
+| file://:0:0:0:0 | ' | ' |
+| file://:0:0:0:0 | , quoted:  | , quoted:  |
+| file://:0:0:0:0 | Here are  | Here are  |

ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getStringValue.ql

@@ -0,0 +1,4 @@
+import go
+
+from StringOps::ConcatenationElement elt
+select elt, elt.getStringValue()

ql/test/library-tests/semmle/go/StringOps/Concatenation/main.go

@@ -0,0 +1,12 @@
+package main
+
+import "fmt"
+
+func test(s1, s2, s3 string, i int) {
+	fmt.Println(s1 + s2 + s3)
+	fmt.Println(s1 + (s2 + s3))
+	fmt.Println((s1 + s2) + s3)
+	fmt.Sprintf("'%s'", s1)
+	fmt.Sprintf("Here are %s and %s!", s1, s2)
+	fmt.Sprintf("%v, quoted: %q", s1, s1)
+}

ql/test/query-tests/Security/CWE-089/StringBreak.expected

@@ -0,0 +1,7 @@
+edges
+| StringBreak.go:10:20:10:40 | call to Marshal : tuple type | StringBreak.go:14:47:14:57 | versionJSON |
+nodes
+| StringBreak.go:10:20:10:40 | call to Marshal : tuple type | semmle.label | call to Marshal : tuple type |
+| StringBreak.go:14:47:14:57 | versionJSON | semmle.label | versionJSON |
+#select
+| StringBreak.go:14:47:14:57 | versionJSON | StringBreak.go:10:20:10:40 | call to Marshal : tuple type | StringBreak.go:14:47:14:57 | versionJSON | If this $@ contains a single quote, it could break out of the enclosing quotes. | StringBreak.go:10:20:10:40 | call to Marshal | JSON value |

ql/test/query-tests/Security/CWE-089/StringBreak.go

@@ -0,0 +1,16 @@
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+	sq "github.com/Masterminds/squirrel"
+)
+
+func save(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr(fmt.Sprintf("md5('%s')", versionJSON))).
+		Exec()
+}

ql/test/query-tests/Security/CWE-089/StringBreak.qlref

@@ -0,0 +1 @@
+Security/CWE-089/StringBreak.ql

ql/test/query-tests/Security/CWE-089/StringBreakGood.go

@@ -0,0 +1,15 @@
+package main
+
+import (
+	"encoding/json"
+	sq "github.com/Masterminds/squirrel"
+)
+
+func saveGood(id string, version interface{}) {
+	versionJSON, _ := json.Marshal(version)
+	sq.StatementBuilder.
+		Insert("resources").
+		Columns("resource_id", "version_md5").
+		Values(id, sq.Expr("md5(?)", versionJSON)).
+		Exec()
+}

ql/test/query-tests/Security/CWE-089/go.mod

@@ -0,0 +1,5 @@
+module Security.CWE-089
+
+go 1.13
+
+require github.com/Masterminds/squirrel v1.1.0

ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/LICENSE

@@ -0,0 +1,23 @@
+Squirrel
+The Masterminds
+Copyright (C) 2014-2015, Lann Martin
+Copyright (C) 2015-2016, Google
+Copyright (C) 2015, Matt Farina and Matt Butcher
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.

ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/README.md

@@ -0,0 +1,3 @@
+This is a simple stub for https://github.com/Masterminds/squirrel, strictly for use in query testing.
+
+See the LICENSE file in this folder for information about the licensing of the original library.

ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/go.mod

@@ -0,0 +1 @@
+module github.com/Masterminds/squirrel

ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/squirrel.go

@@ -0,0 +1,24 @@
+package squirrel
+
+type StatementBuilderType struct{}
+
+func Expr(e string) string {
+	return Expr(e)
+}
+
+var StatementBuilder = &StatementBuilderType{}
+
+func (b *StatementBuilderType) Insert(table string) *StatementBuilderType {
+	return b
+}
+
+func (b *StatementBuilderType) Columns(columns ...string) *StatementBuilderType {
+	return b
+}
+
+func (b *StatementBuilderType) Values(strings ...string) *StatementBuilderType {
+	return b
+}
+
+func (b *StatementBuilderType) Exec() {
+}

ql/test/query-tests/Security/CWE-089/vendor/modules.txt

@@ -0,0 +1,2 @@
+# github.com/Masterminds/squirrel v1.1.0
+github.com/Masterminds/squirrel