Constant-oauth2-state: exclude strings returned alongside an error value

For example, getState() { ... return "", someError } is commonly seen in the wild.

Author: smowton

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.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) {

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.