github · kaeluka · Dec 8, 2021 · Jan 7, 2022 · intrigus-lgtm · Dec 22, 2021
@@ -84,6 +84,17 @@
 abstract class DatabaseAccess extends DataFlow::Node {
   /** Gets an argument to this database access that is interpreted as a query. */
   abstract DataFlow::Node getAQueryArgument();
+
+  /** Gets a node to which a result of the access may flow. */
+  abstract DataFlow::Node getAResult();
+
+  /** Might the data returned as a JSON object like document databases do?
+   * */
+  predicate returnsUserControlledObject() {
+    // NB: Most data bases support JSON data (some via plugins),
+    // which is why this has a default implementation.
+    any()
+  }
 }
 
 /**
@@ -105,3 +116,16 @@
    */
   abstract DataFlow::Node getValue();
 }
+
+/**
+ * Data read from a database.
+ */
+class DatabaseAccessResultRemoteFlowSource extends RemoteFlowSource {
+  DatabaseAccessResultRemoteFlowSource() {
+    exists(DatabaseAccess dba | this = dba.getAResult())
+  }
+
+  override string getSourceType() { result = "Database query result" }
+
+  override predicate isUserControlledObject() { this.(DatabaseAccess).returnsUserControlledObject() }
+}
@@ -46,17 +46,38 @@ module Knex {
     RawKnexSqlString() { this = any(RawKnexCall call).getArgument(0).asExpr() }
   }
 
-  /** A call that triggers a SQL query submission. */
-  private class KnexDatabaseAccess extends DatabaseAccess {
-    KnexDatabaseAccess() {
+  /** A call that triggers a SQL query submission by calling then/stream/asCallback. */
+  private class KnexDatabaseCallback extends DatabaseAccess, DataFlow::CallNode {
+    KnexDatabaseCallback() {
       this = knexObject().getMember(["then", "stream", "asCallback"]).getACall()
+    }
+
+    override DataFlow::Node getAResult() { 
+      this = knexObject().getMember("then").getACall() and
+        result = this.getCallback(0).getParameter(0)
       or
-      exists(AwaitExpr await |
-        this = await.flow() and
-        await.getOperand() = knexObject().getAUse().asExpr()
-      )
+        this = knexObject().getMember("asCallback").getACall() and
+        result = this.getCallback(0).getParameter(1)
+    }
+
+    override DataFlow::Node getAQueryArgument() {
+      none()
+    }
+  }
+
+  private class KnexDatabaseAwait extends DatabaseAccess, DataFlow::ValueNode {
+    AwaitExpr enclosingAwait;
+    KnexDatabaseAwait() {
+      this = enclosingAwait.flow() and
+      enclosingAwait.getOperand() = knexObject().getAUse().asExpr()
     }
 
-    override DataFlow::Node getAQueryArgument() { none() }
+    override DataFlow::Node getAResult() { 
+      result = this
+    }
+
+    override DataFlow::Node getAQueryArgument() {
+      none()
+    }
   }
 }
@@ -3,6 +3,7 @@
  */
 
 import javascript
+import semmle.javascript.Promises
 
 module NoSQL {
   /** An expression that is interpreted as a NoSQL query. */
@@ -65,6 +66,10 @@ private module MongoDB {
 
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     DataFlow::Node getACodeOperator() {
       result = getADollarWhereProperty(this.getParameter(queryArgIdx))
     }
@@ -537,14 +542,22 @@ private module Mongoose {
       // NB: the complete information is not easily accessible for deeply chained calls
       f.getQueryArgument().getARhs() = result
     }
+
+    override DataFlow::Node getAResult() {
+      result = this.getCallback(this.getNumArgument() - 1).getParameter(1)
+    }
   }
 
-  class ExplicitQueryEvaluation extends DatabaseAccess {
+class ExplicitQueryEvaluation extends DatabaseAccess {
     ExplicitQueryEvaluation() {
       // explicit execution using a Query method call
       Query::getAMongooseQuery().getMember(["exec", "then", "catch"]).getACall() = this
     }
 
+    override DataFlow::Node getAResult() {
+      result = this.(DataFlow::CallNode).getCallback(_).getParameter(1)
+    }
+
     override DataFlow::Node getAQueryArgument() {
       // NB: the complete information is not easily accessible for deeply chained calls
       none()
@@ -588,6 +601,10 @@ private module Minimongo {
 
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     DataFlow::Node getACodeOperator() {
       result = getADollarWhereProperty(this.getParameter(queryArgIdx))
     }
@@ -623,6 +640,10 @@ private module MarsDB {
 
     string getMethod() { result = method }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.(DataFlow::SourceNode).getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { none() }
   }
 
@@ -638,6 +659,10 @@ private module MarsDB {
       )
     }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.(DataFlow::SourceNode).getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) }
 
     DataFlow::Node getACodeOperator() {
@@ -747,6 +772,10 @@ private module Redis {
   class RedisDatabaseAccess extends DatabaseAccess {
     RedisDatabaseAccess() { this = redis().getMember(_).getACall() }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.(DataFlow::SourceNode).getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { none() }
   }
 }
@@ -771,6 +800,10 @@ private module IoRedis {
   class IoRedisDatabaseAccess extends DatabaseAccess {
     IoRedisDatabaseAccess() { this = ioredis().getMember(_).getACall() }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.(DataFlow::SourceNode).getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { none() }
   }
 }
@@ -3,6 +3,7 @@
  */
 
 import javascript
+import semmle.javascript.Promises
 
 module SQL {
   /** A string-valued expression that is interpreted as a SQL command. */
@@ -77,6 +78,10 @@ private module MySql {
       )
     }
 
+    override DataFlow::Node getAResult() {
+      result = this.getCallback(_).getParameter(1)
+    }
+
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
   }
 
@@ -174,6 +179,16 @@ private module Postgres {
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
     QueryCall() { this = [client(), pool()].getMember("query").getACall() }
 
+    override DataFlow::Node getAResult() {
+      this.(DataFlow::CallNode).getNumArgument() = 2 and
+      result = this.(DataFlow::CallNode).getCallback(1).getParameter(1)
+      or
+        this.(DataFlow::CallNode).getNumArgument() = 1 and
+        result = this.getAMethodCall("then").getCallback(0).getParameter(0)
+      or
+        PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
   }
 
@@ -318,6 +333,10 @@ private module Postgres {
       )
     }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() {
       result = this.getADirectQueryArgument()
       or
@@ -366,6 +385,11 @@ private module Sqlite {
       this = database().getMember("prepare").getACall()
     }
 
+    override DataFlow::Node getAResult() {
+      result = this.getCallback(1).getParameter(1) or
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
   }
 
@@ -416,6 +440,10 @@ private module MsSql {
       mssql().getMember("query").getAUse() = DataFlow::valueNode(astNode.getTag())
     }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.(DataFlow::SourceNode).getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() {
       result = DataFlow::valueNode(astNode.getTemplate().getAnElement())
     }
@@ -425,6 +453,12 @@ private module MsSql {
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
     QueryCall() { this = [mssql(), request()].getMember(["query", "batch"]).getACall() }
 
+    override DataFlow::Node getAResult() {
+      result = this.getCallback(1).getParameter(1)
+      or
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
   }
 
@@ -488,6 +522,10 @@ private module Sequelize {
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
     QueryCall() { this = instance().getMember("query").getACall() }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() {
       result = this.getArgument(0)
       or
@@ -580,6 +618,16 @@ private module Spanner {
     result = API::Node::ofType("@google-cloud/spanner", "Transaction")
   }
 
+  API::Node snapshot() {
+    result =
+      database()
+          .getMember("getSnapshot")
+          .getParameter([0,1])
+          .getParameter(1)
+    or
+    result = API::Node::ofType("@google-cloud/spanner", "Snapshot")
+  }
+
   /** Gets an API node referring to a `BatchTransaction` object. */
   API::Node batchTransaction() {
     result = database().getMember("batchTransaction").getReturn()
@@ -604,6 +652,14 @@ private module Spanner {
       this = transaction().getMember(["run", "runStream", "runUpdate"]).getACall()
       or
       this = batchTransaction().getMember("createQueryPartitions").getACall()
+      or
+      this = snapshot().getMember(["run", "runStream"]).getACall()
+    }
+
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+      or
+      (this = [database(), transaction(), snapshot()].getMember("run").getACall() and result = this.getCallback(_).getParameter(1))
     }
 
     override DataFlow::Node getAQueryArgument() {
@@ -619,6 +675,10 @@ private module Spanner {
   class SqlExecutionBatch extends SqlExecution, API::CallNode {
     SqlExecutionBatch() { this = transaction().getMember("batchUpdate").getACall() }
 
+    override DataFlow::Node getAResult() {
+      none() // no results, batch update callbacks get only row counts.
+    }
+
     override DataFlow::Node getAQueryArgument() {
       // just use the whole array as the query argument, as arrays becomes tainted if one of the elements
       // are tainted
@@ -637,6 +697,13 @@ private module Spanner {
       this = v1SpannerClient().getMember(["executeSql", "executeStreamingSql"]).getACall()
     }
 
+    override DataFlow::Node getAResult() {
+      (
+        this = v1SpannerClient().getMember("executeSql").getACall() and
+        result = this.(DataFlow::CallNode).getCallback(_).getParameter(1)
+      )
+    }
+
     override DataFlow::Node getAQueryArgument() { result = this.getOptionArgument(0, "sql") }
   }
 

@@ -14,7 +14,7 @@ private import semmle.javascript.security.dataflow.CommandInjectionCustomization
 abstract class HeuristicSource extends DataFlow::Node { }
 
 /**
- * An access to a password, viewed a source of remote flow.
+ * An access to a password, viewed as a source of remote flow.
  */
 private class RemoteFlowPassword extends HeuristicSource, RemoteFlowSource {
   RemoteFlowPassword() { isReadFrom(this, "(?is).*(password|passwd).*") }
@@ -52,3 +52,16 @@ class RemoteServerResponse extends HeuristicSource, RemoteFlowSource {
 
   override string getSourceType() { result = "a response from a remote server" }
 }
+
+// /**
+//  * Data read from a database.
+//  */
+// class DatabaseAccessResultRemoteFlowSource extends HeuristicSource, RemoteFlowSource {
+//   DatabaseAccessResultRemoteFlowSource() {
+//     exists(DatabaseAccess dba | this = dba.getAResult())
+//   }
+
+//   override string getSourceType() { result = "Database query result" }
+
+//   override predicate isUserControlledObject() { this.(DatabaseAccess).returnsUserControlledObject() }
+// }
@@ -38,4 +38,4 @@
 | sinks.js:56:12:56:15 | sink |
 | sinks.js:57:5:57:15 | csrf + sink |
 | sinks.js:71:16:71:39 | req.que ...  x \|\| y |
-| sinks.js:72:16:72:39 | x \|\| re ... le \|\| y |
+| sinks.js:72:16:72:39 | x \|\| re ... le \|\| y |
@@ -1,4 +1,4 @@
 import javascript
 private import semmle.javascript.heuristics.AdditionalSinks
 
-select any(HeuristicSink s)
+select any(HeuristicSink s | s.getFile().getBaseName() = "sinks.js")
@@ -1,2 +0,0 @@
-| sources.js:2:5:2:12 | password |
-| sources.js:3:5:3:20 | JSON.stringify() |

@@ -1,4 +1,15 @@
 import javascript
 private import semmle.javascript.heuristics.AdditionalSources
+import testUtilities.ConsistencyChecking
 
-select any(HeuristicSource s)
+class Taint extends TaintTracking::Configuration {
+    Taint() { this = "Taint" }
+
+    override predicate isSource(DataFlow::Node node) {
+        node instanceof HeuristicSource
+    }
+
+    override predicate isSink(DataFlow::Node node) {
+        node = any(DataFlow::CallNode call | call.getCalleeName() = "sink").getAnArgument()
+    }
+}