From c889cb350124fcd6dbb9cc6df1e9e5abccf2e5fd Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Wed, 8 Jan 2020 15:45:42 -0800 Subject: [PATCH 1/9] Add getAnOperand to OperatorExpr --- ql/src/semmle/go/Expr.qll | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) 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) { From 9c6aa807183fc6e9d8ebe8802743d05911932367 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Wed, 8 Jan 2020 17:48:29 -0800 Subject: [PATCH 2/9] Move OpenUrlRedirect tests into their own directory --- .../CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirect.expected | 0 .../Security/CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirect.go | 0 .../Security/CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirect.qlref | 0 .../Security/CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirectGood.go | 0 .../query-tests/Security/CWE-601/{ => OpenUrlRedirect}/stdlib.go | 0 .../query-tests/Security/CWE-601/{ => OpenUrlRedirect}/util.go | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename ql/test/query-tests/Security/CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirect.expected (100%) rename ql/test/query-tests/Security/CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirect.go (100%) rename ql/test/query-tests/Security/CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirect.qlref (100%) rename ql/test/query-tests/Security/CWE-601/{ => OpenUrlRedirect}/OpenUrlRedirectGood.go (100%) rename ql/test/query-tests/Security/CWE-601/{ => OpenUrlRedirect}/stdlib.go (100%) rename ql/test/query-tests/Security/CWE-601/{ => OpenUrlRedirect}/util.go (100%) 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 From aa28724f7c95cf3cbad237f46fe0c41ac759c2b8 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Wed, 8 Jan 2020 18:15:23 -0800 Subject: [PATCH 3/9] Add `BadRedirectCheck` query --- change-notes/1.24/analysis-go.md | 1 + ql/src/Security/CWE-601/BadRedirectCheck.go | 8 ++ .../Security/CWE-601/BadRedirectCheck.qhelp | 40 +++++++++ ql/src/Security/CWE-601/BadRedirectCheck.ql | 84 +++++++++++++++++++ .../Security/CWE-601/BadRedirectCheckGood.go | 8 ++ .../BadRedirectCheck.expected | 5 ++ .../BadRedirectCheck/BadRedirectCheck.go | 8 ++ .../BadRedirectCheck/BadRedirectCheck.qlref | 1 + .../BadRedirectCheck/BadRedirectCheckGood.go | 8 ++ .../Security/CWE-601/BadRedirectCheck/cves.go | 32 +++++++ .../Security/CWE-601/BadRedirectCheck/main.go | 23 +++++ 11 files changed, 218 insertions(+) create mode 100644 ql/src/Security/CWE-601/BadRedirectCheck.go create mode 100644 ql/src/Security/CWE-601/BadRedirectCheck.qhelp create mode 100644 ql/src/Security/CWE-601/BadRedirectCheck.ql create mode 100644 ql/src/Security/CWE-601/BadRedirectCheckGood.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.qlref create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheckGood.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/cves.go create mode 100644 ql/test/query-tests/Security/CWE-601/BadRedirectCheck/main.go diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 94aca0bf065..04323ca0467 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -13,6 +13,7 @@ | Constant length comparison (`go/constant-length-comparison`) | correctness | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. | | Impossible interface nil check (`go/impossible-interface-nil-check`) | correctness | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default. | | Incomplete URL scheme check (`go/incomplete-url-scheme-check`) | correctness, security, external/cwe/cwe-020 | Highlights checks for `javascript` URLs that do not take `data` or `vbscript` URLs into account. Results are shown on LGTM by default. | +| Bad check of redirect URL (`go/bad-redirect-check`) | correctness, security, external/cwe/cwe-601 | Highlights checks for redirect URLs ensuring they start with `/` but don't check for `//` or `/\`. Results are shown on LGTM by default. | ## Changes to existing queries 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..45eb906ca73 --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp @@ -0,0 +1,40 @@ + + + + +

+Redirect URLs should be checked in order 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 the patterns //* and /\* when checking redirect URLs. +

+ + + +

+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..72738fd554c --- /dev/null +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -0,0 +1,84 @@ +/** + * @name Bad redirect check + * @description A redirect check that checks for a leading slash but not two + * leading slashes or a leading slash then backslash is + * incomplete. + * @kind problem + * @problem.severity warning + * @id go/bad-redirect-check + * @tags security + * external/cwe/cwe-601 + * @precision high + */ + +import go + +predicate checksForLeadingSlash(Expr e, ValueEntity v) { + exists(LogicalExpr le | le = e | checksForLeadingSlash(le.getAnOperand(), v)) + or + exists(StringOps::HasPrefix hp | + hp.getBaseString().(Read).reads(v) and + // ASCII value for '/' + (hp.getSubstring().getStringValue() = "/" or hp.getSubstring().getIntValue() = 47) + ) +} + +predicate checksForSecondSlash(Expr e, ValueEntity v) { + exists(LogicalExpr le | le = e | checksForSecondSlash(le.getAnOperand(), v)) + or + exists(StringOps::HasPrefix hp | + hp.getBaseString().(Read).reads(v) and + hp.getSubstring().getStringValue() = "//" + ) + or + exists(EqualityTestExpr eq, Expr slash, IndexExpr ie | e = eq | + slash.getIntValue() = 47 and // ASCII value for '/' + ie.getBase() = v.getAUse() and + ie.getIndex().getIntValue() = 1 and + eq.hasOperands(ie, slash) + ) +} + +predicate checksForSecondBackslash(Expr e, ValueEntity v) { + exists(LogicalExpr le | le = e | checksForSecondBackslash(le.getAnOperand(), v)) + or + exists(StringOps::HasPrefix hp | + hp.getBaseString().(Read).reads(v) and + hp.getSubstring().getStringValue() = "/\\" + ) + or + exists(EqualityTestExpr eq, Expr slash, IndexExpr ie | e = eq | + slash.getIntValue() = 92 and // ASCII value for '\' + ie.getBase() = v.getAUse() and + ie.getIndex().getIntValue() = 1 and + eq.hasOperands(ie, slash) + ) +} + +predicate isBadRedirectCheck(Expr e, ValueEntity v) { + checksForLeadingSlash(e, v) and + not (checksForSecondSlash(e, v) and checksForSecondBackslash(e, v)) +} + +predicate isCond(Expr e) { + e = any(ForStmt fs).getCond() + or + e = any(IfStmt is).getCond() + or + e = any(ExpressionSwitchStmt ess | not exists(ess.getExpr())).getACase().getAnExpr() +} + +from Expr e, ValueEntity v +where + isBadRedirectCheck(e, v) and + ( + // this expression is a condition + isCond(e) + or + // or is returned from a function + DataFlow::exprNode(e).getASuccessor*() instanceof DataFlow::ResultNode + ) and + v.getName().regexpMatch("(?i).*url.*|.*redir.*") +select e, + "This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position.", + v, v.getName() 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/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..7ba64bf8346 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected @@ -0,0 +1,5 @@ +| BadRedirectCheck.go:4:5:4:37 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redir | redir | +| cves.go:11:9:11:55 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | url | url | +| cves.go:22:5:22:37 | !... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | +| cves.go:29:5:29:74 | ...\|\|... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | +| main.go:8:7:8:76 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | 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 + } +} From 3a73658a9cf8f9b1d5d3bd6233163d702d0dc554 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 9 Jan 2020 04:16:09 -0800 Subject: [PATCH 4/9] BadRedirectSanitizer: Bind e to hp Address doc review comments --- change-notes/1.24/analysis-go.md | 2 +- ql/src/Security/CWE-601/BadRedirectCheck.qhelp | 2 +- ql/src/Security/CWE-601/BadRedirectCheck.ql | 16 ++++++++-------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 04323ca0467..f707947ca01 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -10,10 +10,10 @@ | **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. | -| Bad check of redirect URL (`go/bad-redirect-check`) | correctness, security, external/cwe/cwe-601 | Highlights checks for redirect URLs ensuring they start with `/` but don't check for `//` or `/\`. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.qhelp b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp index 45eb906ca73..54ca1ca78fa 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.qhelp +++ b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp @@ -3,7 +3,7 @@

    -Redirect URLs should be checked in order to ensure that user input cannot cause a site to redirect +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 diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql index 72738fd554c..c3365ffba0d 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.ql +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -16,18 +16,19 @@ import go predicate checksForLeadingSlash(Expr e, ValueEntity v) { exists(LogicalExpr le | le = e | checksForLeadingSlash(le.getAnOperand(), v)) or - exists(StringOps::HasPrefix hp | - hp.getBaseString().(Read).reads(v) and - // ASCII value for '/' - (hp.getSubstring().getStringValue() = "/" or hp.getSubstring().getIntValue() = 47) + exists(StringOps::HasPrefix hp, DataFlow::Node substr | + e = hp.asExpr() and hp.getBaseString() = v.getARead() and hp.getSubstring() = substr + | + substr.getStringValue() = "/" + or + substr.getIntValue() = 47 // ASCII value for '/' ) } predicate checksForSecondSlash(Expr e, ValueEntity v) { exists(LogicalExpr le | le = e | checksForSecondSlash(le.getAnOperand(), v)) or - exists(StringOps::HasPrefix hp | - hp.getBaseString().(Read).reads(v) and + exists(StringOps::HasPrefix hp | e = hp.asExpr() and hp.getBaseString() = v.getARead() | hp.getSubstring().getStringValue() = "//" ) or @@ -42,8 +43,7 @@ predicate checksForSecondSlash(Expr e, ValueEntity v) { predicate checksForSecondBackslash(Expr e, ValueEntity v) { exists(LogicalExpr le | le = e | checksForSecondBackslash(le.getAnOperand(), v)) or - exists(StringOps::HasPrefix hp | - hp.getBaseString().(Read).reads(v) and + exists(StringOps::HasPrefix hp | e = hp.asExpr() and hp.getBaseString() = v.getARead() | hp.getSubstring().getStringValue() = "/\\" ) or From abc9438cd3313c3ded76974ca5a741ab11ec99e3 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 16 Jan 2020 16:11:15 -0800 Subject: [PATCH 5/9] Apply suggestions from code review Co-Authored-By: Max Schaefer --- ql/src/Security/CWE-601/BadRedirectCheck.qhelp | 4 ++-- ql/src/Security/CWE-601/BadRedirectCheck.ql | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.qhelp b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp index 54ca1ca78fa..b6036f4399b 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.qhelp +++ b/ql/src/Security/CWE-601/BadRedirectCheck.qhelp @@ -14,14 +14,14 @@ also check the second character of redirect URLs.

    -Also disallow the patterns //* and /\* when checking 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 +does not begin with /, the harmless placeholder redirect URL / is returned to prevent an open redirect; otherwise redir itself is returned.

    diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql index c3365ffba0d..4d464ab8467 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.ql +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -1,7 +1,7 @@ /** * @name Bad redirect check * @description A redirect check that checks for a leading slash but not two - * leading slashes or a leading slash then backslash is + * leading slashes or a leading slash followed by a backslash is * incomplete. * @kind problem * @problem.severity warning From a31ad88fc96f5a82fdb9c17663b4f079aab40222 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 17 Jan 2020 16:33:33 -0800 Subject: [PATCH 6/9] BadRedirectSanitizer: Transition to using data-flow API --- ql/src/Security/CWE-601/BadRedirectCheck.ql | 69 +++++++------------ .../BadRedirectCheck.expected | 10 +-- 2 files changed, 31 insertions(+), 48 deletions(-) diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql index 4d464ab8467..5c254924f35 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.ql +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -13,11 +13,9 @@ import go -predicate checksForLeadingSlash(Expr e, ValueEntity v) { - exists(LogicalExpr le | le = e | checksForLeadingSlash(le.getAnOperand(), v)) - or +DataFlow::Node checkForLeadingSlash(ValueEntity v) { exists(StringOps::HasPrefix hp, DataFlow::Node substr | - e = hp.asExpr() and hp.getBaseString() = v.getARead() and hp.getSubstring() = substr + result = hp and hp.getBaseString() = v.getARead() and hp.getSubstring() = substr | substr.getStringValue() = "/" or @@ -25,60 +23,45 @@ predicate checksForLeadingSlash(Expr e, ValueEntity v) { ) } -predicate checksForSecondSlash(Expr e, ValueEntity v) { - exists(LogicalExpr le | le = e | checksForSecondSlash(le.getAnOperand(), v)) - or - exists(StringOps::HasPrefix hp | e = hp.asExpr() and hp.getBaseString() = v.getARead() | +DataFlow::Node checkForSecondSlash(ValueEntity v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getARead() | hp.getSubstring().getStringValue() = "//" ) or - exists(EqualityTestExpr eq, Expr slash, IndexExpr ie | e = eq | + exists(DataFlow::EqualityTestNode eq, DataFlow::Node slash, DataFlow::ElementReadNode er | + result = eq + | slash.getIntValue() = 47 and // ASCII value for '/' - ie.getBase() = v.getAUse() and - ie.getIndex().getIntValue() = 1 and - eq.hasOperands(ie, slash) + er.getBase() = v.getARead() and + er.getIndex().getIntValue() = 1 and + eq.eq(_, er, slash) ) } -predicate checksForSecondBackslash(Expr e, ValueEntity v) { - exists(LogicalExpr le | le = e | checksForSecondBackslash(le.getAnOperand(), v)) - or - exists(StringOps::HasPrefix hp | e = hp.asExpr() and hp.getBaseString() = v.getARead() | +DataFlow::Node checkForSecondBackslash(ValueEntity v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getARead() | hp.getSubstring().getStringValue() = "/\\" ) or - exists(EqualityTestExpr eq, Expr slash, IndexExpr ie | e = eq | + exists(DataFlow::EqualityTestNode eq, DataFlow::Node slash, DataFlow::ElementReadNode er | + result = eq + | slash.getIntValue() = 92 and // ASCII value for '\' - ie.getBase() = v.getAUse() and - ie.getIndex().getIntValue() = 1 and - eq.hasOperands(ie, slash) + er.getBase() = v.getARead() and + er.getIndex().getIntValue() = 1 and + eq.eq(_, er, slash) ) } -predicate isBadRedirectCheck(Expr e, ValueEntity v) { - checksForLeadingSlash(e, v) and - not (checksForSecondSlash(e, v) and checksForSecondBackslash(e, v)) +predicate isBadRedirectCheck(DataFlow::Node node, ValueEntity v) { + node = checkForLeadingSlash(v) and + not (exists(checkForSecondSlash(v)) and exists(checkForSecondBackslash(v))) } -predicate isCond(Expr e) { - e = any(ForStmt fs).getCond() - or - e = any(IfStmt is).getCond() - or - e = any(ExpressionSwitchStmt ess | not exists(ess.getExpr())).getACase().getAnExpr() -} - -from Expr e, ValueEntity v +from DataFlow::Node node, ValueEntity v where - isBadRedirectCheck(e, v) and - ( - // this expression is a condition - isCond(e) - or - // or is returned from a function - DataFlow::exprNode(e).getASuccessor*() instanceof DataFlow::ResultNode - ) and - v.getName().regexpMatch("(?i).*url.*|.*redir.*") -select e, - "This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position.", + isBadRedirectCheck(node, v) and + v.getName().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.getName() diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected index 7ba64bf8346..4735977860c 100644 --- a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected @@ -1,5 +1,5 @@ -| BadRedirectCheck.go:4:5:4:37 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redir | redir | -| cves.go:11:9:11:55 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | url | url | -| cves.go:22:5:22:37 | !... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | -| cves.go:29:5:29:74 | ...\|\|... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | -| main.go:8:7:8:76 | ...&&... | This condition checks '$@' for a leading slash but not for both a '/' and '\\' in the second position. | redirect | redirect | +| 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. | redir | 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. | url | 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. | redirect | 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. | redirect | 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. | redirect | redirect | From f897f68ead05db68946326ee2758f6eb6b22ccdb Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 27 Jan 2020 11:47:36 -0800 Subject: [PATCH 7/9] SsaWithFilds: Add a getQualifiedName predicate --- ql/src/semmle/go/dataflow/SSA.qll | 18 ++++++-- .../semmle/go/dataflow/SSA/DefUse.expected | 8 +++- .../go/dataflow/SSA/SsaDefinition.expected | 11 +++-- .../go/dataflow/SSA/SsaWithFields.expected | 46 +++++++++++++++++++ .../semmle/go/dataflow/SSA/SsaWithFields.ql | 4 ++ .../semmle/go/dataflow/SSA/VarDefs.expected | 21 +++++++-- .../semmle/go/dataflow/SSA/VarUses.expected | 15 ++++-- .../semmle/go/dataflow/SSA/main.go | 24 +++++++++- 8 files changed, 130 insertions(+), 17 deletions(-) create mode 100644 ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.expected create mode 100644 ql/test/library-tests/semmle/go/dataflow/SSA/SsaWithFields.ql 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) +} From 497bfeee83f5b21acf055fcb0216f05833a1fa5e Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 27 Jan 2020 11:49:28 -0800 Subject: [PATCH 8/9] BadRedirectSanitizer: Use SsaWithFields instead of ValueEntity --- ql/src/Security/CWE-601/BadRedirectCheck.ql | 24 +++++++++---------- .../BadRedirectCheck.expected | 10 ++++---- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql index 5c254924f35..51006b02da5 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.ql +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -13,9 +13,9 @@ import go -DataFlow::Node checkForLeadingSlash(ValueEntity v) { +DataFlow::Node checkForLeadingSlash(SsaWithFields v) { exists(StringOps::HasPrefix hp, DataFlow::Node substr | - result = hp and hp.getBaseString() = v.getARead() and hp.getSubstring() = substr + result = hp and hp.getBaseString() = v.getAUse() and hp.getSubstring() = substr | substr.getStringValue() = "/" or @@ -23,8 +23,8 @@ DataFlow::Node checkForLeadingSlash(ValueEntity v) { ) } -DataFlow::Node checkForSecondSlash(ValueEntity v) { - exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getARead() | +DataFlow::Node checkForSecondSlash(SsaWithFields v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getAUse() | hp.getSubstring().getStringValue() = "//" ) or @@ -32,14 +32,14 @@ DataFlow::Node checkForSecondSlash(ValueEntity v) { result = eq | slash.getIntValue() = 47 and // ASCII value for '/' - er.getBase() = v.getARead() and + er.getBase() = v.getAUse() and er.getIndex().getIntValue() = 1 and eq.eq(_, er, slash) ) } -DataFlow::Node checkForSecondBackslash(ValueEntity v) { - exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getARead() | +DataFlow::Node checkForSecondBackslash(SsaWithFields v) { + exists(StringOps::HasPrefix hp | result = hp and hp.getBaseString() = v.getAUse() | hp.getSubstring().getStringValue() = "/\\" ) or @@ -47,21 +47,21 @@ DataFlow::Node checkForSecondBackslash(ValueEntity v) { result = eq | slash.getIntValue() = 92 and // ASCII value for '\' - er.getBase() = v.getARead() and + er.getBase() = v.getAUse() and er.getIndex().getIntValue() = 1 and eq.eq(_, er, slash) ) } -predicate isBadRedirectCheck(DataFlow::Node node, ValueEntity v) { +predicate isBadRedirectCheck(DataFlow::Node node, SsaWithFields v) { node = checkForLeadingSlash(v) and not (exists(checkForSecondSlash(v)) and exists(checkForSecondBackslash(v))) } -from DataFlow::Node node, ValueEntity v +from DataFlow::Node node, SsaWithFields v where isBadRedirectCheck(node, v) and - v.getName().regexpMatch("(?i).*url.*|.*redir.*|.*target.*") + 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.getName() + v, v.getQualifiedName() diff --git a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected index 4735977860c..e2830337608 100644 --- a/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected +++ b/ql/test/query-tests/Security/CWE-601/BadRedirectCheck/BadRedirectCheck.expected @@ -1,5 +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. | redir | 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. | url | 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. | redirect | 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. | redirect | 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. | redirect | redirect | +| 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 | From aa33595b0fbda445b7bcd3be7dd8d734331dec40 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 28 Jan 2020 08:26:37 -0800 Subject: [PATCH 9/9] Address review comments --- ql/src/Security/CWE-601/BadRedirectCheck.ql | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql index 51006b02da5..1e20dfbd5f7 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.ql +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -13,9 +13,9 @@ import go -DataFlow::Node checkForLeadingSlash(SsaWithFields v) { - exists(StringOps::HasPrefix hp, DataFlow::Node substr | - result = hp and hp.getBaseString() = v.getAUse() and hp.getSubstring() = substr +StringOps::HasPrefix checkForLeadingSlash(SsaWithFields v) { + exists(DataFlow::Node substr | + result.getBaseString() = v.getAUse() and result.getSubstring() = substr | substr.getStringValue() = "/" or @@ -53,14 +53,12 @@ DataFlow::Node checkForSecondBackslash(SsaWithFields v) { ) } -predicate isBadRedirectCheck(DataFlow::Node node, SsaWithFields v) { - node = checkForLeadingSlash(v) and - not (exists(checkForSecondSlash(v)) and exists(checkForSecondBackslash(v))) -} - from DataFlow::Node node, SsaWithFields v where - isBadRedirectCheck(node, v) and + // 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.",