From be10f24de71ea454613fdc38e3147da7b65f9c93 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 12 Feb 2019 17:12:05 +0000 Subject: [PATCH] JS: make moduleImport() work for named imports --- .../semmle/javascript/dataflow/DataFlow.qll | 66 +++++++++++++++++++ .../src/semmle/javascript/dataflow/Nodes.qll | 14 ++-- .../semmle/javascript/dataflow/Sources.qll | 2 + .../internal/InterModuleTypeInference.qll | 19 ++++++ .../library-tests/DataFlow/sources.expected | 1 + ...ortNode_getAConstructorInvocation.expected | 1 + ...leImportNode_getAMemberInvocation.expected | 1 + ...ModuleImportNode_getAPropertyRead.expected | 1 + .../ModuleImportNode_getPath.expected | 1 + .../ModuleImportNodes/moduleImport.expected | 8 +++ .../ModuleImportNodes/moduleImport.ql | 4 ++ .../moduleImportProp.expected | 8 +++ .../ModuleImportNodes/moduleImportProp.ql | 4 ++ 13 files changed, 122 insertions(+), 8 deletions(-) create mode 100644 javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.expected create mode 100644 javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.ql create mode 100644 javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.expected create mode 100644 javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.ql diff --git a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll index 71c7c6ef729..a07590e99e4 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll @@ -37,6 +37,9 @@ module DataFlow { TThisNode(StmtContainer f) { f.(Function).getThisBinder() = f or f instanceof TopLevel } or TUnusedParameterNode(SimpleParameter p) { not exists(SsaExplicitDefinition ssa | p = ssa.getDef()) + } or + TDestructuredModuleImportNode(ImportDeclaration decl) { + decl.getASpecifier() instanceof NamedImportSpecifier } /** @@ -342,6 +345,28 @@ module DataFlow { override string toString() { result = "reflective call" } } + /** + * A node referring to the module imported at a named ES2015 import declaration. + * + * Default imports and namespace imports do not fall into this category, as the + * SSA definition of the local variable is used as the source of the module instead. + */ + private class DestructuredModuleImportNode extends Node, TDestructuredModuleImportNode { + ImportDeclaration imprt; + + DestructuredModuleImportNode() { this = TDestructuredModuleImportNode(imprt) } + + override BasicBlock getBasicBlock() { result = imprt.getBasicBlock() } + + override predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ) { + imprt.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } + + override string toString() { result = imprt.toString() } + } + /** * A data flow node that reads or writes an object property or class member. * @@ -372,6 +397,12 @@ module DataFlow { */ abstract string getPropertyName(); + /** + * Holds if this creates an alias for the property, as opposed to + * just being a read or write of the property. + */ + predicate isES2015PropertyBinding() { none() } + /** * Holds if this data flow node accesses property `p` on base node `base`. */ @@ -659,6 +690,31 @@ module DataFlow { override string getPropertyName() { none() } } + /** + * A named import specifier seen as a property read on the imported module. + */ + private class NamedImportSpecifierAsPropRead extends PropRead { + ImportDeclaration imprt; + + NamedImportSpecifier spec; + + NamedImportSpecifierAsPropRead() { + spec = imprt.getASpecifier() and + exists(SsaExplicitDefinition ssa | + ssa.getDef() = spec and + this = TSsaDefNode(ssa) + ) + } + + override Node getBase() { result = TDestructuredModuleImportNode(imprt) } + + override Expr getPropertyNameExpr() { result = spec.getImported() } + + override string getPropertyName() { result = spec.getImportedName() } + + override predicate isES2015PropertyBinding() { any() } + } + /** * A data flow node representing an unused parameter. * @@ -875,6 +931,16 @@ module DataFlow { */ DataFlow::ThisNode thisNode(StmtContainer container) { result = TThisNode(container) } + /** + * INTERNAL. DO NOT USE. + * + * Gets the data flow node holding the reference to the module being destructured at + * the given import declaration. + */ + DataFlow::Node destructuredModuleImportNode(ImportDeclaration imprt) { + result = TDestructuredModuleImportNode(imprt) + } + /** * A classification of flows that are not modeled, or only modeled incompletely, by * `DataFlowNode`: diff --git a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll index c5866bd7d17..be28ac472b7 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll @@ -427,6 +427,12 @@ class ModuleImportNode extends DataFlow::SourceNode { is.getImportedName() = "default" ) or + // `import { createServer } from 'http'` + exists(ImportDeclaration id | + this = DataFlow::destructuredModuleImportNode(id) and + id.getImportedPath().getValue() = path + ) + or // declared AMD dependency exists(AMDModuleDefinition amd | this = DataFlow::parameterNode(amd.getDependencyParameter(path)) @@ -456,14 +462,6 @@ ModuleImportNode moduleImport(string path) { result.getPath() = path } */ DataFlow::SourceNode moduleMember(string path, string m) { result = moduleImport(path).getAPropertyRead(m) - or - exists(ImportDeclaration id, ImportSpecifier is, SsaExplicitDefinition ssa | - id.getImportedPath().getValue() = path and - is = id.getASpecifier() and - is.getImportedName() = m and - ssa.getDef() = is and - result = DataFlow::ssaDefinitionNode(ssa) - ) } /** diff --git a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll index 7c70380ec85..f6a3ce3f391 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll @@ -207,6 +207,8 @@ module SourceNode { this instanceof DataFlow::Impl::InvokeNodeDef or DataFlow::thisNode(this, _) + or + this = DataFlow::destructuredModuleImportNode(_) } } } diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll index da2b54fc14f..b8ada71c500 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll @@ -175,6 +175,25 @@ private class AnalyzedNamespaceImport extends AnalyzedImport { } } +/** + * Flow analysis for namespace imports. + */ +private class AnalyzedDestructuredImport extends AnalyzedPropertyRead { + Module imported; + + AnalyzedDestructuredImport() { + exists(ImportDeclaration id | + this = DataFlow::destructuredModuleImportNode(id) and + imported = id.getImportedModule() + ) + } + + override predicate reads(AbstractValue base, string propName) { + base = TAbstractModuleObject(imported) and + propName = "exports" + } +} + /** * Flow analysis for `require` calls, interpreted as an implicit read of * the `module.exports` property of the imported module. diff --git a/javascript/ql/test/library-tests/DataFlow/sources.expected b/javascript/ql/test/library-tests/DataFlow/sources.expected index 4728bd7c1e5..bfa96adad1a 100644 --- a/javascript/ql/test/library-tests/DataFlow/sources.expected +++ b/javascript/ql/test/library-tests/DataFlow/sources.expected @@ -12,6 +12,7 @@ | sources.js:3:2:5:1 | functio ... x+19;\\n} | | sources.js:3:11:3:11 | x | | tst.js:1:1:1:0 | this | +| tst.js:1:1:1:24 | import ... m 'fs'; | | tst.js:1:10:1:11 | fs | | tst.js:16:1:20:9 | (functi ... ("arg") | | tst.js:16:2:16:1 | this | diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAConstructorInvocation.expected b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAConstructorInvocation.expected index 3c17ef44b5b..65c907d416e 100644 --- a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAConstructorInvocation.expected +++ b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAConstructorInvocation.expected @@ -1,2 +1,3 @@ +| destructuringES6.js:1:1:1:41 | import ... ctron'; | destructuringES6.js:2:1:2:19 | new BrowserWindow() | | destructuringRequire.js:1:27:1:45 | require('electron') | destructuringRequire.js:2:1:2:19 | new BrowserWindow() | | moduleUses.js:1:11:1:24 | require('mod') | moduleUses.js:9:1:9:7 | new K() | diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAMemberInvocation.expected b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAMemberInvocation.expected index f28930bf32c..00019acba5d 100644 --- a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAMemberInvocation.expected +++ b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAMemberInvocation.expected @@ -1,5 +1,6 @@ | amd1.js:1:25:1:26 | fs | amd1.js:2:3:2:29 | fs.read ... a.txt") | | amd2.js:2:12:2:24 | require('fs') | amd2.js:3:3:3:29 | fs.read ... a.txt") | +| destructuringES6.js:1:1:1:41 | import ... ctron'; | destructuringES6.js:2:1:2:19 | new BrowserWindow() | | destructuringRequire.js:1:27:1:45 | require('electron') | destructuringRequire.js:2:1:2:19 | new BrowserWindow() | | moduleUses.js:1:11:1:24 | require('mod') | moduleUses.js:3:1:3:18 | mod.moduleMethod() | | moduleUses.js:1:11:1:24 | require('mod') | moduleUses.js:6:1:6:3 | f() | diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAPropertyRead.expected b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAPropertyRead.expected index aa8a3d06e86..0a00c35d31b 100644 --- a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAPropertyRead.expected +++ b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getAPropertyRead.expected @@ -1,5 +1,6 @@ | amd1.js:1:25:1:26 | fs | amd1.js:2:3:2:17 | fs.readFileSync | | amd2.js:2:12:2:24 | require('fs') | amd2.js:3:3:3:17 | fs.readFileSync | +| destructuringES6.js:1:1:1:41 | import ... ctron'; | destructuringES6.js:1:10:1:22 | BrowserWindow | | destructuringRequire.js:1:27:1:45 | require('electron') | destructuringRequire.js:1:9:1:21 | BrowserWindow | | moduleUses.js:1:11:1:24 | require('mod') | moduleUses.js:3:1:3:16 | mod.moduleMethod | | moduleUses.js:1:11:1:24 | require('mod') | moduleUses.js:5:9:5:26 | mod.moduleFunction | diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getPath.expected b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getPath.expected index b01f10b8d1b..f8cefdc7033 100644 --- a/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getPath.expected +++ b/javascript/ql/test/library-tests/ModuleImportNodes/ModuleImportNode_getPath.expected @@ -1,5 +1,6 @@ | amd1.js:1:25:1:26 | fs | fs | | amd2.js:2:12:2:24 | require('fs') | fs | +| destructuringES6.js:1:1:1:41 | import ... ctron'; | electron | | destructuringRequire.js:1:27:1:45 | require('electron') | electron | | instanceThroughDefaultImport.js:1:8:1:42 | myDefaultImportedModuleInstanceName | myDefaultImportedModuleInstance | | instanceThroughNamespaceImport.js:1:8:1:49 | myNamespaceImportedModuleInstanceName | myNamespaceImportedModuleInstance | diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.expected b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.expected new file mode 100644 index 00000000000..54402457bd2 --- /dev/null +++ b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.expected @@ -0,0 +1,8 @@ +| electron | destructuringES6.js:1:1:1:41 | import ... ctron'; | +| electron | destructuringRequire.js:1:27:1:45 | require('electron') | +| fs | amd1.js:1:25:1:26 | fs | +| fs | amd2.js:2:12:2:24 | require('fs') | +| mod | moduleUses.js:1:11:1:24 | require('mod') | +| myDefaultImportedModuleInstance | instanceThroughDefaultImport.js:1:8:1:42 | myDefaultImportedModuleInstanceName | +| myNamespaceImportedModuleInstance | instanceThroughNamespaceImport.js:1:8:1:49 | myNamespaceImportedModuleInstanceName | +| myRequiredModuleInstance | instanceThroughRequire.js:1:36:1:70 | require ... tance') | diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.ql b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.ql new file mode 100644 index 00000000000..a60c45a2b0c --- /dev/null +++ b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImport.ql @@ -0,0 +1,4 @@ +import javascript + +from string path +select path, DataFlow::moduleImport(path) diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.expected b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.expected new file mode 100644 index 00000000000..aa97bdf8d41 --- /dev/null +++ b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.expected @@ -0,0 +1,8 @@ +| electron | BrowserWindow | destructuringES6.js:1:10:1:22 | BrowserWindow | +| electron | BrowserWindow | destructuringRequire.js:1:9:1:21 | BrowserWindow | +| fs | readFileSync | amd1.js:2:3:2:17 | fs.readFileSync | +| fs | readFileSync | amd2.js:3:3:3:17 | fs.readFileSync | +| mod | constructorFunction | moduleUses.js:8:9:8:31 | mod.con ... unction | +| mod | moduleField | moduleUses.js:11:1:11:15 | mod.moduleField | +| mod | moduleFunction | moduleUses.js:5:9:5:26 | mod.moduleFunction | +| mod | moduleMethod | moduleUses.js:3:1:3:16 | mod.moduleMethod | diff --git a/javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.ql b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.ql new file mode 100644 index 00000000000..46e1938e85b --- /dev/null +++ b/javascript/ql/test/library-tests/ModuleImportNodes/moduleImportProp.ql @@ -0,0 +1,4 @@ +import javascript + +from string path, string prop +select path, prop, DataFlow::moduleImport(path).getAPropertyRead(prop)