From 7cfe3dae85a9662c1b474e4d751fe80da30db567 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Aug 2024 15:05:00 +0200 Subject: [PATCH 1/2] JS: Port step for dynamic imports --- .../ql/lib/semmle/javascript/Promises.qll | 1 - .../flow_summaries/AllFlowSummaries.qll | 1 + .../flow_summaries/DynamicImportStep.qll | 39 +++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 javascript/ql/lib/semmle/javascript/internal/flow_summaries/DynamicImportStep.qll diff --git a/javascript/ql/lib/semmle/javascript/Promises.qll b/javascript/ql/lib/semmle/javascript/Promises.qll index f25fa2bc820..c254128f87b 100644 --- a/javascript/ql/lib/semmle/javascript/Promises.qll +++ b/javascript/ql/lib/semmle/javascript/Promises.qll @@ -705,7 +705,6 @@ private module DynamicImportSteps { */ class DynamicImportStep extends LegacyPreCallGraphStep { override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { - // TODO: this step needs to be ported to dataflow2 exists(DynamicImportExpr imprt | pred = imprt.getImportedModule().getAnExportedValue("default") and succ = imprt.flow() and diff --git a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AllFlowSummaries.qll b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AllFlowSummaries.qll index d7eba4852db..6094141cc40 100644 --- a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AllFlowSummaries.qll +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/AllFlowSummaries.qll @@ -9,3 +9,4 @@ private import Maps private import Promises private import Sets private import Strings +private import DynamicImportStep diff --git a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/DynamicImportStep.qll b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/DynamicImportStep.qll new file mode 100644 index 00000000000..2976b467315 --- /dev/null +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/DynamicImportStep.qll @@ -0,0 +1,39 @@ +/** + * Contains flow steps to model flow from a module into a dynamic `import()` expression. + */ + +private import javascript +private import semmle.javascript.dataflow.internal.DataFlowNode +private import semmle.javascript.dataflow.internal.AdditionalFlowInternal +private import semmle.javascript.dataflow.internal.DataFlowPrivate + +/** + * Flow steps for dynamic import expressions. + * + * The default export of the imported module must be boxed in a promise, so we pass + * it through a synthetic node. + */ +class DynamicImportStep extends AdditionalFlowInternal { + override predicate needsSynthesizedNode(AstNode node, string tag, DataFlowCallable container) { + node instanceof DynamicImportExpr and + tag = "imported-value" and + container.asSourceCallable() = node.getContainer() + } + + override predicate jumpStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(DynamicImportExpr expr | + pred = expr.getImportedModule().getAnExportedValue("default") and + succ = getSynthesizedNode(expr, "imported-value") + ) + } + + override predicate storeStep( + DataFlow::Node pred, DataFlow::ContentSet contents, DataFlow::Node succ + ) { + exists(DynamicImportExpr expr | + pred = getSynthesizedNode(expr, "imported-value") and + contents = DataFlow::ContentSet::promiseValue() and + succ = TValueNode(expr) + ) + } +} From 47c519fc0a0457722006b85b2d542ff4794379bb Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 26 Aug 2024 15:15:49 +0200 Subject: [PATCH 2/2] JS: Add test for flow through dynamic imports --- .../TaintTracking/BasicTaintTracking.expected | 4 ++++ .../TaintTracking/DataFlowTracking.expected | 4 ++++ .../library-tests/TaintTracking/export-taint.js | 4 ++++ .../library-tests/TaintTracking/import-taint.js | 16 ++++++++++++++++ 4 files changed, 28 insertions(+) create mode 100644 javascript/ql/test/library-tests/TaintTracking/export-taint.js create mode 100644 javascript/ql/test/library-tests/TaintTracking/import-taint.js diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index 91a7f386516..7fff1f8f963 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -14,6 +14,8 @@ legacyDataFlowDifference | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library | | constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library | | exceptions.js:53:14:53:21 | source() | exceptions.js:54:10:54:10 | e | only flow with NEW data flow library | +| export-taint.js:3:22:3:29 | source() | import-taint.js:7:10:7:25 | mod.object.taint | only flow with OLD data flow library | +| export-taint.js:3:22:3:29 | source() | import-taint.js:14:14:14:29 | mod.object.taint | only flow with OLD data flow library | | getters-and-setters.js:53:21:53:28 | source() | getters-and-setters.js:53:10:53:30 | getX(ne ... rce())) | only flow with NEW data flow library | | nested-props.js:14:15:14:22 | source() | nested-props.js:15:10:15:16 | obj.x.y | only flow with NEW data flow library | | nested-props.js:27:18:27:25 | source() | nested-props.js:28:10:28:14 | obj.x | only flow with NEW data flow library | @@ -165,6 +167,8 @@ flow | exceptions.js:144:9:144:16 | source() | exceptions.js:132:8:132:27 | returnThrownSource() | | exceptions.js:150:13:150:20 | source() | exceptions.js:153:10:153:10 | e | | exceptions.js:158:13:158:20 | source() | exceptions.js:161:10:161:10 | e | +| export-taint.js:2:12:2:19 | source() | import-taint.js:6:10:6:18 | mod.taint | +| export-taint.js:2:12:2:19 | source() | import-taint.js:13:14:13:22 | mod.taint | | factory-function.js:21:13:21:20 | source() | factory-function.js:7:10:7:12 | obj | | factory-function.js:22:13:22:20 | source() | factory-function.js:7:10:7:12 | obj | | factory-function.js:26:7:26:14 | source() | factory-function.js:16:14:16:16 | obj | diff --git a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected index de977a8ff92..fe96a5e3db5 100644 --- a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected @@ -15,6 +15,8 @@ legacyDataFlowDifference | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:44:8:44:19 | f_safe.taint | only flow with NEW data flow library | | constructor-calls.js:20:15:20:22 | source() | constructor-calls.js:39:8:39:14 | e.param | only flow with NEW data flow library | | exceptions.js:53:14:53:21 | source() | exceptions.js:54:10:54:10 | e | only flow with NEW data flow library | +| export-taint.js:3:22:3:29 | source() | import-taint.js:7:10:7:25 | mod.object.taint | only flow with OLD data flow library | +| export-taint.js:3:22:3:29 | source() | import-taint.js:14:14:14:29 | mod.object.taint | only flow with OLD data flow library | | getters-and-setters.js:53:21:53:28 | source() | getters-and-setters.js:53:10:53:30 | getX(ne ... rce())) | only flow with NEW data flow library | | nested-props.js:14:15:14:22 | source() | nested-props.js:15:10:15:16 | obj.x.y | only flow with NEW data flow library | | nested-props.js:27:18:27:25 | source() | nested-props.js:28:10:28:14 | obj.x | only flow with NEW data flow library | @@ -115,6 +117,8 @@ flow | exceptions.js:144:9:144:16 | source() | exceptions.js:132:8:132:27 | returnThrownSource() | | exceptions.js:150:13:150:20 | source() | exceptions.js:153:10:153:10 | e | | exceptions.js:158:13:158:20 | source() | exceptions.js:161:10:161:10 | e | +| export-taint.js:2:12:2:19 | source() | import-taint.js:6:10:6:18 | mod.taint | +| export-taint.js:2:12:2:19 | source() | import-taint.js:13:14:13:22 | mod.taint | | factory-function.js:21:13:21:20 | source() | factory-function.js:7:10:7:12 | obj | | factory-function.js:22:13:22:20 | source() | factory-function.js:7:10:7:12 | obj | | factory-function.js:26:7:26:14 | source() | factory-function.js:16:14:16:16 | obj | diff --git a/javascript/ql/test/library-tests/TaintTracking/export-taint.js b/javascript/ql/test/library-tests/TaintTracking/export-taint.js new file mode 100644 index 00000000000..aa27847b64d --- /dev/null +++ b/javascript/ql/test/library-tests/TaintTracking/export-taint.js @@ -0,0 +1,4 @@ +export default { + taint: source(), + object: { taint: source() } +}; diff --git a/javascript/ql/test/library-tests/TaintTracking/import-taint.js b/javascript/ql/test/library-tests/TaintTracking/import-taint.js new file mode 100644 index 00000000000..3b4e0fff59f --- /dev/null +++ b/javascript/ql/test/library-tests/TaintTracking/import-taint.js @@ -0,0 +1,16 @@ +import 'dummy'; + +async function test1() { + let mod = await import("./export-taint"); + sink(mod); // OK + sink(mod.taint); // NOT OK + sink(mod.object.taint); // NOT OK [INCONSISTENCY] - blocked by access path limit +} + +function test2() { + import("./export-taint").then(mod => { + sink(mod); // OK + sink(mod.taint); // NOT OK + sink(mod.object.taint); // NOT OK [INCONSISTENCY] - blocked by access path limit + }); +}