Add query StringBreak.

change-notes/1.24/analysis-go.md

@@ -13,6 +13,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/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/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