From af3d91ffd35943f6b299590c1efaab3f48efb837 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 24 Jan 2020 16:05:19 +0000 Subject: [PATCH] Add query StringBreak. --- change-notes/1.24/analysis-go.md | 1 + ql/src/Security/CWE-089/StringBreak.go | 16 +++ ql/src/Security/CWE-089/StringBreak.qhelp | 49 ++++++++++ ql/src/Security/CWE-089/StringBreak.ql | 25 +++++ ql/src/Security/CWE-089/StringBreakGood.go | 15 +++ ql/src/semmle/go/security/StringBreak.qll | 31 ++++++ .../go/security/StringBreakCustomizations.qll | 97 +++++++++++++++++++ .../Security/CWE-089/StringBreak.expected | 7 ++ .../Security/CWE-089/StringBreak.go | 16 +++ .../Security/CWE-089/StringBreak.qlref | 1 + .../Security/CWE-089/StringBreakGood.go | 15 +++ ql/test/query-tests/Security/CWE-089/go.mod | 5 + .../github.com/Masterminds/squirrel/LICENSE | 23 +++++ .../github.com/Masterminds/squirrel/README.md | 3 + .../github.com/Masterminds/squirrel/go.mod | 1 + .../Masterminds/squirrel/squirrel.go | 24 +++++ .../Security/CWE-089/vendor/modules.txt | 2 + 17 files changed, 331 insertions(+) create mode 100644 ql/src/Security/CWE-089/StringBreak.go create mode 100644 ql/src/Security/CWE-089/StringBreak.qhelp create mode 100644 ql/src/Security/CWE-089/StringBreak.ql create mode 100644 ql/src/Security/CWE-089/StringBreakGood.go create mode 100644 ql/src/semmle/go/security/StringBreak.qll create mode 100644 ql/src/semmle/go/security/StringBreakCustomizations.qll create mode 100644 ql/test/query-tests/Security/CWE-089/StringBreak.expected create mode 100644 ql/test/query-tests/Security/CWE-089/StringBreak.go create mode 100644 ql/test/query-tests/Security/CWE-089/StringBreak.qlref create mode 100644 ql/test/query-tests/Security/CWE-089/StringBreakGood.go create mode 100644 ql/test/query-tests/Security/CWE-089/go.mod create mode 100644 ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/LICENSE create mode 100644 ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/README.md create mode 100644 ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/go.mod create mode 100644 ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/squirrel.go create mode 100644 ql/test/query-tests/Security/CWE-089/vendor/modules.txt diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 94aca0bf065..a1e4705fe8c 100644 --- a/change-notes/1.24/analysis-go.md +++ b/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 diff --git a/ql/src/Security/CWE-089/StringBreak.go b/ql/src/Security/CWE-089/StringBreak.go new file mode 100644 index 00000000000..d5aec9777d4 --- /dev/null +++ b/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() +} diff --git a/ql/src/Security/CWE-089/StringBreak.qhelp b/ql/src/Security/CWE-089/StringBreak.qhelp new file mode 100644 index 00000000000..1c4f2863037 --- /dev/null +++ b/ql/src/Security/CWE-089/StringBreak.qhelp @@ -0,0 +1,49 @@ + + + + +

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

+ + + +

+Sanitize the embedded data appropriately to ensure quotes are escaped, or use an API that does +not rely on manually constructing quoted substrings. +

+ + + +

+In the following example, assume that version is an object from an untrusted source. +The code snippet first uses json.Marshal to serialize this object into a string, and +then embeds it into a SQL query built using the Squirrel library. +

+ +

+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 version 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. +

+

+To fix this vulnerability, use Squirrel's placeholder syntax, which avoids the need to explicitly +construct a quoted string. +

+ + + + +
  • Wikipedia: SQL injection.
    • +
  • OWASP: Command Injection.
    • +     +     diff --git a/ql/src/Security/CWE-089/StringBreak.ql b/ql/src/Security/CWE-089/StringBreak.ql new file mode 100644 index 00000000000..51dc8e292d8 --- /dev/null +++ b/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" diff --git a/ql/src/Security/CWE-089/StringBreakGood.go b/ql/src/Security/CWE-089/StringBreakGood.go new file mode 100644 index 00000000000..dd7125629ba --- /dev/null +++ b/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() +} diff --git a/ql/src/semmle/go/security/StringBreak.qll b/ql/src/semmle/go/security/StringBreak.qll new file mode 100644 index 00000000000..09117bbc4bd --- /dev/null +++ b/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() } + } +} diff --git a/ql/src/semmle/go/security/StringBreakCustomizations.qll b/ql/src/semmle/go/security/StringBreakCustomizations.qll new file mode 100644 index 00000000000..29769cb0139 --- /dev/null +++ b/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 } + } +} diff --git a/ql/test/query-tests/Security/CWE-089/StringBreak.expected b/ql/test/query-tests/Security/CWE-089/StringBreak.expected new file mode 100644 index 00000000000..37b1862708e --- /dev/null +++ b/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 | diff --git a/ql/test/query-tests/Security/CWE-089/StringBreak.go b/ql/test/query-tests/Security/CWE-089/StringBreak.go new file mode 100644 index 00000000000..d5aec9777d4 --- /dev/null +++ b/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() +} diff --git a/ql/test/query-tests/Security/CWE-089/StringBreak.qlref b/ql/test/query-tests/Security/CWE-089/StringBreak.qlref new file mode 100644 index 00000000000..2c1abc2050f --- /dev/null +++ b/ql/test/query-tests/Security/CWE-089/StringBreak.qlref @@ -0,0 +1 @@ +Security/CWE-089/StringBreak.ql diff --git a/ql/test/query-tests/Security/CWE-089/StringBreakGood.go b/ql/test/query-tests/Security/CWE-089/StringBreakGood.go new file mode 100644 index 00000000000..dd7125629ba --- /dev/null +++ b/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() +} diff --git a/ql/test/query-tests/Security/CWE-089/go.mod b/ql/test/query-tests/Security/CWE-089/go.mod new file mode 100644 index 00000000000..69ea79cc24e --- /dev/null +++ b/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 diff --git a/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/LICENSE b/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/LICENSE new file mode 100644 index 00000000000..74c20a2b970 --- /dev/null +++ b/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. diff --git a/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/README.md b/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/README.md new file mode 100644 index 00000000000..d51962ba840 --- /dev/null +++ b/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. diff --git a/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/go.mod b/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/go.mod new file mode 100644 index 00000000000..48cf5c380a3 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/go.mod @@ -0,0 +1 @@ +module github.com/Masterminds/squirrel diff --git a/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/squirrel.go b/ql/test/query-tests/Security/CWE-089/vendor/github.com/Masterminds/squirrel/squirrel.go new file mode 100644 index 00000000000..8058595eb01 --- /dev/null +++ b/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() { +} diff --git a/ql/test/query-tests/Security/CWE-089/vendor/modules.txt b/ql/test/query-tests/Security/CWE-089/vendor/modules.txt new file mode 100644 index 00000000000..1f0e9480a50 --- /dev/null +++ b/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