From 8cc8b8ef476ccb1652aa7d158bbf30e05ecf2343 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 16 Jul 2020 12:38:08 +0300 Subject: [PATCH] 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