JS: Improvements from manual data flow investigations

javascript/change-notes/2021-03-29-misc-steps.md

@@ -0,0 +1,6 @@
+lgtm,codescanning
+* The `lodash-es` package is now recognized as a variant of `lodash`.
+* Taint is now propagated through the `babel.transform` function.
+* Improved data flow through React applications using `redux-form` or `react-router`.
+* Base64 decoding using the `react-native-base64` package is now recognized.
+* An expression of form `o[o.length] = y` is now recognized as appending to an array.

javascript/ql/src/semmle/javascript/Base64.qll

@@ -150,7 +150,8 @@ private class NpmBase64Encode extends Base64::Encode::Range, DataFlow::CallNode
       enc = DataFlow::moduleMember("base64url", "toBase64") or
       enc = DataFlow::moduleMember("js-base64", "Base64").getAPropertyRead("encode") or
       enc = DataFlow::moduleMember("js-base64", "Base64").getAPropertyRead("encodeURI") or
-      enc = DataFlow::moduleMember("urlsafe-base64", "encode")
+      enc = DataFlow::moduleMember("urlsafe-base64", "encode") or
+      enc = DataFlow::moduleMember("react-native-base64", ["encode", "encodeFromByteArray"])
     |
       this = enc.getACall()
     )
@@ -186,7 +187,8 @@ private class NpmBase64Decode extends Base64::Decode::Range, DataFlow::CallNode
       dec = DataFlow::moduleMember("base64url", "decode") or
       dec = DataFlow::moduleMember("base64url", "fromBase64") or
       dec = DataFlow::moduleMember("js-base64", "Base64").getAPropertyRead("decode") or
-      dec = DataFlow::moduleMember("urlsafe-base64", "decode")
+      dec = DataFlow::moduleMember("urlsafe-base64", "decode") or
+      dec = DataFlow::moduleMember("react-native-base64", "decode")
     |
       this = dec.getACall()
     )

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

@@ -584,22 +584,26 @@ module TaintTracking {
 
   /**
    * A taint propagating data flow edge for assignments of the form `o[k] = v`, where
-   * `k` is not a constant and `o` refers to some object literal; in this case, we consider
-   * taint to flow from `v` to that object literal.
+   * one of the following holds:
    *
-   * The rationale for this heuristic is that if properties of `o` are accessed by
-   * computed (that is, non-constant) names, then `o` is most likely being treated as
-   * a map, not as a real object. In this case, it makes sense to consider the entire
-   * map to be tainted as soon as one of its entries is.
+   * - `k` is not a constant and `o` refers to some object literal. The rationale
+   *   here is that `o` is most likely being used like a dictionary object.
+   *
+   * - `k` refers to `o.length`, that is, the assignment is of form `o[o.length] = v`.
+   *   In this case, the assignment behaves like `o.push(v)`.
    */
-  private class DictionaryTaintStep extends SharedTaintStep {
+  private class ComputedPropWriteTaintStep extends SharedTaintStep {
     override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(AssignExpr assgn, IndexExpr idx, DataFlow::ObjectLiteralNode obj |
+      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
         succ = obj
+      |
+        obj instanceof DataFlow::ObjectLiteralNode
+        or
+        obj.getAPropertyRead("length").flowsToExpr(idx.getPropertyNameExpr())
       )
     }
   }

javascript/ql/src/semmle/javascript/frameworks/Babel.qll

@@ -188,4 +188,20 @@ module Babel {
     /** Gets the name of the variable used to create JSX elements. */
     string getJsxFactoryVariableName() { result = getOption("pragma").(JSONString).getValue() }
   }
+
+  /**
+   * A taint step through a call to the Babel `transform` function.
+   */
+  private class TransformTaintStep extends TaintTracking::SharedTaintStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(API::CallNode call |
+        call =
+          API::moduleImport(["@babel/standalone", "@babel/core"])
+              .getMember(["transform", "transformSync", "transformAsync"])
+              .getACall() and
+        pred = call.getArgument(0) and
+        succ = [call, call.getParameter(2).getParameter(0).getAnImmediateUse()]
+      )
+    }
+  }
 }

javascript/ql/src/semmle/javascript/frameworks/LodashUnderscore.qll

@@ -21,11 +21,9 @@ module LodashUnderscore {
     string name;
 
     DefaultMember() {
-      this = DataFlow::moduleMember("underscore", name)
+      this = DataFlow::moduleMember(["underscore", "lodash", "lodash-es"], name)
       or
-      this = DataFlow::moduleMember("lodash", name)
-      or
-      this = DataFlow::moduleImport("lodash/" + name)
+      this = DataFlow::moduleImport(["lodash/", "lodash-es/"] + name)
       or
       this = DataFlow::moduleImport("lodash." + name.toLowerCase()) and isLodashMember(name)
       or

javascript/ql/src/semmle/javascript/frameworks/PropertyProjection.qll

@@ -75,10 +75,6 @@ private DataFlow::SourceNode getASimplePropertyProjectionCallee(
 ) {
   singleton = false and
   (
-    result = LodashUnderscore::member("pick") and
-    objectIndex = 0 and
-    selectorIndex = [1 .. max(result.getACall().getNumArgument())]
-    or
     result = LodashUnderscore::member("pickBy") and
     objectIndex = 0 and
     selectorIndex = 1
@@ -131,6 +127,19 @@ private class SimplePropertyProjection extends PropertyProjection::Range {
   override predicate isSingletonProjection() { singleton = true }
 }
 
+/**
+ * A property projection with a variable number of selector indices.
+ */
+private class VarArgsPropertyProjection extends PropertyProjection::Range {
+  VarArgsPropertyProjection() { this = LodashUnderscore::member("pick").getACall() }
+
+  override DataFlow::Node getObject() { result = getArgument(0) }
+
+  override DataFlow::Node getASelector() { result = getArgument(any(int i | i > 0)) }
+
+  override predicate isSingletonProjection() { none() }
+}
+
 /**
  * A taint step for a property projection.
  */

javascript/ql/src/semmle/javascript/frameworks/React.qll

@@ -691,15 +691,22 @@ private DataFlow::SourceNode reactRouterDom() {
   result = DataFlow::moduleImport("react-router-dom")
 }
 
+private DataFlow::SourceNode reactRouterMatchObject() {
+  result = reactRouterDom().getAMemberCall(["useRouteMatch", "matchPath"])
+  or
+  exists(ReactComponent c |
+    dependedOnByReactRouterClient(c.getTopLevel()) and
+    result = c.getAPropRead("match")
+  )
+}
+
 private class ReactRouterSource extends ClientSideRemoteFlowSource {
   ClientSideRemoteFlowKind kind;
 
   ReactRouterSource() {
     this = reactRouterDom().getAMemberCall("useParams") and kind.isPath()
     or
-    exists(string prop |
-      this = reactRouterDom().getAMemberCall("useRouteMatch").getAPropertyRead(prop)
-    |
+    exists(string prop | this = reactRouterMatchObject().getAPropertyRead(prop) |
       prop = "params" and kind.isPath()
       or
       prop = "url" and kind.isUrl()
@@ -713,16 +720,25 @@ private class ReactRouterSource extends ClientSideRemoteFlowSource {
 
 /**
  * Holds if `mod` transitively depends on `react-router-dom`.
- *
- * We assume any React component in such a file may be used in a context where react-router
- * injects the `location` property in its `props` object.
  */
 private predicate dependsOnReactRouter(Module mod) {
   mod.getAnImport().getImportedPath().getValue() = "react-router-dom"
   or
   dependsOnReactRouter(mod.getAnImportedModule())
 }
 
+/**
+ * Holds if `mod` is imported from a module that transitively depends on `react-router-dom`.
+ *
+ * We assume any React component in such a file may be used in a context where react-router
+ * injects the `location` and `match` properties in its `props` object.
+ */
+private predicate dependedOnByReactRouterClient(Module mod) {
+  dependsOnReactRouter(mod)
+  or
+  dependedOnByReactRouterClient(any(Module m | m.getAnImportedModule() = mod))
+}
+
 /**
  * A reference to the DOM location obtained through `react-router-dom`
  *
@@ -740,7 +756,7 @@ private class ReactRouterLocationSource extends DOM::LocationSource::Range {
     this = reactRouterDom().getAMemberCall("useLocation")
     or
     exists(ReactComponent component |
-      dependsOnReactRouter(component.getTopLevel()) and
+      dependedOnByReactRouterClient(component.getTopLevel()) and
       this = component.getAPropRead("location")
     )
   }
@@ -759,6 +775,8 @@ private DataFlow::SourceNode higherOrderComponentBuilder() {
   or
   result = DataFlow::moduleMember(["react-hot-loader", "react-hot-loader/root"], "hot").getACall()
   or
+  result = DataFlow::moduleMember("redux-form", "reduxForm").getACall()
+  or
   result = reactRouterDom().getAPropertyRead("withRouter")
   or
   exists(FunctionCompositionCall compose |

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -11,6 +11,7 @@ typeInferenceMismatch
 | array-mutation.js:27:16:27:23 | source() | array-mutation.js:28:8:28:8 | g |
 | array-mutation.js:31:33:31:40 | source() | array-mutation.js:32:8:32:8 | h |
 | array-mutation.js:35:36:35:43 | source() | array-mutation.js:36:8:36:8 | i |
+| array-mutation.js:39:17:39:24 | source() | array-mutation.js:40:8:40:8 | j |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:4:8:4:8 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:13:10:13:10 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:19:10:19:10 | x |

javascript/ql/test/library-tests/TaintTracking/array-mutation.js

@@ -34,4 +34,8 @@ function test(x, y) {
   let i = [];
   Array.prototype.unshift.apply(i, source());
   sink(i); // NOT OK
+
+  let j = [];
+  j[j.length] = source();
+  sink(j); // NOT OK
 }

javascript/ql/test/library-tests/frameworks/ReactJS/importedComponent.jsx

@@ -1,5 +1,5 @@
 import { MyComponent } from "./exportedComponent";
 
-export function render(color) {
+export function render({color, location}) {
     return <MyComponent color={color}/>
 }

javascript/ql/test/library-tests/frameworks/ReactJS/tests.expected

@@ -16,6 +16,7 @@ test_ReactComponent_getInstanceMethod
 | es5.js:18:33:22:1 | {\\n  ren ... ;\\n  }\\n} | render | es5.js:19:11:21:3 | functio ... 1>;\\n  } |
 | es6.js:1:1:8:1 | class H ... ;\\n  }\\n} | render | es6.js:2:9:4:3 | () {\\n   ... v>;\\n  } |
 | exportedComponent.jsx:1:8:3:1 | functio ... r}}/>\\n} | render | exportedComponent.jsx:1:8:3:1 | functio ... r}}/>\\n} |
+| importedComponent.jsx:3:8:5:1 | functio ... or}/>\\n} | render | importedComponent.jsx:3:8:5:1 | functio ... or}/>\\n} |
 | plainfn.js:1:1:3:1 | functio ... div>;\\n} | render | plainfn.js:1:1:3:1 | functio ... div>;\\n} |
 | plainfn.js:5:1:7:1 | functio ... iv");\\n} | render | plainfn.js:5:1:7:1 | functio ... iv");\\n} |
 | plainfn.js:9:1:12:1 | functio ... rn x;\\n} | render | plainfn.js:9:1:12:1 | functio ... rn x;\\n} |
@@ -97,6 +98,7 @@ test_ReactComponent_ref
 | es6.js:14:1:20:1 | class H ...     }\\n} | es6.js:17:9:17:12 | this |
 | es6.js:14:1:20:1 | class H ...     }\\n} | es6.js:18:9:18:12 | this |
 | exportedComponent.jsx:1:8:3:1 | functio ... r}}/>\\n} | exportedComponent.jsx:1:8:1:7 | this |
+| importedComponent.jsx:3:8:5:1 | functio ... or}/>\\n} | importedComponent.jsx:3:8:3:7 | this |
 | namedImport.js:3:1:3:28 | class C ... nent {} | namedImport.js:3:27:3:26 | this |
 | namedImport.js:5:1:5:20 | class D extends C {} | namedImport.js:5:19:5:18 | this |
 | plainfn.js:1:1:3:1 | functio ... div>;\\n} | plainfn.js:1:1:1:0 | this |
@@ -198,6 +200,7 @@ test_ReactComponent_getADirectPropsSource
 | es6.js:1:1:8:1 | class H ... ;\\n  }\\n} | es6.js:1:37:1:36 | args |
 | es6.js:1:1:8:1 | class H ... ;\\n  }\\n} | es6.js:3:24:3:33 | this.props |
 | exportedComponent.jsx:1:8:3:1 | functio ... r}}/>\\n} | exportedComponent.jsx:1:29:1:33 | props |
+| importedComponent.jsx:3:8:5:1 | functio ... or}/>\\n} | importedComponent.jsx:3:24:3:40 | {color, location} |
 | namedImport.js:3:1:3:28 | class C ... nent {} | namedImport.js:3:27:3:26 | args |
 | namedImport.js:5:1:5:20 | class D extends C {} | namedImport.js:5:19:5:18 | args |
 | plainfn.js:1:1:3:1 | functio ... div>;\\n} | plainfn.js:1:16:1:20 | props |
@@ -236,6 +239,7 @@ test_ReactComponent
 | es6.js:1:1:8:1 | class H ... ;\\n  }\\n} |
 | es6.js:14:1:20:1 | class H ...     }\\n} |
 | exportedComponent.jsx:1:8:3:1 | functio ... r}}/>\\n} |
+| importedComponent.jsx:3:8:5:1 | functio ... or}/>\\n} |
 | namedImport.js:3:1:3:28 | class C ... nent {} |
 | namedImport.js:5:1:5:20 | class D extends C {} |
 | plainfn.js:1:1:3:1 | functio ... div>;\\n} |
@@ -264,6 +268,8 @@ test_ReactComponent_getAPropRead
 | es5.js:18:33:22:1 | {\\n  ren ... ;\\n  }\\n} | name | es5.js:20:24:20:38 | this.props.name |
 | es6.js:1:1:8:1 | class H ... ;\\n  }\\n} | name | es6.js:3:24:3:38 | this.props.name |
 | exportedComponent.jsx:1:8:3:1 | functio ... r}}/>\\n} | color | exportedComponent.jsx:2:32:2:42 | props.color |
+| importedComponent.jsx:3:8:5:1 | functio ... or}/>\\n} | color | importedComponent.jsx:3:25:3:29 | color |
+| importedComponent.jsx:3:8:5:1 | functio ... or}/>\\n} | location | importedComponent.jsx:3:32:3:39 | location |
 | plainfn.js:1:1:3:1 | functio ... div>;\\n} | name | plainfn.js:2:22:2:31 | props.name |
 | preact.js:1:1:7:1 | class H ...     }\\n} | name | preact.js:3:9:3:18 | props.name |
 | probably-a-component.js:1:1:6:1 | class H ...     }\\n} | name | probably-a-component.js:3:9:3:23 | this.props.name |
@@ -288,6 +294,9 @@ test_JSXname
 | thisAccesses.js:60:19:60:41 | <this.n ... s.name> | thisAccesses.js:60:20:60:28 | this.name | this.name | dot |
 | thisAccesses.js:61:19:61:41 | <this.t ... s.this> | thisAccesses.js:61:20:61:28 | this.this | this.this | dot |
 | thisAccesses_importedMappers.js:13:16:13:21 | <div/> | thisAccesses_importedMappers.js:13:17:13:19 | div | div | Identifier |
+| use-react-router.jsx:5:17:5:87 | <Router ... Router> | use-react-router.jsx:5:18:5:23 | Router | Router | Identifier |
+| use-react-router.jsx:5:25:5:78 | <Route> ... /Route> | use-react-router.jsx:5:26:5:30 | Route | Route | Identifier |
+| use-react-router.jsx:5:32:5:70 | <Import ... ponent> | use-react-router.jsx:5:33:5:49 | ImportedComponent | ImportedComponent | Identifier |
 | useHigherOrderComponent.jsx:5:12:5:39 | <SomeCo ... "red"/> | useHigherOrderComponent.jsx:5:13:5:25 | SomeComponent | SomeComponent | Identifier |
 | useHigherOrderComponent.jsx:11:12:11:46 | <LazyLo ... lazy"/> | useHigherOrderComponent.jsx:11:13:11:31 | LazyLoadedComponent | LazyLoadedComponent | Identifier |
 | useHigherOrderComponent.jsx:17:12:17:48 | <LazyLo ... azy2"/> | useHigherOrderComponent.jsx:17:13:17:32 | LazyLoadedComponent2 | LazyLoadedComponent2 | Identifier |
@@ -298,3 +307,5 @@ test_JSXName_this
 | statePropertyWrites.js:38:12:38:45 | <div>He ... }</div> | statePropertyWrites.js:38:24:38:27 | this |
 | thisAccesses.js:60:19:60:41 | <this.n ... s.name> | thisAccesses.js:60:20:60:23 | this |
 | thisAccesses.js:61:19:61:41 | <this.t ... s.this> | thisAccesses.js:61:20:61:23 | this |
+locationSource
+| importedComponent.jsx:3:32:3:39 | location |

javascript/ql/test/library-tests/frameworks/ReactJS/tests.ql

@@ -9,3 +9,5 @@ import ReactComponent_getACandidatePropsValue
 import ReactComponent
 import ReactComponent_getAPropRead
 import ReactName
+
+query DataFlow::SourceNode locationSource() { result = DOM::locationSource() }

javascript/ql/test/library-tests/frameworks/ReactJS/use-react-router.jsx

@@ -0,0 +1,5 @@
+import * as ReactDOM from "react-dom"
+import { Route, Router } from "react-router-dom";
+import ImportedComponent from "./importedComponent";
+
+ReactDOM.render(<Router><Route><ImportedComponent></ImportedComponent></Route></Router>);