From aeb98401445e42213280398de011f67242ced054 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 6 Jan 2020 15:36:54 +0000 Subject: [PATCH 1/4] Add `SliceNode` class. --- ql/src/semmle/go/controlflow/IR.qll | 21 ++++++++++ .../go/dataflow/internal/DataFlowUtil.qll | 41 +++++++++++++++---- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/ql/src/semmle/go/controlflow/IR.qll b/ql/src/semmle/go/controlflow/IR.qll index 7de15bb3b1d..0b37deadbbd 100644 --- a/ql/src/semmle/go/controlflow/IR.qll +++ b/ql/src/semmle/go/controlflow/IR.qll @@ -1413,4 +1413,25 @@ module IR { ExtractTupleElementInstruction extractTupleElement(Instruction base, int idx) { result.extractsElement(base, idx) } + + /** + * Gets the instruction corresponding to the implicit lower bound of slice `e`, if any. + */ + EvalImplicitLowerSliceBoundInstruction implicitLowerSliceBoundInstruction(SliceExpr e) { + result = MkImplicitLowerSliceBound(e) + } + + /** + * Gets the instruction corresponding to the implicit upper bound of slice `e`, if any. + */ + EvalImplicitUpperSliceBoundInstruction implicitUpperSliceBoundInstruction(SliceExpr e) { + result = MkImplicitUpperSliceBound(e) + } + + /** + * Gets the instruction corresponding to the implicit maximum bound of slice `e`, if any. + */ + EvalImplicitMaxSliceBoundInstruction implicitMaxSliceBoundInstruction(SliceExpr e) { + result = MkImplicitMaxSliceBound(e) + } } diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 69e1383187f..6f53a53686e 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -112,9 +112,7 @@ class Node extends TNode { * Holds if the result of this instruction is known at compile time, and is guaranteed not to * depend on the platform where it is evaluated. */ - predicate isPlatformIndependentConstant() { - this.asInstruction().isPlatformIndependentConstant() - } + predicate isPlatformIndependentConstant() { this.asInstruction().isPlatformIndependentConstant() } /** * Gets a data-flow node to which data may flow from this node in one (intra-procedural) step. @@ -449,7 +447,7 @@ class ReadNode extends InstructionNode { /** * Holds if this data-flow node evaluates to value of `v`, which is a value entity, that is, a - * constant, variable, field, function, or method. + * constant, variable, field, function, or method. */ predicate reads(ValueEntity v) { insn.reads(v) } @@ -492,6 +490,35 @@ class ElementReadNode extends ReadNode { predicate reads(Node base, Node index) { getBase() = base and getIndex() = index } } +/** + * A data-flow node that extracts a substring or slice from a string, array, pointer to array, + * or slice. + */ +class SliceNode extends ExprNode { + override SliceExpr expr; + + /** Gets the base of this slice node. */ + Node getBase() { result = exprNode(expr.getBase()) } + + /** Gets the lower bound of this slice node. */ + Node getLow() { + result = exprNode(expr.getLow()) or + result = instructionNode(IR::implicitLowerSliceBoundInstruction(expr)) + } + + /** Gets the upper bound of this slice node. */ + Node getHigh() { + result = exprNode(expr.getHigh()) or + result = instructionNode(IR::implicitUpperSliceBoundInstruction(expr)) + } + + /** Gets the maximum of this slice node. */ + Node getMax() { + result = exprNode(expr.getMax()) or + result = instructionNode(IR::implicitMaxSliceBoundInstruction(expr)) + } +} + /** * A data-flow node corresponding to an expression with a binary operator. */ @@ -512,7 +539,7 @@ class BinaryOperationNode extends Node { | left = exprNode(assgn.getLhs()) and right = exprNode(assgn.getRhs()) and - op = o.substring(0, o.length()-1) + op = o.substring(0, o.length() - 1) ) or exists(IR::EvalIncDecRhsInstruction rhs, IncDecStmt ids | @@ -683,9 +710,7 @@ Node extractTupleElement(Node t, int i) { * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local * (intra-procedural) step. */ -predicate localFlowStep(Node nodeFrom, Node nodeTo) { - simpleLocalFlowStep(nodeFrom, nodeTo) -} +predicate localFlowStep(Node nodeFrom, Node nodeTo) { simpleLocalFlowStep(nodeFrom, nodeTo) } /** * INTERNAL: do not use. From 9cff56b975df6a8dde6780234f43fb4647e5ab2e Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 6 Jan 2020 15:37:14 +0000 Subject: [PATCH 2/4] Rename `StringConcatenation.qll` to `StringOps.qll` and add `HasPrefix` class. --- ql/src/go.qll | 2 +- ql/src/semmle/go/StringConcatenation.qll | 59 ------ ql/src/semmle/go/StringOps.qll | 199 ++++++++++++++++++ .../semmle/go/StringOps/HasPrefix.expected | 6 + .../semmle/go/StringOps/HasPrefix.ql | 4 + .../library-tests/semmle/go/StringOps/main.go | 13 ++ 6 files changed, 223 insertions(+), 60 deletions(-) delete mode 100644 ql/src/semmle/go/StringConcatenation.qll create mode 100644 ql/src/semmle/go/StringOps.qll create mode 100644 ql/test/library-tests/semmle/go/StringOps/HasPrefix.expected create mode 100644 ql/test/library-tests/semmle/go/StringOps/HasPrefix.ql create mode 100644 ql/test/library-tests/semmle/go/StringOps/main.go diff --git a/ql/src/go.qll b/ql/src/go.qll index b6b4f339420..b7d8af964a1 100644 --- a/ql/src/go.qll +++ b/ql/src/go.qll @@ -13,7 +13,7 @@ import semmle.go.Locations import semmle.go.Packages import semmle.go.Scopes import semmle.go.Stmt -import semmle.go.StringConcatenation +import semmle.go.StringOps import semmle.go.Types import semmle.go.controlflow.BasicBlocks import semmle.go.controlflow.ControlFlowGraph diff --git a/ql/src/semmle/go/StringConcatenation.qll b/ql/src/semmle/go/StringConcatenation.qll deleted file mode 100644 index e7020e9d190..00000000000 --- a/ql/src/semmle/go/StringConcatenation.qll +++ /dev/null @@ -1,59 +0,0 @@ -/** - * Provides predicates for analyzing string concatenations and their operands. - */ - -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) } -} diff --git a/ql/src/semmle/go/StringOps.qll b/ql/src/semmle/go/StringOps.qll new file mode 100644 index 00000000000..17b310a4210 --- /dev/null +++ b/ql/src/semmle/go/StringOps.qll @@ -0,0 +1,199 @@ +/** + * Provides predicates and classes for working with string operations. + */ + +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 { + /** + * A expression that is equivalent to `strings.HasPrefix(A, B)` or `!strings.HasPrefix(A, B)`. + * + * Extends this class to refine existing API models. If you want to model new APIs, + * extend `StringOps::HasPrefix::Range` instead. + */ + class HasPrefix extends DataFlow::Node { + HasPrefix::Range range; + + HasPrefix() { range = this } + + /** + * Gets the `A` in `strings.HasPrefix(A, B)`. + */ + DataFlow::Node getBaseString() { result = range.getBaseString() } + + /** + * Gets the `B` in `strings.HasPrefix(A, B)`. + */ + DataFlow::Node getSubstring() { result = range.getSubstring() } + + /** + * Gets the polarity of the check. + * + * If the polarity is `false` the check returns `true` if the string does not start + * with the given substring. + */ + boolean getPolarity() { result = range.getPolarity() } + } + + class StartsWith = HasPrefix; + + module HasPrefix { + /** + * A expression that is equivalent to `strings.HasPrefix(A, B)` or `!strings.HasPrefix(A, B)`. + * + * Extends this class to model new APIs. If you want to refine existing API models, extend + * `StringOps::HasPrefix` instead. + */ + abstract class Range extends DataFlow::Node { + /** + * Gets the `A` in `strings.HasPrefix(A, B)`. + */ + abstract DataFlow::Node getBaseString(); + + /** + * Gets the `B` in `strings.HasPrefix(A, B)`. + */ + abstract DataFlow::Node getSubstring(); + + /** + * Gets the polarity of the check. + * + * If the polarity is `false` the check returns `true` if the string does not start + * with the given substring. + */ + boolean getPolarity() { result = true } + } + + /** + * An expression of form `strings.HasPrefix(A, B)`. + */ + private class StringsHasPrefix extends Range, DataFlow::CallNode { + StringsHasPrefix() { getTarget().hasQualifiedName("strings", "HasPrefix") } + + override DataFlow::Node getBaseString() { result = getArgument(0) } + + override DataFlow::Node getSubstring() { result = getArgument(1) } + } + + /** + * An expression of form `strings.Index(A, B) === 0`. + */ + private class HasPrefix_IndexOfEquals extends Range, DataFlow::EqualityTestNode { + DataFlow::CallNode indexOf; + + HasPrefix_IndexOfEquals() { + indexOf.getTarget().hasQualifiedName("strings", "Index") and + getAnOperand() = globalValueNumber(indexOf).getANode() and + getAnOperand().getIntValue() = 0 + } + + override DataFlow::Node getBaseString() { result = indexOf.getArgument(0) } + + override DataFlow::Node getSubstring() { result = indexOf.getArgument(1) } + + override boolean getPolarity() { result = expr.getPolarity() } + } + + /** + * A comparison of form `x[0] === 'k'` for some rune literal `k`. + */ + private class HasPrefix_FirstCharacter extends Range, DataFlow::EqualityTestNode { + DataFlow::ElementReadNode read; + DataFlow::Node runeLiteral; + + HasPrefix_FirstCharacter() { + read.getBase().getType().getUnderlyingType() instanceof StringType and + read.getIndex().getIntValue() = 0 and + eq(_, globalValueNumber(read).getANode(), runeLiteral) + } + + override DataFlow::Node getBaseString() { result = read.getBase() } + + override DataFlow::Node getSubstring() { result = runeLiteral } + + override boolean getPolarity() { result = expr.getPolarity() } + } + + /** + * A comparison of form `x[:len(y)] === y`. + */ + private class HasPrefix_Substring extends Range, DataFlow::EqualityTestNode { + DataFlow::SliceNode slice; + DataFlow::Node substring; + + HasPrefix_Substring() { + eq(_, slice, substring) and + slice.getLow().getIntValue() = 0 and + ( + exists(DataFlow::CallNode len | + len = Builtin::len().getACall() and + len.getArgument(0) = globalValueNumber(substring).getANode() and + slice.getHigh() = globalValueNumber(len).getANode() + ) + or + substring.getStringValue().length() = slice.getHigh().getIntValue() + ) + } + + override DataFlow::Node getBaseString() { result = slice.getBase() } + + override DataFlow::Node getSubstring() { result = substring } + + override boolean getPolarity() { result = expr.getPolarity() } + } + } +} diff --git a/ql/test/library-tests/semmle/go/StringOps/HasPrefix.expected b/ql/test/library-tests/semmle/go/StringOps/HasPrefix.expected new file mode 100644 index 00000000000..24fb68401ef --- /dev/null +++ b/ql/test/library-tests/semmle/go/StringOps/HasPrefix.expected @@ -0,0 +1,6 @@ +| main.go:7:9:7:33 | call to HasPrefix | main.go:7:27:7:28 | s1 | main.go:7:31:7:32 | s2 | true | +| main.go:8:3:8:10 | ...==... | main.go:6:23:6:24 | s1 | main.go:6:27:6:28 | s2 | true | +| main.go:9:3:9:14 | ...!=... | main.go:9:3:9:4 | s1 | main.go:9:12:9:14 | 'x' | false | +| main.go:10:3:10:21 | ...==... | main.go:10:3:10:4 | s1 | main.go:10:20:10:21 | s2 | true | +| main.go:11:3:11:20 | ...==... | main.go:11:3:11:4 | s1 | main.go:11:19:11:20 | s2 | true | +| main.go:12:3:12:18 | ...==... | main.go:12:3:12:4 | s1 | main.go:12:14:12:18 | "hi!" | true | diff --git a/ql/test/library-tests/semmle/go/StringOps/HasPrefix.ql b/ql/test/library-tests/semmle/go/StringOps/HasPrefix.ql new file mode 100644 index 00000000000..0bf1cf03a47 --- /dev/null +++ b/ql/test/library-tests/semmle/go/StringOps/HasPrefix.ql @@ -0,0 +1,4 @@ +import go + +from StringOps::HasPrefix hp +select hp, hp.getBaseString(), hp.getSubstring(), hp.getPolarity() diff --git a/ql/test/library-tests/semmle/go/StringOps/main.go b/ql/test/library-tests/semmle/go/StringOps/main.go new file mode 100644 index 00000000000..7e482c7d20e --- /dev/null +++ b/ql/test/library-tests/semmle/go/StringOps/main.go @@ -0,0 +1,13 @@ +package main + +import "strings" + +func test(s1, s2 string) bool { + idx := strings.Index(s1, s2) + return strings.HasPrefix(s1, s2) || + idx == 0 || + s1[0] != 'x' || + s1[0:len(s2)] == s2 || + s1[:len(s2)] == s2 || + s1[0:3] == "hi!" +} From 0d2fe473d78044c90fe01332f4d03423c1935783 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 7 Jan 2020 11:07:02 +0000 Subject: [PATCH 3/4] Add `IncompleteUrlSchemeCheck` query. --- change-notes/1.24/analysis-go.md | 1 + .../CWE-020/IncompleteUrlSchemeCheck.go | 11 ++++ .../CWE-020/IncompleteUrlSchemeCheck.qhelp | 42 +++++++++++++ .../CWE-020/IncompleteUrlSchemeCheck.ql | 60 +++++++++++++++++++ .../CWE-020/IncompleteUrlSchemeCheckGood.go | 11 ++++ .../IncompleteUrlSchemeCheck.expected | 2 + .../IncompleteUrlSchemeCheck.go | 11 ++++ .../IncompleteUrlSchemeCheck.qlref | 1 + .../IncompleteUrlSchemeCheckGood.go | 11 ++++ .../CWE-020/IncompleteUrlSchemeCheck/main.go | 20 +++++++ 10 files changed, 170 insertions(+) create mode 100644 ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go create mode 100644 ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp create mode 100644 ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql create mode 100644 ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go create mode 100644 ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected create mode 100644 ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.go create mode 100644 ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref create mode 100644 ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheckGood.go create mode 100644 ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/main.go diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index a2428712ea6..8edb61b33b8 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -10,6 +10,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. | ## Changes to existing queries diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go new file mode 100644 index 00000000000..364ec89d237 --- /dev/null +++ b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.go @@ -0,0 +1,11 @@ +package main + +import "net/url" + +func sanitizeUrl(urlstr string) string { + u, err := url.Parse(urlstr) + if err != nil || u.Scheme == "javascript" { + return "about:blank" + } + return urlstr +} diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp new file mode 100644 index 00000000000..0c04811c18d --- /dev/null +++ b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.qhelp @@ -0,0 +1,42 @@ + + + +

+URLs with the special scheme javascript can be used to encode JavaScript code to be +executed when the URL is visited. While this is a powerful mechanism for creating feature-rich and +responsive web applications, it is also a potential security risk: if the URL comes from an +untrusted source, it might contain harmful JavaScript code. For this reason, many frameworks and +libraries first check the URL scheme of any untrusted URL, and reject URLs with the +javascript scheme. +

+

+However, the data and vbscript schemes can be used to represent +executable code in a very similar way, so any validation logic that checks against +javascript, but not against data and vbscript, is likely +to be insufficient. +

+ + +

+Add checks covering both data: and vbscript:. +

+ + +

+The following function validates a (presumably untrusted) URL urlstr. If its scheme is +javascript, the harmless placeholder URL about:blank is returned to +prevent code injection; otherwise urlstr itself is returned. +

+ +

+While this check provides partial projection, it should be extended to cover data +and vbscript as well: +

+ + + +
  • WHATWG: URL schemes.
    • +     +     diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql new file mode 100644 index 00000000000..e1227b76d6b --- /dev/null +++ b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql @@ -0,0 +1,60 @@ +/** + * @name Incomplete URL scheme check + * @description Checking for the "javascript:" URL scheme without also checking for "vbscript:" + * and "data:" suggests a logic error or even a security vulnerability. + * @kind problem + * @problem.severity warning + * @precision high + * @id go/incomplete-url-scheme-check + * @tags security + * correctness + * external/cwe/cwe-020 + */ + +import go + +/** A URL scheme that can be used to represent executable code. */ +class DangerousScheme extends string { + DangerousScheme() { this = "data" or this = "javascript" or this = "vbscript" } +} + +/** Gets a data-flow node that checks an instance of `g` against the given `scheme`. */ +DataFlow::Node schemeCheck(GVN g, DangerousScheme scheme) { + // check of the form `nd.Scheme == scheme` + exists(NamedType url, DataFlow::FieldReadNode fr, DataFlow::Node s | + url.hasQualifiedName("net/url", "URL") and + fr.readsField(g.getANode(), url.getField("Scheme")) and + s.getStringValue() = scheme and + result.(DataFlow::EqualityTestNode).eq(_, fr, s) + ) + or + // check of the form `strings.HasPrefix(nd, "scheme:")` + exists(StringOps::HasPrefix hasPrefix | result = hasPrefix | + hasPrefix.getBaseString() = g.getANode() and + hasPrefix.getSubstring().getStringValue() = scheme + ":" + ) + or + // propagate through various string transformations + exists(string stringop, DataFlow::CallNode c | + stringop = "Map" or + stringop.matches("Replace%") or + stringop.matches("To%") or + stringop.matches("Trim%") + | + c.getTarget().hasQualifiedName("strings", stringop) and + c.getAnArgument() = g.getANode() and + result = schemeCheck(globalValueNumber(c), scheme) + ) +} + +/** + * Gets a scheme that `g`, which is checked against at least one scheme, is not checked against. + */ +DangerousScheme getAMissingScheme(GVN g) { + exists(schemeCheck(g, _)) and + not exists(schemeCheck(g, result)) +} + +from GVN g +select schemeCheck(g, "javascript"), + "This check does not consider " + strictconcat(getAMissingScheme(g), " and ") + "." diff --git a/ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go new file mode 100644 index 00000000000..1d3fb46a65e --- /dev/null +++ b/ql/src/Security/CWE-020/IncompleteUrlSchemeCheckGood.go @@ -0,0 +1,11 @@ +package main + +import "net/url" + +func sanitizeUrlGod(urlstr string) string { + u, err := url.Parse(urlstr) + if err != nil || u.Scheme == "javascript" || u.Scheme == "data" || u.Scheme == "vbscript" { + return "about:blank" + } + return urlstr +} diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected new file mode 100644 index 00000000000..d1bbee8dd70 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected @@ -0,0 +1,2 @@ +| IncompleteUrlSchemeCheck.go:7:22:7:45 | ...==... | This check does not consider data and vbscript. | +| main.go:17:5:17:44 | call to HasPrefix | This check does not consider vbscript. | diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.go b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.go new file mode 100644 index 00000000000..364ec89d237 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.go @@ -0,0 +1,11 @@ +package main + +import "net/url" + +func sanitizeUrl(urlstr string) string { + u, err := url.Parse(urlstr) + if err != nil || u.Scheme == "javascript" { + return "about:blank" + } + return urlstr +} diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref new file mode 100644 index 00000000000..b27571781b3 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref @@ -0,0 +1 @@ +Security/CWE-020/IncompleteUrlSchemeCheck.ql diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheckGood.go b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheckGood.go new file mode 100644 index 00000000000..1d3fb46a65e --- /dev/null +++ b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheckGood.go @@ -0,0 +1,11 @@ +package main + +import "net/url" + +func sanitizeUrlGod(urlstr string) string { + u, err := url.Parse(urlstr) + if err != nil || u.Scheme == "javascript" || u.Scheme == "data" || u.Scheme == "vbscript" { + return "about:blank" + } + return urlstr +} diff --git a/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/main.go b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/main.go new file mode 100644 index 00000000000..ebe18f142f8 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/main.go @@ -0,0 +1,20 @@ +package main + +import ( + "net/url" + "strings" +) + +func test(urlstr string) { + u, _ := url.Parse(urlstr) + sch := u.Scheme + if sch == "javascript" || u.Scheme == "data" || sch == "vbscript" { // OK + return + } + + urlstr = strings.NewReplacer("\n", "", "\r", "", "\t", "", "\u0000", "").Replace(urlstr) + urlstr = strings.ToLower(urlstr) + if strings.HasPrefix(urlstr, "javascript:") || strings.HasPrefix(urlstr, "data:") { // NOT OK + return + } +} From 3d7046e38c49feeb17ed1d7417e2387e4fa35f8a Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 7 Jan 2020 20:07:44 +0000 Subject: [PATCH 4/4] Apply suggestions from code review Co-Authored-By: Shati Patel --- ql/src/semmle/go/StringOps.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/StringOps.qll b/ql/src/semmle/go/StringOps.qll index 17b310a4210..437c5294f56 100644 --- a/ql/src/semmle/go/StringOps.qll +++ b/ql/src/semmle/go/StringOps.qll @@ -60,7 +60,7 @@ module StringConcatenation { module StringOps { /** - * A expression that is equivalent to `strings.HasPrefix(A, B)` or `!strings.HasPrefix(A, B)`. + * An expression that is equivalent to `strings.HasPrefix(A, B)` or `!strings.HasPrefix(A, B)`. * * Extends this class to refine existing API models. If you want to model new APIs, * extend `StringOps::HasPrefix::Range` instead. @@ -93,7 +93,7 @@ module StringOps { module HasPrefix { /** - * A expression that is equivalent to `strings.HasPrefix(A, B)` or `!strings.HasPrefix(A, B)`. + * An expression that is equivalent to `strings.HasPrefix(A, B)` or `!strings.HasPrefix(A, B)`. * * Extends this class to model new APIs. If you want to refine existing API models, extend * `StringOps::HasPrefix` instead.