Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Commit 3018874

Browse files
owen-mcowen-mc
authored
Merge pull request #259 from gagliardetto/oauth2-fixed-state
CWE-352: Use of constant `state` in Oauth2 flow
2 parents a625a4c + 02b5fce commit 3018874

12 files changed

Lines changed: 560 additions & 1 deletion

File tree

‎ql/src/experimental/CWE-352/ConstantOauth2State.qhelp‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
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
8+
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.
9+
</p>
10+
</overview>
11+
<recommendation>
12+
<p>
13+
Always include a unique, non-guessable <code>state</code> value (provided to the call to <code>AuthCodeURL</code> function) that is also bound to the user's authenticated state with each authentication request, and then validated in the redirect callback.
14+
</p>
15+
</recommendation>
16+
<example>
17+
<p>
18+
The first example shows you the use of a constant state (bad).
19+
</p>
20+
<sample src="ConstantOauth2StateBad.go" />
21+
<p>
22+
The second example shows a better implementation idea.
23+
</p>
24+
<sample src="ConstantOauth2StateBetter.go" />
25+
</example>
26+
</qhelp>

‎ql/src/experimental/CWE-352/ConstantOauth2State.ql‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* @name Use of constant `state` value in OAuth 2.0 URL.
3+
* @description Using a constant value for the `state` in the OAuth 2.0 URL makes the application
4+
* susceptible to CSRF attacks.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @id go/constant-oauth2-state
8+
* @tags security
9+
* external/cwe/cwe-352
10+
*/
11+
12+
import go
13+
import DataFlow::PathGraph
14+
15+
/**
16+
* A method that creates a new URL that will send the user
17+
* to the OAuth 2.0 authorization dialog of the provider.
18+
*/
19+
class AuthCodeURL extends Method {
20+
AuthCodeURL() { this.hasQualifiedName("golang.org/x/oauth2", "Config", "AuthCodeURL") }
21+
}
22+
23+
/**
24+
* A flow of a constant string value to a call to AuthCodeURL as the
25+
* `state` parameter.
26+
*/
27+
class ConstantStateFlowConf extends DataFlow::Configuration {
28+
ConstantStateFlowConf() { this = "ConstantStateFlowConf" }
29+
30+
predicate isSource(DataFlow::Node source, Literal state) {
31+
state.isConst() and source.asExpr() = state
32+
}
33+
34+
predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) {
35+
exists(AuthCodeURL m | call = m.getACall() | sink = call.getArgument(0))
36+
}
37+
38+
override predicate isSource(DataFlow::Node source) { isSource(source, _) }
39+
40+
override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
41+
}
42+
43+
/** A flow to a printer function of the fmt package. */
44+
class FlowToPrint extends DataFlow::Configuration {
45+
FlowToPrint() { this = "FlowToPrint" }
46+
47+
predicate isSource(DataFlow::Node source, DataFlow::CallNode call) {
48+
exists(AuthCodeURL m | call = m.getACall() | source = call.getResult())
49+
}
50+
51+
predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) {
52+
exists(Fmt::Printer printer | call = printer.getACall() | sink = call.getArgument(_))
53+
}
54+
55+
override predicate isSource(DataFlow::Node source) { isSource(source, _) }
56+
57+
override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
58+
}
59+
60+
/** Holds if the provided CallNode's result flows to a Printer call as argument. */
61+
predicate resultFlowsToPrinter(DataFlow::CallNode authCodeURLCall) {
62+
exists(FlowToPrint cfg, DataFlow::PathNode source, DataFlow::PathNode sink |
63+
cfg.hasFlowPath(source, sink) and
64+
cfg.isSource(source.getNode(), authCodeURLCall)
65+
)
66+
}
67+
68+
/**
69+
* Holds if the provided CallNode is within the same root as a call
70+
* to a scanner that reads from os.Stdin.
71+
*/
72+
predicate rootContainsCallToStdinScanner(DataFlow::CallNode authCodeURLCall) {
73+
exists(Fmt::ScannerCall scannerCall | scannerCall.getRoot() = authCodeURLCall.getRoot())
74+
or
75+
exists(Fmt::FScannerCall fScannerCall |
76+
fScannerCall.getReader() = any(ValueEntity v | v.hasQualifiedName("os", "Stdin")).getARead() and
77+
fScannerCall.getRoot() = authCodeURLCall.getRoot()
78+
)
79+
}
80+
81+
/**
82+
* Holds if the authCodeURLCall seems to be done within a terminal
83+
* because there are calls to a Printer (fmt.Println and similar),
84+
* and a call to a Scanner (fmt.Scan and similar),
85+
* all of which are typically done within a terminal session.
86+
*/
87+
predicate seemsLikeDoneWithinATerminal(DataFlow::CallNode authCodeURLCall) {
88+
resultFlowsToPrinter(authCodeURLCall) and
89+
rootContainsCallToStdinScanner(authCodeURLCall)
90+
}
91+
92+
from
93+
ConstantStateFlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink,
94+
DataFlow::CallNode sinkCall
95+
where
96+
cfg.hasFlowPath(source, sink) and
97+
cfg.isSink(sink.getNode(), sinkCall) and
98+
// Exclude cases that seem to be oauth flows done from within a terminal:
99+
not seemsLikeDoneWithinATerminal(sinkCall)
100+
select sink.getNode(), source, sink, "Using a constant $@ to create oauth2 URLs.", source.getNode(),
101+
"state string"

‎ql/src/experimental/CWE-352/ConstantOauth2StateBad.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package main
2+
3+
import (
4+
"golang.org/x/oauth2"
5+
)
6+
7+
func main() {}
8+
9+
var stateStringVar = "state"
10+
11+
func badWithStringLiteralState() {
12+
conf := &oauth2.Config{
13+
ClientID: "YOUR_CLIENT_ID",
14+
ClientSecret: "YOUR_CLIENT_SECRET",
15+
Scopes: []string{"SCOPE1", "SCOPE2"},
16+
Endpoint: oauth2.Endpoint{
17+
AuthURL: "https://provider.com/o/oauth2/auth",
18+
TokenURL: "https://provider.com/o/oauth2/token",
19+
},
20+
}
21+
22+
url := conf.AuthCodeURL(stateStringVar)
23+
// ...
24+
}

‎ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
package main
2+
3+
import (
4+
"crypto/rand"
5+
"encoding/base64"
6+
"net/http"
7+
8+
"golang.org/x/oauth2"
9+
)
10+
11+
func betterWithVariableStateReturned(w http.ResponseWriter) {
12+
conf := &oauth2.Config{
13+
ClientID: "YOUR_CLIENT_ID",
14+
ClientSecret: "YOUR_CLIENT_SECRET",
15+
Scopes: []string{"SCOPE1", "SCOPE2"},
16+
Endpoint: oauth2.Endpoint{
17+
AuthURL: "https://provider.com/o/oauth2/auth",
18+
TokenURL: "https://provider.com/o/oauth2/token",
19+
},
20+
}
21+
22+
state := generateStateOauthCookie(w)
23+
url := conf.AuthCodeURL(state)
24+
_ = url
25+
// ...
26+
}
27+
func generateStateOauthCookie(w http.ResponseWriter) string {
28+
b := make([]byte, 128)
29+
rand.Read(b)
30+
// TODO: save the state string to cookies or HTML storage,
31+
// and bind it to the authenticated status of the user.
32+
state := base64.URLEncoding.EncodeToString(b)
33+
34+
return state
35+
}

‎ql/src/semmle/go/frameworks/Stdlib.qll‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ module Fmt {
9494
}
9595

9696
/** The `Print` function or one of its variants. */
97-
private class Printer extends Function {
97+
class Printer extends Function {
9898
Printer() { this.hasQualifiedName("fmt", ["Print", "Printf", "Println"]) }
9999
}
100100

@@ -131,6 +131,35 @@ module Fmt {
131131
exists(int i | if getName() = "Sscanf" then i > 1 else i > 0 | output.isParameter(i))
132132
}
133133
}
134+
135+
/** The `Scan` function or one of its variants, all of which read from os.Stdin */
136+
class Scanner extends Function {
137+
Scanner() { this.hasQualifiedName("fmt", ["Scan", "Scanf", "Scanln"]) }
138+
}
139+
140+
/** A call to a `Scanner`. */
141+
class ScannerCall extends DataFlow::CallNode {
142+
ScannerCall() { this.getTarget() instanceof Scanner }
143+
}
144+
145+
/**
146+
* The `Fscan` function or one of its variants,
147+
* all of which read from a specified io.Reader
148+
*/
149+
class FScanner extends Function {
150+
FScanner() { this.hasQualifiedName("fmt", ["Fscan", "Fscanf", "Fscanln"]) }
151+
}
152+
153+
/** A call to a `FScanner`. */
154+
class FScannerCall extends DataFlow::CallNode {
155+
FScannerCall() { this.getTarget() instanceof FScanner }
156+
157+
/**
158+
* Returns the node corresponding to the io.Reader
159+
* argument provided in the call.
160+
*/
161+
DataFlow::Node getReader() { result = this.getArgument(0) }
162+
}
134163
}
135164

136165
/** Provides models of commonly used functions in the `io` package. */

‎ql/test/experimental/CWE-352/ConstantOauth2State.expected‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
edges
2+
| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:48:26:48:41 | stateStringConst |
3+
| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:145:26:145:41 | stateStringConst |
4+
| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:167:26:167:41 | stateStringConst |
5+
| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | ConstantOauth2State.go:189:26:189:41 | stateStringConst |
6+
| ConstantOauth2State.go:20:22:20:28 | "state" : string | ConstantOauth2State.go:63:26:63:39 | stateStringVar |
7+
| ConstantOauth2State.go:78:11:78:25 | call to newFixedState : string | ConstantOauth2State.go:79:26:79:30 | state |
8+
| ConstantOauth2State.go:84:9:84:15 | "state" : string | ConstantOauth2State.go:78:11:78:25 | call to newFixedState : string |
9+
| ConstantOauth2State.go:145:9:145:42 | call to AuthCodeURL : string | ConstantOauth2State.go:146:54:146:56 | url |
10+
| ConstantOauth2State.go:167:9:167:42 | call to AuthCodeURL : string | ConstantOauth2State.go:168:54:168:56 | url |
11+
| ConstantOauth2State.go:189:9:189:42 | call to AuthCodeURL : string | ConstantOauth2State.go:190:28:190:30 | url |
12+
nodes
13+
| ConstantOauth2State.go:18:26:18:32 | "state" : string literal | semmle.label | "state" : string literal |
14+
| ConstantOauth2State.go:20:22:20:28 | "state" : string | semmle.label | "state" : string |
15+
| ConstantOauth2State.go:33:26:33:32 | "state" | semmle.label | "state" |
16+
| ConstantOauth2State.go:48:26:48:41 | stateStringConst | semmle.label | stateStringConst |
17+
| ConstantOauth2State.go:63:26:63:39 | stateStringVar | semmle.label | stateStringVar |
18+
| ConstantOauth2State.go:78:11:78:25 | call to newFixedState : string | semmle.label | call to newFixedState : string |
19+
| ConstantOauth2State.go:79:26:79:30 | state | semmle.label | state |
20+
| ConstantOauth2State.go:84:9:84:15 | "state" : string | semmle.label | "state" : string |
21+
| ConstantOauth2State.go:145:9:145:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string |
22+
| ConstantOauth2State.go:145:26:145:41 | stateStringConst | semmle.label | stateStringConst |
23+
| ConstantOauth2State.go:146:54:146:56 | url | semmle.label | url |
24+
| ConstantOauth2State.go:167:9:167:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string |
25+
| ConstantOauth2State.go:167:26:167:41 | stateStringConst | semmle.label | stateStringConst |
26+
| ConstantOauth2State.go:168:54:168:56 | url | semmle.label | url |
27+
| ConstantOauth2State.go:189:9:189:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string |
28+
| ConstantOauth2State.go:189:26:189:41 | stateStringConst | semmle.label | stateStringConst |
29+
| ConstantOauth2State.go:190:28:190:30 | url | semmle.label | url |
30+
#select
31+
| 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 |
32+
| 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 |
33+
| 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 |
34+
| 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 |
35+
| 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 |

0 commit comments

Comments
 (0)