diff --git a/change-notes/2020-08-18-oauth2.md b/change-notes/2020-08-18-oauth2.md new file mode 100644 index 00000000000..aa4b566a3f0 --- /dev/null +++ b/change-notes/2020-08-18-oauth2.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Use of constant `state` value in OAuth 2.0 URL" (`go/constant-oauth2-state`) has been promoted from experimental status. This checks for use of a constant state value in generating an OAuth2 redirect URL, which may open the way for a CSRF attack. diff --git a/ql/src/Security/CWE-327/InsecureTLS.ql b/ql/src/Security/CWE-327/InsecureTLS.ql index 879605ea434..edfbdc9bfc5 100644 --- a/ql/src/Security/CWE-327/InsecureTLS.ql +++ b/ql/src/Security/CWE-327/InsecureTLS.ql @@ -50,20 +50,6 @@ int getASecureTlsVersion() { */ int getATlsVersion() { result = getASecureTlsVersion() or isInsecureTlsVersion(result, _, _) } -/** - * Holds if `node` refers to a value returned alongside a non-nil error value. - * - * For example, `0` in `func tryGetInt() (int, error) { return 0, errors.New("no good") }` - */ -predicate isReturnedWithError(DataFlow::Node node) { - exists(ReturnStmt ret | - ret.getExpr(0) = node.asExpr() and - ret.getNumExpr() = 2 and - ret.getExpr(1).getType() instanceof ErrorType - // That last condition implies ret.getExpr(1) is non-nil, since nil doesn't implement `error` - ) -} - /** * Flow of TLS versions into a `tls.Config` struct, to the `MinVersion` and `MaxVersion` fields. */ @@ -76,7 +62,7 @@ class TlsVersionFlowConfig extends TaintTracking::Configuration { predicate isSource(DataFlow::Node source, int val) { val = source.getIntValue() and val = getATlsVersion() and - not isReturnedWithError(source) + not DataFlow::isReturnedWithError(source) } /** diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp b/ql/src/Security/CWE-352/ConstantOauth2State.qhelp similarity index 63% rename from ql/src/experimental/CWE-352/ConstantOauth2State.qhelp rename to ql/src/Security/CWE-352/ConstantOauth2State.qhelp index dc15b5de1d9..4400d60e304 100644 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.qhelp +++ b/ql/src/Security/CWE-352/ConstantOauth2State.qhelp @@ -5,7 +5,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 - 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. + the user's authenticated state. The Go OAuth 2.0 library allows you to specify a "state" value which is then included in the auth code URL. That state is 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,8 @@

+ +
  • IETF: The OAuth 2.0 Authorization Framework
    • +
  • IETF: OAuth 2.0 Security Best Current Practice
    • +     diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql new file mode 100644 index 00000000000..f00cf749400 --- /dev/null +++ b/ql/src/Security/CWE-352/ConstantOauth2State.ql @@ -0,0 +1,201 @@ +/** + * @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 + * @precision high + * @id go/constant-oauth2-state + * @tags security + * external/cwe/cwe-352 + */ + +import go +import DataFlow::PathGraph + +/** + * A method that creates a new URL that will send the user + * to the OAuth 2.0 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 DataFlow::Configuration { + ConstantStateFlowConf() { this = "ConstantStateFlowConf" } + + predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) { + exists(AuthCodeURL m | call = m.getACall() | sink = call.getArgument(0)) + } + + override predicate isSource(DataFlow::Node source) { + source.isConst() and + not DataFlow::isReturnedWithError(source) and + // Avoid duplicate paths by not considering reads from constants as sources themselves: + ( + source.asExpr() instanceof StringLit + or + source.asExpr() instanceof AddExpr + ) + } + + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } +} + +/** + * Holds if `pred` writes a URL to the `RedirectURL` field of the `succ` `Config` object. + * + * This propagates flow from the RedirectURL field to the whole Config object. + */ +predicate isUrlTaintingConfigStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(Write w, Field f | f.hasQualifiedName("golang.org/x/oauth2", "Config", "RedirectURL") | + w.writesField(succ.(DataFlow::PostUpdateNode).getPreUpdateNode(), f, pred) + ) +} + +/** + * Gets a URL or pseudo-URL that suggests an out-of-band OAuth2 flow or use of a transient + * local listener to receive an OAuth2 redirect. + */ +bindingset[result] +string getAnOobOauth2Url() { + // The following are pseudo-URLs seen in the wild to indicate the authenticating site + // should display a code for the user to manually convey, rather than directing: + result in ["urn:ietf:wg:oauth:2.0:oob", "urn:ietf:wg:oauth:2.0:oob:auto", "oob", "code"] or + // Alternatively some non-web tools will create a temporary local webserver to handle the + // OAuth2 redirect: + result.matches("%://localhost%") or + result.matches("%://127.0.0.1%") +} + +/** + * A flow of a URL indicating the OAuth redirect doesn't point to a publicly + * accessible address, to the receiver of an `AuthCodeURL` call. + * + * Note we accept localhost and 127.0.0.1 on the assumption this is probably a transient + * listener; if it actually is a persistent server then that really is vulnerable to CSRF. + */ +class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration { + PrivateUrlFlowsToAuthCodeUrlCall() { this = "PrivateUrlFlowsToConfig" } + + override predicate isSource(DataFlow::Node source) { + source.getStringValue() = getAnOobOauth2Url() and + // Avoid duplicate paths by excluding constant variable references from + // themselves being sources: + ( + source.asExpr() instanceof StringLit + or + source.asExpr() instanceof AddExpr + ) + } + + override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { + // Propagate from a RedirectURL field to a whole Config + isUrlTaintingConfigStep(pred, succ) + or + // Propagate across deref and address-taking steps + TaintTracking::referenceStep(pred, succ) + or + // Propagate across Sprintf and similar calls + TaintTracking::functionModelStep(any(Fmt::Sprinter s), pred, succ) + } + + predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) { + exists(AuthCodeURL m | call = m.getACall() | sink = call.getReceiver()) + } + + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } +} + +/** + * Holds if a URL indicating the OAuth redirect doesn't point to a publicly + * accessible address, to the receiver of an `AuthCodeURL` call. + * + * Note we accept localhost and 127.0.0.1 on the assumption this is probably a transient + * listener; if it actually is a persistent server then that really is vulnerable to CSRF. + */ +predicate privateUrlFlowsToAuthCodeUrlCall(DataFlow::CallNode call) { + exists(PrivateUrlFlowsToAuthCodeUrlCall flowConfig, DataFlow::Node receiver | + flowConfig.hasFlowTo(receiver) and + flowConfig.isSink(receiver, call) + ) +} + +/** A flow from `golang.org/x/oauth2.Config.AuthCodeURL`'s result to a logging function. */ +class FlowToPrint extends DataFlow::Configuration { + FlowToPrint() { this = "FlowToPrint" } + + predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) { + exists(LoggerCall logCall | call = logCall | sink = logCall.getAMessageComponent()) + } + + override predicate isSource(DataFlow::Node source) { + source = any(AuthCodeURL m).getACall().getResult() + } + + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } +} + +/** Holds if the provided `CallNode`'s result flows to an argument of a printer call. */ +predicate resultFlowsToPrinter(DataFlow::CallNode authCodeURLCall) { + exists(FlowToPrint cfg, DataFlow::PathNode source, DataFlow::PathNode sink | + cfg.hasFlowPath(source, sink) and + authCodeURLCall.getResult() = source.getNode() + ) +} + +/** Get a data-flow node that reads the value of `os.Stdin`. */ +DataFlow::Node getAStdinNode() { + exists(ValueEntity v | + v.hasQualifiedName("os", "Stdin") and result = globalValueNumber(v.getARead()).getANode() + ) +} + +/** + * Gets a call to a scanner function that reads from `os.Stdin`, or which creates a scanner + * instance wrapping `os.Stdin`. + */ +DataFlow::CallNode getAScannerCall() { + result = any(Fmt::Scanner f).getACall() + or + exists(Fmt::FScanner f | + result = f.getACall() and f.getReader().getNode(result) = getAStdinNode() + ) + or + exists(Bufio::NewScanner f | + result = f.getACall() and f.getReader().getNode(result) = getAStdinNode() + ) +} + +/** + * Holds if the provided `CallNode` is within the same root as a call + * to a scanner that reads from `os.Stdin`. + */ +predicate containsCallToStdinScanner(FuncDef funcDef) { getAScannerCall().getRoot() = funcDef } + +/** + * 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 + containsCallToStdinScanner(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 seemsLikeDoneWithinATerminal(sinkCall) and + not privateUrlFlowsToAuthCodeUrlCall(sinkCall) +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/Security/CWE-352/ConstantOauth2StateBad.go similarity index 100% rename from ql/src/experimental/CWE-352/ConstantOauth2StateBad.go rename to ql/src/Security/CWE-352/ConstantOauth2StateBad.go diff --git a/ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go b/ql/src/Security/CWE-352/ConstantOauth2StateBetter.go similarity index 100% rename from ql/src/experimental/CWE-352/ConstantOauth2StateBetter.go rename to ql/src/Security/CWE-352/ConstantOauth2StateBetter.go diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql deleted file mode 100644 index 96c095f637e..00000000000 --- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql +++ /dev/null @@ -1,101 +0,0 @@ -/** - * @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 - * @id go/constant-oauth2-state - * @tags security - * external/cwe/cwe-352 - */ - -import go -import DataFlow::PathGraph - -/** - * A method that creates a new URL that will send the user - * to the OAuth 2.0 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 DataFlow::Configuration { - ConstantStateFlowConf() { this = "ConstantStateFlowConf" } - - 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, _) } -} - -/** 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 resultFlowsToPrinter(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() - ) -} - -/** - * 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 -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 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/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll index 5fdd170574e..b3aee357dc1 100644 --- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -875,6 +875,20 @@ Node extractTupleElement(Node t, int i) { ) } +/** + * Holds if `node` refers to a value returned alongside a non-nil error value. + * + * For example, `0` in `func tryGetInt() (int, error) { return 0, errors.New("no good") }` + */ +predicate isReturnedWithError(Node node) { + exists(ReturnStmt ret, int nodeArg, int errorArg | + ret.getExpr(nodeArg) = node.asExpr() and + nodeArg != errorArg and + ret.getExpr(errorArg).getType() instanceof ErrorType + // That last condition implies ret.getExpr(errorArg) is non-nil, since nil doesn't implement `error` + ) +} + /** * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local * (intra-procedural) step. diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index de32af9b659..919cb1b14ce 100644 --- a/ql/src/semmle/go/frameworks/Stdlib.qll +++ b/ql/src/semmle/go/frameworks/Stdlib.qll @@ -107,28 +107,18 @@ module Fmt { 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 } /** * Returns the node corresponding to the io.Reader * argument provided in the call. */ - DataFlow::Node getReader() { result = this.getArgument(0) } + FunctionInput getReader() { result.isParameter(0) } } } diff --git a/ql/src/semmle/go/frameworks/stdlib/Bufio.qll b/ql/src/semmle/go/frameworks/stdlib/Bufio.qll index d5a6f80ead9..45012addcd6 100644 --- a/ql/src/semmle/go/frameworks/stdlib/Bufio.qll +++ b/ql/src/semmle/go/frameworks/stdlib/Bufio.qll @@ -6,6 +6,19 @@ import go /** Provides models of commonly used functions in the `bufio` package. */ module Bufio { + /** + * The function `bufio.NewScanner`. + */ + class NewScanner extends Function { + NewScanner() { this.hasQualifiedName("bufio", "NewScanner") } + + /** + * Gets the input corresponding to the `io.Reader` + * argument provided in the call. + */ + FunctionInput getReader() { result.isParameter(0) } + } + private class FunctionModels extends TaintTracking::FunctionModel { FunctionInput inp; FunctionOutput outp; diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.expected b/ql/test/experimental/CWE-352/ConstantOauth2State.expected deleted file mode 100644 index ecd1543cd1a..00000000000 --- a/ql/test/experimental/CWE-352/ConstantOauth2State.expected +++ /dev/null @@ -1,35 +0,0 @@ -edges -| 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: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: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 | diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.qlref b/ql/test/experimental/CWE-352/ConstantOauth2State.qlref deleted file mode 100644 index 82c080c7754..00000000000 --- a/ql/test/experimental/CWE-352/ConstantOauth2State.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/CWE-352/ConstantOauth2State.ql diff --git a/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.expected b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.expected new file mode 100644 index 00000000000..8e7dc5d59de --- /dev/null +++ b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.expected @@ -0,0 +1,60 @@ +edges +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:50:26:50:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:147:26:147:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:169:26:169:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:191:26:191:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:210:26:210:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:232:26:232:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:249:26:249:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:266:26:266:41 | stateStringConst | +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:282:26:282:41 | stateStringConst | +| ConstantOauth2State.go:22:22:22:28 | "state" : string | ConstantOauth2State.go:65:26:65:39 | stateStringVar | +| ConstantOauth2State.go:80:11:80:25 | call to newFixedState : string | ConstantOauth2State.go:81:26:81:30 | state | +| ConstantOauth2State.go:86:9:86:15 | "state" : string | ConstantOauth2State.go:80:11:80:25 | call to newFixedState : string | +| ConstantOauth2State.go:147:9:147:42 | call to AuthCodeURL : string | ConstantOauth2State.go:148:54:148:56 | url | +| ConstantOauth2State.go:169:9:169:42 | call to AuthCodeURL : string | ConstantOauth2State.go:170:54:170:56 | url | +| ConstantOauth2State.go:191:9:191:42 | call to AuthCodeURL : string | ConstantOauth2State.go:192:54:192:56 | url | +| ConstantOauth2State.go:210:9:210:42 | call to AuthCodeURL : string | ConstantOauth2State.go:211:54:211:56 | url | +| ConstantOauth2State.go:232:9:232:42 | call to AuthCodeURL : string | ConstantOauth2State.go:233:28:233:30 | url | +| ConstantOauth2State.go:239:17:239:39 | "http://localhost:8080" : string | ConstantOauth2State.go:249:9:249:12 | conf | +| ConstantOauth2State.go:256:38:256:60 | "http://localhost:8080" : string | ConstantOauth2State.go:266:9:266:12 | conf | +| ConstantOauth2State.go:272:17:272:21 | "oob" : string | ConstantOauth2State.go:282:9:282:12 | conf | +nodes +| ConstantOauth2State.go:20:26:20:32 | "state" : string literal | semmle.label | "state" : string literal | +| ConstantOauth2State.go:22:22:22:28 | "state" : string | semmle.label | "state" : string | +| ConstantOauth2State.go:35:26:35:32 | "state" | semmle.label | "state" | +| ConstantOauth2State.go:50:26:50:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:65:26:65:39 | stateStringVar | semmle.label | stateStringVar | +| ConstantOauth2State.go:80:11:80:25 | call to newFixedState : string | semmle.label | call to newFixedState : string | +| ConstantOauth2State.go:81:26:81:30 | state | semmle.label | state | +| ConstantOauth2State.go:86:9:86:15 | "state" : string | semmle.label | "state" : string | +| ConstantOauth2State.go:147:9:147:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:147:26:147:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:148:54:148:56 | url | semmle.label | url | +| ConstantOauth2State.go:169:9:169:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:169:26:169:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:170:54:170:56 | url | semmle.label | url | +| ConstantOauth2State.go:191:9:191:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:191:26:191:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:192:54:192:56 | url | semmle.label | url | +| ConstantOauth2State.go:210:9:210:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:210:26:210:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:211:54:211:56 | url | semmle.label | url | +| ConstantOauth2State.go:232:9:232:42 | call to AuthCodeURL : string | semmle.label | call to AuthCodeURL : string | +| ConstantOauth2State.go:232:26:232:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:233:28:233:30 | url | semmle.label | url | +| ConstantOauth2State.go:239:17:239:39 | "http://localhost:8080" : string | semmle.label | "http://localhost:8080" : string | +| ConstantOauth2State.go:249:9:249:12 | conf | semmle.label | conf | +| ConstantOauth2State.go:249:26:249:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:256:38:256:60 | "http://localhost:8080" : string | semmle.label | "http://localhost:8080" : string | +| ConstantOauth2State.go:266:9:266:12 | conf | semmle.label | conf | +| ConstantOauth2State.go:266:26:266:41 | stateStringConst | semmle.label | stateStringConst | +| ConstantOauth2State.go:272:17:272:21 | "oob" : string | semmle.label | "oob" : string | +| ConstantOauth2State.go:282:9:282:12 | conf | semmle.label | conf | +| ConstantOauth2State.go:282:26:282:41 | stateStringConst | semmle.label | stateStringConst | +#select +| ConstantOauth2State.go:35:26:35:32 | "state" | ConstantOauth2State.go:35:26:35:32 | "state" | ConstantOauth2State.go:35:26:35:32 | "state" | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:35:26:35:32 | "state" | state string | +| ConstantOauth2State.go:50:26:50:41 | stateStringConst | ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:50:26:50:41 | stateStringConst | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:20:26:20:32 | "state" | state string | +| ConstantOauth2State.go:65:26:65:39 | stateStringVar | ConstantOauth2State.go:22:22:22:28 | "state" : string | ConstantOauth2State.go:65:26:65:39 | stateStringVar | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:22:22:22:28 | "state" | state string | +| ConstantOauth2State.go:81:26:81:30 | state | ConstantOauth2State.go:86:9:86:15 | "state" : string | ConstantOauth2State.go:81:26:81:30 | state | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:86:9:86:15 | "state" | state string | +| ConstantOauth2State.go:232:26:232:41 | stateStringConst | ConstantOauth2State.go:20:26:20:32 | "state" : string literal | ConstantOauth2State.go:232:26:232:41 | stateStringConst | Using a constant $@ to create oauth2 URLs. | ConstantOauth2State.go:20:26:20:32 | "state" | state string | diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.go b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.go similarity index 59% rename from ql/test/experimental/CWE-352/ConstantOauth2State.go rename to ql/test/query-tests/Security/CWE-352/ConstantOauth2State.go index b3200670fb9..75f899aea51 100644 --- a/ql/test/experimental/CWE-352/ConstantOauth2State.go +++ b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.go @@ -3,8 +3,10 @@ package main //go:generate depstubber -vendor golang.org/x/oauth2 Config,Endpoint import ( + "bufio" "crypto/rand" "encoding/base64" + "errors" "fmt" "log" "net/http" @@ -175,6 +177,47 @@ func okWithConstStateFPrinter(w http.ResponseWriter) { _ = code // ... } +func okWithConstStateBufio(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) + // ... + + scanner := bufio.NewScanner(os.Stdin) + _ = scanner + // ... +} +func okWithConstStateLogger(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. + log.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", @@ -190,3 +233,78 @@ func badWithConstStatePrinter(w http.ResponseWriter) { fmt.Printf("LOG: URL %v", url) // ... } + +func okWithLocalUrl(w http.ResponseWriter) { + conf := &oauth2.Config{ + RedirectURL: "http://localhost:8080", + 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 the config uses a local url + _ = url +} + +func okWithLocalUrlSprintf(w http.ResponseWriter) { + port := 8080 + conf := &oauth2.Config{ + RedirectURL: fmt.Sprintf("%s:%d", "http://localhost:8080", port), + 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 the config uses a local url + _ = url +} + +func okWithOutOfBoundsToken(w http.ResponseWriter) { + conf := &oauth2.Config{ + RedirectURL: "oob", + 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 the config uses a token indicating out-of-band communication + _ = url +} + +func tryGetState(success bool) (string, string, int, error) { + if success { + return NewCSRFToken(), "dummy", 0, nil + } else { + return "", "", 0, errors.New("success not set") + } +} + +func okConstantOnlySuppliedAlongsideError(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", + }, + } + + token, _, _, err := tryGetState(len(os.Args)%3 == 1) + if err != nil { + url := conf.AuthCodeURL(token) // OK because constant states coming from tryGetState only occur with errors + _ = url + } +} diff --git a/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.qlref b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.qlref new file mode 100644 index 00000000000..7898f39d415 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.qlref @@ -0,0 +1 @@ +Security/CWE-352/ConstantOauth2State.ql diff --git a/ql/test/experimental/CWE-352/go.mod b/ql/test/query-tests/Security/CWE-352/go.mod similarity index 100% rename from ql/test/experimental/CWE-352/go.mod rename to ql/test/query-tests/Security/CWE-352/go.mod diff --git a/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/LICENSE b/ql/test/query-tests/Security/CWE-352/vendor/golang.org/x/oauth2/LICENSE similarity index 100% rename from ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/LICENSE rename to ql/test/query-tests/Security/CWE-352/vendor/golang.org/x/oauth2/LICENSE diff --git a/ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/stub.go b/ql/test/query-tests/Security/CWE-352/vendor/golang.org/x/oauth2/stub.go similarity index 100% rename from ql/test/experimental/CWE-352/vendor/golang.org/x/oauth2/stub.go rename to ql/test/query-tests/Security/CWE-352/vendor/golang.org/x/oauth2/stub.go diff --git a/ql/test/experimental/CWE-352/vendor/modules.txt b/ql/test/query-tests/Security/CWE-352/vendor/modules.txt similarity index 100% rename from ql/test/experimental/CWE-352/vendor/modules.txt rename to ql/test/query-tests/Security/CWE-352/vendor/modules.txt