Merge pull request #291 from smowton/smowton/admin/oauth2-query-polish

Promote OAuth2-misuse query to mainline

Author: max-schaefer

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.

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)
   }
 
   /**

ql/src/experimental/CWE-352/ConstantOauth2State.qhelp renamed to 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>

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"

ql/src/experimental/CWE-352/ConstantOauth2StateBad.go renamed to ql/src/Security/CWE-352/ConstantOauth2StateBad.go

@@ -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.