From 6fee4f382f58b382df476f7ec1c95540acdb8e34 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Mon, 17 Aug 2020 14:12:11 +0100
Subject: [PATCH 01/14] Constant-oauth2-state: exclude strings returned
 alongside an error value

For example, getState() { ... return "", someError } is commonly seen in the wild.
---
 ql/src/Security/CWE-327/InsecureTLS.ql           | 16 +---------------
 .../experimental/CWE-352/ConstantOauth2State.ql  |  2 +-
 .../semmle/go/dataflow/internal/DataFlowUtil.qll | 14 ++++++++++++++
 3 files changed, 16 insertions(+), 16 deletions(-)

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.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
index 96c095f637e..9c15fb075ea 100644
--- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
@@ -28,7 +28,7 @@ class ConstantStateFlowConf extends DataFlow::Configuration {
   ConstantStateFlowConf() { this = "ConstantStateFlowConf" }
 
   predicate isSource(DataFlow::Node source, Literal state) {
-    state.isConst() and source.asExpr() = state
+    state.isConst() and source.asExpr() = state and not DataFlow::isReturnedWithError(source)
   }
 
   predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) {
diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
index 5fdd170574e..0399fd0594d 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 |
+    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`
+  )
+}
+
 /**
  * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local
  * (intra-procedural) step.

From 3d877fc67d6ad21fd8c7df2b0c71a7dd2df5dd7e Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Mon, 17 Aug 2020 15:10:02 +0100
Subject: [PATCH 02/14] Oauth2 state: note bufio.NewScanner is also a sign of
 probable terminal-interactive use

---
 .../CWE-352/ConstantOauth2State.ql            | 26 +++++++++++++------
 ql/src/semmle/go/frameworks/stdlib/Bufio.qll  | 20 ++++++++++++++
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
index 9c15fb075ea..dfb8ad1c368 100644
--- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
@@ -65,17 +65,27 @@ predicate resultFlowsToPrinter(DataFlow::CallNode authCodeURLCall) {
   )
 }
 
+/** Gets dataflow nodes that read the value of os.Stdin */
+DataFlow::Node getAStdinNode() {
+  result = any(ValueEntity v | v.hasQualifiedName("os", "Stdin")).getARead()
+}
+
+/**
+ * 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 instanceof Fmt::ScannerCall or
+  result.(Fmt::FScannerCall).getReader() = getAStdinNode() or
+  result.(Bufio::NewScannerCall).getReader() = getAStdinNode()
+}
+
 /**
  * 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()
-  )
+predicate containsCallToStdinScanner(FuncDef funcDef) {
+  exists(DataFlow::CallNode call | call = getAScannerCall() | call.getRoot() = funcDef)
 }
 
 /**
@@ -86,7 +96,7 @@ predicate rootContainsCallToStdinScanner(DataFlow::CallNode authCodeURLCall) {
  */
 predicate seemsLikeDoneWithinATerminal(DataFlow::CallNode authCodeURLCall) {
   resultFlowsToPrinter(authCodeURLCall) and
-  rootContainsCallToStdinScanner(authCodeURLCall)
+  containsCallToStdinScanner(authCodeURLCall.getRoot())
 }
 
 from
diff --git a/ql/src/semmle/go/frameworks/stdlib/Bufio.qll b/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
index d5a6f80ead9..b08b0ce3750 100644
--- a/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
+++ b/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
@@ -6,6 +6,26 @@ 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") }
+  }
+
+  /**
+   * A call to bufio.NewScanner.
+   */
+  class NewScannerCall extends DataFlow::CallNode {
+    NewScannerCall() { this.getTarget() instanceof NewScanner }
+
+    /**
+     * Returns the node corresponding to the io.Reader
+     * argument provided in the call.
+     */
+    DataFlow::Node getReader() { result = this.getArgument(0) }
+  }
+
   private class FunctionModels extends TaintTracking::FunctionModel {
     FunctionInput inp;
     FunctionOutput outp;

From bcb65157e616add82879635c31a3a449c64df758 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Mon, 17 Aug 2020 16:14:29 +0100
Subject: [PATCH 03/14] Oauth2-state query: treat log calls the same as stdout
 printers

These presumably get to the user somehow, and in conjunction with stdin use are enough to identify use of oauth at the terminal.
---
 ql/src/experimental/CWE-352/ConstantOauth2State.ql | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
index dfb8ad1c368..c715bf60b1d 100644
--- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
@@ -50,6 +50,8 @@ class FlowToPrint extends DataFlow::Configuration {
 
   predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) {
     exists(Fmt::Printer printer | call = printer.getACall() | sink = call.getArgument(_))
+    or
+    exists(LoggerCall logCall | call = logCall | sink = logCall.getAMessageComponent())
   }
 
   override predicate isSource(DataFlow::Node source) { isSource(source, _) }

From 050a82339712a35035aec1166b90cc4f4a11233d Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Tue, 18 Aug 2020 11:55:00 +0100
Subject: [PATCH 04/14] OAuth2 exclusion: hide cases that clearly target an
 out-of-band process or private HTTP server

---
 .../CWE-352/ConstantOauth2State.ql            | 55 ++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
index c715bf60b1d..d03ad71a597 100644
--- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
@@ -40,6 +40,58 @@ class ConstantStateFlowConf extends DataFlow::Configuration {
   override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
 }
 
+/**
+ * A flow of a URL indicating the OAuth redirect doesn't point to a publically
+ * 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) {
+    // The following are all common ways to indicate out-of-band OAuth2 flow, in which case
+    // the authenticating party does not redirect but presents a code for the user to copy
+    // instead.
+    source.asExpr().(StringLit).getValue() 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:
+    source.asExpr().(StringLit).getValue().matches("%://localhost%") or
+    source.asExpr().(StringLit).getValue().matches("%://127.0.0.1%")
+  }
+
+  /**
+   * Propagates a URL written to a RedirectURL field to the whole Config object.
+   */
+  override predicate isAdditionalFlowStep(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)
+    )
+  }
+
+  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 publically
+ * 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 to a printer function of the fmt package. */
 class FlowToPrint extends DataFlow::Configuration {
   FlowToPrint() { this = "FlowToPrint" }
@@ -108,6 +160,7 @@ 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)
+  not seemsLikeDoneWithinATerminal(sinkCall) and
+  not privateUrlFlowsToAuthCodeUrlCall(sinkCall)
 select sink.getNode(), source, sink, "Using a constant $@ to create oauth2 URLs.", source.getNode(),
   "state string"

From 9e4ee0accfd5664eec619cbbaca7f8c77f495a99 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Tue, 18 Aug 2020 14:34:39 +0100
Subject: [PATCH 05/14] OAuth2 constant state query: trace local URLs across
 reference operations and Sprintf calls

---
 .../experimental/CWE-352/ConstantOauth2State.ql | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
index d03ad71a597..a36bd5ee092 100644
--- a/ql/src/experimental/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/experimental/CWE-352/ConstantOauth2State.ql
@@ -65,12 +65,27 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
   /**
    * Propagates a URL written to a RedirectURL field to the whole Config object.
    */
-  override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+  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)
     )
   }
 
+  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
+    exists(DataFlow::CallNode c |
+      c = any(Fmt::Sprinter s).getACall() and
+      pred = c.getAnArgument() and
+      succ = c.getResult()
+    )
+  }
+
   predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) {
     exists(AuthCodeURL m | call = m.getACall() | sink = call.getReceiver())
   }

From f61c62d2d85da63467a1fc67b0c4d90b1d288def Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Tue, 18 Aug 2020 14:35:23 +0100
Subject: [PATCH 06/14] Generalise isReturnedWithError

It now recognises any function returning an Error alongside other return values
---
 ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
index 0399fd0594d..b3aee357dc1 100644
--- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
+++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
@@ -881,11 +881,11 @@ Node extractTupleElement(Node t, int i) {
  * For example, `0` in `func tryGetInt() (int, error) { return 0, errors.New("no good") }`
  */
 predicate isReturnedWithError(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`
+  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`
   )
 }
 

From 0ee7bbbaa7ff76efd06c82681e11f95c83babbbd Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Tue, 18 Aug 2020 15:22:01 +0100
Subject: [PATCH 07/14] Extend oauth2 tests

---
 .../CWE-352/ConstantOauth2State.expected      |  89 ++++++++-----
 .../CWE-352/ConstantOauth2State.go            | 118 ++++++++++++++++++
 2 files changed, 175 insertions(+), 32 deletions(-)

diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.expected b/ql/test/experimental/CWE-352/ConstantOauth2State.expected
index ecd1543cd1a..8e7dc5d59de 100644
--- a/ql/test/experimental/CWE-352/ConstantOauth2State.expected
+++ b/ql/test/experimental/CWE-352/ConstantOauth2State.expected
@@ -1,35 +1,60 @@
 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 |
+| 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: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 |
+| 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: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 |
+| 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/experimental/CWE-352/ConstantOauth2State.go
index b3200670fb9..75f899aea51 100644
--- a/ql/test/experimental/CWE-352/ConstantOauth2State.go
+++ b/ql/test/experimental/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
+	}
+}

From faf43efb60199d88f60281b359aa0e4ef14cb011 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Tue, 18 Aug 2020 15:30:22 +0100
Subject: [PATCH 08/14] Promote OAuth2 constant-state query to mainline

---
 change-notes/2020-08-18-oauth2.md                               | 2 ++
 .../CWE-352/ConstantOauth2State.qhelp                           | 0
 .../{experimental => Security}/CWE-352/ConstantOauth2State.ql   | 0
 .../CWE-352/ConstantOauth2StateBad.go                           | 0
 .../CWE-352/ConstantOauth2StateBetter.go                        | 0
 ql/test/experimental/CWE-352/ConstantOauth2State.qlref          | 1 -
 .../Security}/CWE-352/ConstantOauth2State.expected              | 0
 .../Security}/CWE-352/ConstantOauth2State.go                    | 0
 ql/test/query-tests/Security/CWE-352/ConstantOauth2State.qlref  | 1 +
 ql/test/{experimental => query-tests/Security}/CWE-352/go.mod   | 0
 .../Security}/CWE-352/vendor/golang.org/x/oauth2/LICENSE        | 0
 .../Security}/CWE-352/vendor/golang.org/x/oauth2/stub.go        | 0
 .../Security}/CWE-352/vendor/modules.txt                        | 0
 13 files changed, 3 insertions(+), 1 deletion(-)
 create mode 100644 change-notes/2020-08-18-oauth2.md
 rename ql/src/{experimental => Security}/CWE-352/ConstantOauth2State.qhelp (100%)
 rename ql/src/{experimental => Security}/CWE-352/ConstantOauth2State.ql (100%)
 rename ql/src/{experimental => Security}/CWE-352/ConstantOauth2StateBad.go (100%)
 rename ql/src/{experimental => Security}/CWE-352/ConstantOauth2StateBetter.go (100%)
 delete mode 100644 ql/test/experimental/CWE-352/ConstantOauth2State.qlref
 rename ql/test/{experimental => query-tests/Security}/CWE-352/ConstantOauth2State.expected (100%)
 rename ql/test/{experimental => query-tests/Security}/CWE-352/ConstantOauth2State.go (100%)
 create mode 100644 ql/test/query-tests/Security/CWE-352/ConstantOauth2State.qlref
 rename ql/test/{experimental => query-tests/Security}/CWE-352/go.mod (100%)
 rename ql/test/{experimental => query-tests/Security}/CWE-352/vendor/golang.org/x/oauth2/LICENSE (100%)
 rename ql/test/{experimental => query-tests/Security}/CWE-352/vendor/golang.org/x/oauth2/stub.go (100%)
 rename ql/test/{experimental => query-tests/Security}/CWE-352/vendor/modules.txt (100%)

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/experimental/CWE-352/ConstantOauth2State.qhelp b/ql/src/Security/CWE-352/ConstantOauth2State.qhelp
similarity index 100%
rename from ql/src/experimental/CWE-352/ConstantOauth2State.qhelp
rename to ql/src/Security/CWE-352/ConstantOauth2State.qhelp
diff --git a/ql/src/experimental/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql
similarity index 100%
rename from ql/src/experimental/CWE-352/ConstantOauth2State.ql
rename to ql/src/Security/CWE-352/ConstantOauth2State.ql
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/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/experimental/CWE-352/ConstantOauth2State.expected b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.expected
similarity index 100%
rename from ql/test/experimental/CWE-352/ConstantOauth2State.expected
rename to ql/test/query-tests/Security/CWE-352/ConstantOauth2State.expected
diff --git a/ql/test/experimental/CWE-352/ConstantOauth2State.go b/ql/test/query-tests/Security/CWE-352/ConstantOauth2State.go
similarity index 100%
rename from ql/test/experimental/CWE-352/ConstantOauth2State.go
rename to ql/test/query-tests/Security/CWE-352/ConstantOauth2State.go
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

From 406ea741f4efe3946f20463fa9fbe70e2a6774e9 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Thu, 20 Aug 2020 11:21:00 +0100
Subject: [PATCH 09/14] Improve comment style

---
 .../Security/CWE-352/ConstantOauth2State.ql   | 42 +++++++++----------
 ql/src/semmle/go/frameworks/stdlib/Bufio.qll  |  6 +--
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql
index a36bd5ee092..2130ecc92b7 100644
--- a/ql/src/Security/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/Security/CWE-352/ConstantOauth2State.ql
@@ -21,28 +21,26 @@ class AuthCodeURL extends Method {
 }
 
 /**
- * A flow of a constant string value to a call to AuthCodeURL as the
+ * 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 and not DataFlow::isReturnedWithError(source)
-  }
-
   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 isSource(DataFlow::Node source) {
+    source.isConst() and not DataFlow::isReturnedWithError(source)
+  }
 
   override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
 }
 
 /**
- * A flow of a URL indicating the OAuth redirect doesn't point to a publically
- * accessible address, to the receiver of an AuthCodeURL call.
+ * 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.
@@ -63,7 +61,9 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
   }
 
   /**
-   * Propagates a URL written to a RedirectURL field to the whole Config object.
+   * 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") |
@@ -94,8 +94,8 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
 }
 
 /**
- * Holds if a URL indicating the OAuth redirect doesn't point to a publically
- * accessible address, to the receiver of an AuthCodeURL call.
+ * 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.
@@ -107,7 +107,7 @@ predicate privateUrlFlowsToAuthCodeUrlCall(DataFlow::CallNode call) {
   )
 }
 
-/** A flow to a printer function of the fmt package. */
+/** A flow from `golang.org/x/oauth2.Config.AuthCodeURL`'s result to a logging function. */
 class FlowToPrint extends DataFlow::Configuration {
   FlowToPrint() { this = "FlowToPrint" }
 
@@ -126,7 +126,7 @@ class FlowToPrint extends DataFlow::Configuration {
   override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
 }
 
-/** Holds if the provided CallNode's result flows to a Printer call as argument. */
+/** 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
@@ -134,14 +134,14 @@ predicate resultFlowsToPrinter(DataFlow::CallNode authCodeURLCall) {
   )
 }
 
-/** Gets dataflow nodes that read the value of os.Stdin */
+/** Get a data-flow node that reads the value of `os.Stdin`. */
 DataFlow::Node getAStdinNode() {
   result = any(ValueEntity v | v.hasQualifiedName("os", "Stdin")).getARead()
 }
 
 /**
- * Gets a call to a scanner function that reads from os.Stdin, or which creates a scanner
- * instance wrapping os.Stdin.
+ * 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 instanceof Fmt::ScannerCall or
@@ -150,17 +150,17 @@ DataFlow::CallNode getAScannerCall() {
 }
 
 /**
- * Holds if the provided CallNode is within the same root as a call
- * to a scanner that reads from os.Stdin.
+ * 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) {
   exists(DataFlow::CallNode call | call = getAScannerCall() | call.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),
+ * 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) {
diff --git a/ql/src/semmle/go/frameworks/stdlib/Bufio.qll b/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
index b08b0ce3750..86811656cbc 100644
--- a/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
+++ b/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
@@ -7,20 +7,20 @@ import go
 /** Provides models of commonly used functions in the `bufio` package. */
 module Bufio {
   /**
-   * The function bufio.NewScanner.
+   * The function `bufio.NewScanner`.
    */
   class NewScanner extends Function {
     NewScanner() { this.hasQualifiedName("bufio", "NewScanner") }
   }
 
   /**
-   * A call to bufio.NewScanner.
+   * A call to `bufio.NewScanner`.
    */
   class NewScannerCall extends DataFlow::CallNode {
     NewScannerCall() { this.getTarget() instanceof NewScanner }
 
     /**
-     * Returns the node corresponding to the io.Reader
+     * Returns the node corresponding to the `io.Reader`
      * argument provided in the call.
      */
     DataFlow::Node getReader() { result = this.getArgument(0) }

From 0ba42f7f8781338895ae5887db772d1cd3b44bfa Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Fri, 21 Aug 2020 10:38:50 +0100
Subject: [PATCH 10/14] OAuth2 state query: set precision

---
 ql/src/Security/CWE-352/ConstantOauth2State.ql | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql
index 2130ecc92b7..ea8ce5b4875 100644
--- a/ql/src/Security/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/Security/CWE-352/ConstantOauth2State.ql
@@ -4,6 +4,7 @@
  *              susceptible to CSRF attacks.
  * @kind path-problem
  * @problem.severity error
+ * @precision high
  * @id go/constant-oauth2-state
  * @tags security
  *       external/cwe/cwe-352

From 8f9997283352cfadde6f690185806df7539b9182 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Thu, 27 Aug 2020 14:45:00 +0100
Subject: [PATCH 11/14] OAuth2 CSRF query: improve documentation

---
 ql/src/Security/CWE-352/ConstantOauth2State.qhelp | 6 +++++-
 ql/src/Security/CWE-352/ConstantOauth2State.ql    | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.qhelp b/ql/src/Security/CWE-352/ConstantOauth2State.qhelp
index dc15b5de1d9..4400d60e304 100644
--- a/ql/src/Security/CWE-352/ConstantOauth2State.qhelp
+++ b/ql/src/Security/CWE-352/ConstantOauth2State.qhelp
@@ -5,7 +5,7 @@
     <overview>
         <p>
             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.
         </p>
     </overview>
     <recommendation>
@@ -23,4 +23,8 @@
         </p>
         <sample src="ConstantOauth2StateBetter.go" />
     </example>
+    <references>
+        <li>IETF: <a href="https://tools.ietf.org/html/rfc6749#section-10.12">The OAuth 2.0 Authorization Framework</a></li>
+        <li>IETF: <a href="https://tools.ietf.org/html/draft-ietf-oauth-security-topics-15#section-2.1">OAuth 2.0 Security Best Current Practice</a></li>
+    </references>
 </qhelp>
diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql
index ea8ce5b4875..23d55e69e71 100644
--- a/ql/src/Security/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/Security/CWE-352/ConstantOauth2State.ql
@@ -1,5 +1,5 @@
 /**
- * @name Use of constant `state` value in OAuth 2.0 URL.
+ * @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

From 2f175e365e4a512594615138691847c25d72abe6 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Wed, 2 Sep 2020 14:57:57 +0100
Subject: [PATCH 12/14] Oauth2 state query: remove unnecessary isSource
 overload

---
 ql/src/Security/CWE-352/ConstantOauth2State.ql | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql
index 23d55e69e71..042b5b1231c 100644
--- a/ql/src/Security/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/Security/CWE-352/ConstantOauth2State.ql
@@ -112,17 +112,15 @@ predicate privateUrlFlowsToAuthCodeUrlCall(DataFlow::CallNode call) {
 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(_))
     or
     exists(LoggerCall logCall | call = logCall | sink = logCall.getAMessageComponent())
   }
 
-  override predicate isSource(DataFlow::Node source) { isSource(source, _) }
+  override predicate isSource(DataFlow::Node source) {
+    source = any(AuthCodeURL m).getACall().getResult()
+  }
 
   override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
 }
@@ -131,7 +129,7 @@ class FlowToPrint extends DataFlow::Configuration {
 predicate resultFlowsToPrinter(DataFlow::CallNode authCodeURLCall) {
   exists(FlowToPrint cfg, DataFlow::PathNode source, DataFlow::PathNode sink |
     cfg.hasFlowPath(source, sink) and
-    cfg.isSource(source.getNode(), authCodeURLCall)
+    authCodeURLCall.getResult() = source.getNode()
   )
 }
 

From 6fea8abd82ed6342c7d607fb749ee315c0de2a47 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Wed, 2 Sep 2020 15:00:56 +0100
Subject: [PATCH 13/14] Oauth2 state query: improve code style

No behavioural changes intended.
---
 .../Security/CWE-352/ConstantOauth2State.ql   | 58 +++++++++----------
 ql/src/semmle/go/frameworks/Stdlib.qll        | 12 +---
 ql/src/semmle/go/frameworks/stdlib/Bufio.qll  | 11 +---
 3 files changed, 32 insertions(+), 49 deletions(-)

diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql
index 042b5b1231c..b0610d853c3 100644
--- a/ql/src/Security/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/Security/CWE-352/ConstantOauth2State.ql
@@ -39,6 +39,17 @@ class ConstantStateFlowConf extends DataFlow::Configuration {
   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)
+  )
+}
+
 /**
  * A flow of a URL indicating the OAuth redirect doesn't point to a publicly
  * accessible address, to the receiver of an `AuthCodeURL` call.
@@ -53,23 +64,12 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
     // The following are all common ways to indicate out-of-band OAuth2 flow, in which case
     // the authenticating party does not redirect but presents a code for the user to copy
     // instead.
-    source.asExpr().(StringLit).getValue() in ["urn:ietf:wg:oauth:2.0:oob",
-          "urn:ietf:wg:oauth:2.0:oob:auto", "oob", "code"] or
+    source.getStringValue() 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:
-    source.asExpr().(StringLit).getValue().matches("%://localhost%") or
-    source.asExpr().(StringLit).getValue().matches("%://127.0.0.1%")
-  }
-
-  /**
-   * 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)
-    )
+    source.getStringValue().matches("%://localhost%") or
+    source.getStringValue().matches("%://127.0.0.1%")
   }
 
   override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
@@ -80,11 +80,7 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
     TaintTracking::referenceStep(pred, succ)
     or
     // Propagate across Sprintf and similar calls
-    exists(DataFlow::CallNode c |
-      c = any(Fmt::Sprinter s).getACall() and
-      pred = c.getAnArgument() and
-      succ = c.getResult()
-    )
+    TaintTracking::functionModelStep(any(Fmt::Sprinter s), pred, succ)
   }
 
   predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) {
@@ -113,8 +109,6 @@ class FlowToPrint extends DataFlow::Configuration {
   FlowToPrint() { this = "FlowToPrint" }
 
   predicate isSink(DataFlow::Node sink, DataFlow::CallNode call) {
-    exists(Fmt::Printer printer | call = printer.getACall() | sink = call.getArgument(_))
-    or
     exists(LoggerCall logCall | call = logCall | sink = logCall.getAMessageComponent())
   }
 
@@ -135,7 +129,9 @@ predicate resultFlowsToPrinter(DataFlow::CallNode authCodeURLCall) {
 
 /** Get a data-flow node that reads the value of `os.Stdin`. */
 DataFlow::Node getAStdinNode() {
-  result = any(ValueEntity v | v.hasQualifiedName("os", "Stdin")).getARead()
+  exists(ValueEntity v |
+    v.hasQualifiedName("os", "Stdin") and result = globalValueNumber(v.getARead()).getANode()
+  )
 }
 
 /**
@@ -143,18 +139,22 @@ DataFlow::Node getAStdinNode() {
  * instance wrapping `os.Stdin`.
  */
 DataFlow::CallNode getAScannerCall() {
-  result instanceof Fmt::ScannerCall or
-  result.(Fmt::FScannerCall).getReader() = getAStdinNode() or
-  result.(Bufio::NewScannerCall).getReader() = getAStdinNode()
+  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) {
-  exists(DataFlow::CallNode call | call = getAScannerCall() | call.getRoot() = funcDef)
-}
+predicate containsCallToStdinScanner(FuncDef funcDef) { getAScannerCall().getRoot() = funcDef }
 
 /**
  * Holds if the `authCodeURLCall` seems to be done within a terminal
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 86811656cbc..45012addcd6 100644
--- a/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
+++ b/ql/src/semmle/go/frameworks/stdlib/Bufio.qll
@@ -11,19 +11,12 @@ module Bufio {
    */
   class NewScanner extends Function {
     NewScanner() { this.hasQualifiedName("bufio", "NewScanner") }
-  }
-
-  /**
-   * A call to `bufio.NewScanner`.
-   */
-  class NewScannerCall extends DataFlow::CallNode {
-    NewScannerCall() { this.getTarget() instanceof NewScanner }
 
     /**
-     * Returns the node corresponding to the `io.Reader`
+     * Gets the input corresponding to the `io.Reader`
      * argument provided in the call.
      */
-    DataFlow::Node getReader() { result = this.getArgument(0) }
+    FunctionInput getReader() { result.isParameter(0) }
   }
 
   private class FunctionModels extends TaintTracking::FunctionModel {

From b487799f697ac3709dbe80f0369e5f1131dd05e3 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Wed, 2 Sep 2020 17:40:53 +0100
Subject: [PATCH 14/14] Oauth2 state query: avoid duplicate paths by excluding
 variable references as sources

---
 .../Security/CWE-352/ConstantOauth2State.ql   | 41 ++++++++++++++-----
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/ql/src/Security/CWE-352/ConstantOauth2State.ql b/ql/src/Security/CWE-352/ConstantOauth2State.ql
index b0610d853c3..f00cf749400 100644
--- a/ql/src/Security/CWE-352/ConstantOauth2State.ql
+++ b/ql/src/Security/CWE-352/ConstantOauth2State.ql
@@ -33,7 +33,14 @@ class ConstantStateFlowConf extends DataFlow::Configuration {
   }
 
   override predicate isSource(DataFlow::Node source) {
-    source.isConst() and not DataFlow::isReturnedWithError(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, _) }
@@ -50,6 +57,21 @@ predicate isUrlTaintingConfigStep(DataFlow::Node pred, DataFlow::Node succ) {
   )
 }
 
+/**
+ * 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.
@@ -61,15 +83,14 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
   PrivateUrlFlowsToAuthCodeUrlCall() { this = "PrivateUrlFlowsToConfig" }
 
   override predicate isSource(DataFlow::Node source) {
-    // The following are all common ways to indicate out-of-band OAuth2 flow, in which case
-    // the authenticating party does not redirect but presents a code for the user to copy
-    // instead.
-    source.getStringValue() 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:
-    source.getStringValue().matches("%://localhost%") or
-    source.getStringValue().matches("%://127.0.0.1%")
+    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) {