Merge pull request #8604 from erik-krogh/httpNode

JS: refactor most library models away from AST nodes

Author: erik-krogh

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/CoreKnowledge.qll

@@ -175,7 +175,7 @@ predicate isOtherModeledArgument(DataFlow::Node n, FilteringReason reason) {
   or
   n instanceof CryptographicKey and reason instanceof CryptographicKeyReason
   or
-  any(CryptographicOperation op).getInput().flow() = n and
+  any(CryptographicOperation op).getInput() = n and
   reason instanceof CryptographicOperationFlowReason
   or
   exists(DataFlow::CallNode call | n = call.getAnArgument() |

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll

@@ -144,7 +144,7 @@ private module AccessPaths {
       not param = base.getReceiver()
     |
       result = param and
-      name = param.asSource().asExpr().(Parameter).getName()
+      name = param.asSource().(DataFlow::ParameterNode).getName()
       or
       param.asSource().asExpr() instanceof DestructuringPattern and
       result = param.getMember(name)

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll

@@ -42,10 +42,10 @@ module SinkEndpointFilter {
       result = "modeled database access"
       or
       // Remove calls to APIs that aren't relevant to NoSQL injection
-      call.getReceiver().asExpr() instanceof HTTP::RequestExpr and
+      call.getReceiver() instanceof HTTP::RequestNode and
       result = "receiver is a HTTP request expression"
       or
-      call.getReceiver().asExpr() instanceof HTTP::ResponseExpr and
+      call.getReceiver() instanceof HTTP::ResponseNode and
       result = "receiver is a HTTP response expression"
     )
     or
@@ -115,7 +115,7 @@ predicate isBaseAdditionalFlowStep(
   inlbl = TaintedObject::label() and
   outlbl = TaintedObject::label() and
   exists(NoSql::Query query, DataFlow::SourceNode queryObj |
-    queryObj.flowsToExpr(query) and
+    queryObj.flowsTo(query) and
     queryObj.flowsTo(trg) and
     src = queryObj.getAPropertyWrite().getRhs()
   )

javascript/ql/lib/change-notes/2022-04-04-dataflow-models.md

@@ -0,0 +1,57 @@
+---
+category: breaking
+---
+* Many library models have been rewritten to use dataflow nodes instead of the AST.
+  The types of some classes have been changed, and these changes may break existing code.
+  Other classes and predicates have been renamed, in these cases the old name is still available as a deprecated feature.
+
+* The basetype of the following list of classes has changed from an expression to a dataflow node, and thus code using these classes might break. 
+  The fix to these breakages is usually to use `asExpr()` to get an expression from a dataflow node, or to use `.flow()` to get a dataflow node from an expression.  
+   - DOM.qll#WebStorageWrite
+   - CryptoLibraries.qll#CryptographicOperation
+   - Express.qll#Express::RequestBodyAccess
+   - HTTP.qll#HTTP::ResponseBody
+   - HTTP.qll#HTTP::CookieDefinition
+   - HTTP.qll#HTTP::ServerDefinition
+   - HTTP.qll#HTTP::RouteSetup
+   - NoSQL.qll#NoSql::Query
+   - SQL.qll#SQL::SqlString
+   - SQL.qll#SQL::SqlSanitizer
+   - HTTP.qll#ResponseBody
+   - HTTP.qll#CookieDefinition
+   - HTTP.qll#ServerDefinition
+   - HTTP.qll#RouteSetup
+   - HTTP.qll#HTTP::RedirectInvocation
+   - HTTP.qll#RedirectInvocation
+   - Express.qll#Express::RouterDefinition
+   - AngularJSCore.qll#LinkFunction
+   - Connect.qll#Connect::StandardRouteHandler
+   - CryptoLibraries.qll#CryptographicKeyCredentialsExpr
+   - AWS.qll#AWS::Credentials
+   - Azure.qll#Azure::Credentials
+   - Connect.qll#Connect::Credentials
+   - DigitalOcean.qll#DigitalOcean::Credentials
+   - Express.qll#Express::Credentials
+   - NodeJSLib.qll#NodeJSLib::Credentials
+   - PkgCloud.qll#PkgCloud::Credentials
+   - Request.qll#Request::Credentials
+   - ServiceDefinitions.qll#InjectableFunctionServiceRequest
+   - SensitiveActions.qll#SensitiveVariableAccess
+   - SensitiveActions.qll#CleartextPasswordExpr
+   - Connect.qll#Connect::ServerDefinition
+   - Restify.qll#Restify::ServerDefinition
+   - Connect.qll#Connect::RouteSetup
+   - Express.qll#Express::RouteSetup
+   - Fastify.qll#Fastify::RouteSetup
+   - Hapi.qll#Hapi::RouteSetup
+   - Koa.qll#Koa::RouteSetup
+   - Restify.qll#Restify::RouteSetup
+   - NodeJSLib.qll#NodeJSLib::RouteSetup
+   - Express.qll#Express::StandardRouteHandler
+   - Express.qll#Express::SetCookie
+   - Hapi.qll#Hapi::RouteHandler
+   - HTTP.qll#HTTP::Servers::StandardHeaderDefinition
+   - HTTP.qll#Servers::StandardHeaderDefinition
+   - Hapi.qll#Hapi::ServerDefinition
+   - Koa.qll#Koa::AppDefinition
+   - SensitiveActions.qll#SensitiveCall

javascript/ql/lib/semmle/javascript/Expr.qll

@@ -167,7 +167,7 @@ class Expr extends @expr, ExprOrStmt, ExprOrType, AST::ValueNode {
   /**
    * Holds if this expression may refer to the initial value of parameter `p`.
    */
-  predicate mayReferToParameter(Parameter p) { this.flow().mayReferToParameter(p) }
+  predicate mayReferToParameter(Parameter p) { DataFlow::parameterNode(p).flowsToExpr(this) }
 
   /**
    * Gets the static type of this expression, as determined by the TypeScript type system.

javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll

@@ -139,9 +139,12 @@ module DataFlow {
     }
 
     /**
+     * DEPRECATED: Use `DataFlow::ParameterNode::flowsTo()` instead.
      * Holds if this expression may refer to the initial value of parameter `p`.
      */
-    predicate mayReferToParameter(Parameter p) { parameterNode(p).(SourceNode).flowsTo(this) }
+    deprecated predicate mayReferToParameter(Parameter p) {
+      parameterNode(p).(SourceNode).flowsTo(this)
+    }
 
     /**
      * Holds if this element is at the specified location.

javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll

@@ -472,6 +472,12 @@ class FunctionNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   /** Gets a parameter of this function. */
   ParameterNode getAParameter() { result = this.getParameter(_) }
 
+  /** Gets the parameter named `name` of this function, if any. */
+  DataFlow::ParameterNode getParameterByName(string name) {
+    result = this.getAParameter() and
+    result.getName() = name
+  }
+
   /** Gets the number of parameters declared on this function. */
   int getNumParameter() { result = count(astNode.getAParameter()) }
 
@@ -556,6 +562,14 @@ class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   DataFlow::FunctionNode getPropertySetter(string name) {
     result = astNode.getPropertyByName(name).(PropertySetter).getInit().flow()
   }
+
+  /** Gets the value of a computed property name of this object literal, such as `x` in `{[x]: 1}` */
+  DataFlow::Node getAComputedPropertyName() {
+    exists(Property prop | prop = astNode.getAProperty() |
+      prop.isComputed() and
+      result = prop.getNameExpr().flow()
+    )
+  }
 }
 
 /**

javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

@@ -482,17 +482,13 @@ module TaintTracking {
    */
   private class HeapTaintStep extends SharedTaintStep {
     override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(Expr e, Expr f | e = succ.asExpr() and f = pred.asExpr() |
-        exists(Property prop | e.(ObjectExpr).getAProperty() = prop |
-          prop.isComputed() and f = prop.getNameExpr()
-        )
-        or
-        // spreading a tainted object into an object literal gives a tainted object
-        e.(ObjectExpr).getAProperty().(SpreadProperty).getInit().(SpreadElement).getOperand() = f
-        or
-        // spreading a tainted value into an array literal gives a tainted array
-        e.(ArrayExpr).getAnElement().(SpreadElement).getOperand() = f
-      )
+      succ.(DataFlow::ObjectLiteralNode).getAComputedPropertyName() = pred
+      or
+      // spreading a tainted object into an object literal gives a tainted object
+      succ.(DataFlow::ObjectLiteralNode).getASpreadProperty() = pred
+      or
+      // spreading a tainted value into an array literal gives a tainted array
+      succ.(DataFlow::ArrayCreationNode).getASpreadArgument() = pred
       or
       // arrays with tainted elements and objects with tainted property names are tainted
       succ.(DataFlow::ArrayCreationNode).getAnElement() = pred and
@@ -546,16 +542,16 @@ module TaintTracking {
    */
   private class ComputedPropWriteTaintStep extends SharedTaintStep {
     override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(AssignExpr assgn, IndexExpr idx, DataFlow::SourceNode obj |
-        assgn.getTarget() = idx and
-        obj.flowsToExpr(idx.getBase()) and
-        not exists(idx.getPropertyName()) and
-        pred = DataFlow::valueNode(assgn.getRhs()) and
+      exists(DataFlow::PropWrite assgn, DataFlow::SourceNode obj |
+        not exists(assgn.getPropertyName()) and
+        not assgn.getWriteNode() instanceof Property and // not a write inside an object literal
+        pred = assgn.getRhs() and
+        assgn = obj.getAPropertyWrite() and
         succ = obj
       |
         obj instanceof DataFlow::ObjectLiteralNode
         or
-        obj.getAPropertyRead("length").flowsToExpr(idx.getPropertyNameExpr())
+        obj.getAPropertyRead("length").flowsToExpr(assgn.getPropertyNameExpr())
       )
     }
   }
@@ -580,8 +576,8 @@ module TaintTracking {
     override predicate stringManipulationStep(DataFlow::Node pred, DataFlow::Node target) {
       exists(DataFlow::ValueNode succ | target = succ |
         // string operations that propagate taint
-        exists(string name | name = succ.getAstNode().(MethodCallExpr).getMethodName() |
-          pred.asExpr() = succ.getAstNode().(MethodCallExpr).getReceiver() and
+        exists(string name | name = succ.(DataFlow::MethodCallNode).getMethodName() |
+          pred = succ.(DataFlow::MethodCallNode).getReceiver() and
           (
             // sorted, interesting, properties of String.prototype
             name =
@@ -600,7 +596,7 @@ module TaintTracking {
             name = "join"
           )
           or
-          exists(int i | pred.asExpr() = succ.getAstNode().(MethodCallExpr).getArgument(i) |
+          exists(int i | pred = succ.(DataFlow::MethodCallNode).getArgument(i) |
             name = "concat"
             or
             name = ["replace", "replaceAll"] and i = 1
@@ -615,10 +611,10 @@ module TaintTracking {
         )
         or
         // String.fromCharCode and String.fromCodePoint
-        exists(int i, MethodCallExpr mce |
-          mce = succ.getAstNode() and
-          pred.asExpr() = mce.getArgument(i) and
-          (mce.getMethodName() = "fromCharCode" or mce.getMethodName() = "fromCodePoint")
+        exists(int i, DataFlow::MethodCallNode mcn |
+          mcn = succ and
+          pred = mcn.getArgument(i) and
+          mcn.getMethodName() = ["fromCharCode", "fromCodePoint"]
         )
         or
         // `(encode|decode)URI(Component)?` propagate taint
@@ -744,11 +740,11 @@ module TaintTracking {
    * the parameters in `input`.
    */
   predicate isUrlSearchParams(DataFlow::SourceNode params, DataFlow::Node input) {
-    exists(DataFlow::GlobalVarRefNode urlSearchParams, NewExpr newUrlSearchParams |
+    exists(DataFlow::GlobalVarRefNode urlSearchParams, DataFlow::NewNode newUrlSearchParams |
       urlSearchParams.getName() = "URLSearchParams" and
-      newUrlSearchParams = urlSearchParams.getAnInstantiation().asExpr() and
-      params.asExpr() = newUrlSearchParams and
-      input.asExpr() = newUrlSearchParams.getArgument(0)
+      newUrlSearchParams = urlSearchParams.getAnInstantiation() and
+      params = newUrlSearchParams and
+      input = newUrlSearchParams.getArgument(0)
     )
   }
 

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

@@ -8,19 +8,19 @@ module AWS {
   /**
    * Holds if the `i`th argument of `invk` is an object hash for `AWS.Config`.
    */
-  private predicate takesConfigurationObject(InvokeExpr invk, int i) {
+  private predicate takesConfigurationObject(DataFlow::InvokeNode invk, int i) {
     exists(DataFlow::ModuleImportNode mod | mod.getPath() = "aws-sdk" |
       // `AWS.config.update(nd)`
-      invk = mod.getAPropertyRead("config").getAMemberCall("update").asExpr() and
+      invk = mod.getAPropertyRead("config").getAMemberCall("update") and
       i = 0
       or
       exists(DataFlow::SourceNode cfg | cfg = mod.getAConstructorInvocation("Config") |
         // `new AWS.Config(nd)`
-        invk = cfg.asExpr() and
+        invk = cfg and
         i = 0
         or
         // `var config = new AWS.Config(...); config.update(nd);`
-        invk = cfg.getAMemberCall("update").asExpr() and
+        invk = cfg.getAMemberCall("update") and
         i = 0
       )
     )
@@ -29,13 +29,13 @@ module AWS {
   /**
    * An expression that is used as an AWS config value: `{ accessKeyId: <user>, secretAccessKey: <password>}`.
    */
-  class Credentials extends CredentialsExpr {
+  class Credentials extends CredentialsNode {
     string kind;
 
     Credentials() {
-      exists(string prop, InvokeExpr invk, int i |
+      exists(string prop, DataFlow::InvokeNode invk, int i |
         takesConfigurationObject(invk, i) and
-        invk.hasOptionArgument(i, prop, this)
+        this = invk.getOptionArgument(i, prop)
       |
         prop = "accessKeyId" and kind = "user name"
         or