From 8cc8b8ef476ccb1652aa7d158bbf30e05ecf2343 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 16 Jul 2020 12:38:08 +0300 Subject: [PATCH 1/8] Add CWE-352: CSRF because of constant oauth2 state value --- .../CWE-352/ConstantOauth2State.qhelp | 26 +++++ .../CWE-352/ConstantOauth2State.ql | 38 +++++++ .../CWE-352/ConstantOauth2StateBad.go | 27 +++++ .../CWE-352/ConstantOauth2StateBetter.go | 34 ++++++ .../CWE-352/ConstantOauth2State.expected | 19 ++++ .../CWE-352/ConstantOauth2State.go | 106 ++++++++++++++++++ .../CWE-352/ConstantOauth2State.qlref | 1 + .../vendor/golang.org/x/oauth2/LICENSE | 27 +++++ .../vendor/golang.org/x/oauth2/stub.go | 81 +++++++++++++ .../experimental/CWE-352/vendor/modules.txt | 3 + 10 files changed, 362 insertions(+) create mode 100644 ql/src/experimental/CWE-352/ConstantOauth2State.qhelp create mode 100644 ql/src/experimental/CWE-352/ConstantOauth2State.ql create mode 100644 ql/src/experimental/CWE-352/ConstantOauth2StateBad.go create mode 100644 ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go create mode 100644 ql/test/experimental/CWE-352/ConstantOauth2State.expected create mode 100644 ql/test/experimental/CWE-352/ConstantOauth2State.go create mode 100644 ql/test/experimental/CWE-352/ConstantOauth2State.qlref create mode 100644 ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/LICENSE create mode 100644 ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/stub.go create mode 100644 ql/test/experimental/CWE-352/vendor/modules.txt diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp b/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp new file mode 100644 index 00000000000..2e23f288718 --- /dev/null +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp @@ -0,0 +1,26 @@ + + + +

+ Oauth2 clients must implement CSRF protection for the redirection URI, which is typically accomplished by including a "state" value that binds the request to + the user's authenticated state. The Go Oauth2 library allows to specify a "state" value which is then included in the auth code URL, and then provided back by the remote authentication server in the redirect callback, from where it must be validated; failure to do so makes the client susceptible to an CSRF attack. +

+ + +

+ Always include a unique, non-guessable state value to the AuthCodeURL that is also bound to the user's authenticated state, and then validated in the redirect callback. +

+ + +

+ The first example shows you the use of a constant state (bad). +

+ +

+ The second example shows a better implementation idea. +

+ + + \ No newline at end of file diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql new file mode 100644 index 00000000000..7cb5d292a28 --- /dev/null +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql @@ -0,0 +1,38 @@ +/** + * @name Use of constant `state` value in Oauth2 URL. + * @description Using a constant value for the `state` in the oauth2 URL makes the application + * susceptible to CSRF attacks. + * @kind path-problem + * @problem.severity error + * @id go/constant-oauth2-state + * @tags security + * external/cwe/cwe-352 + */ + +import go +import DataFlow::PathGraph + +class AuthCodeURL extends Method { + AuthCodeURL() { this.hasQualifiedName("golang.org/x/oauth2", "Config", "AuthCodeURL") } +} + +class FlowConf extends TaintTracking::Configuration { + FlowConf() { this = "FlowConf" } + + predicate isSource(DataFlow::Node source, Literal state) { + state.isConst() and source.asExpr() = state + } + + predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) { + exists(AuthCodeURL m | call = m.getACall() | sink = call.getArgument(0)) + } + + override predicate isSource(DataFlow::Node source) { isSource(source, _) } + + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } +} + +from FlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Using a constant $@ to create oauth2 URLs.", source.getNode(), + "state string" diff --git a/ql/src/experimental/CWE-352/ConstantOauth2StateBad.go b/ql/src/experimental/CWE-352/ConstantOauth2StateBad.go new file mode 100644 index 00000000000..57b5c295fc1 --- /dev/null +++ b/ql/src/experimental/CWE-352/ConstantOauth2StateBad.go @@ -0,0 +1,27 @@ +package main + +import ( + "fmt" + + "golang.org/x/oauth2" +) + +func main() {} + +var stateStringVar = "state" + +func badWithStringLiteralState() { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + url := conf.AuthCodeURL(stateStringVar, oauth2.AccessTypeOffline) + fmt.Printf("Visit the URL for the auth dialog: %v", url) + // ... +} diff --git a/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go b/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go new file mode 100644 index 00000000000..a6854947ff3 --- /dev/null +++ b/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go @@ -0,0 +1,34 @@ +package main + +import ( + "crypto/rand" + "encoding/base64" + "net/http" + + "golang.org/x/oauth2" +) + +func betterWithVariableStateReturned(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + state := generateStateOauthCookie(w) + url := conf.AuthCodeURL(state) + _ = url + // ... +} +func generateStateOauthCookie(w http.ResponseWriter) string { + b := make([]byte, 128) + rand.Read(b) + // TODO: save the state string to cookies or HTML storage. + state := base64.URLEncoding.EncodeToString(b) + + return state +} diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.expected b/ql/test/experimental/CWE-352/ConstantOauth2State.expected new file mode 100644 index 00000000000..00414516e70 --- /dev/null +++ b/ql/test/experimental/CWE-352/ConstantOauth2State.expected @@ -0,0 +1,19 @@ +edges +| ConstantOauth2State.go:15:26:15:32 | "state" : string literal | ConstantOauth2State.go:45:26:45:41 | stateStringConst | +| ConstantOauth2State.go:17:22:17:28 | "state" : string | ConstantOauth2State.go:60:26:60:39 | stateStringVar | +| ConstantOauth2State.go:75:11:75:25 | call to newFixedState : string | ConstantOauth2State.go:76:26:76:30 | state | +| ConstantOauth2State.go:81:9:81:15 | "state" : string | ConstantOauth2State.go:75:11:75:25 | call to newFixedState : string | +nodes +| ConstantOauth2State.go:15:26:15:32 | "state" : string literal | semmle.label | "state" : string literal | +| ConstantOauth2State.go:17:22:17:28 | "state" : string | semmle.label | "state" : string | +| ConstantOauth2State.go:30:26:30:32 | "state" | semmle.label | "state" | +| ConstantOauth2State.go:45:26:45:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:60:26:60:39 | stateStringVar | semmle.label | stateStringVar | +| ConstantOauth2State.go:75:11:75:25 | call to newFixedState : string | semmle.label | call to newFixedState : string | +| ConstantOauth2State.go:76:26:76:30 | state | semmle.label | state | +| ConstantOauth2State.go:81:9:81:15 | "state" : string | semmle.label | "state" : string | +#select +| ConstantOauth2State.go:30:26:30:32 | "state" | ConstantOauth2State.go:30:26:30:32 | "state" | ConstantOauth2State.go:30:26:30:32 | "state" | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:30:26:30:32 | "state" | state string | +| ConstantOauth2State.go:45:26:45:41 | stateStringConst | ConstantOauth2State.go:15:26:15:32 | "state" : string literal | ConstantOauth2State.go:45:26:45:41 | stateStringConst | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:15:26:15:32 | "state" | state string | +| ConstantOauth2State.go:60:26:60:39 | stateStringVar | ConstantOauth2State.go:17:22:17:28 | "state" : string | ConstantOauth2State.go:60:26:60:39 | stateStringVar | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:17:22:17:28 | "state" | state string | +| ConstantOauth2State.go:76:26:76:30 | state | ConstantOauth2State.go:81:9:81:15 | "state" : string | ConstantOauth2State.go:76:26:76:30 | state | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:81:9:81:15 | "state" | state string | diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.go b/ql/test/experimental/CWE-352/ConstantOauth2State.go new file mode 100644 index 00000000000..9f7c7f056df --- /dev/null +++ b/ql/test/experimental/CWE-352/ConstantOauth2State.go @@ -0,0 +1,106 @@ +package main + +//go:generate depstubber -vendor golang.org/x/oauth2 Config,Endpoint + +import ( + "crypto/rand" + "encoding/base64" + "net/http" + + "golang.org/x/oauth2" +) + +func main() {} + +const stateStringConst = "state" + +var stateStringVar = "state" + +func badWithStringLiteralState(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + url := conf.AuthCodeURL("state") // BAD + _ = url + // ... +} +func badWithConstState(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + url := conf.AuthCodeURL(stateStringConst) // BAD + _ = url + // ... +} +func badWithFixedVarState(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + url := conf.AuthCodeURL(stateStringVar) // BAD + _ = url + // ... +} +func badWithFixedStateReturned(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + state := newFixedState() + url := conf.AuthCodeURL(state) // BAD + _ = url + // ... +} +func newFixedState() string { + return "state" +} + +func betterWithVariableStateReturned(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + state := generateStateOauthCookie(w) + url := conf.AuthCodeURL(state) // GOOD + _ = url + // ... +} +func generateStateOauthCookie(w http.ResponseWriter) string { + b := make([]byte, 128) + rand.Read(b) + state := base64.URLEncoding.EncodeToString(b) + // TODO: save the state string to cookies or HTML storage. + return state +} diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.qlref b/ql/test/experimental/CWE-352/ConstantOauth2State.qlref new file mode 100644 index 00000000000..82c080c7754 --- /dev/null +++ b/ql/test/experimental/CWE-352/ConstantOauth2State.qlref @@ -0,0 +1 @@ +experimental/CWE-352/ConstantOauth2State.ql diff --git a/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/LICENSE b/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/LICENSE new file mode 100644 index 00000000000..6a66aea5eaf --- /dev/null +++ b/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/LICENSE @@ -0,0 +1,27 @@ +Copyright (c) 2009 The Go Authors. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/stub.go b/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/stub.go new file mode 100644 index 00000000000..d4012cb20d9 --- /dev/null +++ b/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/stub.go @@ -0,0 +1,81 @@ +// Code generated by depstubber. DO NOT EDIT. +// This is a simple stub for golang.org/x/oauth2, strictly for use in testing. + +// See the LICENSE file for information about the licensing of the original library. +// Source: golang.org/x/oauth2 (exports: Config,Endpoint; functions: ) + +// Package oauth2 is a stub of golang.org/x/oauth2, generated by depstubber. +package oauth2 + +import ( + context "context" + http "net/http" + time "time" +) + +type AuthCodeOption interface{} + +type AuthStyle int + +type Config struct { + ClientID string + ClientSecret string + Endpoint Endpoint + RedirectURL string + Scopes []string +} + +func (_ *Config) AuthCodeURL(_ string, _ ...AuthCodeOption) string { + return "" +} + +func (_ *Config) Client(_ context.Context, _ *Token) *http.Client { + return nil +} + +func (_ *Config) Exchange(_ context.Context, _ string, _ ...AuthCodeOption) (*Token, error) { + return nil, nil +} + +func (_ *Config) PasswordCredentialsToken(_ context.Context, _ string, _ string) (*Token, error) { + return nil, nil +} + +func (_ *Config) TokenSource(_ context.Context, _ *Token) TokenSource { + return nil +} + +type Endpoint struct { + AuthURL string + TokenURL string + AuthStyle AuthStyle +} + +type Token struct { + AccessToken string + TokenType string + RefreshToken string + Expiry time.Time +} + +func (_ *Token) Extra(_ string) interface{} { + return nil +} + +func (_ *Token) SetAuthHeader(_ *http.Request) {} + +func (_ *Token) Type() string { + return "" +} + +func (_ *Token) Valid() bool { + return false +} + +func (_ *Token) WithExtra(_ interface{}) *Token { + return nil +} + +type TokenSource interface { + Token() (*Token, error) +} diff --git a/ql/test/experimental/CWE-352/vendor/modules.txt b/ql/test/experimental/CWE-352/vendor/modules.txt new file mode 100644 index 00000000000..5f8149cc625 --- /dev/null +++ b/ql/test/experimental/CWE-352/vendor/modules.txt @@ -0,0 +1,3 @@ +# golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d +## explicit +golang.org/x/oauth2 From 282f7af6d9d81b83dfef2b9ee8193d768acae8b7 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 16 Jul 2020 12:52:41 +0300 Subject: [PATCH 2/8] Improve comments, naming, docs --- .../CWE-352/ConstantOauth2State.qhelp | 2 +- .../experimental/CWE-352/ConstantOauth2State.ql | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp b/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp index 2e23f288718..4d14d003781 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp @@ -10,7 +10,7 @@

- Always include a unique, non-guessable state value to the AuthCodeURL that is also bound to the user's authenticated state, and then validated in the redirect callback. + Always include a unique, non-guessable state value (provided to the call to AuthCodeURL function) that is also bound to the user's authenticated state with each authentication request, and then validated in the redirect callback.

diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql index 7cb5d292a28..e6c8aef4d1b 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql @@ -12,12 +12,22 @@ import go import DataFlow::PathGraph +/* + * A method that creates a new URL that will send the user + * to the oauth2 authorization dialog of the provider. + */ + class AuthCodeURL extends Method { AuthCodeURL() { this.hasQualifiedName("golang.org/x/oauth2", "Config", "AuthCodeURL") } } -class FlowConf extends TaintTracking::Configuration { - FlowConf() { this = "FlowConf" } +/* + * A flow of a constant string value to a call to AuthCodeURL as the + * `state` parameter. + */ + +class ConstantStateFlowConf extends TaintTracking::Configuration { + ConstantStateFlowConf() { this = "ConstantStateFlowConf" } predicate isSource(DataFlow::Node source, Literal state) { state.isConst() and source.asExpr() = state @@ -32,7 +42,7 @@ class FlowConf extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } } -from FlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink +from ConstantStateFlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink where cfg.hasFlowPath(source, sink) select sink.getNode(), source, sink, "Using a constant $@ to create oauth2 URLs.", source.getNode(), "state string" From ef7198c0cb337164541794f682811ac454d492b8 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 16 Jul 2020 18:29:15 +0300 Subject: [PATCH 3/8] Improve query scenarios --- .../CWE-352/ConstantOauth2State.ql | 59 +++++++++++-- ql/src/semmle/go/frameworks/Stdlib.qll | 27 +++++- .../CWE-352/ConstantOauth2State.go | 88 ++++++++++++++++++- 3 files changed, 165 insertions(+), 9 deletions(-) diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql index e6c8aef4d1b..43f35befc99 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql @@ -12,21 +12,19 @@ import go import DataFlow::PathGraph -/* +/** * A method that creates a new URL that will send the user * to the oauth2 authorization dialog of the provider. */ - class AuthCodeURL extends Method { AuthCodeURL() { this.hasQualifiedName("golang.org/x/oauth2", "Config", "AuthCodeURL") } } -/* +/** * A flow of a constant string value to a call to AuthCodeURL as the * `state` parameter. */ - -class ConstantStateFlowConf extends TaintTracking::Configuration { +class ConstantStateFlowConf extends DataFlow::Configuration { ConstantStateFlowConf() { this = "ConstantStateFlowConf" } predicate isSource(DataFlow::Node source, Literal state) { @@ -42,7 +40,54 @@ class ConstantStateFlowConf extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } } -from ConstantStateFlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink -where cfg.hasFlowPath(source, sink) +/** A flow to a printer function of the fmt package. */ +class FlowToPrint extends DataFlow::Configuration { + FlowToPrint() { this = "FlowToPrint" } + + predicate isSource(DataFlow::Node source, DataFlow::CallNode call) { + exists(AuthCodeURL m | call = m.getACall() | source = call.getResult()) + } + + predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) { + exists(Fmt::Printer printer | call = printer.getACall() | sink = call.getArgument(_)) + } + + override predicate isSource(DataFlow::Node source) { isSource(source, _) } + + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } +} + +/** Holds if the provided CallNode's result flows to a Printer call as argument. */ +predicate flowsToPrinter(DataFlow::CallNode authCodeURLCall) { + exists(FlowToPrint cfg, DataFlow::PathNode source, DataFlow::PathNode sink | + cfg.hasFlowPath(source, sink) and + cfg.isSource(source.getNode(), authCodeURLCall) + ) +} + +/** + * Holds if the provided CallNode is within the same root as a call + * to a scanner that reads from os.Stdin. + */ +predicate rootContainsCallToStdinScanner(DataFlow::CallNode authCodeURLCall) { + exists(Fmt::ScannerCall scannerCall | scannerCall.getRoot() = authCodeURLCall.getRoot()) + or + exists(Fmt::FScannerCall fScannerCall | + fScannerCall.getReader() = any(ValueEntity v | v.hasQualifiedName("os", "Stdin")).getARead() and + fScannerCall.getRoot() = authCodeURLCall.getRoot() + ) +} + +from + ConstantStateFlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink, + DataFlow::CallNode sinkCall +where + cfg.hasFlowPath(source, sink) and + cfg.isSink(sink.getNode(), sinkCall) and + // Exclude cases that seem to be oauth flows done from within a terminal: + not ( + flowsToPrinter(sinkCall) and + rootContainsCallToStdinScanner(sinkCall) + ) select sink.getNode(), source, sink, "Using a constant $@ to create oauth2 URLs.", source.getNode(), "state string" diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index 8c4ef5b9017..868318db7af 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -94,7 +94,7 @@ module Fmt { } /** The `Print` function or one of its variants. */ - private class Printer extends Function { + class Printer extends Function { Printer() { this.hasQualifiedName("fmt", ["Print", "Printf", "Println"]) } } @@ -131,6 +131,31 @@ module Fmt { exists(int i | if getName() = "Sscanf" then i > 1 else i > 0 | output.isParameter(i)) } } + + /** The `Scan` function or one of its variants, all of which read from os.Stdin */ + class Scanner extends Function { + Scanner() { this.hasQualifiedName("fmt", ["Scan", "Scanf", "Scanln"]) } + } + + /** A call to a `Scanner`. */ + class ScannerCall extends DataFlow::CallNode { + ScannerCall() { this.getTarget() instanceof Scanner } + } + + /** + * The `Fscan` function or one of its variants, + * all of which read from a specified io.Reader + */ + class FScanner extends Function { + FScanner() { this.hasQualifiedName("fmt", ["Fscan", "Fscanf", "Fscanln"]) } + } + + /** A call to a `FScanner`. */ + class FScannerCall extends DataFlow::CallNode { + FScannerCall() { this.getTarget() instanceof FScanner } + + DataFlow::Node getReader() { result = this.getArgument(0) } + } } /** Provides models of commonly used functions in the `io` package. */ diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.go b/ql/test/experimental/CWE-352/ConstantOauth2State.go index 9f7c7f056df..b3200670fb9 100644 --- a/ql/test/experimental/CWE-352/ConstantOauth2State.go +++ b/ql/test/experimental/CWE-352/ConstantOauth2State.go @@ -5,7 +5,10 @@ package main import ( "crypto/rand" "encoding/base64" + "fmt" + "log" "net/http" + "os" "golang.org/x/oauth2" ) @@ -93,7 +96,7 @@ func betterWithVariableStateReturned(w http.ResponseWriter) { } state := generateStateOauthCookie(w) - url := conf.AuthCodeURL(state) // GOOD + url := conf.AuthCodeURL(state) // OK, because the state is not a constant. _ = url // ... } @@ -104,3 +107,86 @@ func generateStateOauthCookie(w http.ResponseWriter) string { // TODO: save the state string to cookies or HTML storage. return state } +func okWithMixedVarState(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + state := fmt.Sprintf("%s-%s", stateStringVar, NewCSRFToken()) + + url := conf.AuthCodeURL(state) // OK, because the state is not a constant. + _ = url + // ... +} + +func NewCSRFToken() string { + b := make([]byte, 128) + rand.Read(b) + randomToken := base64.URLEncoding.EncodeToString(b) + return randomToken +} +func okWithConstStatePrinter(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + url := conf.AuthCodeURL(stateStringConst) // OK, because we're supposedly not exposed to the web, but within a terminal. + fmt.Printf("Visit the URL for the auth dialog: %v", url) + // ... + + var code string + if _, err := fmt.Scan(&code); err != nil { + log.Fatal(err) + } + _ = code + // ... +} +func okWithConstStateFPrinter(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + url := conf.AuthCodeURL(stateStringConst) // OK, because we're supposedly not exposed to the web, but within a terminal. + fmt.Printf("Visit the URL for the auth dialog: %v", url) + // ... + + var code string + if _, err := fmt.Fscan(os.Stdin, &code); err != nil { + log.Fatal(err) + } + _ = code + // ... +} +func badWithConstStatePrinter(w http.ResponseWriter) { + conf := &oauth2.Config{ + ClientID: "YOUR_CLIENT_ID", + ClientSecret: "YOUR_CLIENT_SECRET", + Scopes: []string{"SCOPE1", "SCOPE2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://provider.com/o/oauth2/auth", + TokenURL: "https://provider.com/o/oauth2/token", + }, + } + + url := conf.AuthCodeURL(stateStringConst) // BAD + fmt.Printf("LOG: URL %v", url) + // ... +} From fb78818db73f256b853faf55dfe6cc42a87a4169 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 16 Jul 2020 18:33:35 +0300 Subject: [PATCH 4/8] Fix .expected --- .../CWE-352/ConstantOauth2State.expected | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.expected b/ql/test/experimental/CWE-352/ConstantOauth2State.expected index 00414516e70..ecd1543cd1a 100644 --- a/ql/test/experimental/CWE-352/ConstantOauth2State.expected +++ b/ql/test/experimental/CWE-352/ConstantOauth2State.expected @@ -1,19 +1,35 @@ edges -| ConstantOauth2State.go:15:26:15:32 | "state" : string literal | ConstantOauth2State.go:45:26:45:41 | stateStringConst | -| ConstantOauth2State.go:17:22:17:28 | "state" : string | ConstantOauth2State.go:60:26:60:39 | stateStringVar | -| ConstantOauth2State.go:75:11:75:25 | call to newFixedState : string | ConstantOauth2State.go:76:26:76:30 | state | -| ConstantOauth2State.go:81:9:81:15 | "state" : string | ConstantOauth2State.go:75:11:75:25 | call to newFixedState : string | +| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:48:26:48:41 | stateStringConst | +| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:145:26:145:41 | stateStringConst | +| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:167:26:167:41 | stateStringConst | +| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:189:26:189:41 | stateStringConst | +| ConstantOauth2State.go:20:22:20:28 | "state" : string | ConstantOauth2State.go:63:26:63:39 | stateStringVar | +| ConstantOauth2State.go:78:11:78:25 | call to newFixedState : string | ConstantOauth2State.go:79:26:79:30 | state | +| ConstantOauth2State.go:84:9:84:15 | "state" : string | ConstantOauth2State.go:78:11:78:25 | call to newFixedState : string | +| ConstantOauth2State.go:145:9:145:42 | call to AuthCodeURL : string | ConstantOauth2State.go:146:54:146:56 | url | +| ConstantOauth2State.go:167:9:167:42 | call to AuthCodeURL : string | ConstantOauth2State.go:168:54:168:56 | url | +| ConstantOauth2State.go:189:9:189:42 | call to AuthCodeURL : string | ConstantOauth2State.go:190:28:190:30 | url | nodes -| ConstantOauth2State.go:15:26:15:32 | "state" : string literal | semmle.label | "state" : string literal | -| ConstantOauth2State.go:17:22:17:28 | "state" : string | semmle.label | "state" : string | -| ConstantOauth2State.go:30:26:30:32 | "state" | semmle.label | "state" | -| ConstantOauth2State.go:45:26:45:41 | stateStringConst | semmle.label | stateStringConst | -| ConstantOauth2State.go:60:26:60:39 | stateStringVar | semmle.label | stateStringVar | -| ConstantOauth2State.go:75:11:75:25 | call to newFixedState : string | semmle.label | call to newFixedState : string | -| ConstantOauth2State.go:76:26:76:30 | state | semmle.label | state | -| ConstantOauth2State.go:81:9:81:15 | "state" : string | semmle.label | "state" : string | +| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | semmle.label | "state" : string literal | +| ConstantOauth2State.go:20:22:20:28 | "state" : string | semmle.label | "state" : string | +| ConstantOauth2State.go:33:26:33:32 | "state" | semmle.label | "state" | +| ConstantOauth2State.go:48:26:48:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:63:26:63:39 | stateStringVar | semmle.label | stateStringVar | +| ConstantOauth2State.go:78:11:78:25 | call to newFixedState : string | semmle.label | call to newFixedState : string | +| ConstantOauth2State.go:79:26:79:30 | state | semmle.label | state | +| ConstantOauth2State.go:84:9:84:15 | "state" : string | semmle.label | "state" : string | +| ConstantOauth2State.go:145:9:145:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:145:26:145:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:146:54:146:56 | url | semmle.label | url | +| ConstantOauth2State.go:167:9:167:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:167:26:167:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:168:54:168:56 | url | semmle.label | url | +| ConstantOauth2State.go:189:9:189:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:189:26:189:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:190:28:190:30 | url | semmle.label | url | #select -| ConstantOauth2State.go:30:26:30:32 | "state" | ConstantOauth2State.go:30:26:30:32 | "state" | ConstantOauth2State.go:30:26:30:32 | "state" | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:30:26:30:32 | "state" | state string | -| ConstantOauth2State.go:45:26:45:41 | stateStringConst | ConstantOauth2State.go:15:26:15:32 | "state" : string literal | ConstantOauth2State.go:45:26:45:41 | stateStringConst | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:15:26:15:32 | "state" | state string | -| ConstantOauth2State.go:60:26:60:39 | stateStringVar | ConstantOauth2State.go:17:22:17:28 | "state" : string | ConstantOauth2State.go:60:26:60:39 | stateStringVar | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:17:22:17:28 | "state" | state string | -| ConstantOauth2State.go:76:26:76:30 | state | ConstantOauth2State.go:81:9:81:15 | "state" : string | ConstantOauth2State.go:76:26:76:30 | state | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:81:9:81:15 | "state" | state string | +| ConstantOauth2State.go:33:26:33:32 | "state" | ConstantOauth2State.go:33:26:33:32 | "state" | ConstantOauth2State.go:33:26:33:32 | "state" | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:33:26:33:32 | "state" | state string | +| ConstantOauth2State.go:48:26:48:41 | stateStringConst | ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:48:26:48:41 | stateStringConst | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:18:26:18:32 | "state" | state string | +| ConstantOauth2State.go:63:26:63:39 | stateStringVar | ConstantOauth2State.go:20:22:20:28 | "state" : string | ConstantOauth2State.go:63:26:63:39 | stateStringVar | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:20:22:20:28 | "state" | state string | +| ConstantOauth2State.go:79:26:79:30 | state | ConstantOauth2State.go:84:9:84:15 | "state" : string | ConstantOauth2State.go:79:26:79:30 | state | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:84:9:84:15 | "state" | state string | +| ConstantOauth2State.go:189:26:189:41 | stateStringConst | ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:189:26:189:41 | stateStringConst | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:18:26:18:32 | "state" | state string | From ee4356501aad78901f48bec7de1e43fa40da34e5 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 16 Jul 2020 18:36:40 +0300 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- ql/src/experimental/CWE-352/ConstantOauth2State.qhelp | 6 +++--- ql/src/experimental/CWE-352/ConstantOauth2State.ql | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp b/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp index 4d14d003781..dc15b5de1d9 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp @@ -4,8 +4,8 @@

- Oauth2 clients must implement CSRF protection for the redirection URI, which is typically accomplished by including a "state" value that binds the request to - the user's authenticated state. The Go Oauth2 library allows to specify a "state" value which is then included in the auth code URL, and then provided back by the remote authentication server in the redirect callback, from where it must be validated; failure to do so makes the client susceptible to an CSRF attack. + OAuth 2.0 clients must implement CSRF protection for the redirection URI, which is typically accomplished by including a "state" value that binds the request to + the user's authenticated state. The Go OAuth 2.0 library allows to specify a "state" value which is then included in the auth code URL, and then provided back by the remote authentication server in the redirect callback, from where it must be validated; failure to do so makes the client susceptible to an CSRF attack.

@@ -23,4 +23,4 @@

- \ No newline at end of file + diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql index 43f35befc99..46eed76acaf 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql @@ -1,6 +1,6 @@ /** - * @name Use of constant `state` value in Oauth2 URL. - * @description Using a constant value for the `state` in the oauth2 URL makes the application + * @name Use of constant `state` value in OAuth 2.0 URL. + * @description Using a constant value for the `state` in the OAuth 2.0 URL makes the application * susceptible to CSRF attacks. * @kind path-problem * @problem.severity error @@ -14,7 +14,7 @@ import DataFlow::PathGraph /** * A method that creates a new URL that will send the user - * to the oauth2 authorization dialog of the provider. + * to the OAuth 2.0 authorization dialog of the provider. */ class AuthCodeURL extends Method { AuthCodeURL() { this.hasQualifiedName("golang.org/x/oauth2", "Config", "AuthCodeURL") } From ee2804dfb1731fdfb770522adabb534832857d46 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Fri, 17 Jul 2020 11:01:25 +0300 Subject: [PATCH 6/8] Improve comments --- .../CWE-352/ConstantOauth2State.ql | 18 +++++++++++++----- ql/src/semmle/go/frameworks/Stdlib.qll | 4 ++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql index 46eed76acaf..96c095f637e 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql +++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql @@ -58,7 +58,7 @@ class FlowToPrint extends DataFlow::Configuration { } /** Holds if the provided CallNode's result flows to a Printer call as argument. */ -predicate flowsToPrinter(DataFlow::CallNode authCodeURLCall) { +predicate resultFlowsToPrinter(DataFlow::CallNode authCodeURLCall) { exists(FlowToPrint cfg, DataFlow::PathNode source, DataFlow::PathNode sink | cfg.hasFlowPath(source, sink) and cfg.isSource(source.getNode(), authCodeURLCall) @@ -78,6 +78,17 @@ predicate rootContainsCallToStdinScanner(DataFlow::CallNode authCodeURLCall) { ) } +/** + * Holds if the authCodeURLCall seems to be done within a terminal + * because there are calls to a Printer (fmt.Println and similar), + * and a call to a Scanner (fmt.Scan and similar), + * all of which are typically done within a terminal session. + */ +predicate seemsLikeDoneWithinATerminal(DataFlow::CallNode authCodeURLCall) { + resultFlowsToPrinter(authCodeURLCall) and + rootContainsCallToStdinScanner(authCodeURLCall) +} + from ConstantStateFlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::CallNode sinkCall @@ -85,9 +96,6 @@ where cfg.hasFlowPath(source, sink) and cfg.isSink(sink.getNode(), sinkCall) and // Exclude cases that seem to be oauth flows done from within a terminal: - not ( - flowsToPrinter(sinkCall) and - rootContainsCallToStdinScanner(sinkCall) - ) + not seemsLikeDoneWithinATerminal(sinkCall) select sink.getNode(), source, sink, "Using a constant $@ to create oauth2 URLs.", source.getNode(), "state string" diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index 868318db7af..29d58b89664 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -154,6 +154,10 @@ module Fmt { class FScannerCall extends DataFlow::CallNode { FScannerCall() { this.getTarget() instanceof FScanner } + /** + * Returns the node corresponding to the io.Reader + * argument provided in the call. + */ DataFlow::Node getReader() { result = this.getArgument(0) } } } From 27f62b0b3a7b4194bf76bda8f13b6390e90b052b Mon Sep 17 00:00:00 2001 From: Slavomir Date: Fri, 17 Jul 2020 13:12:18 +0300 Subject: [PATCH 7/8] Fix examples --- ql/src/experimental/CWE-352/ConstantOauth2StateBad.go | 5 +---- ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go | 3 ++- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/ql/src/experimental/CWE-352/ConstantOauth2StateBad.go b/ql/src/experimental/CWE-352/ConstantOauth2StateBad.go index 57b5c295fc1..422736fc610 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2StateBad.go +++ b/ql/src/experimental/CWE-352/ConstantOauth2StateBad.go @@ -1,8 +1,6 @@ package main import ( - "fmt" - "golang.org/x/oauth2" ) @@ -21,7 +19,6 @@ func badWithStringLiteralState() { }, } - url := conf.AuthCodeURL(stateStringVar, oauth2.AccessTypeOffline) - fmt.Printf("Visit the URL for the auth dialog: %v", url) + url := conf.AuthCodeURL(stateStringVar) // ... } diff --git a/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go b/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go index a6854947ff3..50d587dca7f 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go +++ b/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go @@ -27,7 +27,8 @@ func betterWithVariableStateReturned(w http.ResponseWriter) { func generateStateOauthCookie(w http.ResponseWriter) string { b := make([]byte, 128) rand.Read(b) - // TODO: save the state string to cookies or HTML storage. + // TODO: save the state string to cookies or HTML storage, + // and bind it to the authenticated status of the user. state := base64.URLEncoding.EncodeToString(b) return state From 02b5fce67e90272c12788920802fdde6fc5435f3 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 20 Jul 2020 17:46:12 +0300 Subject: [PATCH 8/8] Add go.mod to CWE-352 test folder --- ql/test/experimental/CWE-352/go.mod | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ql/test/experimental/CWE-352/go.mod diff --git a/ql/test/experimental/CWE-352/go.mod b/ql/test/experimental/CWE-352/go.mod new file mode 100644 index 00000000000..037451d4bd2 --- /dev/null +++ b/ql/test/experimental/CWE-352/go.mod @@ -0,0 +1,5 @@ +module custom-queries-go-tests/constant-oauth2-state + +go 1.14 + +require golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d