diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index f707947ca01..a6b39b5e786 100644 --- a/change-notes/1.24/analysis-go.md +++ b/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 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/StringOps.qll b/ql/src/semmle/go/StringOps.qll index 437c5294f56..ec5ebdb218c 100644 --- a/ql/src/semmle/go/StringOps.qll +++ b/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() } + } } 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/src/semmle/go/security/UrlConcatenation.qll b/ql/src/semmle/go/security/UrlConcatenation.qll index 898db550c62..9c75b3f8279 100644 --- a/ql/src/semmle/go/security/UrlConcatenation.qll +++ b/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])) ) } diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement.expected b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement.expected new file mode 100644 index 00000000000..9d272cce459 --- /dev/null +++ b/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 | diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement.ql b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement.ql new file mode 100644 index 00000000000..a02ec3f93e9 --- /dev/null +++ b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement.ql @@ -0,0 +1,4 @@ +import go + +from StringOps::ConcatenationElement elt +select elt diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_asNode.expected b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_asNode.expected new file mode 100644 index 00000000000..447e4d44021 --- /dev/null +++ b/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 | diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_asNode.ql b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_asNode.ql new file mode 100644 index 00000000000..449f7e6b7df --- /dev/null +++ b/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() diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getOperand.expected b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getOperand.expected new file mode 100644 index 00000000000..f4297d0ce56 --- /dev/null +++ b/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 | diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getOperand.ql b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getOperand.ql new file mode 100644 index 00000000000..c54b41d3f61 --- /dev/null +++ b/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) diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getStringValue.expected b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getStringValue.expected new file mode 100644 index 00000000000..c801c7fedb7 --- /dev/null +++ b/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 | diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getStringValue.ql b/ql/test/library-tests/semmle/go/StringOps/Concatenation/ConcatenationElement_getStringValue.ql new file mode 100644 index 00000000000..046450378ba --- /dev/null +++ b/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() diff --git a/ql/test/library-tests/semmle/go/StringOps/Concatenation/main.go b/ql/test/library-tests/semmle/go/StringOps/Concatenation/main.go new file mode 100644 index 00000000000..f5832834edc --- /dev/null +++ b/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) +} diff --git a/ql/test/library-tests/semmle/go/StringOps/HasPrefix.expected b/ql/test/library-tests/semmle/go/StringOps/HasPrefix/HasPrefix.expected similarity index 100% rename from ql/test/library-tests/semmle/go/StringOps/HasPrefix.expected rename to ql/test/library-tests/semmle/go/StringOps/HasPrefix/HasPrefix.expected diff --git a/ql/test/library-tests/semmle/go/StringOps/HasPrefix.ql b/ql/test/library-tests/semmle/go/StringOps/HasPrefix/HasPrefix.ql similarity index 100% rename from ql/test/library-tests/semmle/go/StringOps/HasPrefix.ql rename to ql/test/library-tests/semmle/go/StringOps/HasPrefix/HasPrefix.ql diff --git a/ql/test/library-tests/semmle/go/StringOps/main.go b/ql/test/library-tests/semmle/go/StringOps/HasPrefix/main.go similarity index 100% rename from ql/test/library-tests/semmle/go/StringOps/main.go rename to ql/test/library-tests/semmle/go/StringOps/HasPrefix/main.go 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