Skip to content

Commit 16ef11a

Browse files
owen-mcowen-mc
committed
Make ConstantOauth2State use new API
Removed edges were only there originally due to multiple configurations being in scope. `DataFlow::PathNode` has union semantics for configurations. Nodes are only generated if they are reachable from a source, but this includes sources from other configurations. No alerts are lost.
1 parent fbd0c4e commit 16ef11a

2 files changed

Lines changed: 29 additions & 72 deletions

File tree

‎go/ql/src/Security/CWE-352/ConstantOauth2State.ql‎

Lines changed: 29 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
*/
1313

1414
import go
15-
import DataFlow::PathGraph
1615

1716
/**
1817
* A method that creates a new URL that will send the user
@@ -24,18 +23,12 @@ class AuthCodeUrl extends Method {
2423
}
2524
}
2625

27-
/**
28-
* A flow of a constant string value to a call to `AuthCodeURL` as the
29-
* `state` parameter.
30-
*/
31-
class ConstantStateFlowConf extends DataFlow::Configuration {
32-
ConstantStateFlowConf() { this = "ConstantStateFlowConf" }
33-
34-
predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
26+
module ConstantStateFlowConfig implements DataFlow::ConfigSig {
27+
additional predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
3528
exists(AuthCodeUrl m | call = m.getACall() | sink = call.getArgument(0))
3629
}
3730

38-
override predicate isSource(DataFlow::Node source) {
31+
predicate isSource(DataFlow::Node source) {
3932
source.isConst() and
4033
not DataFlow::isReturnedWithError(source) and
4134
// Avoid duplicate paths by not considering reads from constants as sources themselves:
@@ -46,9 +39,13 @@ class ConstantStateFlowConf extends DataFlow::Configuration {
4639
)
4740
}
4841

49-
override predicate isSink(DataFlow::Node sink) { this.isSinkCall(sink, _) }
42+
predicate isSink(DataFlow::Node sink) { isSinkCall(sink, _) }
5043
}
5144

45+
module Flow = DataFlow::Global<ConstantStateFlowConfig>;
46+
47+
import Flow::PathGraph
48+
5249
/**
5350
* Holds if `pred` writes a URL to the `RedirectURL` field of the `succ` `Config` object.
5451
*
@@ -77,17 +74,8 @@ string getAnOobOauth2Url() {
7774
result.matches("%://127.0.0.1%")
7875
}
7976

80-
/**
81-
* A flow of a URL indicating the OAuth redirect doesn't point to a publicly
82-
* accessible address, to the receiver of an `AuthCodeURL` call.
83-
*
84-
* Note we accept localhost and 127.0.0.1 on the assumption this is probably a transient
85-
* listener; if it actually is a persistent server then that really is vulnerable to CSRF.
86-
*/
87-
class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
88-
PrivateUrlFlowsToAuthCodeUrlCall() { this = "PrivateUrlFlowsToConfig" }
89-
90-
override predicate isSource(DataFlow::Node source) {
77+
module PrivateUrlFlowsToAuthCodeUrlCallConfig implements DataFlow::ConfigSig {
78+
predicate isSource(DataFlow::Node source) {
9179
source.getStringValue() = getAnOobOauth2Url() and
9280
// Avoid duplicate paths by excluding constant variable references from
9381
// themselves being sources:
@@ -98,7 +86,7 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
9886
)
9987
}
10088

101-
override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
89+
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
10290
// Propagate from a RedirectURL field to a whole Config
10391
isUrlTaintingConfigStep(pred, succ)
10492
or
@@ -113,13 +101,16 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
113101
)
114102
}
115103

116-
predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
104+
additional predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
117105
exists(AuthCodeUrl m | call = m.getACall() | sink = call.getReceiver())
118106
}
119107

120-
override predicate isSink(DataFlow::Node sink) { this.isSinkCall(sink, _) }
108+
predicate isSink(DataFlow::Node sink) { isSinkCall(sink, _) }
121109
}
122110

111+
module PrivateUrlFlowsToAuthCodeUrlCallFlow =
112+
DataFlow::Global<PrivateUrlFlowsToAuthCodeUrlCallConfig>;
113+
123114
/**
124115
* Holds if a URL indicating the OAuth redirect doesn't point to a publicly
125116
* accessible address, to the receiver of an `AuthCodeURL` call.
@@ -128,33 +119,27 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
128119
* listener; if it actually is a persistent server then that really is vulnerable to CSRF.
129120
*/
130121
predicate privateUrlFlowsToAuthCodeUrlCall(DataFlow::CallNode call) {
131-
exists(PrivateUrlFlowsToAuthCodeUrlCall flowConfig, DataFlow::Node receiver |
132-
flowConfig.hasFlowTo(receiver) and
133-
flowConfig.isSinkCall(receiver, call)
122+
exists(DataFlow::Node receiver |
123+
PrivateUrlFlowsToAuthCodeUrlCallFlow::flowTo(receiver) and
124+
PrivateUrlFlowsToAuthCodeUrlCallConfig::isSinkCall(receiver, call)
134125
)
135126
}
136127

137-
/** A flow from `golang.org/x/oauth2.Config.AuthCodeUrl`'s result to a logging function. */
138-
class FlowToPrint extends DataFlow::Configuration {
139-
FlowToPrint() { this = "FlowToPrint" }
140-
141-
predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
128+
module FlowToPrintConfig implements DataFlow::ConfigSig {
129+
additional predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
142130
exists(LoggerCall logCall | call = logCall | sink = logCall.getAMessageComponent())
143131
}
144132

145-
override predicate isSource(DataFlow::Node source) {
146-
source = any(AuthCodeUrl m).getACall().getResult()
147-
}
133+
predicate isSource(DataFlow::Node source) { source = any(AuthCodeUrl m).getACall().getResult() }
148134

149-
override predicate isSink(DataFlow::Node sink) { this.isSinkCall(sink, _) }
135+
predicate isSink(DataFlow::Node sink) { isSinkCall(sink, _) }
150136
}
151137

138+
module FlowToPrintFlow = DataFlow::Global<FlowToPrintConfig>;
139+
152140
/** Holds if the provided `CallNode`'s result flows to an argument of a printer call. */
153141
predicate resultFlowsToPrinter(DataFlow::CallNode authCodeUrlCall) {
154-
exists(FlowToPrint cfg, DataFlow::PathNode source |
155-
cfg.hasFlowPath(source, _) and
156-
authCodeUrlCall.getResult() = source.getNode()
157-
)
142+
FlowToPrintFlow::flow(authCodeUrlCall.getResult(), _)
158143
}
159144

160145
/** Get a data-flow node that reads the value of `os.Stdin`. */
@@ -197,12 +182,10 @@ predicate seemsLikeDoneWithinATerminal(DataFlow::CallNode authCodeUrlCall) {
197182
containsCallToStdinScanner(authCodeUrlCall.getRoot())
198183
}
199184

200-
from
201-
ConstantStateFlowConf cfg, DataFlow::PathNode source, DataFlow::PathNode sink,
202-
DataFlow::CallNode sinkCall
185+
from Flow::PathNode source, Flow::PathNode sink, DataFlow::CallNode sinkCall
203186
where
204-
cfg.hasFlowPath(source, sink) and
205-
cfg.isSinkCall(sink.getNode(), sinkCall) and
187+
Flow::flowPath(source, sink) and
188+
ConstantStateFlowConfig::isSinkCall(sink.getNode(), sinkCall) and
206189
// Exclude cases that seem to be oauth flows done from within a terminal:
207190
not seemsLikeDoneWithinATerminal(sinkCall) and
208191
not privateUrlFlowsToAuthCodeUrlCall(sinkCall)

‎go/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.expected‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,6 @@ edges
1111
| ConstantOauth2State.go:22:22:22:28 | "state" | ConstantOauth2State.go:65:26:65:39 | stateStringVar |
1212
| ConstantOauth2State.go:80:11:80:25 | call to newFixedState | ConstantOauth2State.go:81:26:81:30 | state |
1313
| ConstantOauth2State.go:86:9:86:15 | "state" | ConstantOauth2State.go:80:11:80:25 | call to newFixedState |
14-
| ConstantOauth2State.go:147:9:147:42 | call to AuthCodeURL | ConstantOauth2State.go:148:54:148:56 | url |
15-
| ConstantOauth2State.go:169:9:169:42 | call to AuthCodeURL | ConstantOauth2State.go:170:54:170:56 | url |
16-
| ConstantOauth2State.go:191:9:191:42 | call to AuthCodeURL | ConstantOauth2State.go:192:54:192:56 | url |
17-
| ConstantOauth2State.go:210:9:210:42 | call to AuthCodeURL | ConstantOauth2State.go:211:54:211:56 | url |
18-
| ConstantOauth2State.go:232:9:232:42 | call to AuthCodeURL | ConstantOauth2State.go:233:28:233:30 | url |
19-
| ConstantOauth2State.go:239:17:239:39 | "http://localhost:8080" | ConstantOauth2State.go:249:9:249:12 | conf |
20-
| ConstantOauth2State.go:256:17:256:67 | call to Sprintf | ConstantOauth2State.go:266:9:266:12 | conf |
21-
| ConstantOauth2State.go:256:38:256:60 | "http://localhost:8080" | ConstantOauth2State.go:256:17:256:67 | call to Sprintf |
22-
| ConstantOauth2State.go:272:17:272:21 | "oob" | ConstantOauth2State.go:282:9:282:12 | conf |
2314
nodes
2415
| ConstantOauth2State.go:20:26:20:32 | "state" | semmle.label | "state" |
2516
| ConstantOauth2State.go:22:22:22:28 | "state" | semmle.label | "state" |
@@ -29,30 +20,13 @@ nodes
2920
| ConstantOauth2State.go:80:11:80:25 | call to newFixedState | semmle.label | call to newFixedState |
3021
| ConstantOauth2State.go:81:26:81:30 | state | semmle.label | state |
3122
| ConstantOauth2State.go:86:9:86:15 | "state" | semmle.label | "state" |
32-
| ConstantOauth2State.go:147:9:147:42 | call to AuthCodeURL | semmle.label | call to AuthCodeURL |
3323
| ConstantOauth2State.go:147:26:147:41 | stateStringConst | semmle.label | stateStringConst |
34-
| ConstantOauth2State.go:148:54:148:56 | url | semmle.label | url |
35-
| ConstantOauth2State.go:169:9:169:42 | call to AuthCodeURL | semmle.label | call to AuthCodeURL |
3624
| ConstantOauth2State.go:169:26:169:41 | stateStringConst | semmle.label | stateStringConst |
37-
| ConstantOauth2State.go:170:54:170:56 | url | semmle.label | url |
38-
| ConstantOauth2State.go:191:9:191:42 | call to AuthCodeURL | semmle.label | call to AuthCodeURL |
3925
| ConstantOauth2State.go:191:26:191:41 | stateStringConst | semmle.label | stateStringConst |
40-
| ConstantOauth2State.go:192:54:192:56 | url | semmle.label | url |
41-
| ConstantOauth2State.go:210:9:210:42 | call to AuthCodeURL | semmle.label | call to AuthCodeURL |
4226
| ConstantOauth2State.go:210:26:210:41 | stateStringConst | semmle.label | stateStringConst |
43-
| ConstantOauth2State.go:211:54:211:56 | url | semmle.label | url |
44-
| ConstantOauth2State.go:232:9:232:42 | call to AuthCodeURL | semmle.label | call to AuthCodeURL |
4527
| ConstantOauth2State.go:232:26:232:41 | stateStringConst | semmle.label | stateStringConst |
46-
| ConstantOauth2State.go:233:28:233:30 | url | semmle.label | url |
47-
| ConstantOauth2State.go:239:17:239:39 | "http://localhost:8080" | semmle.label | "http://localhost:8080" |
48-
| ConstantOauth2State.go:249:9:249:12 | conf | semmle.label | conf |
4928
| ConstantOauth2State.go:249:26:249:41 | stateStringConst | semmle.label | stateStringConst |
50-
| ConstantOauth2State.go:256:17:256:67 | call to Sprintf | semmle.label | call to Sprintf |
51-
| ConstantOauth2State.go:256:38:256:60 | "http://localhost:8080" | semmle.label | "http://localhost:8080" |
52-
| ConstantOauth2State.go:266:9:266:12 | conf | semmle.label | conf |
5329
| ConstantOauth2State.go:266:26:266:41 | stateStringConst | semmle.label | stateStringConst |
54-
| ConstantOauth2State.go:272:17:272:21 | "oob" | semmle.label | "oob" |
55-
| ConstantOauth2State.go:282:9:282:12 | conf | semmle.label | conf |
5630
| ConstantOauth2State.go:282:26:282:41 | stateStringConst | semmle.label | stateStringConst |
5731
subpaths
5832
#select

0 commit comments

Comments
 (0)