diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 94aca0bf065..f707947ca01 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -10,6 +10,7 @@ | **Query** | **Tags** | **Purpose** | |---------------------------------------------------------------------------|----------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------| +| Bad check of redirect URL (`go/bad-redirect-check`) | correctness, security, external/cwe/cwe-601 | Highlights checks that ensure redirect URLs start with `/` but don't check for `//` or `/\`. Results are shown on LGTM by default. | | 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. | diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.go b/ql/src/Security/CWE-601/BadRedirectCheck.go new file mode 100644 index 00000000000..279e59c9cfb --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl(redir string) string { + if len(redir) > 0 && redir[0] == '/' { + return redir + } + return "/" +} diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.qhelp b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp new file mode 100644 index 00000000000..b6036f4399b --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp @@ -0,0 +1,40 @@ + + + + +

+Redirect URLs should be checked to ensure that user input cannot cause a site to redirect +to arbitrary domains. This is often done with a check that the redirect URL begins with a slash, +which most of the time is an absolute redirect on the same host. However, browsers interpret URLs +beginning with // or /\ as absolute URLs. For example, a redirect to +//lgtm.com will redirect to https://lgtm.com. Thus, redirect checks must +also check the second character of redirect URLs. +

+
+ + +

+Also disallow redirect URLs starting with // or /\. +

+
+ + +

+The following function validates a (presumably untrusted) redirect URL redir. If it +does not begin with /, the harmless placeholder redirect URL / is +returned to prevent an open redirect; otherwise redir itself is returned. +

+ +

+While this check provides partial protection, it should be extended to cover // and +/\ as well: +

+ +
+ + +
  • OWASP: + XSS Unvalidated Redirects and Forwards Cheat Sheet.
  • +
    + +
    diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql new file mode 100644 index 00000000000..1e20dfbd5f7 --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -0,0 +1,65 @@ +/** + * @name Bad redirect check + * @description A redirect check that checks for a leading slash but not two + * leading slashes or a leading slash followed by a backslash is + * incomplete. + * @kind problem + * @problem.severity warning + * @id go/bad-redirect-check + * @tags security + * external/cwe/cwe-601 + * @precision high + */ + +import go + +StringOps::HasPrefix checkForLeadingSlash(SsaWithFields v) { + exists(DataFlow::Node substr | + result.getBaseString() = v.getAUse() and result.getSubstring() = substr + | + substr.getStringValue() = "/" + or + substr.getIntValue() = 47 // ASCII value for '/' + ) +} + +DataFlow::Node checkForSecondSlash(SsaWithFields v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getAUse() | + hp.getSubstring().getStringValue() = "//" + ) + or + exists(DataFlow::EqualityTestNode eq, DataFlow::Node slash, DataFlow::ElementReadNode er | + result = eq + | + slash.getIntValue() = 47 and // ASCII value for '/' + er.getBase() = v.getAUse() and + er.getIndex().getIntValue() = 1 and + eq.eq(_, er, slash) + ) +} + +DataFlow::Node checkForSecondBackslash(SsaWithFields v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getAUse() | + hp.getSubstring().getStringValue() = "/\\" + ) + or + exists(DataFlow::EqualityTestNode eq, DataFlow::Node slash, DataFlow::ElementReadNode er | + result = eq + | + slash.getIntValue() = 92 and // ASCII value for '\' + er.getBase() = v.getAUse() and + er.getIndex().getIntValue() = 1 and + eq.eq(_, er, slash) + ) +} + +from DataFlow::Node node, SsaWithFields v +where + // there is a check for a leading slash + node = checkForLeadingSlash(v) and + // but not a check for both a second slash and a second backslash + not (exists(checkForSecondSlash(v)) and exists(checkForSecondBackslash(v))) and + v.getQualifiedName().regexpMatch("(?i).*url.*|.*redir.*|.*target.*") +select node, + "This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position.", + v, v.getQualifiedName() diff --git a/ql/src/Security/CWE-601/BadRedirectCheckGood.go b/ql/src/Security/CWE-601/BadRedirectCheckGood.go new file mode 100644 index 00000000000..89af5110a29 --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheckGood.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl1(redir string) string { + if len(redir) > 1 && redir[0] == '/' && redir[1] != '/' && redir[1] != '\\' { + return redir + } + return "/" +} diff --git a/ql/src/semmle/go/Expr.qll b/ql/src/semmle/go/Expr.qll index a84a9d69beb..2fa9559809c 100644 --- a/ql/src/semmle/go/Expr.qll +++ b/ql/src/semmle/go/Expr.qll @@ -318,7 +318,9 @@ class ParenExpr extends @parenexpr, Expr { override Expr stripParens() { result = getExpression().stripParens() } - override predicate isPlatformIndependentConstant() { getExpression().isPlatformIndependentConstant() } + override predicate isPlatformIndependentConstant() { + getExpression().isPlatformIndependentConstant() + } override string toString() { result = "(...)" } } @@ -393,7 +395,9 @@ class TypeAssertExpr extends @typeassertexpr, Expr { override predicate mayHaveOwnSideEffects() { any() } - override predicate isPlatformIndependentConstant() { getExpression().isPlatformIndependentConstant() } + override predicate isPlatformIndependentConstant() { + getExpression().isPlatformIndependentConstant() + } override string toString() { result = "type assertion" } } @@ -419,7 +423,9 @@ class ConversionExpr extends CallOrConversionExpr { /** Gets the operand of the type conversion. */ Expr getOperand() { result = getChildExpr(1) } - override predicate isPlatformIndependentConstant() { getOperand().isPlatformIndependentConstant() } + override predicate isPlatformIndependentConstant() { + getOperand().isPlatformIndependentConstant() + } override string toString() { result = "type conversion" } } @@ -638,6 +644,9 @@ class MapTypeExpr extends @maptypeexpr, Expr { class OperatorExpr extends @operatorexpr, Expr { /** Gets the operator of this expression. */ string getOperator() { none() } + + /** Gets an operand of this expression. */ + Expr getAnOperand() { none() } } /** @@ -662,7 +671,11 @@ class UnaryExpr extends @unaryexpr, OperatorExpr { /** Gets the operand of this unary expression. */ Expr getOperand() { result = getChildExpr(0) } - override predicate isPlatformIndependentConstant() { getOperand().isPlatformIndependentConstant() } + override Expr getAnOperand() { result = this.getOperand() } + + override predicate isPlatformIndependentConstant() { + getOperand().isPlatformIndependentConstant() + } override string toString() { result = getOperator() + "..." } } @@ -747,8 +760,7 @@ class BinaryExpr extends @binaryexpr, OperatorExpr { /** Gets the right operand of this binary expression. */ Expr getRightOperand() { result = getChildExpr(1) } - /** Gets an operand of this binary expression. */ - Expr getAnOperand() { result = getChildExpr([0 .. 1]) } + override Expr getAnOperand() { result = getChildExpr([0 .. 1]) } /** Holds if `e` and `f` (in either order) are the two operands of this binary expression. */ predicate hasOperands(Expr e, Expr f) { diff --git a/ql/src/semmle/go/dataflow/SSA.qll b/ql/src/semmle/go/dataflow/SSA.qll index da3303d24b5..5801dd779f0 100644 --- a/ql/src/semmle/go/dataflow/SSA.qll +++ b/ql/src/semmle/go/dataflow/SSA.qll @@ -10,9 +10,7 @@ private import SsaImpl * declared in file scope. */ class SsaSourceVariable extends LocalVariable { - SsaSourceVariable() { - not getScope() instanceof FileScope - } + SsaSourceVariable() { not getScope() instanceof FileScope } /** * Holds if there may be indirect references of this variable that are not covered by `getAReference()`. @@ -339,6 +337,20 @@ class SsaWithFields extends TSsaWithFields { exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base + "." + f.getName()) } + /** + * Gets the qualified name of the source variable or variable and fields that this represents. + * + * For example, for an SSA variable that represents the field `a.b`, this would get the string + * `"a.b"`. + */ + string getQualifiedName() { + exists(SsaVariable v | this = TRoot(v) and result = v.getSourceVariable().getName()) + or + exists(SsaWithFields base, Field f | this = TStep(base, f) | + result = base.getQualifiedName() + "." + f.getName() + ) + } + /** * Holds if this element is at the specified location. * The location spans column `startcolumn` of line `startline` to diff --git a/ql/test/library-tests/semmle/go/dataflow/SSA/DefUse.expected b/ql/test/library-tests/semmle/go/dataflow/SSA/DefUse.expected index 0ac0dc33fe7..f330cabad50 100644 --- a/ql/test/library-tests/semmle/go/dataflow/SSA/DefUse.expected +++ b/ql/test/library-tests/semmle/go/dataflow/SSA/DefUse.expected @@ -26,5 +26,9 @@ | main.go:82:25:82:25 | implicit read of b | main.go:82:25:82:25 | definition of b | main.go:82:25:82:25 | b | | main.go:84:9:84:9 | x | main.go:83:2:83:2 | definition of x | main.go:83:2:83:2 | x | | main.go:84:15:84:15 | x | main.go:83:2:83:2 | definition of x | main.go:83:2:83:2 | x | -| main.go:94:2:94:8 | wrapper | main.go:92:22:92:28 | definition of wrapper | main.go:92:22:92:28 | wrapper | -| main.go:97:9:97:9 | x | main.go:94:2:96:3 | capture variable x | main.go:93:2:93:2 | x | +| main.go:97:2:97:8 | wrapper | main.go:95:22:95:28 | definition of wrapper | main.go:95:22:95:28 | wrapper | +| main.go:100:9:100:9 | x | main.go:97:2:99:3 | capture variable x | main.go:96:2:96:2 | x | +| main.go:117:2:117:2 | p | main.go:117:2:117:2 | p = phi(def@112:3, def@114:3) | main.go:110:6:110:6 | p | +| main.go:119:12:119:12 | p | main.go:117:2:117:2 | p = phi(def@112:3, def@114:3) | main.go:110:6:110:6 | p | +| main.go:119:17:119:17 | p | main.go:117:2:117:2 | p = phi(def@112:3, def@114:3) | main.go:110:6:110:6 | p | +| main.go:119:24:119:24 | p | main.go:117:2:117:2 | p = phi(def@112:3, def@114:3) | main.go:110:6:110:6 | p | diff --git a/ql/test/library-tests/semmle/go/dataflow/SSA/SsaDefinition.expected b/ql/test/library-tests/semmle/go/dataflow/SSA/SsaDefinition.expected index faca455b364..3be49ec06f7 100644 --- a/ql/test/library-tests/semmle/go/dataflow/SSA/SsaDefinition.expected +++ b/ql/test/library-tests/semmle/go/dataflow/SSA/SsaDefinition.expected @@ -32,7 +32,10 @@ | main.go:82:25:82:25 | definition of b | | main.go:83:2:83:2 | definition of x | | main.go:84:5:84:5 | definition of a | -| main.go:92:22:92:28 | definition of wrapper | -| main.go:93:2:93:2 | definition of x | -| main.go:94:2:96:3 | capture variable x | -| main.go:95:3:95:3 | definition of x | +| main.go:95:22:95:28 | definition of wrapper | +| main.go:96:2:96:2 | definition of x | +| main.go:97:2:99:3 | capture variable x | +| main.go:98:3:98:3 | definition of x | +| main.go:112:3:112:3 | definition of p | +| main.go:114:3:114:3 | definition of p | +| main.go:117:2:117:2 | p = phi(def@112:3, def@114:3) | diff --git a/ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.expected b/ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.expected new file mode 100644 index 00000000000..bcaf5bd749d --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.expected @@ -0,0 +1,46 @@ +| main.go:13:6:13:6 | (def@13:6) | x | +| main.go:14:2:14:2 | (def@14:2) | y | +| main.go:17:3:17:3 | (def@17:3) | y | +| main.go:19:2:19:2 | (phi@19:2) | y | +| main.go:21:3:21:3 | (def@21:3) | x | +| main.go:23:2:23:2 | (phi@23:2) | x | +| main.go:26:10:26:10 | (def@26:10) | x | +| main.go:27:2:27:2 | (def@27:2) | a | +| main.go:27:5:27:5 | (def@27:5) | b | +| main.go:29:3:29:3 | (def@29:3) | a | +| main.go:29:6:29:6 | (def@29:6) | b | +| main.go:31:9:31:9 | (phi@31:9) | a | +| main.go:31:9:31:9 | (phi@31:9) | b | +| main.go:34:11:34:11 | (def@34:11) | x | +| main.go:39:2:39:2 | (def@39:2) | x | +| main.go:40:2:40:4 | (def@40:2) | ptr | +| main.go:48:2:48:7 | (def@48:2) | result | +| main.go:52:14:52:19 | (def@52:14) | result | +| main.go:57:6:57:6 | (def@57:6) | x | +| main.go:58:6:58:6 | (phi@58:6) | x | +| main.go:59:3:59:3 | (def@59:3) | x | +| main.go:63:2:63:2 | (def@63:2) | y | +| main.go:64:6:64:6 | (def@64:6) | i | +| main.go:64:16:64:18 | (def@64:16) | i | +| main.go:65:6:65:6 | (phi@65:6) | i | +| main.go:65:6:65:6 | (phi@65:6) | y | +| main.go:68:3:68:3 | (def@68:3) | y | +| main.go:73:6:73:6 | (def@73:6) | i | +| main.go:73:16:73:18 | (def@73:16) | i | +| main.go:74:3:74:3 | (def@74:3) | z | +| main.go:74:3:74:3 | (phi@74:3) | i | +| main.go:82:25:82:25 | (def@82:25) | b | +| main.go:83:2:83:2 | (def@83:2) | x | +| main.go:84:5:84:5 | (def@84:5) | a | +| main.go:95:22:95:28 | (def@95:22) | wrapper | +| main.go:95:22:95:28 | (def@95:22).s | wrapper.s | +| main.go:96:2:96:2 | (def@96:2) | x | +| main.go:97:2:99:3 | (capture@97:2) | x | +| main.go:98:3:98:3 | (def@98:3) | x | +| main.go:112:3:112:3 | (def@112:3) | p | +| main.go:114:3:114:3 | (def@114:3) | p | +| main.go:117:2:117:2 | (phi@117:2) | p | +| main.go:117:2:117:2 | (phi@117:2).a | p.a | +| main.go:117:2:117:2 | (phi@117:2).b | p.b | +| main.go:117:2:117:2 | (phi@117:2).b.a | p.b.a | +| main.go:117:2:117:2 | (phi@117:2).c | p.c | diff --git a/ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.ql b/ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.ql new file mode 100644 index 00000000000..a2862a72697 --- /dev/null +++ b/ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.ql @@ -0,0 +1,4 @@ +import go + +from SsaWithFields v +select v, v.getQualifiedName() diff --git a/ql/test/library-tests/semmle/go/dataflow/SSA/VarDefs.expected b/ql/test/library-tests/semmle/go/dataflow/SSA/VarDefs.expected index aa083e6740e..edc2e5cc22d 100644 --- a/ql/test/library-tests/semmle/go/dataflow/SSA/VarDefs.expected +++ b/ql/test/library-tests/semmle/go/dataflow/SSA/VarDefs.expected @@ -28,7 +28,20 @@ | main.go:83:2:83:2 | assignment to x | main.go:83:2:83:2 | x | main.go:83:7:83:8 | 23 | | main.go:84:2:84:2 | assignment to x | main.go:83:2:83:2 | x | main.go:84:9:84:12 | ...+... | | main.go:84:5:84:5 | assignment to a | main.go:82:18:82:18 | a | main.go:84:15:84:15 | x | -| main.go:90:15:90:16 | initialization of cb | main.go:90:15:90:16 | cb | main.go:90:15:90:16 | argument corresponding to cb | -| main.go:92:22:92:28 | initialization of wrapper | main.go:92:22:92:28 | wrapper | main.go:92:22:92:28 | argument corresponding to wrapper | -| main.go:93:2:93:2 | assignment to x | main.go:93:2:93:2 | x | main.go:93:7:93:7 | 0 | -| main.go:95:3:95:3 | assignment to x | main.go:93:2:93:2 | x | main.go:95:7:95:7 | 1 | +| main.go:93:15:93:16 | initialization of cb | main.go:93:15:93:16 | cb | main.go:93:15:93:16 | argument corresponding to cb | +| main.go:95:22:95:28 | initialization of wrapper | main.go:95:22:95:28 | wrapper | main.go:95:22:95:28 | argument corresponding to wrapper | +| main.go:96:2:96:2 | assignment to x | main.go:96:2:96:2 | x | main.go:96:7:96:7 | 0 | +| main.go:98:3:98:3 | assignment to x | main.go:96:2:96:2 | x | main.go:98:7:98:7 | 1 | +| main.go:110:6:110:6 | assignment to p | main.go:110:6:110:6 | p | main.go:110:6:110:6 | zero value for p | +| main.go:112:3:112:3 | assignment to p | main.go:110:6:110:6 | p | main.go:112:7:112:24 | composite literal | +| main.go:112:9:112:9 | init of 2 | main.go:104:2:104:2 | a | main.go:112:9:112:9 | 2 | +| main.go:112:12:112:18 | init of composite literal | main.go:105:2:105:2 | b | main.go:112:12:112:18 | composite literal | +| main.go:112:14:112:14 | init of 1 | main.go:89:2:89:2 | a | main.go:112:14:112:14 | 1 | +| main.go:112:17:112:17 | init of 5 | main.go:90:2:90:2 | b | main.go:112:17:112:17 | 5 | +| main.go:112:21:112:23 | init of 'n' | main.go:106:2:106:2 | c | main.go:112:21:112:23 | 'n' | +| main.go:114:3:114:3 | assignment to p | main.go:110:6:110:6 | p | main.go:114:7:114:24 | composite literal | +| main.go:114:9:114:9 | init of 3 | main.go:104:2:104:2 | a | main.go:114:9:114:9 | 3 | +| main.go:114:12:114:18 | init of composite literal | main.go:105:2:105:2 | b | main.go:114:12:114:18 | composite literal | +| main.go:114:14:114:14 | init of 4 | main.go:89:2:89:2 | a | main.go:114:14:114:14 | 4 | +| main.go:114:17:114:17 | init of 5 | main.go:90:2:90:2 | b | main.go:114:17:114:17 | 5 | +| main.go:114:21:114:23 | init of '2' | main.go:106:2:106:2 | c | main.go:114:21:114:23 | '2' | diff --git a/ql/test/library-tests/semmle/go/dataflow/SSA/VarUses.expected b/ql/test/library-tests/semmle/go/dataflow/SSA/VarUses.expected index e38be3b04e4..332f859f051 100644 --- a/ql/test/library-tests/semmle/go/dataflow/SSA/VarUses.expected +++ b/ql/test/library-tests/semmle/go/dataflow/SSA/VarUses.expected @@ -26,6 +26,15 @@ | main.go:82:25:82:25 | implicit read of b | main.go:82:25:82:25 | b | | main.go:84:9:84:9 | x | main.go:83:2:83:2 | x | | main.go:84:15:84:15 | x | main.go:83:2:83:2 | x | -| main.go:94:2:94:8 | wrapper | main.go:92:22:92:28 | wrapper | -| main.go:94:2:94:10 | selection of s | main.go:92:38:92:38 | s | -| main.go:97:9:97:9 | x | main.go:93:2:93:2 | x | +| main.go:97:2:97:8 | wrapper | main.go:95:22:95:28 | wrapper | +| main.go:97:2:97:10 | selection of s | main.go:95:38:95:38 | s | +| main.go:100:9:100:9 | x | main.go:96:2:96:2 | x | +| main.go:117:2:117:2 | p | main.go:110:6:110:6 | p | +| main.go:117:2:117:4 | selection of b | main.go:105:2:105:2 | b | +| main.go:119:12:119:12 | p | main.go:110:6:110:6 | p | +| main.go:119:12:119:14 | selection of a | main.go:104:2:104:2 | a | +| main.go:119:17:119:17 | p | main.go:110:6:110:6 | p | +| main.go:119:17:119:19 | selection of b | main.go:105:2:105:2 | b | +| main.go:119:17:119:21 | selection of a | main.go:89:2:89:2 | a | +| main.go:119:24:119:24 | p | main.go:110:6:110:6 | p | +| main.go:119:24:119:26 | selection of c | main.go:106:2:106:2 | c | diff --git a/ql/test/library-tests/semmle/go/dataflow/SSA/main.go b/ql/test/library-tests/semmle/go/dataflow/SSA/main.go index 8c20bb6da2f..cda85fdfc66 100644 --- a/ql/test/library-tests/semmle/go/dataflow/SSA/main.go +++ b/ql/test/library-tests/semmle/go/dataflow/SSA/main.go @@ -85,7 +85,10 @@ func multiRes() (a int, b float32) { return } -type s struct{} +type s struct { + a int + b int +} func (*s) foo(cb func()) {} @@ -96,3 +99,22 @@ func updateInClosure(wrapper struct{ s }) int { }) return x } + +type t struct { + a int + b s + c rune +} + +func ssaPhi() { + var p t + if cond() { + p = t{2, s{1, 5}, 'n'} + } else { + p = t{3, s{4, 5}, '2'} + } + + p.b.foo(func() {}) + + fmt.Print(p.a, p.b.a, p.c) +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected new file mode 100644 index 00000000000..e2830337608 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected @@ -0,0 +1,5 @@ +| BadRedirectCheck.go:4:23:4:37 | ...==... | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | BadRedirectCheck.go:3:18:3:22 | (def@3:18) | redir | +| cves.go:11:26:11:38 | ...==... | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | cves.go:10:22:10:24 | (def@10:22) | url | +| cves.go:22:6:22:37 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | cves.go:21:2:21:9 | (def@21:2) | redirect | +| cves.go:29:6:29:37 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | cves.go:28:2:28:9 | (def@28:2) | redirect | +| main.go:8:7:8:38 | call to HasPrefix | This expression checks '$@' for a leading slash but checks do not exist for both '/' and '\\' in the second position. | main.go:5:19:5:26 | (def@5:19) | redirect | diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go new file mode 100644 index 00000000000..279e59c9cfb --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl(redir string) string { + if len(redir) > 0 && redir[0] == '/' { + return redir + } + return "/" +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref new file mode 100644 index 00000000000..fa21b83def6 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref @@ -0,0 +1 @@ +Security/CWE-601/BadRedirectCheck.ql diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go new file mode 100644 index 00000000000..89af5110a29 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go @@ -0,0 +1,8 @@ +package main + +func sanitizeUrl1(redir string) string { + if len(redir) > 1 && redir[0] == '/' && redir[1] != '/' && redir[1] != '\\' { + return redir + } + return "/" +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go new file mode 100644 index 00000000000..cd91df4d4a7 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go @@ -0,0 +1,32 @@ +package main + +import ( + "net/http" + "strings" +) + +// CVE-2018-15178 +// Code from github.com/gogs/gogs +func isValidRedirect(url string) bool { + return len(url) >= 2 && url[0] == '/' && url[1] != '/' // NOT OK +} + +func isValidRedirect1(url string) bool { + return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\' // OK +} + +// CVE-2017-1000070 (both vulnerable!) +// Code from github.com/bitly/oauth2_proxy +func OAuthCallback(rw http.ResponseWriter, req *http.Request) { + redirect := req.Form.Get("state") + if !strings.HasPrefix(redirect, "/") { // NOT OK + redirect = "/" + } +} + +func OAuthCallback1(rw http.ResponseWriter, req *http.Request) { + redirect := req.Form.Get("state") + if !strings.HasPrefix(redirect, "/") || strings.HasPrefix(redirect, "//") { // NOT OK + redirect = "/" + } +} diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go new file mode 100644 index 00000000000..97b7b9c5f89 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go @@ -0,0 +1,23 @@ +package main + +import "strings" + +func isValidRedir(redirect string) bool { + switch { + // Not OK: does not check for '/\' + case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//"): + return true + default: + return false + } +} + +func isValidRedir1(redirect string) bool { + switch { + // OK + case strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !strings.HasPrefix(redirect, "/\\"): + return true + default: + return false + } +} diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect.expected b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected similarity index 100% rename from ql/test/query-tests/Security/CWE-601/OpenUrlRedirect.expected rename to ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.go similarity index 100% rename from ql/test/query-tests/Security/CWE-601/OpenUrlRedirect.go rename to ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.go diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect.qlref b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.qlref similarity index 100% rename from ql/test/query-tests/Security/CWE-601/OpenUrlRedirect.qlref rename to ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.qlref diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirectGood.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirectGood.go similarity index 100% rename from ql/test/query-tests/Security/CWE-601/OpenUrlRedirectGood.go rename to ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirectGood.go diff --git a/ql/test/query-tests/Security/CWE-601/stdlib.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go similarity index 100% rename from ql/test/query-tests/Security/CWE-601/stdlib.go rename to ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go diff --git a/ql/test/query-tests/Security/CWE-601/util.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/util.go similarity index 100% rename from ql/test/query-tests/Security/CWE-601/util.go rename to ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/util.go