Merge pull request #7474 from kaeluka/db-reads-as-taint-sources

JS: DB reads as taint sources

Author: Stephan Brandauer

javascript/ql/lib/semmle/javascript/Concepts.qll

@@ -84,6 +84,11 @@ abstract class FileNameSource extends DataFlow::Node { }
 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. */
+  DataFlow::Node getAResult() {
+    none() // Overridden in subclass
+  }
 }
 
 /**

javascript/ql/lib/semmle/javascript/frameworks/Knex.qll

@@ -46,17 +46,35 @@ module Knex {
     RawKnexSqlString() { this = any(RawKnexCall call).getArgument(0).asExpr() }
   }
 
-  /** A call that triggers a SQL query submission. */
-  private class KnexDatabaseAccess extends DatabaseAccess {
-    KnexDatabaseAccess() {
-      this = knexObject().getMember(["then", "stream", "asCallback"]).getACall()
+  /** A call that triggers a SQL query submission by calling then/stream/asCallback. */
+  private class KnexDatabaseCallback extends DatabaseAccess, DataFlow::CallNode {
+    string member;
+
+    KnexDatabaseCallback() {
+      member = ["then", "stream", "asCallback"] and
+      this = knexObject().getMember(member).getACall()
+    }
+
+    override DataFlow::Node getAResult() {
+      member = "then" and
+      result = this.getCallback(0).getParameter(0)
       or
-      exists(AwaitExpr await |
-        this = await.flow() and
-        await.getOperand() = knexObject().getAUse().asExpr()
+      member = "asCallback" and
+      result = this.getCallback(0).getParameter(1)
+    }
+
+    override DataFlow::Node getAQueryArgument() { none() }
+  }
+
+  private class KnexDatabaseAwait extends DatabaseAccess, DataFlow::ValueNode {
+    KnexDatabaseAwait() {
+      exists(AwaitExpr enclosingAwait | this = enclosingAwait.flow() |
+        enclosingAwait.getOperand() = knexObject().getAUse().asExpr()
       )
     }
 
+    override DataFlow::Node getAResult() { result = this }
+
     override DataFlow::Node getAQueryArgument() { none() }
   }
 }

javascript/ql/lib/semmle/javascript/frameworks/NoSQL.qll

@@ -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,12 +542,29 @@ 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, DataFlow::CallNode {
+    string member;
+
     ExplicitQueryEvaluation() {
       // explicit execution using a Query method call
-      Query::getAMongooseQuery().getMember(["exec", "then", "catch"]).getACall() = this
+      member = ["exec", "then", "catch"] and
+      Query::getAMongooseQuery().getMember(member).getACall() = this
+    }
+
+    private int resultParamIndex() {
+      member = "then" and result = 0
+      or
+      member = "exec" and result = 1
+    }
+
+    override DataFlow::Node getAResult() {
+      result = this.getCallback(_).getParameter(this.resultParamIndex())
     }
 
     override DataFlow::Node getAQueryArgument() {
@@ -588,6 +610,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))
     }
@@ -609,7 +635,7 @@ private module Minimongo {
  * Provides classes modeling the MarsDB library.
  */
 private module MarsDB {
-  private class MarsDBAccess extends DatabaseAccess {
+  private class MarsDBAccess extends DatabaseAccess, DataFlow::CallNode {
     string method;
 
     MarsDBAccess() {
@@ -623,21 +649,29 @@ private module MarsDB {
 
     string getMethod() { result = method }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { none() }
   }
 
   /** A call to a MarsDB query method. */
-  private class QueryCall extends DatabaseAccess, API::CallNode {
+  private class QueryCall extends MarsDBAccess, API::CallNode {
     int queryArgIdx;
 
     QueryCall() {
       exists(string m |
-        this.(MarsDBAccess).getMethod() = m and
+        this.getMethod() = m and
         // implements parts of the Minimongo interface
         Minimongo::CollectionMethodSignatures::interpretsArgumentAsQuery(m, queryArgIdx)
       )
     }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(queryArgIdx) }
 
     DataFlow::Node getACodeOperator() {
@@ -744,9 +778,13 @@ private module Redis {
   /**
    * An access to a database through redis
    */
-  class RedisDatabaseAccess extends DatabaseAccess {
+  class RedisDatabaseAccess extends DatabaseAccess, DataFlow::CallNode {
     RedisDatabaseAccess() { this = redis().getMember(_).getACall() }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { none() }
   }
 }
@@ -768,9 +806,13 @@ private module IoRedis {
   /**
    * An access to a database through ioredis
    */
-  class IoRedisDatabaseAccess extends DatabaseAccess {
+  class IoRedisDatabaseAccess extends DatabaseAccess, DataFlow::CallNode {
     IoRedisDatabaseAccess() { this = ioredis().getMember(_).getACall() }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() { none() }
   }
 }

javascript/ql/lib/semmle/javascript/frameworks/SQL.qll

@@ -3,6 +3,7 @@
  */
 
 import javascript
+import semmle.javascript.Promises
 
 module SQL {
   /** A string-valued expression that is interpreted as a SQL command. */
@@ -81,6 +82,8 @@ private module MySql {
       )
     }
 
+    override DataFlow::Node getAResult() { result = this.getCallback(_).getParameter(1) }
+
     override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
   }
 
@@ -178,6 +181,16 @@ private module Postgres {
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
     QueryCall() { this = [client(), pool()].getMember("query").getACall() }
 
+    override DataFlow::Node getAResult() {
+      this.getNumArgument() = 2 and
+      result = this.getCallback(1).getParameter(1)
+      or
+      this.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) }
   }
 
@@ -322,6 +335,10 @@ private module Postgres {
       )
     }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() {
       result = this.getADirectQueryArgument()
       or
@@ -370,6 +387,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) }
   }
 
@@ -413,13 +435,17 @@ private module MsSql {
   API::Node pool() { result = mssqlClass("ConnectionPool") }
 
   /** A tagged template evaluated as a query. */
-  private class QueryTemplateExpr extends DatabaseAccess, DataFlow::ValueNode {
+  private class QueryTemplateExpr extends DatabaseAccess, DataFlow::ValueNode, DataFlow::SourceNode {
     override TaggedTemplateExpr astNode;
 
     QueryTemplateExpr() {
       mssql().getMember("query").getAUse() = DataFlow::valueNode(astNode.getTag())
     }
 
+    override DataFlow::Node getAResult() {
+      PromiseFlow::loadStep(this.getALocalUse(), result, Promises::valueProp())
+    }
+
     override DataFlow::Node getAQueryArgument() {
       result = DataFlow::valueNode(astNode.getTemplate().getAnElement())
     }
@@ -429,6 +455,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) }
   }
 
@@ -505,6 +537,12 @@ private module Sequelize {
         ]
     }
   }
+
+  class SequelizeSource extends ModelInput::SourceModelCsv {
+    override predicate row(string row) {
+      row = "sequelize;Sequelize;Member[query].ReturnValue.Awaited;database-access-result"
+    }
+  }
 }
 
 private module SpannerCsv {
@@ -516,7 +554,10 @@ private module SpannerCsv {
           "@google-cloud/spanner;;@google-cloud/spanner;;Member[Spanner]",
           "@google-cloud/spanner;Database;@google-cloud/spanner;;ReturnValue.Member[instance].ReturnValue.Member[database].ReturnValue",
           "@google-cloud/spanner;v1.SpannerClient;@google-cloud/spanner;;Member[v1].Member[SpannerClient].Instance",
-          "@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[runTransaction,runTransactionAsync].Argument[0..1].Parameter[1]",
+          "@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[runTransaction,runTransactionAsync,getTransaction].Argument[0..1].Parameter[1]",
+          "@google-cloud/spanner;Transaction;@google-cloud/spanner;Database;Member[getTransaction].ReturnValue.Awaited",
+          "@google-cloud/spanner;Snapshot;@google-cloud/spanner;Database;Member[getSnapshot].Argument[0..1].Parameter[1]",
+          "@google-cloud/spanner;Snapshot;@google-cloud/spanner;Database;Member[getSnapshot].ReturnValue.Awaited",
           "@google-cloud/spanner;BatchTransaction;@google-cloud/spanner;Database;Member[batchTransaction].ReturnValue",
           "@google-cloud/spanner;BatchTransaction;@google-cloud/spanner;Database;Member[createBatchTransaction].ReturnValue.Awaited",
           "@google-cloud/spanner;~SqlExecutorDirect;@google-cloud/spanner;Database;Member[run,runPartitionedUpdate,runStream]",
@@ -539,4 +580,23 @@ private module SpannerCsv {
         ]
     }
   }
+
+  class SpannerSources extends ModelInput::SourceModelCsv {
+    string spannerClass() { result = ["v1.SpannerClient", "Database", "Transaction", "Snapshot",] }
+
+    string resultPath() {
+      result =
+        [
+          "Member[executeSql].Argument[0..].Parameter[1]",
+          "Member[executeSql].ReturnValue.Awaited.Member[0]", "Member[run].ReturnValue.Awaited",
+          "Member[run].Argument[0..].Parameter[1]",
+        ]
+    }
+
+    override predicate row(string row) {
+      row =
+        "@google-cloud/spanner;" + this.spannerClass() + ";" + this.resultPath() +
+          ";database-access-result"
+    }
+  }
 }

javascript/ql/lib/semmle/javascript/frameworks/data/internal/Shared.qll

@@ -31,7 +31,7 @@
  *     - Instance: the value returned by a constructor call
  *     - Awaited: the value from a resolved promise/future-like object
  *     - WithArity[n]: match a call with the given arity. May be a range of form `x..y` (inclusive) and/or a comma-separated list.
- *     - Other langauge-specific tokens mentioned in `ModelsAsData.qll`.
+ *     - Other language-specific tokens mentioned in `ModelsAsData.qll`.
  * 4. The `input` and `output` columns specify how data enters and leaves the element selected by the
  *    first `(package, type, path)` tuple. Both strings are `.`-separated access paths
  *    of the same syntax as the `path` column.

javascript/ql/lib/semmle/javascript/heuristics/AdditionalSources.qll

@@ -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,20 @@ class RemoteServerResponse extends HeuristicSource, RemoteFlowSource {
 
   override string getSourceType() { result = "a response from a remote server" }
 }
+
+/**
+ * A remote flow source originating from a database access.
+ */
+private class RemoteFlowSourceFromDBAccess extends RemoteFlowSource, HeuristicSource {
+  RemoteFlowSourceFromDBAccess() {
+    this = ModelOutput::getASourceNode("database-access-result").getAUse() or
+    exists(DatabaseAccess dba | this = dba.getAResult())
+  }
+
+  override string getSourceType() { result = "Database access" }
+
+  override predicate isUserControlledObject() {
+    // NB. supported databases all might return JSON.
+    any()
+  }
+}

javascript/ql/test/library-tests/Security/heuristics/HeuristicSink.ql

@@ -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")

javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.expected

@@ -1,2 +0,0 @@
-| sources.js:2:5:2:12 | password |
-| sources.js:3:5:3:20 | JSON.stringify() |

javascript/ql/test/library-tests/Security/heuristics/HeuristicSource.ql

@@ -1,4 +1,13 @@
 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()
+  }
+}