Merge pull request #1954 from asger-semmle/type-tracking-through-captured-vars

Approved by xiemaisi
2025-12-16 16:53:25 +01:00 · 2019-09-23 12:10:30 +01:00
parent e2c941c577 69a88c4fcd
commit 825a3d2917
13 changed files with 125 additions and 5 deletions
--- a/javascript/ql/src/meta/analysis-quality/CallGraphQuality.qll
+++ b/javascript/ql/src/meta/analysis-quality/CallGraphQuality.qll
@@ -173,12 +173,20 @@ SourceNode nodeLeadingToInvocation() {
  )
 }

+/**
+ * Holds if there is a call edge `invoke -> f` between a relevant invocation
+ * and a relevant function.
+ */
+predicate relevantCall(RelevantInvoke invoke, RelevantFunction f) {
+  FlowSteps::calls(invoke, f)
+}
+
 /**
 * A call site that can be resolved to a function in the same project.
 */
 class ResolvableCall extends RelevantInvoke {
  ResolvableCall() {
-    FlowSteps::calls(this, _)
+    relevantCall(this, _)
    or
    this = resolvableCallback().getAnInvocation()
  }
--- a/javascript/ql/src/meta/analysis-quality/DomValueRefs.ql
+++ b/javascript/ql/src/meta/analysis-quality/DomValueRefs.ql
@@ -0,0 +1,14 @@
+/**
+ * @name DOM value references
+ * @description The number of references to a DOM value.
+ * @kind metric
+ * @metricType project
+ * @metricAggregate sum
+ * @tags meta
+ * @id js/meta/dom-value-refs
+ */
+
+import javascript
+import CallGraphQuality
+
+select projectRoot(), count(DOM::domValueRef())
--- a/javascript/ql/src/meta/analysis-quality/ResolvableCallCandidates.ql
+++ b/javascript/ql/src/meta/analysis-quality/ResolvableCallCandidates.ql
@@ -11,4 +11,4 @@
 import javascript
 import CallGraphQuality

-select projectRoot(), count(NonExternalCall call)
+select projectRoot(), count(RelevantInvoke call)
--- a/javascript/ql/src/meta/analysis-quality/ResolvableCallRatio.ql
+++ b/javascript/ql/src/meta/analysis-quality/ResolvableCallRatio.ql
@@ -11,4 +11,4 @@
 import javascript
 import CallGraphQuality

-select projectRoot(), 100.0 * count(ResolvableCall call) / count(NonExternalCall call).(float)
+select projectRoot(), 100.0 * count(ResolvableCall call) / count(RelevantInvoke call).(float)
--- a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll
@@ -709,7 +709,8 @@ class ClassNode extends DataFlow::SourceNode {
    result = getAClassReference(t.continue()).getAnInstantiation()
    or
    t.start() and
-    result.(AnalyzedNode).getAValue() = getAbstractInstanceValue()
+    result.(AnalyzedNode).getAValue() = getAbstractInstanceValue() and
+    not result = any(DataFlow::ClassNode cls).getAReceiverNode()
    or
    t.start() and
    result = getAReceiverNode()
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -107,6 +107,27 @@ module StepSummary {
      pred = DataFlow::globalAccessPathRootPseudoNode() and
      summary = LoadStep(name)
    )
+    or
+    // Summarize calls with flow directly from a parameter to a return.
+    exists(DataFlow::ParameterNode param, DataFlow::FunctionNode fun |
+      (
+        param.flowsTo(fun.getAReturn()) and
+        summary = LevelStep()
+        or
+        exists(string prop |
+          param.getAPropertyRead(prop).flowsTo(fun.getAReturn()) and
+          summary = LoadStep(prop)
+        )
+      ) and
+      if param = fun.getAParameter() then (
+        // Step from argument to call site.
+        argumentPassing(succ, pred, fun.getFunction(), param)
+      ) else (
+        // Step from captured parameter to local call sites
+        pred = param and
+        succ = fun.getAnInvocation()
+      )
+    )
  }
 }

--- a/javascript/ql/test/library-tests/TypeTracking/ClassStyle.expected
+++ b/javascript/ql/test/library-tests/TypeTracking/ClassStyle.expected
@@ -24,6 +24,20 @@ test_Connection
 | tst.js:63:38:63:77 | api.cha ... ction() |
 | tst.js:67:14:67:47 | MyAppli ... nection |
 | tst.js:78:35:78:49 | getConnection() |
+| tst.js:80:16:80:19 | conn |
+| tst.js:84:22:84:22 | x |
+| tst.js:88:3:88:16 | innerCapture() |
+| tst.js:89:3:89:17 | innerCall(conn) |
+| tst.js:93:5:93:18 | innerCapture() |
+| tst.js:99:7:99:21 | getConnection() |
+| tst.js:100:12:100:26 | getConnection() |
+| tst.js:103:17:103:17 | x |
+| tst.js:104:10:106:6 | (functi ... \\n  })() |
+| tst.js:108:1:108:23 | shared( ... tion()) |
+| tst.js:108:8:108:22 | getConnection() |
+| tst.js:112:10:112:14 | obj.x |
+| tst.js:114:1:114:28 | getX({  ... on() }) |
+| tst.js:114:11:114:25 | getConnection() |
 | tst_conflict.js:6:38:6:77 | api.cha ... ction() |
 test_DataCallback
 | client.js:3:28:3:34 | x => {} |
@@ -35,6 +49,7 @@ test_DataCallback
 | tst.js:40:32:40:45 | getDataCurry() |
 | tst.js:45:19:45:20 | cb |
 | tst.js:48:32:48:60 | identit ... llback) |
+| tst.js:51:1:51:37 | functio ... ata) {} |
 | tst.js:58:16:58:22 | x => {} |
 | tst.js:68:16:70:3 | data => ... a);\\n  } |
 test_DataValue
@@ -43,5 +58,6 @@ test_DataValue
 | tst.js:25:19:25:22 | data |
 | tst.js:33:17:33:20 | data |
 | tst.js:38:10:38:13 | data |
+| tst.js:51:30:51:33 | data |
 | tst.js:58:16:58:16 | x |
 | tst.js:68:16:68:19 | data |
--- a/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected
+++ b/javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected
@@ -12,7 +12,16 @@ apiObject
 connection
 | type tracker with call steps | tst.js:7:15:7:18 | conn |
 | type tracker with call steps | tst.js:11:5:11:19 | this.connection |
+| type tracker with call steps | tst.js:80:16:80:19 | conn |
+| type tracker with call steps | tst.js:84:22:84:22 | x |
+| type tracker with call steps | tst.js:88:3:88:16 | innerCapture() |
+| type tracker with call steps | tst.js:89:3:89:17 | innerCall(conn) |
+| type tracker with call steps | tst.js:93:5:93:18 | innerCapture() |
+| type tracker with call steps | tst.js:103:17:103:17 | x |
+| type tracker with call steps | tst.js:104:10:106:6 | (functi ... \\n  })() |
+| type tracker with call steps | tst.js:112:10:112:14 | obj.x |
 | type tracker with call steps with property connection | tst.js:7:14:7:13 | this |
+| type tracker with call steps with property x | tst.js:111:15:111:17 | obj |
 | type tracker without call steps | client.js:1:10:1:27 | exportedConnection |
 | type tracker without call steps | tst.js:16:10:16:49 | api.cha ... ction() |
 | type tracker without call steps | tst.js:19:7:19:21 | getConnection() |
@@ -25,11 +34,18 @@ connection
 | type tracker without call steps | tst.js:63:38:63:77 | api.cha ... ction() |
 | type tracker without call steps | tst.js:67:14:67:47 | MyAppli ... nection |
 | type tracker without call steps | tst.js:78:35:78:49 | getConnection() |
+| type tracker without call steps | tst.js:99:7:99:21 | getConnection() |
+| type tracker without call steps | tst.js:100:12:100:26 | getConnection() |
+| type tracker without call steps | tst.js:108:1:108:23 | shared( ... tion()) |
+| type tracker without call steps | tst.js:108:8:108:22 | getConnection() |
+| type tracker without call steps | tst.js:114:1:114:28 | getX({  ... on() }) |
+| type tracker without call steps | tst.js:114:11:114:25 | getConnection() |
 | type tracker without call steps | tst_conflict.js:6:38:6:77 | api.cha ... ction() |
 | type tracker without call steps with property MyApplication.namespace.connection | file://:0:0:0:0 | global access path |
 | type tracker without call steps with property conflict | tst.js:63:3:63:25 | MyAppli ... mespace |
 | type tracker without call steps with property conflict | tst_conflict.js:6:3:6:25 | MyAppli ... mespace |
 | type tracker without call steps with property connection | tst.js:62:3:62:25 | MyAppli ... mespace |
+| type tracker without call steps with property x | tst.js:114:6:114:27 | { x: ge ... ion() } |
 dataCallback
 | client.js:3:28:3:34 | x => {} |
 | tst.js:10:11:10:12 | cb |
@@ -40,6 +56,7 @@ dataCallback
 | tst.js:40:32:40:45 | getDataCurry() |
 | tst.js:45:19:45:20 | cb |
 | tst.js:48:32:48:60 | identit ... llback) |
+| tst.js:51:1:51:37 | functio ... ata) {} |
 | tst.js:58:16:58:22 | x => {} |
 | tst.js:68:16:70:3 | data => ... a);\\n  } |
 dataValue
@@ -48,5 +65,6 @@ dataValue
 | tst.js:25:19:25:22 | data |
 | tst.js:33:17:33:20 | data |
 | tst.js:38:10:38:13 | data |
+| tst.js:51:30:51:33 | data |
 | tst.js:58:16:58:16 | x |
 | tst.js:68:16:68:19 | data |
--- a/javascript/ql/test/library-tests/TypeTracking/tst.js
+++ b/javascript/ql/test/library-tests/TypeTracking/tst.js
@@ -76,3 +76,40 @@ function useConnection() {
 }

 export const exportedConnection = getConnection();
+
+function outer(conn) {
+  function innerCapture() {
+    return conn;
+  }
+  function innerCall(x) {
+    return x;
+  }
+
+  innerCapture();
+  innerCall(conn);
+  innerCall(somethingElse());
+
+  function otherInner() {
+    innerCapture();
+  }
+  return class {
+    get() { return conn }
+  }
+}
+outer(getConnection());
+new (outer(getConnection())).get();
+new (outer(somethingElse())).get();
+
+function shared(x) {
+  return (function() {
+    return x;
+  })();
+}
+shared(getConnection());
+shared(somethingElse());
+
+function getX(obj) {
+  return obj.x;
+}
+getX({ x: getConnection() });
+getX({ x: somethingElse() });
--- a/javascript/ql/test/library-tests/frameworks/Electron/BrowserObject.expected
+++ b/javascript/ql/test/library-tests/frameworks/Electron/BrowserObject.expected
@@ -6,7 +6,9 @@
 | electron.js:4:10:4:46 | new Bro ... s: {}}) |
 | electron.js:35:14:35:14 | x |
 | electron.js:36:12:36:12 | x |
+| electron.js:39:1:39:7 | foo(bw) |
 | electron.js:39:5:39:6 | bw |
+| electron.js:40:1:40:7 | foo(bv) |
 | electron.js:40:5:40:6 | bv |
 | electron.ts:3:12:3:13 | bw |
 | electron.ts:3:40:3:41 | bv |
--- a/javascript/ql/test/library-tests/frameworks/Electron/WebContents.expected
+++ b/javascript/ql/test/library-tests/frameworks/Electron/WebContents.expected
@@ -1,2 +1,4 @@
+| electron.js:39:1:39:19 | foo(bw).webContents |
+| electron.js:40:1:40:19 | foo(bv).webContents |
 | electron.ts:4:3:4:16 | bw.webContents |
 | electron.ts:5:3:5:16 | bv.webContents |
--- a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp.expected
@@ -3,6 +3,7 @@
 | tst-IncompleteHostnameRegExp.js:6:2:6:43 | /^http: ... b).com/ | This regular expression has an unescaped '.' before '(example-a\|example-b).com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:6:2:6:43 | /^http: ... b).com/ | here |
 | tst-IncompleteHostnameRegExp.js:11:13:11:38 | "^http: ... le.com" | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:11:13:11:38 | "^http: ... le.com" | here |
 | tst-IncompleteHostnameRegExp.js:12:10:12:35 | "^http: ... le.com" | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:12:10:12:35 | "^http: ... le.com" | here |
+| tst-IncompleteHostnameRegExp.js:15:22:15:47 | "^http: ... le.com" | This string, which is used as a regular expression $@, has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:15:13:15:50 | id(id(i ... com"))) | here |
 | tst-IncompleteHostnameRegExp.js:17:13:17:31 | `test.example.com$` | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:17:13:17:31 | `test.example.com$` | here |
 | tst-IncompleteHostnameRegExp.js:17:14:17:30 | test.example.com$ | This string, which is used as a regular expression $@, has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:17:13:17:31 | `test.example.com$` | here |
 | tst-IncompleteHostnameRegExp.js:19:17:19:35 | '^test.example.com' | This string, which is used as a regular expression $@, has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:20:13:20:26 | `${hostname}$` | here |
--- a/javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteHostnameRegExp.js
+++ b/javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteHostnameRegExp.js
@@ -12,7 +12,7 @@
 	s.match("^http://test.example.com"); // NOT OK

 	function id(e) { return e; }
-	new RegExp(id(id(id("^http://test.example.com")))); // NOT OK, but not supported by type tracking
+	new RegExp(id(id(id("^http://test.example.com")))); // NOT OK

 	new RegExp(`test.example.com$`); // NOT OK